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

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

[Joe Damato]

Greetings:

Welcome to v2. Cover letter updated from v1.

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.

To allow for this, two ioctls have been added for epoll contexts for
getting and setting a new struct, struct epoll_params.

This makes epoll-based busy polling much more usable for user
applications than the current system-wide sysctl and hardcoded budget.

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 [1]).

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 form 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 [2] and a recent
academic paper about measured performance improvements of busy polling [3]
(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

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/20170324170836.15226.87178.stgit@localhost.localdomain/
[2]: https://netdevconf.info/2.1/papers/BusyPollingNextGen.pdf
[3]: https://dl.acm.org/doi/pdf/10.1145/3626780

Joe Damato (4):
  eventpoll: support busy poll per epoll instance
  eventpoll: Add per-epoll busy poll packet budget
  eventpoll: Add epoll ioctl for epoll_params
  net: print error if SO_BUSY_POLL_BUDGET is large

 .../userspace-api/ioctl/ioctl-number.rst      |   1 +
 fs/eventpoll.c                                | 105 +++++++++++++++++-
 include/uapi/linux/eventpoll.h                |  12 ++
 net/core/sock.c                               |   3 +
 4 files changed, 116 insertions(+), 5 deletions(-)

-- 
2.25.1

[net-next v2 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.

Note that this change uses an xor: either per epoll instance busy polling
is enabled on the epoll instance or system wide epoll is enabled. Enabling
both is disallowed.

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..4503fec01278 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

[net-next v2 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 4503fec01278..40bd97477b91 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

[net-next v2 3/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 or packet budget
params for a specific epoll context.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 .../userspace-api/ioctl/ioctl-number.rst      |  1 +
 fs/eventpoll.c                                | 47 +++++++++++++++++++
 include/uapi/linux/eventpoll.h                | 12 +++++
 3 files changed, 60 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 40bd97477b91..c1ee0fe01da1 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -6,6 +6,8 @@
  *  Davide Libenzi <davidel@xmailserver.org>
  */
 
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/sched/signal.h>
@@ -869,6 +871,49 @@ 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;
+	struct eventpoll *ep;
+	struct epoll_params epoll_params;
+	void __user *uarg = (void __user *) arg;
+
+	if (!is_file_epoll(file))
+		return -EINVAL;
+
+	ep = file->private_data;
+
+	switch (cmd) {
+#ifdef CONFIG_NET_RX_BUSY_POLL
+	case EPIOCSPARAMS:
+		if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
+			return -EFAULT;
+
+		if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
+			pr_err("busy poll budget %u exceeds suggested maximum %u\n",
+					epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
+
+		ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
+		ep->busy_poll_budget = epoll_params.busy_poll_budget;
+		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;
+		if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
+			return -EFAULT;
+
+		return 0;
+#endif
+	default:
+		ret = -EINVAL;
+		break;
+	}
+
+	return ret;
+}
+
 static int ep_eventpoll_release(struct inode *inode, struct file *file)
 {
 	struct eventpoll *ep = file->private_data;
@@ -975,6 +1020,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..8eb0fdbce995 100644
--- a/include/uapi/linux/eventpoll.h
+++ b/include/uapi/linux/eventpoll.h
@@ -85,4 +85,16 @@ struct epoll_event {
 	__u64 data;
 } EPOLL_PACKED;
 
+struct epoll_params {
+	u64 busy_poll_usecs;
+	u16 busy_poll_budget;
+
+	/* for future fields */
+	u8 data[118];
+} EPOLL_PACKED;
+
+#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

[net-next v2 4/4] net: print error if SO_BUSY_POLL_BUDGET is large

[Joe Damato]

When drivers call netif_napi_add_weight with a weight that is larger
than NAPI_POLL_WEIGHT, the networking code allows the larger weight, but
prints an error.

Replicate this check for SO_BUSY_POLL_BUDGET; check if the user
specified amount exceeds NAPI_POLL_WEIGHT, allow it anyway, but print an
error.

Signed-off-by: Joe Damato <jdamato@fastly.com>
---
 net/core/sock.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/net/core/sock.c b/net/core/sock.c
index 158dbdebce6a..ed243bd0dd77 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1153,6 +1153,9 @@ int sk_setsockopt(struct sock *sk, int level, int optname,
 			return -EPERM;
 		if (val < 0 || val > U16_MAX)
 			return -EINVAL;
+		if (val > NAPI_POLL_WEIGHT)
+			pr_err("SO_BUSY_POLL_BUDGET %u exceeds suggested maximum %u\n", val,
+			       NAPI_POLL_WEIGHT);
 		WRITE_ONCE(sk->sk_busy_poll_budget, val);
 		return 0;
 #endif
-- 
2.25.1

Re: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params

[Willem de Bruijn]

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 or packet budget
> params for a specific epoll context.
> 
> Signed-off-by: Joe Damato <jdamato@fastly.com>

Please be sure to include the lists and people suggested by
`get_maintainer.pl -f fs/eventpoll.c`.

Adding ioctls is generally discouraged.

As this affects the behavior of epoll_wait, should this just be a
flag to (a new variant of) epoll_wait?

Speaking from some experience with adding epoll_pwait2. I initially
there added a stateful change that would affect wait behavior. The
sensible feedback as the time was to just change the behavior of the
syscall it affected. Even if that requires a syscall (which is not
that different from an ioctl, if better defined).

The discussion in that thread may be informative to decide on API:
https://lwn.net/ml/linux-kernel/20201116161001.1606608-1-willemdebruijn.kernel@gmail.com/

Agreed on the overall principle that it is preferable to be able to
enable busypolling selectively. We already do for SO_BUSY_POLL and
sysctl busy_read.

> ---
>  .../userspace-api/ioctl/ioctl-number.rst      |  1 +
>  fs/eventpoll.c                                | 47 +++++++++++++++++++
>  include/uapi/linux/eventpoll.h                | 12 +++++
>  3 files changed, 60 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 40bd97477b91..c1ee0fe01da1 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -6,6 +6,8 @@
>   *  Davide Libenzi <davidel@xmailserver.org>
>   */
>  
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
>  #include <linux/init.h>
>  #include <linux/kernel.h>
>  #include <linux/sched/signal.h>
> @@ -869,6 +871,49 @@ 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;
> +	struct eventpoll *ep;
> +	struct epoll_params epoll_params;
> +	void __user *uarg = (void __user *) arg;
> +
> +	if (!is_file_epoll(file))
> +		return -EINVAL;
> +
> +	ep = file->private_data;
> +
> +	switch (cmd) {
> +#ifdef CONFIG_NET_RX_BUSY_POLL
> +	case EPIOCSPARAMS:
> +		if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> +			return -EFAULT;
> +
> +		if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
> +			pr_err("busy poll budget %u exceeds suggested maximum %u\n",
> +					epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
> +
> +		ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
> +		ep->busy_poll_budget = epoll_params.busy_poll_budget;
> +		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;
> +		if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
> +			return -EFAULT;
> +
> +		return 0;
> +#endif
> +	default:
> +		ret = -EINVAL;
> +		break;
> +	}
> +
> +	return ret;
> +}
> +
>  static int ep_eventpoll_release(struct inode *inode, struct file *file)
>  {
>  	struct eventpoll *ep = file->private_data;
> @@ -975,6 +1020,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..8eb0fdbce995 100644
> --- a/include/uapi/linux/eventpoll.h
> +++ b/include/uapi/linux/eventpoll.h
> @@ -85,4 +85,16 @@ struct epoll_event {
>  	__u64 data;
>  } EPOLL_PACKED;
>  
> +struct epoll_params {
> +	u64 busy_poll_usecs;
> +	u16 busy_poll_budget;
> +
> +	/* for future fields */
> +	u8 data[118];
> +} EPOLL_PACKED;
> +
> +#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: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params

[Joe Damato]

On Wed, Jan 24, 2024 at 09:46:23PM -0500, Willem de Bruijn wrote:
> 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 or packet budget
> > params for a specific epoll context.
> > 
> > Signed-off-by: Joe Damato <jdamato@fastly.com>
> 
> Please be sure to include the lists and people suggested by
> `get_maintainer.pl -f fs/eventpoll.c`.

Thanks - I must have done something wrong when trying to get the maintainer
list.

Should I resend this v2? Not sure what the appropriate thing to do is in
this case. My apologies.

> Adding ioctls is generally discouraged.
> 
> As this affects the behavior of epoll_wait, should this just be a
> flag to (a new variant of) epoll_wait?

I have no strong preference either way. It seems to me that adding a new
system call is a fairly significant change vs adding an ioctl, but I am
open to whatever is preferred by the maintainers.

I have no idea who would need to weigh-in to make this decision.

> Speaking from some experience with adding epoll_pwait2. I initially
> there added a stateful change that would affect wait behavior. The
> sensible feedback as the time was to just change the behavior of the
> syscall it affected. Even if that requires a syscall (which is not
> that different from an ioctl, if better defined).
> 
> The discussion in that thread may be informative to decide on API:
> https://lwn.net/ml/linux-kernel/20201116161001.1606608-1-willemdebruijn.kernel@gmail.com/

Interesting thread, thanks for sending.

> Agreed on the overall principle that it is preferable to be able to
> enable busypolling selectively. We already do for SO_BUSY_POLL and
> sysctl busy_read.

Thanks for taking a look and providing feedback.

>
> > ---
> >  .../userspace-api/ioctl/ioctl-number.rst      |  1 +
> >  fs/eventpoll.c                                | 47 +++++++++++++++++++
> >  include/uapi/linux/eventpoll.h                | 12 +++++
> >  3 files changed, 60 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 40bd97477b91..c1ee0fe01da1 100644
> > --- a/fs/eventpoll.c
> > +++ b/fs/eventpoll.c
> > @@ -6,6 +6,8 @@
> >   *  Davide Libenzi <davidel@xmailserver.org>
> >   */
> >  
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> >  #include <linux/init.h>
> >  #include <linux/kernel.h>
> >  #include <linux/sched/signal.h>
> > @@ -869,6 +871,49 @@ 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;
> > +	struct eventpoll *ep;
> > +	struct epoll_params epoll_params;
> > +	void __user *uarg = (void __user *) arg;
> > +
> > +	if (!is_file_epoll(file))
> > +		return -EINVAL;
> > +
> > +	ep = file->private_data;
> > +
> > +	switch (cmd) {
> > +#ifdef CONFIG_NET_RX_BUSY_POLL
> > +	case EPIOCSPARAMS:
> > +		if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> > +			return -EFAULT;
> > +
> > +		if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
> > +			pr_err("busy poll budget %u exceeds suggested maximum %u\n",
> > +					epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
> > +
> > +		ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
> > +		ep->busy_poll_budget = epoll_params.busy_poll_budget;
> > +		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;
> > +		if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
> > +			return -EFAULT;
> > +
> > +		return 0;
> > +#endif
> > +	default:
> > +		ret = -EINVAL;
> > +		break;
> > +	}
> > +
> > +	return ret;
> > +}
> > +
> >  static int ep_eventpoll_release(struct inode *inode, struct file *file)
> >  {
> >  	struct eventpoll *ep = file->private_data;
> > @@ -975,6 +1020,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..8eb0fdbce995 100644
> > --- a/include/uapi/linux/eventpoll.h
> > +++ b/include/uapi/linux/eventpoll.h
> > @@ -85,4 +85,16 @@ struct epoll_event {
> >  	__u64 data;
> >  } EPOLL_PACKED;
> >  
> > +struct epoll_params {
> > +	u64 busy_poll_usecs;
> > +	u16 busy_poll_budget;
> > +
> > +	/* for future fields */
> > +	u8 data[118];
> > +} EPOLL_PACKED;
> > +
> > +#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: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params

[kernel test robot]

Hi Joe,

kernel test robot noticed the following build warnings:

[auto build test WARNING on net-next/main]

url:    https://github.com/intel-lab-lkp/linux/commits/Joe-Damato/eventpoll-support-busy-poll-per-epoll-instance/20240125-083418
base:   net-next/main
patch link:    https://lore.kernel.org/r/20240125003014.43103-4-jdamato%40fastly.com
patch subject: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params
config: openrisc-allnoconfig (https://download.01.org/0day-ci/archive/20240125/202401252141.eUlgsF08-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240125/202401252141.eUlgsF08-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401252141.eUlgsF08-lkp@intel.com/

All warnings (new ones prefixed by >>):

   fs/eventpoll.c: In function 'ep_eventpoll_ioctl':
>> fs/eventpoll.c:879:22: warning: unused variable 'uarg' [-Wunused-variable]
     879 |         void __user *uarg = (void __user *) arg;
         |                      ^~~~
>> fs/eventpoll.c:878:29: warning: unused variable 'epoll_params' [-Wunused-variable]
     878 |         struct epoll_params epoll_params;
         |                             ^~~~~~~~~~~~
   fs/eventpoll.c:877:27: warning: variable 'ep' set but not used [-Wunused-but-set-variable]
     877 |         struct eventpoll *ep;
         |                           ^~


vim +/uarg +879 fs/eventpoll.c

   873	
   874	static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
   875	{
   876		int ret;
   877		struct eventpoll *ep;
 > 878		struct epoll_params epoll_params;
 > 879		void __user *uarg = (void __user *) arg;
   880	
   881		if (!is_file_epoll(file))
   882			return -EINVAL;
   883	
   884		ep = file->private_data;
   885	
   886		switch (cmd) {
   887	#ifdef CONFIG_NET_RX_BUSY_POLL
   888		case EPIOCSPARAMS:
   889			if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
   890				return -EFAULT;
   891	
   892			if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
   893				pr_err("busy poll budget %u exceeds suggested maximum %u\n",
   894						epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
   895	
   896			ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
   897			ep->busy_poll_budget = epoll_params.busy_poll_budget;
   898			return 0;
   899	
   900		case EPIOCGPARAMS:
   901			memset(&epoll_params, 0, sizeof(epoll_params));
   902			epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
   903			epoll_params.busy_poll_budget = ep->busy_poll_budget;
   904			if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
   905				return -EFAULT;
   906	
   907			return 0;
   908	#endif
   909		default:
   910			ret = -EINVAL;
   911			break;
   912		}
   913	
   914		return ret;
   915	}
   916	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [net-next v2 3/4] eventpoll: Add epoll ioctl for epoll_params

[Willem de Bruijn]

Joe Damato wrote:
> On Wed, Jan 24, 2024 at 09:46:23PM -0500, Willem de Bruijn wrote:
> > 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 or packet budget
> > > params for a specific epoll context.
> > > 
> > > Signed-off-by: Joe Damato <jdamato@fastly.com>
> > 
> > Please be sure to include the lists and people suggested by
> > `get_maintainer.pl -f fs/eventpoll.c`.
> 
> Thanks - I must have done something wrong when trying to get the maintainer
> list.
> 
> Should I resend this v2? Not sure what the appropriate thing to do is in
> this case. My apologies.

If you don't get additional feedback in a few days and still prefer
this option that might be an approach.

After reading the below thread, compare the different possible APIs
and either revise the code or perhaps add a small paragraph why you
think this is the preferred path.
 
> > Adding ioctls is generally discouraged.
> > 
> > As this affects the behavior of epoll_wait, should this just be a
> > flag to (a new variant of) epoll_wait?
> 
> I have no strong preference either way. It seems to me that adding a new
> system call is a fairly significant change vs adding an ioctl, but I am
> open to whatever is preferred by the maintainers.
> 
> I have no idea who would need to weigh-in to make this decision.
> 
> > Speaking from some experience with adding epoll_pwait2. I initially
> > there added a stateful change that would affect wait behavior. The
> > sensible feedback as the time was to just change the behavior of the
> > syscall it affected. Even if that requires a syscall (which is not
> > that different from an ioctl, if better defined).
> > 
> > The discussion in that thread may be informative to decide on API:
> > https://lwn.net/ml/linux-kernel/20201116161001.1606608-1-willemdebruijn.kernel@gmail.com/
> 
> Interesting thread, thanks for sending.
> 
> > Agreed on the overall principle that it is preferable to be able to
> > enable busypolling selectively. We already do for SO_BUSY_POLL and
> > sysctl busy_read.
> 
> Thanks for taking a look and providing feedback.
> 
> >
> > > ---
> > >  .../userspace-api/ioctl/ioctl-number.rst      |  1 +
> > >  fs/eventpoll.c                                | 47 +++++++++++++++++++
> > >  include/uapi/linux/eventpoll.h                | 12 +++++
> > >  3 files changed, 60 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 40bd97477b91..c1ee0fe01da1 100644
> > > --- a/fs/eventpoll.c
> > > +++ b/fs/eventpoll.c
> > > @@ -6,6 +6,8 @@
> > >   *  Davide Libenzi <davidel@xmailserver.org>
> > >   */
> > >  
> > > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > > +
> > >  #include <linux/init.h>
> > >  #include <linux/kernel.h>
> > >  #include <linux/sched/signal.h>
> > > @@ -869,6 +871,49 @@ 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;
> > > +	struct eventpoll *ep;
> > > +	struct epoll_params epoll_params;
> > > +	void __user *uarg = (void __user *) arg;
> > > +
> > > +	if (!is_file_epoll(file))
> > > +		return -EINVAL;
> > > +
> > > +	ep = file->private_data;
> > > +
> > > +	switch (cmd) {
> > > +#ifdef CONFIG_NET_RX_BUSY_POLL
> > > +	case EPIOCSPARAMS:
> > > +		if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
> > > +			return -EFAULT;
> > > +
> > > +		if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT)
> > > +			pr_err("busy poll budget %u exceeds suggested maximum %u\n",
> > > +					epoll_params.busy_poll_budget, NAPI_POLL_WEIGHT);
> > > +
> > > +		ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
> > > +		ep->busy_poll_budget = epoll_params.busy_poll_budget;
> > > +		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;
> > > +		if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
> > > +			return -EFAULT;
> > > +
> > > +		return 0;
> > > +#endif
> > > +	default:
> > > +		ret = -EINVAL;
> > > +		break;
> > > +	}
> > > +
> > > +	return ret;
> > > +}
> > > +
> > >  static int ep_eventpoll_release(struct inode *inode, struct file *file)
> > >  {
> > >  	struct eventpoll *ep = file->private_data;
> > > @@ -975,6 +1020,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..8eb0fdbce995 100644
> > > --- a/include/uapi/linux/eventpoll.h
> > > +++ b/include/uapi/linux/eventpoll.h
> > > @@ -85,4 +85,16 @@ struct epoll_event {
> > >  	__u64 data;
> > >  } EPOLL_PACKED;
> > >  
> > > +struct epoll_params {
> > > +	u64 busy_poll_usecs;
> > > +	u16 busy_poll_budget;
> > > +
> > > +	/* for future fields */
> > > +	u8 data[118];
> > > +} EPOLL_PACKED;
> > > +
> > > +#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
> > > 
> > 
> >