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

[Warner Losh]

> On Jan 20, 2015, at 2:37 AM, Hans Petter Selasky <hps at selasky.org> wrote:
> 
> On 01/20/15 10:00, Konstantin Belousov wrote:
>> On Tue, Jan 20, 2015 at 08:58:34AM +0100, Hans Petter Selasky wrote:
>>> On 01/20/15 08:51, Konstantin Belousov wrote:
>>>> On Tue, Jan 20, 2015 at 05:30:25AM +0100, Hans Petter Selasky wrote:
>>>>> On 01/19/15 22:59, Adrian Chadd wrote:
>>>>>> Hi,
>>>>>> 
>>>>>> Would you please check what the results of this are with CPU specific
>>>>>> callwheels?
>>>>>> 
>>>>>> I'm doing some 10+ gig traffic testing on -HEAD with RSS enabled (on
>>>>>> ixgbe) and with this setup, the per-CPU TCP callwheel stuff is
>>>>>> enabled. But all the callwheels are now back on clock(0) and so is the
>>>>>> lock contention. :(
>>>>>> 
>>>>>> Thanks,
>>>>>> 
>>>>> 
>>>>> Hi,
>>>>> 
>>>>> Like stated in the manual page, callout_reset_curcpu/on() does not work
>>>>> with MPSAFE callouts any more!
>>>> I.e. you 'fixed' some undeterminate bugs in callout migration by not
>>>> doing migration at all anymore.
>>>> 
>>>>> 
>>>>> You need to use callout_init_{mtx,rm,rw} and remove the custom locking
>>>>> inside the callback in the TCP stack to get it working like before!
>>>> 
>>>> No, you need to do this, if you think that whole callout KPI must be
>>>> rototiled.  It is up to the person who modifies the KPI, to ensure that
>>>> existing code is not broken.
> 
> Hi,
> 
> It is not very hard to update existing callout clients and you can do it too, if you need the extra bits of performance.
> 
> Are there more API's than the TCP stack which you think needs an update and are performance critical?
> 
>>>> 
>>>> As I understand, currently we are back to the one-cpu callouts.
>>>> Do other people consider this situation acceptable ?
> 
> For the TCP stack - yes, but not for other clients like cv_timedwait() and such.
> 
> If you think you have a better way to solve the callout problems, please tell me! In order for a callout to change its CPU you need a lock to protect which CPU the callout is on. Instead of introducing a third lock in the callout path, which will be a congestion point, to protect against changing the CPU number, I decided that we will use the client's mutex and the MPSAFE implies the client doesn't have any mutex. So it won't work with callout clients which use the CALLOUT_MPSAFE flag. Honestly CALLOUT_MPSAFE should not be used, because it leads to extra complexity in the clients catching the race when tearing down the callouts and any pending callbacks.

Then it is incumbent on you to fix them. You can’t just fix one instance and wash your hands of the problem.

Maybe this is a real and legitimate bug. However, until you’ve followed your solution through by actually fixing the abusers of it, my confidence that another issue won’t present itself is quite low. The code seems half baked to me. And from reading this thread, it seems like perhaps I’m not the only one.

>>> Please read the callout 9 manual page first.
>> 
>> Assume I read it.  How this changes any of my points above ?
>> """
>> A change in the CPU selection cannot happen if this function is
>> re-scheduled inside a callout function. Else the callback function given
>> by the func argument will be executed on the same CPU like previously
>> done.
>> """
>> You cannot do this without fixing consumers.
>> 
> 
> The code simply needs an update. It is not broken in any ways - right? If it is not broken, fixing it is not that urgent.

Radically changing the performance characteristics is breaking the code. Performance regression in the TCP stack is urgent to fix. Not being able to enumerate what all the consumers are that use this and provide an analysis about why they aren’t important to fix is a bug in your process, and in your interaction with the project. We simply do not operate that way.

Warner