Date: Tue, 20 Jan 2015 16:22:44 -0800 From: Sean Bruno <sbruno@ignoranthack.me> To: "K. Macy" <kmacy@freebsd.org> Cc: Hans Petter Selasky <hps@selasky.org>, Adrian Chadd <adrian@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, Jason Wolfe <nitroboost@gmail.com>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, Gleb Smirnoff <glebius@freebsd.org>, Konstantin Belousov <kostikbel@gmail.com> Subject: Re: svn commit: r277213 - in head: share/man/man9 sys/kern sys/ofed/include/linux sys/sys Message-ID: <54BEF154.3030606@ignoranthack.me> In-Reply-To: <CAHM0Q_PtJ7JHFTiu9_dmi_Ce=rmu1j72z2OYQ2CD3%2BEbcoEGsA@mail.gmail.com> References: <201501151532.t0FFWV2Y037455@svn.freebsd.org> <CAJ-Vmok0GXZoojyi=jE=b5D-d338APztaf3Pw0_AAQ-173XSWw@mail.gmail.com> <54BDD9E1.6090505@selasky.org> <20150120075126.GA42409@kib.kiev.ua> <20150120211137.GY15484@FreeBSD.org> <54BED6FB.8060401@selasky.org> <54BEE62D.2060703@ignoranthack.me> <CAHM0Q_MDJN_8sTvTDXfqA7UtJVO3Y8S8%2BNRCs_=6Nj4dkTzjOA@mail.gmail.com> <54BEE8E6.3080009@ignoranthack.me> <CAHM0Q_N_53BM-6RvXu8UpjfDzQHEn5oXZo1Nn8RO0cuOUhe8tg@mail.gmail.com> <54BEEA7F.1070301@ignoranthack.me> <CAHM0Q_PtJ7JHFTiu9_dmi_Ce=rmu1j72z2OYQ2CD3%2BEbcoEGsA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
-----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
> <sbruno@ignoranthack.me> 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
>>>> <sbruno@ignoranthack.me> 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
>>>>>>> <sbruno@ignoranthack.me> 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-----
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54BEF154.3030606>