[PATCH net-next v6 0/4] Per epoll context busy poll support

[PATCH net-next v6 0/4] Per epoll context busy poll support

[Joe Damato]

Greetings:

Welcome to v6.

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, set a busy poll packet budget, and enable or
disable prefer busy poll 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 as of v4 uses an or (||) instead of an xor. I thought about
it some more and I realized that if the user enables both the per-epoll
context setting and the system wide sysctl, then busy poll should be
enabled and not disabled. Using xor doesn't seem to make much sense after
thinking through this a bit.

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 or (as of v4) instead of an xor. If the
user has enabled both the system-wide sysctl and also the per epoll-context
busy poll settings, then epoll should probably busy poll (vs being
disabled). 

Thanks,
Joe

v5 -> v6:
  - patch 1/3 no functional change, but commit message corrected to explain
    that an or (||) is being used instead of xor.

  - patch 3/4 is a new patch which adds support for per epoll context
    prefer busy poll setting.

  - patch 4/4 updated to allow getting/setting per epoll context prefer
    busy poll setting; this setting is limited to either 0 or 1.

v4 -> v5:
  - patch 3/3 updated to use memchr_inv to ensure that __pad is zero for
    the EPIOCSPARAMS ioctl. Recommended by Greg K-H [5], Dave Chinner [6],
    and Jiri Slaby [7].

v3 -> v4:
  - patch 1/3 was updated to include an important functional change:
    ep_busy_loop_on was updated to use or (||) instead of xor (^). After
    thinking about it a bit more, I thought xor didn't make much sense.
    Enabling both the per-epoll context and the system-wide sysctl should
    probably enable busy poll, not disable it. So, or (||) makes more
    sense, I think.

  - patch 3/3 was updated:
    - to change the epoll_params fields to be __u64, __u16, and __u8 and
      to pad the struct to a multiple of 64bits. Suggested by Greg K-H [8]
      and Arnd Bergmann [9].
    - remove an unused pr_fmt, left over from the previous revision.
    - ioctl now returns -EINVAL when epoll_params.busy_poll_usecs >
      U32_MAX.

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 [10].

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/2024013001-prison-strum-899d@gregkh/
[6]: https://lore.kernel.org/lkml/Zbm3AXgcwL9D6TNM@dread.disaster.area/
[7]: https://lore.kernel.org/lkml/efee9789-4f05-4202-9a95-21d88f6307b0@kernel.org/
[8]: https://lore.kernel.org/lkml/2024012551-anyone-demeaning-867b@gregkh/
[9]: https://lore.kernel.org/lkml/57b62135-2159-493d-a6bb-47d5be55154a@app.fastmail.com/
[10]: https://lore.kernel.org/lkml/CANn89i+uXsdSVFiQT9fDfGw+h_5QOcuHwPdWi9J=5U6oLXkQTA@mail.gmail.com/

Joe Damato (4):
  eventpoll: support busy poll per epoll instance
  eventpoll: Add per-epoll busy poll packet budget
  eventpoll: Add per-epoll prefer busy poll option
  eventpoll: Add epoll ioctl for epoll_params

 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 fs/eventpoll.c                                | 136 +++++++++++++++++-
 include/uapi/linux/eventpoll.h                |  13 ++
 3 files changed, 144 insertions(+), 6 deletions(-)

-- 
2.25.1

[PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance

[Joe Damato]

Allow busy polling on a per-epoll context basis. The per-epoll context
usec timeout value is preferred, but the pre-existing system wide sysctl
value is still supported if it specified.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 fs/eventpoll.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 3534d36a1474..ce75189d46df 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -227,6 +227,8 @@ struct eventpoll {
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	/* used to track busy poll napi_id */
 	unsigned int napi_id;
+	/* busy poll timeout */
+	u64 busy_poll_usecs;
 #endif
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -386,12 +388,44 @@ static inline int ep_events_available(struct eventpoll *ep)
 		READ_ONCE(ep->ovflist) != EP_UNACTIVE_PTR;
 }
 
+/**
+ * busy_loop_ep_timeout - check if busy poll has timed out. The timeout value
+ * from the epoll instance ep is preferred, but if it is not set fallback to
+ * the system-wide global via busy_loop_timeout.
+ *
+ * @start_time: The start time used to compute the remaining time until timeout.
+ * @ep: Pointer to the eventpoll context.
+ *
+ * Return: true if the timeout has expired, false otherwise.
+ */
+static inline bool busy_loop_ep_timeout(unsigned long start_time, struct eventpoll *ep)
+{
+#ifdef CONFIG_NET_RX_BUSY_POLL
+	unsigned long bp_usec = READ_ONCE(ep->busy_poll_usecs);
+
+	if (bp_usec) {
+		unsigned long end_time = start_time + bp_usec;
+		unsigned long now = busy_loop_current_time();
+
+		return time_after(now, end_time);
+	} else {
+		return busy_loop_timeout(start_time);
+	}
+#endif
+	return true;
+}
+
 #ifdef CONFIG_NET_RX_BUSY_POLL
+static bool ep_busy_loop_on(struct eventpoll *ep)
+{
+	return !!ep->busy_poll_usecs || net_busy_loop_on();
+}
+
 static bool ep_busy_loop_end(void *p, unsigned long start_time)
 {
 	struct eventpoll *ep = p;
 
-	return ep_events_available(ep) || busy_loop_timeout(start_time);
+	return ep_events_available(ep) || busy_loop_ep_timeout(start_time, ep);
 }
 
 /*
@@ -404,7 +438,7 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
 {
 	unsigned int napi_id = READ_ONCE(ep->napi_id);
 
-	if ((napi_id >= MIN_NAPI_ID) && net_busy_loop_on()) {
+	if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
 		napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
 			       BUSY_POLL_BUDGET);
 		if (ep_events_available(ep))
@@ -430,7 +464,8 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
 	struct socket *sock;
 	struct sock *sk;
 
-	if (!net_busy_loop_on())
+	ep = epi->ep;
+	if (!ep_busy_loop_on(ep))
 		return;
 
 	sock = sock_from_file(epi->ffd.file);
@@ -442,7 +477,6 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
 		return;
 
 	napi_id = READ_ONCE(sk->sk_napi_id);
-	ep = epi->ep;
 
 	/* Non-NAPI IDs can be rejected
 	 *	or
@@ -466,6 +500,10 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
 {
 }
 
+static inline bool ep_busy_loop_on(struct eventpoll *ep)
+{
+	return false;
+}
 #endif /* CONFIG_NET_RX_BUSY_POLL */
 
 /*
@@ -2058,6 +2096,9 @@ static int do_epoll_create(int flags)
 		error = PTR_ERR(file);
 		goto out_free_fd;
 	}
+#ifdef CONFIG_NET_RX_BUSY_POLL
+	ep->busy_poll_usecs = 0;
+#endif
 	ep->file = file;
 	fd_install(fd, file);
 	return fd;
-- 
2.25.1

[PATCH net-next v6 2/4] eventpoll: Add per-epoll busy poll packet budget

[Joe Damato]

When using epoll-based busy poll, the packet budget is hardcoded to
BUSY_POLL_BUDGET (8). Users may desire larger busy poll budgets, which
can potentially increase throughput when busy polling under high network
load.

Other busy poll methods allow setting the busy poll budget via
SO_BUSY_POLL_BUDGET, but epoll-based busy polling uses a hardcoded
value.

Fix this edge case by adding support for a per-epoll context busy poll
packet budget. If not specified, the default value (BUSY_POLL_BUDGET) is
used.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 fs/eventpoll.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index ce75189d46df..3985434df527 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -229,6 +229,8 @@ struct eventpoll {
 	unsigned int napi_id;
 	/* busy poll timeout */
 	u64 busy_poll_usecs;
+	/* busy poll packet budget */
+	u16 busy_poll_budget;
 #endif
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -437,10 +439,14 @@ static bool ep_busy_loop_end(void *p, unsigned long start_time)
 static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
 {
 	unsigned int napi_id = READ_ONCE(ep->napi_id);
+	u16 budget = READ_ONCE(ep->busy_poll_budget);
+
+	if (!budget)
+		budget = BUSY_POLL_BUDGET;
 
 	if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
 		napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
-			       BUSY_POLL_BUDGET);
+			       budget);
 		if (ep_events_available(ep))
 			return true;
 		/*
@@ -2098,6 +2104,7 @@ static int do_epoll_create(int flags)
 	}
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	ep->busy_poll_usecs = 0;
+	ep->busy_poll_budget = 0;
 #endif
 	ep->file = file;
 	fd_install(fd, file);
-- 
2.25.1

[PATCH net-next v6 3/4] eventpoll: Add per-epoll prefer busy poll option

[Joe Damato]

When using epoll-based busy poll, the prefer_busy_poll option is hardcoded
to false. Users may want to enable prefer_busy_poll to be used in
conjunction with gro_flush_timeout and defer_hard_irqs_count to keep device
IRQs masked.

Other busy poll methods allow enabling or disabling prefer busy poll via
SO_PREFER_BUSY_POLL, but epoll-based busy polling uses a hardcoded value.

Fix this edge case by adding support for a per-epoll context
prefer_busy_poll option. The default is false, as it was hardcoded before
this change.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 fs/eventpoll.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 3985434df527..a69ee11682b9 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -231,6 +231,7 @@ struct eventpoll {
 	u64 busy_poll_usecs;
 	/* busy poll packet budget */
 	u16 busy_poll_budget;
+	bool prefer_busy_poll;
 #endif
 
 #ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -440,13 +441,14 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
 {
 	unsigned int napi_id = READ_ONCE(ep->napi_id);
 	u16 budget = READ_ONCE(ep->busy_poll_budget);
+	bool prefer_busy_poll = READ_ONCE(ep->prefer_busy_poll);
 
 	if (!budget)
 		budget = BUSY_POLL_BUDGET;
 
 	if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
-		napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
-			       budget);
+		napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end,
+				ep, prefer_busy_poll, budget);
 		if (ep_events_available(ep))
 			return true;
 		/*
@@ -2105,6 +2107,7 @@ static int do_epoll_create(int flags)
 #ifdef CONFIG_NET_RX_BUSY_POLL
 	ep->busy_poll_usecs = 0;
 	ep->busy_poll_budget = 0;
+	ep->prefer_busy_poll = false;
 #endif
 	ep->file = file;
 	fd_install(fd, file);
-- 
2.25.1

[PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params

[Joe Damato]

Add an ioctl for getting and setting epoll_params. User programs can use
this ioctl to get and set the busy poll usec time, packet budget, and
prefer busy poll params for a specific epoll context.

Parameters are limited:
  - busy_poll_usecs is limited to <= u32_max
  - busy_poll_budget is limited to <= NAPI_POLL_WEIGHT by unprivileged
    users (!capable(CAP_NET_ADMIN))
  - prefer_busy_poll must be 0 or 1
  - __pad must be 0

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 .../userspace-api/ioctl/ioctl-number.rst      |  1 +
 fs/eventpoll.c                                | 73 +++++++++++++++++++
 include/uapi/linux/eventpoll.h                | 13 ++++
 3 files changed, 87 insertions(+)

diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 457e16f06e04..b33918232f78 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -309,6 +309,7 @@ Code  Seq#    Include File                                           Comments
 0x89  0B-DF  linux/sockios.h
 0x89  E0-EF  linux/sockios.h                                         SIOCPROTOPRIVATE range
 0x89  F0-FF  linux/sockios.h                                         SIOCDEVPRIVATE range
+0x8A  00-1F  linux/eventpoll.h
 0x8B  all    linux/wireless.h
 0x8C  00-3F                                                          WiNRADiO driver
                                                                      <http://www.winradio.com.au/>
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index a69ee11682b9..8eb4ea2557af 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -37,6 +37,7 @@
 #include <linux/seq_file.h>
 #include <linux/compat.h>
 #include <linux/rculist.h>
+#include <linux/capability.h>
 #include <net/busy_poll.h>
 
 /*
@@ -497,6 +498,50 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
 	ep->napi_id = napi_id;
 }
 
+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
+				  unsigned long arg)
+{
+	struct eventpoll *ep;
+	struct epoll_params epoll_params;
+	void __user *uarg = (void __user *) arg;
+
+	ep = file->private_data;
+
+	switch (cmd) {
+	case EPIOCSPARAMS:
+		if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
+			return -EFAULT;
+
+		if (memchr_inv(epoll_params.__pad, 0, sizeof(epoll_params.__pad)))
+			return -EINVAL;
+
+		if (epoll_params.busy_poll_usecs > U32_MAX)
+			return -EINVAL;
+
+		if (epoll_params.prefer_busy_poll > 1)
+			return -EINVAL;
+
+		if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
+		    !capable(CAP_NET_ADMIN))
+			return -EPERM;
+
+		ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
+		ep->busy_poll_budget = epoll_params.busy_poll_budget;
+		ep->prefer_busy_poll = !!epoll_params.prefer_busy_poll;
+		return 0;
+	case EPIOCGPARAMS:
+		memset(&epoll_params, 0, sizeof(epoll_params));
+		epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
+		epoll_params.busy_poll_budget = ep->busy_poll_budget;
+		epoll_params.prefer_busy_poll = ep->prefer_busy_poll;
+		if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
+			return -EFAULT;
+		return 0;
+	default:
+		return -ENOIOCTLCMD;
+	}
+}
+
 #else
 
 static inline bool ep_busy_loop(struct eventpoll *ep, int nonblock)
@@ -512,6 +557,12 @@ static inline bool ep_busy_loop_on(struct eventpoll *ep)
 {
 	return false;
 }
+
+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
+				  unsigned long arg)
+{
+	return -EOPNOTSUPP;
+}
 #endif /* CONFIG_NET_RX_BUSY_POLL */
 
 /*
@@ -871,6 +922,26 @@ static void ep_clear_and_put(struct eventpoll *ep)
 		ep_free(ep);
 }
 
+static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	int ret;
+
+	if (!is_file_epoll(file))
+		return -EINVAL;
+
+	switch (cmd) {
+	case EPIOCSPARAMS:
+	case EPIOCGPARAMS:
+		ret = ep_eventpoll_bp_ioctl(file, cmd, arg);
+		break;
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
 static int ep_eventpoll_release(struct inode *inode, struct file *file)
 {
 	struct eventpoll *ep = file->private_data;
@@ -977,6 +1048,8 @@ static const struct file_operations eventpoll_fops = {
 	.release	= ep_eventpoll_release,
 	.poll		= ep_eventpoll_poll,
 	.llseek		= noop_llseek,
+	.unlocked_ioctl	= ep_eventpoll_ioctl,
+	.compat_ioctl   = compat_ptr_ioctl,
 };
 
 /*
diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h
index cfbcc4cc49ac..36a002660955 100644
--- a/include/uapi/linux/eventpoll.h
+++ b/include/uapi/linux/eventpoll.h
@@ -85,4 +85,17 @@ struct epoll_event {
 	__u64 data;
 } EPOLL_PACKED;
 
+struct epoll_params {
+	__aligned_u64 busy_poll_usecs;
+	__u16 busy_poll_budget;
+	__u8 prefer_busy_poll;
+
+	/* pad the struct to a multiple of 64bits for alignment on all arches */
+	__u8 __pad[5];
+};
+
+#define EPOLL_IOC_TYPE 0x8A
+#define EPIOCSPARAMS _IOW(EPOLL_IOC_TYPE, 0x01, struct epoll_params)
+#define EPIOCGPARAMS _IOR(EPOLL_IOC_TYPE, 0x02, struct epoll_params)
+
 #endif /* _UAPI_LINUX_EVENTPOLL_H */
-- 
2.25.1

Re: [PATCH net-next v6 0/4] Per epoll context busy poll support

[Stanislav Fomichev]

On 02/05, Joe Damato wrote:
> Greetings:
> 
> Welcome to v6.
> 
> 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, set a busy poll packet budget, and enable or
> disable prefer busy poll 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 as of v4 uses an or (||) instead of an xor. I thought about
> it some more and I realized that if the user enables both the per-epoll
> context setting and the system wide sysctl, then busy poll should be
> enabled and not disabled. Using xor doesn't seem to make much sense after
> thinking through this a bit.
> 
> 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 or (as of v4) instead of an xor. If the
> user has enabled both the system-wide sysctl and also the per epoll-context
> busy poll settings, then epoll should probably busy poll (vs being
> disabled). 
> 
> Thanks,
> Joe
> 
> v5 -> v6:
>   - patch 1/3 no functional change, but commit message corrected to explain
>     that an or (||) is being used instead of xor.
> 
>   - patch 3/4 is a new patch which adds support for per epoll context
>     prefer busy poll setting.
> 
>   - patch 4/4 updated to allow getting/setting per epoll context prefer
>     busy poll setting; this setting is limited to either 0 or 1.
> 
> v4 -> v5:
>   - patch 3/3 updated to use memchr_inv to ensure that __pad is zero for
>     the EPIOCSPARAMS ioctl. Recommended by Greg K-H [5], Dave Chinner [6],
>     and Jiri Slaby [7].
> 
> v3 -> v4:
>   - patch 1/3 was updated to include an important functional change:
>     ep_busy_loop_on was updated to use or (||) instead of xor (^). After
>     thinking about it a bit more, I thought xor didn't make much sense.
>     Enabling both the per-epoll context and the system-wide sysctl should
>     probably enable busy poll, not disable it. So, or (||) makes more
>     sense, I think.
> 
>   - patch 3/3 was updated:
>     - to change the epoll_params fields to be __u64, __u16, and __u8 and
>       to pad the struct to a multiple of 64bits. Suggested by Greg K-H [8]
>       and Arnd Bergmann [9].
>     - remove an unused pr_fmt, left over from the previous revision.
>     - ioctl now returns -EINVAL when epoll_params.busy_poll_usecs >
>       U32_MAX.
> 
> 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 [10].
> 
> 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/2024013001-prison-strum-899d@gregkh/
> [6]: https://lore.kernel.org/lkml/Zbm3AXgcwL9D6TNM@dread.disaster.area/
> [7]: https://lore.kernel.org/lkml/efee9789-4f05-4202-9a95-21d88f6307b0@kernel.org/
> [8]: https://lore.kernel.org/lkml/2024012551-anyone-demeaning-867b@gregkh/
> [9]: https://lore.kernel.org/lkml/57b62135-2159-493d-a6bb-47d5be55154a@app.fastmail.com/
> [10]: https://lore.kernel.org/lkml/CANn89i+uXsdSVFiQT9fDfGw+h_5QOcuHwPdWi9J=5U6oLXkQTA@mail.gmail.com/
> 
> Joe Damato (4):
>   eventpoll: support busy poll per epoll instance
>   eventpoll: Add per-epoll busy poll packet budget
>   eventpoll: Add per-epoll prefer busy poll option
>   eventpoll: Add epoll ioctl for epoll_params
> 
>  .../userspace-api/ioctl/ioctl-number.rst      |   1 +
>  fs/eventpoll.c                                | 136 +++++++++++++++++-
>  include/uapi/linux/eventpoll.h                |  13 ++
>  3 files changed, 144 insertions(+), 6 deletions(-)

Coincidentally, we were looking into the same area and your patches are
super useful :-) Thank you for plumbing in prefer_busy_poll. 

Acked-by: Stanislav Fomichev <sdf@google.com>

Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params

[Jiri Slaby]

On 05. 02. 24, 22:04, Joe Damato wrote:
> Add an ioctl for getting and setting epoll_params. User programs can use
> this ioctl to get and set the busy poll usec time, packet budget, and
> prefer busy poll params for a specific epoll context.
> 
> Parameters are limited:
>    - busy_poll_usecs is limited to <= u32_max
>    - busy_poll_budget is limited to <= NAPI_POLL_WEIGHT by unprivileged
>      users (!capable(CAP_NET_ADMIN))
>    - prefer_busy_poll must be 0 or 1
>    - __pad must be 0
> 
> Signed-off-by: Joe Damato <jdamato@fastly.com>
...
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
...
> @@ -497,6 +498,50 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
>   	ep->napi_id = napi_id;
>   }
>   
> +static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
> +				  unsigned long arg)
> +{
> +	struct eventpoll *ep;
> +	struct epoll_params epoll_params;
> +	void __user *uarg = (void __user *) arg;
> +
> +	ep = file->private_data;

This might have been on the ep declaration line.

> +	switch (cmd) {
> +	case EPIOCSPARAMS:
> +		if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> +			return -EFAULT;
> +
> +		if (memchr_inv(epoll_params.__pad, 0, sizeof(epoll_params.__pad)))
> +			return -EINVAL;
> +
> +		if (epoll_params.busy_poll_usecs > U32_MAX)
> +			return -EINVAL;
> +
> +		if (epoll_params.prefer_busy_poll > 1)
> +			return -EINVAL;
> +
> +		if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
> +		    !capable(CAP_NET_ADMIN))
> +			return -EPERM;
> +
> +		ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
> +		ep->busy_poll_budget = epoll_params.busy_poll_budget;
> +		ep->prefer_busy_poll = !!epoll_params.prefer_busy_poll;

This !! is unnecessary. Nonzero values shall be "converted" to true.

But FWIW, the above is nothing which should be blocking, so:

Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

> +		return 0;
> +	case EPIOCGPARAMS:
> +		memset(&epoll_params, 0, sizeof(epoll_params));
> +		epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
> +		epoll_params.busy_poll_budget = ep->busy_poll_budget;
> +		epoll_params.prefer_busy_poll = ep->prefer_busy_poll;
> +		if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
> +			return -EFAULT;
> +		return 0;
> +	default:
> +		return -ENOIOCTLCMD;
> +	}
> +}
...
thanks,
-- 
js
suse labs

Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params

[Joe Damato]

On Wed, Feb 07, 2024 at 09:37:14AM +0100, Jiri Slaby wrote:
> On 05. 02. 24, 22:04, Joe Damato wrote:
> >Add an ioctl for getting and setting epoll_params. User programs can use
> >this ioctl to get and set the busy poll usec time, packet budget, and
> >prefer busy poll params for a specific epoll context.
> >
> >Parameters are limited:
> >   - busy_poll_usecs is limited to <= u32_max
> >   - busy_poll_budget is limited to <= NAPI_POLL_WEIGHT by unprivileged
> >     users (!capable(CAP_NET_ADMIN))
> >   - prefer_busy_poll must be 0 or 1
> >   - __pad must be 0
> >
> >Signed-off-by: Joe Damato <jdamato@fastly.com>
> ...
> >--- a/fs/eventpoll.c
> >+++ b/fs/eventpoll.c
> ...
> >@@ -497,6 +498,50 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> >  	ep->napi_id = napi_id;
> >  }
> >+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
> >+				  unsigned long arg)
> >+{
> >+	struct eventpoll *ep;
> >+	struct epoll_params epoll_params;
> >+	void __user *uarg = (void __user *) arg;
> >+
> >+	ep = file->private_data;
> 
> This might have been on the ep declaration line.
> 
> >+	switch (cmd) {
> >+	case EPIOCSPARAMS:
> >+		if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> >+			return -EFAULT;
> >+
> >+		if (memchr_inv(epoll_params.__pad, 0, sizeof(epoll_params.__pad)))
> >+			return -EINVAL;
> >+
> >+		if (epoll_params.busy_poll_usecs > U32_MAX)
> >+			return -EINVAL;
> >+
> >+		if (epoll_params.prefer_busy_poll > 1)
> >+			return -EINVAL;
> >+
> >+		if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
> >+		    !capable(CAP_NET_ADMIN))
> >+			return -EPERM;
> >+
> >+		ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
> >+		ep->busy_poll_budget = epoll_params.busy_poll_budget;
> >+		ep->prefer_busy_poll = !!epoll_params.prefer_busy_poll;
> 
> This !! is unnecessary. Nonzero values shall be "converted" to true.
> 
> But FWIW, the above is nothing which should be blocking, so:
"> 
> Reviewed-by: Jiri Slaby <jirislaby@kernel.org>

netdev maintainers: Jiri marked this with Reviewed-by, but was this review
what caused "Changes Requested" to be the status set for this patch set in
patchwork?

If needed, I'll send a v7 with the changes Jiri suggested and add the
"Reviewed-by" since the changes are cosmetic, but I wanted to make sure
this was the reason.

Thanks.

Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance

[Jakub Kicinski]

On Mon,  5 Feb 2024 21:04:46 +0000 Joe Damato wrote:
> Allow busy polling on a per-epoll context basis. The per-epoll context
> usec timeout value is preferred, but the pre-existing system wide sysctl
> value is still supported if it specified.

Why do we need u64 for usecs? I think u16 would do, and u32 would give
a very solid "engineering margin". If it was discussed in previous
versions I think it's worth explaining in the commit message.

Re: [PATCH net-next v6 2/4] eventpoll: Add per-epoll busy poll packet budget

[Jakub Kicinski]

On Mon,  5 Feb 2024 21:04:47 +0000 Joe Damato wrote:
> When using epoll-based busy poll, the packet budget is hardcoded to
> BUSY_POLL_BUDGET (8). Users may desire larger busy poll budgets, which
> can potentially increase throughput when busy polling under high network
> load.
> 
> Other busy poll methods allow setting the busy poll budget via
> SO_BUSY_POLL_BUDGET, but epoll-based busy polling uses a hardcoded
> value.
> 
> Fix this edge case by adding support for a per-epoll context busy poll
> packet budget. If not specified, the default value (BUSY_POLL_BUDGET) is
> used.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net-next v6 3/4] eventpoll: Add per-epoll prefer busy poll option

[Jakub Kicinski]

On Mon,  5 Feb 2024 21:04:48 +0000 Joe Damato wrote:
> When using epoll-based busy poll, the prefer_busy_poll option is hardcoded
> to false. Users may want to enable prefer_busy_poll to be used in
> conjunction with gro_flush_timeout and defer_hard_irqs_count to keep device
> IRQs masked.
> 
> Other busy poll methods allow enabling or disabling prefer busy poll via
> SO_PREFER_BUSY_POLL, but epoll-based busy polling uses a hardcoded value.
> 
> Fix this edge case by adding support for a per-epoll context
> prefer_busy_poll option. The default is false, as it was hardcoded before
> this change.

Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params

[Jakub Kicinski]

On Wed, 7 Feb 2024 10:50:15 -0800 Joe Damato wrote:
> > This !! is unnecessary. Nonzero values shall be "converted" to true.
> > 
> > But FWIW, the above is nothing which should be blocking, so:
> "> 
> > Reviewed-by: Jiri Slaby <jirislaby@kernel.org>  
> 
> netdev maintainers: Jiri marked this with Reviewed-by, but was this review
> what caused "Changes Requested" to be the status set for this patch set in
> patchwork?
> 
> If needed, I'll send a v7 with the changes Jiri suggested and add the
> "Reviewed-by" since the changes are cosmetic, but I wanted to make sure
> this was the reason.

Yes, I think that's it.

Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance

[Joe Damato]

On Wed, Feb 07, 2024 at 11:04:13AM -0800, Jakub Kicinski wrote:
> On Mon,  5 Feb 2024 21:04:46 +0000 Joe Damato wrote:
> > Allow busy polling on a per-epoll context basis. The per-epoll context
> > usec timeout value is preferred, but the pre-existing system wide sysctl
> > value is still supported if it specified.
> 
> Why do we need u64 for usecs? I think u16 would do, and u32 would give
> a very solid "engineering margin". If it was discussed in previous
> versions I think it's worth explaining in the commit message.

In patch 4/4 the value is limited to U32_MAX, but if you prefer I use a u32
here instead, I can make that change.

Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params

[Joe Damato]

On Wed, Feb 07, 2024 at 11:07:26AM -0800, Jakub Kicinski wrote:
> On Wed, 7 Feb 2024 10:50:15 -0800 Joe Damato wrote:
> > > This !! is unnecessary. Nonzero values shall be "converted" to true.
> > > 
> > > But FWIW, the above is nothing which should be blocking, so:
> > "> 
> > > Reviewed-by: Jiri Slaby <jirislaby@kernel.org>  
> > 
> > netdev maintainers: Jiri marked this with Reviewed-by, but was this review
> > what caused "Changes Requested" to be the status set for this patch set in
> > patchwork?
> > 
> > If needed, I'll send a v7 with the changes Jiri suggested and add the
> > "Reviewed-by" since the changes are cosmetic, but I wanted to make sure
> > this was the reason.
> 
> Yes, I think that's it.

OK, thanks for letting me know. I wasn't sure if it was because of the
netdev/source_inline which marked 1/4 as "fail" because of the inlines
added.

Does that need to be changed, as well?

Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance

[Jakub Kicinski]

On Wed, 7 Feb 2024 11:14:08 -0800 Joe Damato wrote:
> > Why do we need u64 for usecs? I think u16 would do, and u32 would give
> > a very solid "engineering margin". If it was discussed in previous
> > versions I think it's worth explaining in the commit message.  
> 
> In patch 4/4 the value is limited to U32_MAX, but if you prefer I use a u32
> here instead, I can make that change.

Unless you have a clear reason not to, I think using u32 would be more
natural? If my head math is right the range for u32 is 4096 sec,
slightly over an hour? I'd use u32 and limit it to S32_MAX.

Re: [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params

[Jakub Kicinski]

On Wed, 7 Feb 2024 11:16:03 -0800 Joe Damato wrote:
> > > netdev maintainers: Jiri marked this with Reviewed-by, but was this review
> > > what caused "Changes Requested" to be the status set for this patch set in
> > > patchwork?
> > > 
> > > If needed, I'll send a v7 with the changes Jiri suggested and add the
> > > "Reviewed-by" since the changes are cosmetic, but I wanted to make sure
> > > this was the reason.  
> > 
> > Yes, I think that's it.  
> 
> OK, thanks for letting me know. I wasn't sure if it was because of the
> netdev/source_inline which marked 1/4 as "fail" because of the inlines
> added.
> 
> Does that need to be changed, as well?

For background our preference is to avoid using static inline in C
sources, unless the author compiled the code and actually confirmed
the code doesn't get inlined correctly. But it's not a hard
requirement, and technically the code is under fs/.

In general the patchwork checks are a bit noisy, see here the top left
graph of how many of the patches we merge are "all green":
https://netdev.bots.linux.dev/checks.html
Some of the checks are also largely outside of our control (checkpatch)
so consider the patchwork checks as automation for maintainers. 
The maintainers should respond on the list if any of the failures 
are indeed legit. 

Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance

[Joe Damato]

On Wed, Feb 07, 2024 at 12:11:24PM -0800, Jakub Kicinski wrote:
> On Wed, 7 Feb 2024 11:14:08 -0800 Joe Damato wrote:
> > > Why do we need u64 for usecs? I think u16 would do, and u32 would give
> > > a very solid "engineering margin". If it was discussed in previous
> > > versions I think it's worth explaining in the commit message.  
> > 
> > In patch 4/4 the value is limited to U32_MAX, but if you prefer I use a u32
> > here instead, I can make that change.
> 
> Unless you have a clear reason not to, I think using u32 would be more
> natural? If my head math is right the range for u32 is 4096 sec,
> slightly over an hour? I'd use u32 and limit it to S32_MAX.

OK, that seems fine. Sorry for the noob question, but since that represents
a fucntional change to patch 4/4, I believe I would need to drop Jiri's
Reviewed-by, is that right?

Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance

[Jakub Kicinski]

On Wed, 7 Feb 2024 12:23:23 -0800 Joe Damato wrote:
> > Unless you have a clear reason not to, I think using u32 would be more
> > natural? If my head math is right the range for u32 is 4096 sec,
> > slightly over an hour? I'd use u32 and limit it to S32_MAX.  
> 
> OK, that seems fine. Sorry for the noob question, but since that represents
> a fucntional change to patch 4/4, I believe I would need to drop Jiri's
> Reviewed-by, is that right?

I'd default to keeping it. But the review tag retention rules are one
of the more subjective things in kernel developments.

Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance

[Eric Dumazet]

On Mon, Feb 5, 2024 at 10:05 PM Joe Damato <jdamato@fastly.com> wrote:
>
> Allow busy polling on a per-epoll context basis. The per-epoll context
> usec timeout value is preferred, but the pre-existing system wide sysctl
> value is still supported if it specified.
>
> Signed-off-by: Joe Damato <jdamato@fastly.com>
> ---
>  fs/eventpoll.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> index 3534d36a1474..ce75189d46df 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -227,6 +227,8 @@ struct eventpoll {
>  #ifdef CONFIG_NET_RX_BUSY_POLL
>         /* used to track busy poll napi_id */
>         unsigned int napi_id;
> +       /* busy poll timeout */
> +       u64 busy_poll_usecs;
>  #endif
>
>  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> @@ -386,12 +388,44 @@ static inline int ep_events_available(struct eventpoll *ep)
>                 READ_ONCE(ep->ovflist) != EP_UNACTIVE_PTR;
>  }
>
> +/**
> + * busy_loop_ep_timeout - check if busy poll has timed out. The timeout value
> + * from the epoll instance ep is preferred, but if it is not set fallback to
> + * the system-wide global via busy_loop_timeout.
> + *
> + * @start_time: The start time used to compute the remaining time until timeout.
> + * @ep: Pointer to the eventpoll context.
> + *
> + * Return: true if the timeout has expired, false otherwise.
> + */
> +static inline bool busy_loop_ep_timeout(unsigned long start_time, struct eventpoll *ep)
> +{
> +#ifdef CONFIG_NET_RX_BUSY_POLL

It seems this local helper is only called from code compiled when
CONFIG_NET_RX_BUSY_POLL
is set.

Not sure why you need an #ifdef here.

> +       unsigned long bp_usec = READ_ONCE(ep->busy_poll_usecs);
> +
> +       if (bp_usec) {
> +               unsigned long end_time = start_time + bp_usec;
> +               unsigned long now = busy_loop_current_time();
> +
> +               return time_after(now, end_time);
> +       } else {
> +               return busy_loop_timeout(start_time);
> +       }
> +#endif
> +       return true;
> +}
> +
>  #ifdef CONFIG_NET_RX_BUSY_POLL
> +static bool ep_busy_loop_on(struct eventpoll *ep)
> +{
> +       return !!ep->busy_poll_usecs || net_busy_loop_on();
> +}
> +
>  static bool ep_busy_loop_end(void *p, unsigned long start_time)
>  {
>         struct eventpoll *ep = p;
>
> -       return ep_events_available(ep) || busy_loop_timeout(start_time);
> +       return ep_events_available(ep) || busy_loop_ep_timeout(start_time, ep);
>  }
>
>  /*
> @@ -404,7 +438,7 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
>  {
>         unsigned int napi_id = READ_ONCE(ep->napi_id);
>
> -       if ((napi_id >= MIN_NAPI_ID) && net_busy_loop_on()) {
> +       if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
>                 napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
>                                BUSY_POLL_BUDGET);
>                 if (ep_events_available(ep))
> @@ -430,7 +464,8 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
>         struct socket *sock;
>         struct sock *sk;
>
> -       if (!net_busy_loop_on())
> +       ep = epi->ep;
> +       if (!ep_busy_loop_on(ep))
>                 return;
>
>         sock = sock_from_file(epi->ffd.file);
> @@ -442,7 +477,6 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
>                 return;
>
>         napi_id = READ_ONCE(sk->sk_napi_id);
> -       ep = epi->ep;
>
>         /* Non-NAPI IDs can be rejected
>          *      or
> @@ -466,6 +500,10 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
>  {
>  }
>
> +static inline bool ep_busy_loop_on(struct eventpoll *ep)
> +{
> +       return false;
> +}
>  #endif /* CONFIG_NET_RX_BUSY_POLL */
>
>  /*
> @@ -2058,6 +2096,9 @@ static int do_epoll_create(int flags)
>                 error = PTR_ERR(file);
>                 goto out_free_fd;
>         }
> +#ifdef CONFIG_NET_RX_BUSY_POLL
> +       ep->busy_poll_usecs = 0;
> +#endif
>         ep->file = file;
>         fd_install(fd, file);
>         return fd;
> --
> 2.25.1
>

Re: [PATCH net-next v6 2/4] eventpoll: Add per-epoll busy poll packet budget

[Eric Dumazet]

On Wed, Feb 7, 2024 at 8:04 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon,  5 Feb 2024 21:04:47 +0000 Joe Damato wrote:
> > When using epoll-based busy poll, the packet budget is hardcoded to
> > BUSY_POLL_BUDGET (8). Users may desire larger busy poll budgets, which
> > can potentially increase throughput when busy polling under high network
> > load.
> >
> > Other busy poll methods allow setting the busy poll budget via
> > SO_BUSY_POLL_BUDGET, but epoll-based busy polling uses a hardcoded
> > value.
> >
> > Fix this edge case by adding support for a per-epoll context busy poll
> > packet budget. If not specified, the default value (BUSY_POLL_BUDGET) is
> > used.
>
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v6 3/4] eventpoll: Add per-epoll prefer busy poll option

[Eric Dumazet]

On Wed, Feb 7, 2024 at 8:04 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Mon,  5 Feb 2024 21:04:48 +0000 Joe Damato wrote:
> > When using epoll-based busy poll, the prefer_busy_poll option is hardcoded
> > to false. Users may want to enable prefer_busy_poll to be used in
> > conjunction with gro_flush_timeout and defer_hard_irqs_count to keep device
> > IRQs masked.
> >
> > Other busy poll methods allow enabling or disabling prefer busy poll via
> > SO_PREFER_BUSY_POLL, but epoll-based busy polling uses a hardcoded value.
> >
> > Fix this edge case by adding support for a per-epoll context
> > prefer_busy_poll option. The default is false, as it was hardcoded before
> > this change.
>
> Reviewed-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Eric Dumazet <edumazet@google.com>

Re: [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance

[Joe Damato]

On Thu, Feb 08, 2024 at 06:46:25PM +0100, Eric Dumazet wrote:
> On Mon, Feb 5, 2024 at 10:05 PM Joe Damato <jdamato@fastly.com> wrote:
> >
> > Allow busy polling on a per-epoll context basis. The per-epoll context
> > usec timeout value is preferred, but the pre-existing system wide sysctl
> > value is still supported if it specified.
> >
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > ---
> >  fs/eventpoll.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
> >  1 file changed, 45 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/eventpoll.c b/fs/eventpoll.c
> > index 3534d36a1474..ce75189d46df 100644
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
> > @@ -227,6 +227,8 @@ struct eventpoll {
> >  #ifdef CONFIG_NET_RX_BUSY_POLL
> >         /* used to track busy poll napi_id */
> >         unsigned int napi_id;
> > +       /* busy poll timeout */
> > +       u64 busy_poll_usecs;
> >  #endif
> >
> >  #ifdef CONFIG_DEBUG_LOCK_ALLOC
> > @@ -386,12 +388,44 @@ static inline int ep_events_available(struct eventpoll *ep)
> >                 READ_ONCE(ep->ovflist) != EP_UNACTIVE_PTR;
> >  }
> >
> > +/**
> > + * busy_loop_ep_timeout - check if busy poll has timed out. The timeout value
> > + * from the epoll instance ep is preferred, but if it is not set fallback to
> > + * the system-wide global via busy_loop_timeout.
> > + *
> > + * @start_time: The start time used to compute the remaining time until timeout.
> > + * @ep: Pointer to the eventpoll context.
> > + *
> > + * Return: true if the timeout has expired, false otherwise.
> > + */
> > +static inline bool busy_loop_ep_timeout(unsigned long start_time, struct eventpoll *ep)
> > +{
> > +#ifdef CONFIG_NET_RX_BUSY_POLL
> 
> It seems this local helper is only called from code compiled when
> CONFIG_NET_RX_BUSY_POLL
> is set.
> 
> Not sure why you need an #ifdef here.

Thanks, you are right.

I'll move this down to be within CONFIG_NET_RX_BUSY_POLL and get rid of the
#ifdef for the v7.

Thanks for your review.
 
> > +       unsigned long bp_usec = READ_ONCE(ep->busy_poll_usecs);
> > +
> > +       if (bp_usec) {
> > +               unsigned long end_time = start_time + bp_usec;
> > +               unsigned long now = busy_loop_current_time();
> > +
> > +               return time_after(now, end_time);
> > +       } else {
> > +               return busy_loop_timeout(start_time);
> > +       }
> > +#endif
> > +       return true;
> > +}
> > +
> >  #ifdef CONFIG_NET_RX_BUSY_POLL
> > +static bool ep_busy_loop_on(struct eventpoll *ep)
> > +{
> > +       return !!ep->busy_poll_usecs || net_busy_loop_on();
> > +}
> > +
> >  static bool ep_busy_loop_end(void *p, unsigned long start_time)
> >  {
> >         struct eventpoll *ep = p;
> >
> > -       return ep_events_available(ep) || busy_loop_timeout(start_time);
> > +       return ep_events_available(ep) || busy_loop_ep_timeout(start_time, ep);
> >  }
> >
> >  /*
> > @@ -404,7 +438,7 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
> >  {
> >         unsigned int napi_id = READ_ONCE(ep->napi_id);
> >
> > -       if ((napi_id >= MIN_NAPI_ID) && net_busy_loop_on()) {
> > +       if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
> >                 napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
> >                                BUSY_POLL_BUDGET);
> >                 if (ep_events_available(ep))
> > @@ -430,7 +464,8 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> >         struct socket *sock;
> >         struct sock *sk;
> >
> > -       if (!net_busy_loop_on())
> > +       ep = epi->ep;
> > +       if (!ep_busy_loop_on(ep))
> >                 return;
> >
> >         sock = sock_from_file(epi->ffd.file);
> > @@ -442,7 +477,6 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> >                 return;
> >
> >         napi_id = READ_ONCE(sk->sk_napi_id);
> > -       ep = epi->ep;
> >
> >         /* Non-NAPI IDs can be rejected
> >          *      or
> > @@ -466,6 +500,10 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
> >  {
> >  }
> >
> > +static inline bool ep_busy_loop_on(struct eventpoll *ep)
> > +{
> > +       return false;
> > +}
> >  #endif /* CONFIG_NET_RX_BUSY_POLL */
> >
> >  /*
> > @@ -2058,6 +2096,9 @@ static int do_epoll_create(int flags)
> >                 error = PTR_ERR(file);
> >                 goto out_free_fd;
> >         }
> > +#ifdef CONFIG_NET_RX_BUSY_POLL
> > +       ep->busy_poll_usecs = 0;
> > +#endif
> >         ep->file = file;
> >         fd_install(fd, file);
> >         return fd;
> > --
> > 2.25.1
> >