| Message ID | 20240125225704.12781-1-jdamato@fastly.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Per epoll context busy poll support | expand |
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.
On Sat, Jan 27, 2024 at 11:20:51AM -0500, Willem de Bruijn wrote: > 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. Sure, that's a fair point. I think we could probably make reasonable arguments in both directions about the pros/cons of each approach. It's still not clear to me that a new syscall is the best way to go on this, and IMO it does not offer a clear advantage. I understand that part of the premise of your argument is that ioctls are not recommended, but in this particular case it seems like a good use case and there have been new ioctls added recently (at least according to git log). This makes me think that while their use is not recommended, they can serve a purpose in specific use cases. To me, this use case seems very fitting. More of a joke and I hate to mention this, but this setting is changing how io is done and it seems fitting that this done via an ioctl ;) > 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 definitely hear what you are saying; I think I'm still not convinced, but I am thinking it through. In my mind, all of the other busy poll settings are configured by setting options on the sockets using various SO_* options, which modify some state in the kernel. The existing system-wide busy poll sysctl also does this. It feels strange to me to diverge from that pattern just for epoll. In the case of epoll_pwait2 the addition of a new syscall is an approach that I think makes a lot of sense. The new system call is also probably better from an end-user usability perspective, as well. For busy poll, I don't see a clear reasoning why a new system call is better, but maybe I am still missing something. > 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/ Perhaps I am misunderstanding what you are suggesting, but I am opposed to hardcoding a value. If it is currently configurable system-wide and via SO_* options for other forms of busy poll, I think it should similarly be configurable for epoll busy poll. I may yet be convinced by the new syscall argument, but I don't think I'd agree on imposing a default. The value can be modified by other forms of busy poll and the goal of my changes are to: - make epoll-based busy poll per context - allow applications to configure (within reason) how epoll-based busy poll behaves, like they can do now with the existing SO_* options for other busy poll methods. > > 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. >
Joe Damato wrote: > On Sat, Jan 27, 2024 at 11:20:51AM -0500, Willem de Bruijn wrote: > > 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. > > Sure, that's a fair point. I think we could probably make reasonable > arguments in both directions about the pros/cons of each approach. > > It's still not clear to me that a new syscall is the best way to go on > this, and IMO it does not offer a clear advantage. I understand that part > of the premise of your argument is that ioctls are not recommended, but in > this particular case it seems like a good use case and there have been > new ioctls added recently (at least according to git log). > > This makes me think that while their use is not recommended, they can serve > a purpose in specific use cases. To me, this use case seems very fitting. > > More of a joke and I hate to mention this, but this setting is changing how > io is done and it seems fitting that this done via an ioctl ;) > > > 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 definitely hear what you are saying; I think I'm still not convinced, but > I am thinking it through. > > In my mind, all of the other busy poll settings are configured by setting > options on the sockets using various SO_* options, which modify some state > in the kernel. The existing system-wide busy poll sysctl also does this. It > feels strange to me to diverge from that pattern just for epoll. I think the stateful approach for read is because there we do want to support all variants: read, readv, recv, recvfrom, recvmsg, recvmmsg. So there is no way to pass it directly. That said, I don't mean to argue strenously for this API or against yours. Want to make sure the option space is explored. There does not seem to be much other feedback. I don't hold a strong opinion either. > In the case of epoll_pwait2 the addition of a new syscall is an approach > that I think makes a lot of sense. The new system call is also probably > better from an end-user usability perspective, as well. For busy poll, I > don't see a clear reasoning why a new system call is better, but maybe I am > still missing something. > > > 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/ > > Perhaps I am misunderstanding what you are suggesting, but I am opposed to > hardcoding a value. If it is currently configurable system-wide and via > SO_* options for other forms of busy poll, I think it should similarly be > configurable for epoll busy poll. > > I may yet be convinced by the new syscall argument, but I don't think I'd > agree on imposing a default. The value can be modified by other forms of > busy poll and the goal of my changes are to: > - make epoll-based busy poll per context > - allow applications to configure (within reason) how epoll-based busy > poll behaves, like they can do now with the existing SO_* options for > other busy poll methods. Okay. I expected some push back. Was curious if people would come back with examples of where the full range is actually being used. > > > 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. > >
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 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. - previously added epoll_pwait2 helped to bring epoll closer to existing syscalls (like pselect and ppoll) and this busy poll change reflected as a new syscall would not have the same effect. Note: patch 1/4 uses an xor so that busy poll is only enabled if the per-context busy poll usecs is set or the system-wide sysctl. If both are enabled, busy polling does not happen. Calling this out specifically incase there are strong feelings about this one; I felt one xor the other made sense, but I am open to changing it. Longer explanation: Presently epoll has support for a very useful form of busy poll based on the incoming NAPI ID (see also: SO_INCOMING_NAPI_ID [2]). This form of busy poll allows epoll_wait to drive NAPI packet processing which allows for a few interesting user application designs which can reduce latency and also potentially improve L2/L3 cache hit rates by deferring NAPI until userland has finished its work. The documentation available on this is, IMHO, a bit confusing so please allow me to explain how one might use this: 1. Ensure each application thread has its own epoll instance mapping 1-to-1 with NIC RX queues. An n-tuple filter would likely be used to direct connections with specific dest ports to these queues. 2. Optionally: Setup IRQ coalescing for the NIC RX queues where busy polling will occur. This can help avoid the userland app from being pre-empted by a hard IRQ while userland is running. Note this means that userland must take care to call epoll_wait and not take too long in userland since it now drives NAPI via epoll_wait. 3. Optionally: Consider using napi_defer_hard_irqs and gro_flush_timeout to further restrict IRQ generation from the NIC. These settings are system-wide so their impact must be carefully weighed against the running applications. 4. Ensure that all incoming connections added to an epoll instance have the same NAPI ID. This can be done with a BPF filter when SO_REUSEPORT is used or getsockopt + SO_INCOMING_NAPI_ID when a single accept thread is used which dispatches incoming connections to threads. 5. Lastly, busy poll must be enabled via a sysctl (/proc/sys/net/core/busy_poll). Please see Eric Dumazet's paper about busy polling [3] and a recent academic paper about measured performance improvements of busy polling [4] (albeit with a modification that is not currently present in the kernel) for additional context. The unfortunate part about step 5 above is that this enables busy poll system-wide which affects all user applications on the system, including epoll-based network applications which were not intended to be used this way or applications where increased CPU usage for lower latency network processing is unnecessary or not desirable. If the user wants to run one low latency epoll-based server application with epoll-based busy poll, but would like to run the rest of the applications on the system (which may also use epoll) without busy poll, this system-wide sysctl presents a significant problem. This change preserves the system-wide sysctl, but adds a mechanism (via ioctl) to enable or disable busy poll for epoll contexts as needed by individual applications, making epoll-based busy poll more usable. Note that this change includes an xor allowing only the per-context busy poll or the system wide sysctl, not both. If both are enabled, busy polling does not happen. Calling this out specifically incase there are strong feelings about this one; I felt one xor the other made sense, but I am open to changing it. Thanks, Joe v2 -> v3: - cover letter updated to mention why ioctl seems (to me) like a better choice vs a new syscall. - patch 3/4 was modified in 3 ways: - when an unknown ioctl is received, -ENOIOCTLCMD is returned instead of -EINVAL as the ioctl documentation requires. - epoll_params.busy_poll_budget can only be set to a value larger than NAPI_POLL_WEIGHT if code is run by privileged (CAP_NET_ADMIN) users. Otherwise, -EPERM is returned. - busy poll specific ioctl code moved out to its own function. On kernels without busy poll support, -EOPNOTSUPP is returned. This also makes the kernel build robot happier without littering the code with more #ifdefs. - dropped patch 4/4 after Eric Dumazet's review of it when it was sent independently to the list [5]. v1 -> v2: - cover letter updated to make a mention of napi_defer_hard_irqs and gro_flush_timeout as an added step 3 and to cite both Eric Dumazet's busy polling paper and a paper from University of Waterloo for additional context. Specifically calling out the xor in patch 1/4 incase it is missed by reviewers. - Patch 2/4 has its commit message updated, but no functional changes. Commit message now describes that allowing for a settable budget helps to improve throughput and is more consistent with other busy poll mechanisms that allow a settable budget via SO_BUSY_POLL_BUDGET. - Patch 3/4 was modified to check if the epoll_params.busy_poll_budget exceeds NAPI_POLL_WEIGHT. The larger value is allowed, but an error is printed. This was done for consistency with netif_napi_add_weight, which does the same. - Patch 3/4 the struct epoll_params was updated to fix the type of the data field; it was uint8_t and was changed to u8. - Patch 4/4 added to check if SO_BUSY_POLL_BUDGET exceeds NAPI_POLL_WEIGHT. The larger value is allowed, but an error is printed. This was done for consistency with netif_napi_add_weight, which does the same. [1]: https://lore.kernel.org/lkml/65b1cb7f73a6a_250560294bd@willemb.c.googlers.com.notmuch/ [2]: https://lore.kernel.org/lkml/20170324170836.15226.87178.stgit@localhost.localdomain/ [3]: https://netdevconf.info/2.1/papers/BusyPollingNextGen.pdf [4]: https://dl.acm.org/doi/pdf/10.1145/3626780 [5]: https://lore.kernel.org/lkml/CANn89i+uXsdSVFiQT9fDfGw+h_5QOcuHwPdWi9J=5U6oLXkQTA@mail.gmail.com/ Joe Damato (3): eventpoll: support busy poll per epoll instance eventpoll: Add per-epoll busy poll packet budget eventpoll: Add epoll ioctl for epoll_params .../userspace-api/ioctl/ioctl-number.rst | 1 + fs/eventpoll.c | 122 +++++++++++++++++- include/uapi/linux/eventpoll.h | 12 ++ 3 files changed, 130 insertions(+), 5 deletions(-)