From owner-svn-src-head@FreeBSD.ORG Wed Jan 21 00:22:48 2015 Return-Path: Delivered-To: svn-src-head@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 1D455E5A; Wed, 21 Jan 2015 00:22:48 +0000 (UTC) Received: from mail.ignoranthack.me (ignoranthack.me [199.102.79.106]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id E308038E; Wed, 21 Jan 2015 00:22:47 +0000 (UTC) Received: from [192.168.200.212] (unknown [50.136.155.142]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) (Authenticated sender: sbruno@ignoranthack.me) by mail.ignoranthack.me (Postfix) with ESMTPSA id 0A779192A3B; Wed, 21 Jan 2015 00:22:45 +0000 (UTC) Message-ID: <54BEF154.3030606@ignoranthack.me> Date: Tue, 20 Jan 2015 16:22:44 -0800 From: Sean Bruno Reply-To: sbruno@freebsd.org User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:31.0) Gecko/20100101 Thunderbird/31.4.0 MIME-Version: 1.0 To: "K. Macy" Subject: Re: svn commit: r277213 - in head: share/man/man9 sys/kern sys/ofed/include/linux sys/sys 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> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit 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-head@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 21 Jan 2015 00:22:48 -0000 -----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. 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-----