Date: Tue, 20 Jan 2015 15:39:47 -0800 From: Navdeep Parhar <np@FreeBSD.org> To: sbruno@freebsd.org, Hans Petter Selasky <hps@selasky.org>, Gleb Smirnoff <glebius@FreeBSD.org>, Konstantin Belousov <kostikbel@gmail.com> Cc: "svn-src-head@freebsd.org" <svn-src-head@freebsd.org>, Adrian Chadd <adrian@freebsd.org>, "src-committers@freebsd.org" <src-committers@freebsd.org>, "svn-src-all@freebsd.org" <svn-src-all@freebsd.org>, Jason Wolfe <nitroboost@gmail.com> Subject: Re: svn commit: r277213 - in head: share/man/man9 sys/kern sys/ofed/include/linux sys/sys Message-ID: <54BEE743.7030601@FreeBSD.org> In-Reply-To: <54BEE62D.2060703@ignoranthack.me> 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>
next in thread | previous in thread | raw e-mail | index | archive | help
Sean,
Was this really "Reviewed by: sbruno@" or just "Tested by: sbruno@"? I
was very happy to see so many reviewers on the original commit but you
seem to be the only one still left on the list.
Regards,
Navdeep
On 01/20/15 15:35, Sean Bruno wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA512
>
> 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
>>
>>
>>
>
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v2
>
> iQF8BAEBCgBmBQJUvuYrXxSAAAAAAC4AKGlzc3Vlci1mcHJAbm90YXRpb25zLm9w
> ZW5wZ3AuZmlmdGhob3JzZW1hbi5uZXRCQUFENDYzMkU3MTIxREU4RDIwOTk3REQx
> MjAxRUZDQTFFNzI3RTY0AAoJEBIB78oecn5kJTMIAMfh6ghV/AwQauY+a44q1hjJ
> WC7E3u69FK0opgSYg71kk6HckbyB+sTWND6HdXnpyrcMLXUt74zlB8c48wbUUV5+
> EwKNYzGNNnDNhoc0WtPMect8e9Y1kBRvSGfHBdVrqATXfPOyZEa+i4lQAXpiFKIt
> nndqVrAH7bJM6143YDpnIg7vaR+8IQnC2ztSP4ogJzh03DZ7zVsg4BsoCPg50eVZ
> kr46cXcE+SP/TsQBsVNVwRJD5NFie6QJdLoTnwkd0XfQGJMIWivNgUcE4tIxqPIf
> nGQ0xMJCotpNLuPtzYzCIurSaDHdHmL6bjkhjTtBmWsMNfdGH8TKoih79GDxkTg=
> =Y3rd
> -----END PGP SIGNATURE-----
>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?54BEE743.7030601>