Re: [PATCH] epoll: avoid calling ep_call_nested() from ep_poll_safewake()

[Jason Baron]

On 01/18/2018 06:00 AM, Hou Tao wrote:
> Hi Jason,
> 
> On 2017/10/18 22:03, Jason Baron wrote:
>>
>>
>> On 10/17/2017 11:37 AM, Davidlohr Bueso wrote:
>>> On Fri, 13 Oct 2017, Jason Baron wrote:
>>>
>>>> The ep_poll_safewake() function is used to wakeup potentially nested
>>>> epoll
>>>> file descriptors. The function uses ep_call_nested() to prevent entering
>>>> the same wake up queue more than once, and to prevent excessively deep
>>>> wakeup paths (deeper than EP_MAX_NESTS). However, this is not necessary
>>>> since we are already preventing these conditions during EPOLL_CTL_ADD.
>>>> This
>>>> saves extra function calls, and avoids taking a global lock during the
>>>> ep_call_nested() calls.
>>>
>>> This makes sense.
>>>
>>>>
>>>> I have, however, left ep_call_nested() for the CONFIG_DEBUG_LOCK_ALLOC
>>>> case, since ep_call_nested() keeps track of the nesting level, and
>>>> this is
>>>> required by the call to spin_lock_irqsave_nested(). It would be nice to
>>>> remove the ep_call_nested() calls for the CONFIG_DEBUG_LOCK_ALLOC case as
>>>> well, however its not clear how to simply pass the nesting level through
>>>> multiple wake_up() levels without more surgery. In any case, I don't
>>>> think
>>>> CONFIG_DEBUG_LOCK_ALLOC is generally used for production. This patch,
>>>> also
>>>> apparently fixes a workload at Google that Salman Qazi reported by
>>>> completely removing the poll_safewake_ncalls->lock from wakeup paths.
>>>
>>> I'm a bit curious about the workload (which uses lots of EPOLL_CTL_ADDs) as
>>> I was tackling the nested epoll scaling issue with loop and readywalk lists
>>> in mind.
>>>>
>>
>> I'm not sure the details of the workload - perhaps Salman can elaborate
>> further about it.
>>
>> It would seem that the safewake would potentially be the most contended
>> in general in the nested case, because generally you have a few epoll
>> fds attached to lots of sources doing wakeups. So those sources are all
>> going to conflict on the safewake lock. The readywalk is used when
>> performing a 'nested' poll and in general this is likely going to be
>> called on a few epoll fds. That said, we should remove it too. I will
>> post a patch to remove it.
>>
>> The loop lock is used during EPOLL_CTL_ADD to check for loops and deep
>> wakeup paths and so I would expect this to be less common, but I
>> wouldn't doubt there are workloads impacted by it. We can potentially, I
>> think remove this one too - and the global 'epmutex'. I posted some
>> ideas a while ago on it:
>>
>> http://lkml.iu.edu/hypermail//linux/kernel/1501.1/05905.html
>>
>> We can work through these ideas or others...
> 
> I have tested these patches on a simple non-nested epoll workload and found that
> there are performance regression (-0.5% which is better than -1.0% from my patch set)
> under normal nginx conf and performance improvement (+3%, and the number of my
> patch set is +4%) under the one-fd-per-request conf. The reason for performance
> improvement is clear, however, I haven't figured out the reason for the performance
> regression (maybe cache related ?)
> 
> It seems that your patch set will work fine both under normal and nested epoll workload,
> so I don't plan to continue my patch set which only works for normal epoll workload.
> I have one question about your patch set. It is about the newly added fields in
> struct file. I had tried to move these fields into struct eventpoll, so only epoll fds
> will be added into a disjoint set and the target fd will be linked to a disjoint set
> dynamically, but that method doesn't work out because there are multiple target fds and
> the order in which these target fds are added to a disjoint set will lead to deadlock.
> So do you have other ideas to reduce the size of these fields ?
> 
> Thanks,
> Tao

Hi,

I agree that increasing the size of struct file is no go. One idea is to
simply have a single pointer in 'struct file' for epoll usage, that
could be dynamically allocated when a file is added to an epoll set.
That would reduce the size of things from where we currently are.

I'm also not sure that the target fds (or non-epoll fds) need to be
added to the disjoint sets. The sets are to serialize loop checks of
which the target fds can not be a part of.

If there is interest in resurrecting this patchset, I can pick it back
up. I hadn't been pushing it forward because I wasn't convinced it
mattered on real workloads...

Thanks,

-Jason


> 
> The following lines are the result of performance result:
> 
> baseline: result for v4.14
> ep_cc: result for v4.14 + ep_cc patch set
> my: result for  v4.14 + my patch set
> 
> The first 15 columns are the results from wrk and their unit is Requests/sec,
> the lats two columns are their average value and standard derivation.
> 
> * normal scenario: nginx + wrk
> 
> baseline
> 376704.74 377223.88 378096.67 379484.45 379199.04 379526.21 378008.82 \
> 379959.98 377634.47 379127.27 377622.92 379442.12 378994.44 376000.08 377046.58 \
> 378271.4447 1210.673592
> 
> ep_cc
> 373376.08 376897.65 373493.65 376154.44 377374.36 379080.88 375124.44 \
> 376404.16 376539.28 375141.84 379075.32 377214.39 374524.52 375605.87 372167.8 \
> 375878.312 1983.190539
> 
> my
> 373067.96 372369 375317.61 375564.66 373841.88 373699.88 376802.91 369988.97 \
> 376080.45 371580.64 374265.34 376370.34 377033.75 372786.91 375521.61 \
> 374286.1273 2059.92093
> 
> 
> * one-fd-per-request scenario: nginx + wrk
> baseline
> 124509.77 106542.46 118074.46 111434.02 117628.9 108260.56 119037.91 \
> 114413.75 108706.82 116277.99 111588.4 118216.56 121267.63 110950.4 116455.43 \
> 114891.004 5168.30288
> 
> ep_cc
> 124209.45 118297.8 120258.1 122785.95 118744.64 118471.76 117621.07 \
> 117525.14 114797.14 115348.47 117154.02 117208.37 118618.92 118578.8 118945.52 \
> 118571.01 2437.050182
> 
> my
> 125090.99 121408.92 116444.09 120055.4 119399.24 114938.04 122889.56 \
> 114363.49 120934.15 119052.03 118060.47 129976.45 121108.08 114849.93 114810.64 \
> 119558.7653 4338.18401
> 
>> Thanks,
>>
>> -Jason
>>
>>
>>>> Signed-off-by: Jason Baron <jbaron@xxxxxxxxxx>
>>>> Cc: Alexander Viro <viro@xxxxxxxxxxxxxxxxxx>
>>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>>>> Cc: Salman Qazi <sqazi@xxxxxxxxxx>
>>>
>>> Acked-by: Davidlohr Bueso <dbueso@xxxxxxx>
>>>
>>>> ---
>>>> fs/eventpoll.c | 47 ++++++++++++++++++-----------------------------
>>>> 1 file changed, 18 insertions(+), 29 deletions(-)
>>>
>>> Yay for getting rid of some of the callback hell.
>>>
>>> Thanks,
>>> Davidlohr
>>
>> .
>>
>