From owner-svn-src-all@FreeBSD.ORG Wed Jan 21 00:37:45 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 96A2623C; Wed, 21 Jan 2015 00:37:45 +0000 (UTC) Received: from mail-yh0-x22a.google.com (mail-yh0-x22a.google.com [IPv6:2607:f8b0:4002:c01::22a]) (using TLSv1 with cipher ECDHE-RSA-RC4-SHA (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 46DAA6A2; Wed, 21 Jan 2015 00:37:45 +0000 (UTC) Received: by mail-yh0-f42.google.com with SMTP id a41so5580612yho.1; Tue, 20 Jan 2015 16:37:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:sender:in-reply-to:references:date:message-id:subject :from:to:cc:content-type; bh=mYFdu5RixQPZoBkoG9X98f+ZVaEV5J8dEGOdcWow9g8=; b=J0aXZQhmV6zwvXzfmlN660l9sLKqDoEbADtAW6k2uS/kSYTcC7eePDPnBH5i+c7NXv dGcOxiLdScgOelRjGCx74syJNSIgy8c1HsTdecHg3mynNM4BAZLCjNp9VtpINgdkve7w Z0zBXePZ9VOt2v7LzV8PnSfBXs4HrtrPkMCkBpS15nT3pgrLOUboPAFx03tuumHD8qYg ncsqaYnQQG/wmyM1nCnKWmUd2/Czh4B2FS3quE1kdWEXiK23iVWV2w9xeaApihe7HYzz f4FY0EWtpaY8QvD3/gIRaXXr3aty4N9z6auLUJDNwv2RZGfxLEcECXl6Du9aXpfI+FL5 4LCg== MIME-Version: 1.0 X-Received: by 10.170.44.4 with SMTP id 4mr2949166ykm.101.1421800664480; Tue, 20 Jan 2015 16:37:44 -0800 (PST) Sender: kmacybsd@gmail.com Received: by 10.170.70.132 with HTTP; Tue, 20 Jan 2015 16:37:44 -0800 (PST) In-Reply-To: <54BEF154.3030606@ignoranthack.me> References: <201501151532.t0FFWV2Y037455@svn.freebsd.org> <54BDD9E1.6090505@selasky.org> <20150120075126.GA42409@kib.kiev.ua> <20150120211137.GY15484@FreeBSD.org> <54BED6FB.8060401@selasky.org> <54BEE62D.2060703@ignoranthack.me> <54BEE8E6.3080009@ignoranthack.me> <54BEEA7F.1070301@ignoranthack.me> <54BEF154.3030606@ignoranthack.me> Date: Tue, 20 Jan 2015 16:37:44 -0800 X-Google-Sender-Auth: -L9LfQBSVVnEHcsvC2pM95gqdno Message-ID: Subject: Re: svn commit: r277213 - in head: share/man/man9 sys/kern sys/ofed/include/linux sys/sys From: "K. Macy" To: Sean Bruno Content-Type: text/plain; charset=UTF-8 Cc: Hans Petter Selasky , Adrian Chadd , "src-committers@freebsd.org" , Jason Wolfe , "svn-src-all@freebsd.org" , "svn-src-head@freebsd.org" , Gleb Smirnoff , Konstantin Belousov X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Jan 2015 00:37:45 -0000 On Tue, Jan 20, 2015 at 4:22 PM, Sean Bruno wrote: > -----BEGIN PGP SIGNED MESSAGE----- > Hash: SHA512 > > On 01/20/15 15:59, K. Macy wrote: >> On Tue, Jan 20, 2015 at 3:53 PM, Sean Bruno >> wrote: On 01/20/15 15:48, K. Macy wrote: >>>>> Are any other drivers hitting this? e.g. cxgb/cxgbe? >>>>> >>>>> -K >>>>> >> >> Unkown to me. Nor am I aware of anyone else who ever hit our >> panics either. Our environment, and the failure, was only seen in >> the Intel 10GE space (ixgbe). This is an artifact of our use >> cases, and hasn't been expanded nor tested in our environment with >> other vendor interfaces. >> >>> For an ill characterized problem isolated to one environment >>> this seems like a workaround that should not be part of the >>> general code base. >> >>> -K >> > > There was never any indication in our testing that the driver was > involved in any way. > > In our universe, this commit (right or wrong) resolved our panics. I > think that there is some room for improvement based on the commentary > in this thread, but some people do indeed prefer stability over > performance. I hope we can come to a middle ground somewhere here. I would pick stability over performance any day. However, it _seems_ to me, and maybe I simply don't understand some key details, that the fix consisted of largely single-threading the callout system. And as I say I may simply not understand the specifics, but this sort of large scale disabling does not constitute a fix but is a workaround. It's more like disabling preemption because it fixes a panic. Yes, it might "fix" a whole array of bugs that crop up but it could not be seen as a fix to an otherwise working system. -K > > sean > >> >> sean >> >>>>> On Tue, Jan 20, 2015 at 3:46 PM, Sean Bruno >>>>> wrote: On 01/20/15 15:40, K. Macy >>>>> wrote: >>>>>>>> I think you're working around driver locking bugs by >>>>>>>> crippling the callout code. >>>>>>>> >>>>>>>> -K >>>>>>>> >>>>> >>>>> We had zero evidence of this. What leads you down that path? >>>>> I'm totally open to being wrong, e.g. "yeah, you slowed down >>>>> things so that you don't hit a race condition" >>>>> >>>>> sean >>>>> >>>>>>>> On Tue, Jan 20, 2015 at 3:35 PM, Sean Bruno >>>>>>>> wrote: On 01/20/15 14:30, >>>>>>>> Hans Petter Selasky wrote: >>>>>>>>>>> On 01/20/15 22:11, Gleb Smirnoff wrote: >>>>>>>>>>>> On Tue, Jan 20, 2015 at 09:51:26AM +0200, >>>>>>>>>>>> Konstantin Belousov wrote: K> > Like stated in >>>>>>>>>>>> the manual page, callout_reset_curcpu/on() does >>>>>>>>>>>> not work K> > with MPSAFE callouts any more! K> >>>>>>>>>>>> I.e. you 'fixed' some undeterminate bugs in >>>>>>>>>>>> callout migration by not K> doing migration at >>>>>>>>>>>> all anymore. K> K> > K> > You need to use >>>>>>>>>>>> callout_init_{mtx,rm,rw} and remove the custom >>>>>>>>>>>> locking K> > inside the callback in the TCP >>>>>>>>>>>> stack to get it working like before! K> K> No, >>>>>>>>>>>> you need to do this, if you think that whole >>>>>>>>>>>> callout KPI must be K> rototiled. It is up to >>>>>>>>>>>> the person who modifies the KPI, to ensure that >>>>>>>>>>>> K> existing code is not broken. K> K> As I >>>>>>>>>>>> understand, currently we are back to the >>>>>>>>>>>> one-cpu callouts. K> Do other people consider >>>>>>>>>>>> this situation acceptable ? >>>>>>>>>>>> >>>>>>>>>>>> I think this isn't acceptable. The commit to a >>>>>>>>>>>> complex subsystem lacked a review from persons >>>>>>>>>>>> involved in the system before. The commit to >>>>>>>>>>>> subsystem broke consumers of the subsystem and >>>>>>>>>>>> this was even done not accidentially, but due >>>>>>>>>>>> to Hans not caring about it. >>>>>>>>>>>> >>>>>>>>>>>> As for me this is enough to request a backout, >>>>>>>>>>>> and let the change back in only after proper >>>>>>>>>>>> review. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Hi Gleb, >>>>>>>>>>> >>>>>>>>>>> Backing out my callout API patch means we will >>>>>>>>>>> for sure re-introduce an unknown callout spinlock >>>>>>>>>>> hang, as noted to me by several people. What do >>>>>>>>>>> you think about that? dram Maybe "Jason Wolfe" >>>>>>>>>>> CC'ed can add to 10-stable w/o my patches: >>>>>>>>>>> >>>>>>>> >>>>>>>> Jason picked up this patch for work and it resolved >>>>>>>> our instability issues that had remained unsolved for >>>>>>>> quite some time as reported to freebsd-net: >>>>>>>> >>>>>>>> https://lists.freebsd.org/pipermail/freebsd-net/2015-January/040895.html >>>>>>>> >>>>>>>> >>>>>>>> >> >>>>>>>> > This had gone undiagnosed for some time (even with the gracious >>>>>>>> help of jhb in offline emails, thanks btw!). >>>>>>>> >>>>>>>> There's some diagnostics in that email thread that may >>>>>>>> be of value to you folks for determination of the >>>>>>>> validity of changing the callout API or at least >>>>>>>> understanding why we were involved in diagnostics. >>>>>>>> >>>>>>>> While I'd sure love to tune performance, the fact that >>>>>>>> our machines were basically going out to lunch without >>>>>>>> these changes, probably means that others were seeing >>>>>>>> it and didn't know what else to do. As much as I enjoy >>>>>>>> a good "break out the pitch forks and torches" email >>>>>>>> thread, this increased stability for us and is allowing >>>>>>>> us to upgrade from freebsd8 to freebsd10. Bear this in >>>>>>>> mind when you throw your voice in favor of reverting. >>>>>>>> >>>>>>>>>>> int callout_reset_sbt_on(struct callout *c, >>>>>>>>>>> sbintime_t sbt, sbintime_t precision, void >>>>>>>>>>> (*ftn)(void *), void *arg, int cpu, int flags) { >>>>>>>>>>> sbintime_t to_sbt, pr; struct callout_cpu *cc; >>>>>>>>>>> int cancelled, direct; >>>>>>>>>>> >>>>>>>>>>> + cpu = timeout_cpu; /* XXX test code XXX >>>>>>>>>>> */ >>>>>>>>>>> >>>>>>>>>>> cancelled = 0; >>>>>>>>>>> >>>>>>>> >>>>>>>> Jason or I would have to run this in production, which >>>>>>>> would be problematic I fear. We never had a >>>>>>>> deterministic test case that would exhibit the reported >>>>>>>> failure. We merely "tested in production" and saw that >>>>>>>> panics ceased. We didn't note a dropoff in our traffic >>>>>>>> either, perhaps we are not as efficient as others in >>>>>>>> this corner case, but we were consistently seeing the >>>>>>>> spinlock hangs after a day or so of traffic. >>>>>>>> >>>>>>>>>>> And see if he observes a callout spinlock hang or >>>>>>>>>>> not on his test setup. The patch above should >>>>>>>>>>> force all callouts to the same thread basically. >>>>>>>>>>> Then we could maybe see if single threading the >>>>>>>>>>> callouts has anything to do with solving the >>>>>>>>>>> spinlock hang. >>>>>>>>>>> >>>>>>>>>>> The "rewritten" callout API still has all the >>>>>>>>>>> features and capabilities the old one had, when >>>>>>>>>>> used as described in "man 9 callout". >>>>>>>>>>> >>>>>>>>>>> At the present moment I'm not technically >>>>>>>>>>> convinced a backout is correct. >>>>>>>> >>>>>>>> Neither am I, to be honest. Just based on *results*. >>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Gleb: I think we would see far better results >>>>>>>>>>> with high speed internet links using TCP if we >>>>>>>>>>> could extend the LRO (large receive offload) code >>>>>>>>>>> to accumulate more than 64KBytes worth of data >>>>>>>>>>> per call to the TCP stack instead of complaining >>>>>>>>>>> about some callouts ending up on the same thread! >>>>>>>>>>> Actually I have a patch for that. >>>>>>>>>>> >>>>>>>>>>> --HPS >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>> >>>>>>>>> _______________________________________________ >>>>>>>>> svn-src-head@freebsd.org mailing list >>>>>>>>> http://lists.freebsd.org/mailman/listinfo/svn-src-head >>>>>>>>> To unsubscribe, send any mail to >>>>>>>>> "svn-src-head-unsubscribe@freebsd.org" >>>>>>>> >>>>>>>> >>>>> >>>>> >>>>> >> >> >> >> > > -----BEGIN PGP SIGNATURE----- > Version: GnuPG v2 > > iQF8BAEBCgBmBQJUvvFSXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w > ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx > MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5k1AcIAJvIeDso4/a4YqEyXxGj4CAb > QLfESoILRDF3LpczRpIFGnUPRs9pWUTm6DIUut0ZjgLChq1kHzdi2uIFe9QB3ehX > zzw4MEtxDEqXv5zOAmP1gvcx1a2rKZJnTlfW2CHa2QAYTR06BFARu8u3NyC3tZiq > o/NGpidv+nrkcrDSHmzpVgIrlZzyB+YsGyNcvRUkewxMpos9syB2sKiXm9u/MQYQ > nHyjFw9IOjDFAiZgww0y/8QYB9efr639Hgt1BYn86t4iZzwDOajH+jeb9VXMT5s9 > liauQ4NiiMBe6F/mldoJ+XrtlPZ9rdx5jbgz8zBQcB7LzoWv91q0GOVpk/Am8FQ= > =E1eL > -----END PGP SIGNATURE-----