Message-ID: <3c15b1fa-ec79-1f12-35dc-bbbe660bb94f@kernel.dk>
Date:   Mon, 7 Nov 2022 07:19:50 -0700
From:   Jens Axboe <axboe@...nel.dk>
To:     Willem de Bruijn <willemdebruijn.kernel@...il.com>
Cc:     linux-kernel@...r.kernel.org, netdev@...r.kernel.org
Subject: Re: [PATCHSET v3 0/5] Add support for epoll min_wait

On 11/7/22 6:25 AM, Willem de Bruijn wrote:
> On Sat, Nov 5, 2022 at 2:46 PM Jens Axboe <axboe@...nel.dk> wrote:
>>
>> On 11/5/22 12:05 PM, Willem de Bruijn wrote:
>>> On Sat, Nov 5, 2022 at 1:39 PM Jens Axboe <axboe@...nel.dk> wrote:
>>>>
>>>>>> FWIW, when adding nsec resolution I initially opted for an init-based
>>>>>> approach, passing a new flag to epoll_create1. Feedback then was that
>>>>>> it was odd to have one syscall affect the behavior of another. The
>>>>>> final version just added a new epoll_pwait2 with timespec.
>>>>>
>>>>> I'm fine with just doing a pure syscall variant too, it was my original
>>>>> plan. Only changed it to allow for easier experimentation and adoption,
>>>>> and based on the fact that most use cases would likely use a fixed value
>>>>> per context anyway.
>>>>>
>>>>> I think it'd be a shame to drop the ctl, unless there's strong arguments
>>>>> against it. I'm quite happy to add a syscall variant too, that's not a
>>>>> big deal and would be a minor addition. Patch 6 should probably cut out
>>>>> the ctl addition and leave that for a patch 7, and then a patch 8 for
>>>>> adding a syscall.
>>>> I split the ctl patch out from the core change, and then took a look at
>>>> doing a syscall variant too. But there are a few complications there...
>>>> It would seem to make the most sense to build this on top of the newest
>>>> epoll wait syscall, epoll_pwait2(). But we're already at the max number
>>>> of arguments there...
>>>>
>>>> Arguably pwait2 should've been converted to use some kind of versioned
>>>> struct instead. I'm going to take a stab at pwait3 with that kind of
>>>> interface.
>>>
>>> Don't convert to a syscall approach based solely on my feedback. It
>>> would be good to hear from others.
>>
>> It's not just based on your feedback, if you read the original cover
>> letter, then that is the question that is posed in terms of API - ctl to
>> modify it, new syscall, or both? So figured I should at least try and
>> see what the syscall would look like.
>>
>>> At a high level, I'm somewhat uncomfortable merging two syscalls for
>>> behavior that already works, just to save half the syscall overhead.
>>> There is no shortage of calls that may make some sense for a workload
>>> to merge. Is the quoted 6-7% cpu cycle reduction due to saving one
>>> SYSENTER/SYSEXIT (as the high resolution timer wake-up will be the
>>> same), or am I missing something more fundamental?
>>
>> No, it's not really related to saving a single syscall, and you'd
>> potentially save more than just one as well. If we look at the two
>> extremes of applications, one will be low load and you're handling
>> probably just 1 event per loop. Not really interesting. At the other
>> end, you're fully loaded, and by the time you check for events, you have
>> 'maxevents' (or close to) available. That obviously reduces system
>> calls, but more importantly, it also allows the application to get some
>> batching effects from processing these events.
>>
>> In the medium range, there's enough processing to react pretty quickly
>> to events coming in, and you then end up doing just 1 event (or close to
>> that). To overcome that, we have some applications that detect this
>> medium range and do an artificial sleep before calling epoll_wait().
>> That was a nice effiency win for them. But we can do this a lot more
>> efficiently in the kernel. That was the idea behind this, and the
>> initial results from TAO (which does that sleep hack) proved it to be
>> more than worthwhile. Syscall reduction is one thing, improved batching
>> another, and just as importanly is sleep+wakeup reductions.
> 
> Thanks for the context.
> 
> So this is akin to interrupt moderation in network interfaces. Would
> it make sense to wait for timeout or nr of events, whichever comes
> first, similar to rx_usecs/rx_frames. Instead of an unconditional
> sleep at the start.

There's no unconditional sleep at the start with my patches, not sure
where you are getting that from. You already have 'nr of events', that's
the maxevents being passed in. If nr_available >= maxevents, then no
sleep will take place. We did debate doing a minevents kind of thing as
well, but the time based metric is more usable.

-- 
Jens Axboe

Powered by blists - more mailing lists