svn commit: r277213 - in head: share/man/man9 sys/kern sys/ofed/include/linux sys/sys

[K. Macy]

On Tue, Jan 20, 2015 at 4:22 PM, Sean Bruno <sbruno at ignoranthack.me> 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
>> <sbruno at 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.

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
>>>>> <sbruno at 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 at 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 at freebsd.org mailing list
>>>>>>>>> http://lists.freebsd.org/mailman/listinfo/svn-src-head
>>>>>>>>> To unsubscribe, send any mail to
>>>>>>>>> "svn-src-head-unsubscribe at 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-----