Received-SPF: pass (google.com: domain of linux-kernel+bounces-41301-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.80.249 as permitted sender) client-ip=147.75.80.249;
Date: Sat, 27 Jan 2024 11:20:51 -0500
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
To: Joe Damato <jdamato@fastly.com>, 
 linux-kernel@vger.kernel.org, 
 netdev@vger.kernel.org
Cc: chuck.lever@oracle.com, 
 jlayton@kernel.org, 
 linux-api@vger.kernel.org, 
 brauner@kernel.org, 
 edumazet@google.com, 
 davem@davemloft.net, 
 alexander.duyck@gmail.com, 
 sridhar.samudrala@intel.com, 
 kuba@kernel.org, 
 willemdebruijn.kernel@gmail.com, 
 weiwan@google.com, 
 Joe Damato <jdamato@fastly.com>, 
 Alexander Viro <viro@zeniv.linux.org.uk>, 
 Andrew Waterman <waterman@eecs.berkeley.edu>, 
 Arnd Bergmann <arnd@arndb.de>, 
 Dominik Brodowski <linux@dominikbrodowski.net>, 
 Greg Kroah-Hartman <gregkh@linuxfoundation.org>, 
 Jan Kara <jack@suse.cz>, 
 Jiri Slaby <jirislaby@kernel.org>, 
 Jonathan Corbet <corbet@lwn.net>, 
 Julien Panis <jpanis@baylibre.com>, 
 linux-doc@vger.kernel.org (open list:DOCUMENTATION), 
 "(open list:FILESYSTEMS \\(VFS and infrastructure\\))" <linux-fsdevel@vger.kernel.org>, 
 Michael Ellerman <mpe@ellerman.id.au>, 
 Nathan Lynch <nathanl@linux.ibm.com> (open list:FILESYSTEMS \(VFS and infrastructure\)), 
 Palmer Dabbelt <palmer@dabbelt.com>, 
 Steve French <stfrench@microsoft.com>, 
 Thomas Huth <thuth@redhat.com>, 
 Thomas Zimmermann <tzimmermann@suse.de>
Message-ID: <65b52d6381de7_3a9e0b2943d@willemb.c.googlers.com.notmuch>
In-Reply-To: <20240125225704.12781-1-jdamato@fastly.com>
References: <20240125225704.12781-1-jdamato@fastly.com>
Subject: Re: [PATCH net-next v3 0/3] Per epoll context busy poll support
Precedence: bulk
Mime-Version: 1.0
Content-Type: text/plain;
 charset=utf-8
Content-Transfer-Encoding: 7bit

Joe Damato wrote:
> Greetings:
> 
> Welcome to v3. Cover letter updated from v2 to explain why ioctl and
> adjusted my cc_cmd to try to get the correct people in addition to folks
> who were added in v1 & v2. Labeled as net-next because it seems networking
> related to me even though it is fs code.
> 
> TL;DR This builds on commit bf3b9f6372c4 ("epoll: Add busy poll support to
> epoll with socket fds.") by allowing user applications to enable
> epoll-based busy polling and set a busy poll packet budget on a per epoll
> context basis.
> 
> This makes epoll-based busy polling much more usable for user
> applications than the current system-wide sysctl and hardcoded budget.
> 
> To allow for this, two ioctls have been added for epoll contexts for
> getting and setting a new struct, struct epoll_params.
> 
> ioctl was chosen vs a new syscall after reviewing a suggestion by Willem
> de Bruijn [1]. I am open to using a new syscall instead of an ioctl, but it
> seemed that: 
>   - Busy poll affects all existing epoll_wait and epoll_pwait variants in
>     the same way, so new verions of many syscalls might be needed. It

There is no need to support a new feature on legacy calls. Applications have
to be upgraded to the new ioctl, so they can also be upgraded to the latest
epoll_wait variant.

epoll_pwait extends epoll_wait with a sigmask.
epoll_pwait2 extends extends epoll_pwait with nsec resolution timespec.
Since they are supersets, nothing is lots by limiting to the most recent API.

In the discussion of epoll_pwait2 the addition of a forward looking flags
argument was discussed, but eventually dropped. Based on the argument that
adding a syscall is not a big task and does not warrant preemptive code.
This decision did receive a suitably snarky comment from Jonathan Corbet [1].

It is definitely more boilerplate, but essentially it is as feasible to add an
epoll_pwait3 that takes an optional busy poll argument. In which case, I also
believe that it makes more sense to configure the behavior of the syscall
directly, than through another syscall and state stored in the kernel.

I don't think that the usec fine grain busy poll argument is all that useful.
Documentation always suggests setting it to 50us or 100us, based on limited
data. Main point is to set it to exceed the round-trip delay of whatever the
process is trying to wait on. Overestimating is not costly, as the call
returns as soon as the condition is met. An epoll_pwait3 flag EPOLL_BUSY_POLL
with default 100us might be sufficient.

[1] https://lwn.net/Articles/837816/


>     seems much simpler for users to use the correct
>     epoll_wait/epoll_pwait for their app and add a call to ioctl to enable
>     or disable busy poll as needed. This also probably means less work to
>     get an existing epoll app using busy poll.