* [net-next v2 2/4] eventpoll: Add per-epoll busy poll packet budget
From: Joe Damato @ 2024-01-25 0:30 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, weiwan, Joe Damato
In-Reply-To: <20240125003014.43103-1-jdamato@fastly.com>
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
^ permalink raw reply related
* [net-next v2 1/4] eventpoll: support busy poll per epoll instance
From: Joe Damato @ 2024-01-25 0:30 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, weiwan, Joe Damato
In-Reply-To: <20240125003014.43103-1-jdamato@fastly.com>
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
^ permalink raw reply related
* [net-next v2 0/4] Per epoll context busy poll support
From: Joe Damato @ 2024-01-25 0:30 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, weiwan, 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
^ permalink raw reply
* Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.
From: Elizabeth Figura @ 2024-01-24 22:56 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, linux-api,
wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra, Alexandre Julliard
In-Reply-To: <CALCETrU+Eb5CdkqfYK8JvOiPA7K-6Bfs4uEWiu-U9oH95XfvKw@mail.gmail.com>
On Wednesday, 24 January 2024 15:26:15 CST Andy Lutomirski wrote:
> On Tue, Jan 23, 2024 at 4:59 PM Elizabeth Figura
> <zfigura@codeweavers.com> wrote:
> >
> > ntsync uses a misc device as the simplest and least intrusive uAPI interface.
> >
> > Each file description on the device represents an isolated NT instance, intended
> > to correspond to a single NT virtual machine.
>
> If I understand this text right, and if I understood the code right,
> you're saying that each open instance of the device represents an
> entire universe of NT synchronization objects, and no security or
> isolation is possible between those objects. For single-process use,
> this seems fine. But fork() will be a bit odd (although NT doesn't
> really believe in fork, so maybe this is fine).
>
> Except that NT has *named* semaphores and such. And I'm pretty sure
> I've written GUI programs that use named synchronization objects (IIRC
> they were events, and this was a *very* common pattern, regularly
> discussed in MSDN, usenet, etc) to detect whether another instance of
> the program is running. And this all works on real Windows because
> sessions have sufficiently separated namespaces, and the security all
> works out about as any other security on Windows, etc. But
> implementing *that* on top of this
> file-description-plus-integer-equals-object will be fundamentally
> quite subject to one buggy program completely clobbering someone
> else's state.
>
> Would it make sense and scale appropriately for an NT synchronization
> *object* to be a Linux open file description? Then SCM_RIGHTS could
> pass them around, an RPC server could manage *named* objects, and
> they'd generally work just like other "Object Manager" objects like,
> say, files.
It's a sensible concern. I think when I discussed this with Alexandre
Julliard (the Wine maintainer, CC'd) the conclusion was this wasn't
something we were concerned about.
While the current model *does* allow for processes to arbitrarily mess
with each other, accidentally or not, I think we're not concerned with
the scope of that than we are about implementing a whole scheduler in
user space.
For one, you can't corrupt the wineserver state this way—wineserver
being sort of like a dedicated process that handles many of the things
that a kernel would, and so sometimes needs to set or reset events, or
perform NTSYNC_IOC_KILL_MUTEX, but never relies on ntsync object state.
Whereas trying to implement a scheduler in user space would involve the
wineserver taking locks, and hence other processes could deadlock.
For two, it's probably a lot harder to mess with that internal state
accidentally.
[There is also a potential problem where some broken applications
create a million (literally) sync objects. Making these into files runs
into NOFILE. We did specifically push distributions and systemd to
increase those limits because an older solution *did* use eventfds and
*did* run into those limits. Since that push was successful I don't
know if this is *actually* a concern anymore, but avoiding files is
probably not a bad thing either.]
--Zeb
^ permalink raw reply
* Re: [RFC PATCH 5/9] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.
From: Elizabeth Figura @ 2024-01-24 22:28 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel, linux-api, Arnd Bergmann
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <3ec03a12-ee1b-45f8-9f03-258606763d1e@app.fastmail.com>
On Wednesday, 24 January 2024 13:52:52 CST Arnd Bergmann wrote:
> On Wed, Jan 24, 2024, at 19:02, Elizabeth Figura wrote:
> > On Wednesday, 24 January 2024 01:56:52 CST Arnd Bergmann wrote:
> >> On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote:
> >>
> >> > + if (args->timeout) {
> >> > + struct timespec64 to;
> >> > +
> >> > + if (get_timespec64(&to, u64_to_user_ptr(args->timeout)))
> >> > + return -EFAULT;
> >> > + if (!timespec64_valid(&to))
> >> > + return -EINVAL;
> >> > +
> >> > + timeout = timespec64_to_ns(&to);
> >> > + }
> >>
> >> Have you considered just passing the nanosecond value here?
> >> Since you do not appear to write it back, that would avoid
> >> the complexities of dealing with timespec layout differences
> >> and indirection.
> >
> > That'd be nicer in general. I think there was some documentation that advised
> > using timespec64 for new ioctl interfaces but it may have been outdated or
> > misread.
>
> It's probably something I wrote. It depends a bit on
> whether you have an absolute or relative timeout. If
> the timeout is relative to the current time as I understand
> it is here, a 64-bit number seems more logical to me.
>
> For absolute times, I would usually use a __kernel_timespec,
> especially if it's CLOCK_REALTIME. In this case you would
> also need to specify the time domain.
Currently the interface does pass it as an absolute time, with the
domain implicitly being MONOTONIC. This particular choice comes from
process/botching-up-ioctls.rst, which is admittedly focused around GPU
ioctls, but the rationale of having easily restartable ioctls applies
here too.
(E.g. Wine does play games with signals, so we do want to be able to
interrupt arbitrary waits with EINTR. The "usual" fast path for ntsync
waits won't hit that, but we want to have it work.)
On the other hand, if we can pass the timeout as relative, and write it
back on exit like ppoll() does [assuming that's not proscribed], that
would presumably be slightly better for performance.
When writing the patch I just picked the recommended option, and didn't
bother doing any micro-optimizations afterward.
What's the rationale for using timespec for absolute or written-back
timeouts, instead of dealing in ns directly? I'm afraid it's not
obvious to me.
--Zeb
^ permalink raw reply
* Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.
From: Andy Lutomirski @ 2024-01-24 21:26 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Arnd Bergmann, Greg Kroah-Hartman, linux-kernel, linux-api,
wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <20240124004028.16826-2-zfigura@codeweavers.com>
On Tue, Jan 23, 2024 at 4:59 PM Elizabeth Figura
<zfigura@codeweavers.com> wrote:
>
> ntsync uses a misc device as the simplest and least intrusive uAPI interface.
>
> Each file description on the device represents an isolated NT instance, intended
> to correspond to a single NT virtual machine.
If I understand this text right, and if I understood the code right,
you're saying that each open instance of the device represents an
entire universe of NT synchronization objects, and no security or
isolation is possible between those objects. For single-process use,
this seems fine. But fork() will be a bit odd (although NT doesn't
really believe in fork, so maybe this is fine).
Except that NT has *named* semaphores and such. And I'm pretty sure
I've written GUI programs that use named synchronization objects (IIRC
they were events, and this was a *very* common pattern, regularly
discussed in MSDN, usenet, etc) to detect whether another instance of
the program is running. And this all works on real Windows because
sessions have sufficiently separated namespaces, and the security all
works out about as any other security on Windows, etc. But
implementing *that* on top of this
file-description-plus-integer-equals-object will be fundamentally
quite subject to one buggy program completely clobbering someone
else's state.
Would it make sense and scale appropriately for an NT synchronization
*object* to be a Linux open file description? Then SCM_RIGHTS could
pass them around, an RPC server could manage *named* objects, and
they'd generally work just like other "Object Manager" objects like,
say, files.
--Andy
^ permalink raw reply
* Re: [RFC PATCH 8/9] ntsync: Introduce NTSYNC_IOC_PUT_MUTEX.
From: Arnd Bergmann @ 2024-01-24 19:53 UTC (permalink / raw)
To: Elizabeth Figura, Greg Kroah-Hartman, linux-kernel, linux-api
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <2171522.irdbgypaU6@camazotz>
On Wed, Jan 24, 2024, at 19:03, Elizabeth Figura wrote:
> On Wednesday, 24 January 2024 01:42:19 CST Arnd Bergmann wrote:
>> On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote:
>> > @@ -738,6 +803,8 @@ static long ntsync_char_ioctl(struct file *file,
>> > diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
>> > index 26d1b3d4847f..2e44e7e77776 100644
>> > --- a/include/uapi/linux/ntsync.h
>> > +++ b/include/uapi/linux/ntsync.h
>> > @@ -46,5 +46,7 @@ struct ntsync_wait_args {
>> > struct ntsync_wait_args)
>> > #define NTSYNC_IOC_CREATE_MUTEX _IOWR(NTSYNC_IOC_BASE, 5, \
>> > struct ntsync_mutex_args)
>> > +#define NTSYNC_IOC_PUT_MUTEX _IOWR(NTSYNC_IOC_BASE, 6, \
>> > + struct ntsync_mutex_args)
>> >
>>
>> In your implementation, this argument is not written back to
>> user space, so I think this should be _IOW rather than than _IORW.
>>
>> Again, no practical difference here.
>
> Hm, but there is a put_user() at the end of the function, or am I
> missing something?
No, I was just looking at the wrong thing, your version is good.
Arnd
^ permalink raw reply
* Re: [RFC PATCH 5/9] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.
From: Arnd Bergmann @ 2024-01-24 19:52 UTC (permalink / raw)
To: Elizabeth Figura, Greg Kroah-Hartman, linux-kernel, linux-api
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <4864383.GXAFRqVoOG@camazotz>
On Wed, Jan 24, 2024, at 19:02, Elizabeth Figura wrote:
> On Wednesday, 24 January 2024 01:56:52 CST Arnd Bergmann wrote:
>> On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote:
>>
>> > + if (args->timeout) {
>> > + struct timespec64 to;
>> > +
>> > + if (get_timespec64(&to, u64_to_user_ptr(args->timeout)))
>> > + return -EFAULT;
>> > + if (!timespec64_valid(&to))
>> > + return -EINVAL;
>> > +
>> > + timeout = timespec64_to_ns(&to);
>> > + }
>>
>> Have you considered just passing the nanosecond value here?
>> Since you do not appear to write it back, that would avoid
>> the complexities of dealing with timespec layout differences
>> and indirection.
>
> That'd be nicer in general. I think there was some documentation that advised
> using timespec64 for new ioctl interfaces but it may have been outdated or
> misread.
It's probably something I wrote. It depends a bit on
whether you have an absolute or relative timeout. If
the timeout is relative to the current time as I understand
it is here, a 64-bit number seems more logical to me.
For absolute times, I would usually use a __kernel_timespec,
especially if it's CLOCK_REALTIME. In this case you would
also need to specify the time domain.
Arnd
^ permalink raw reply
* Re: [RFC PATCH 8/9] ntsync: Introduce NTSYNC_IOC_PUT_MUTEX.
From: Elizabeth Figura @ 2024-01-24 18:03 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel, linux-api, Arnd Bergmann
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <4027ec4c-1e11-40fc-a9af-07732d7c3c1a@app.fastmail.com>
On Wednesday, 24 January 2024 01:42:19 CST Arnd Bergmann wrote:
> On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote:
> > @@ -738,6 +803,8 @@ static long ntsync_char_ioctl(struct file *file,
> > diff --git a/include/uapi/linux/ntsync.h b/include/uapi/linux/ntsync.h
> > index 26d1b3d4847f..2e44e7e77776 100644
> > --- a/include/uapi/linux/ntsync.h
> > +++ b/include/uapi/linux/ntsync.h
> > @@ -46,5 +46,7 @@ struct ntsync_wait_args {
> > struct ntsync_wait_args)
> > #define NTSYNC_IOC_CREATE_MUTEX _IOWR(NTSYNC_IOC_BASE, 5, \
> > struct ntsync_mutex_args)
> > +#define NTSYNC_IOC_PUT_MUTEX _IOWR(NTSYNC_IOC_BASE, 6, \
> > + struct ntsync_mutex_args)
> >
>
> In your implementation, this argument is not written back to
> user space, so I think this should be _IOW rather than than _IORW.
>
> Again, no practical difference here.
Hm, but there is a put_user() at the end of the function, or am I missing something?
^ permalink raw reply
* Re: [RFC PATCH 5/9] ntsync: Introduce NTSYNC_IOC_WAIT_ANY.
From: Elizabeth Figura @ 2024-01-24 18:02 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel, linux-api, Arnd Bergmann
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <18c814fa-b458-48f9-b7e8-88b23a1825e2@app.fastmail.com>
On Wednesday, 24 January 2024 01:56:52 CST Arnd Bergmann wrote:
> On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote:
>
> > + if (args->timeout) {
> > + struct timespec64 to;
> > +
> > + if (get_timespec64(&to, u64_to_user_ptr(args->timeout)))
> > + return -EFAULT;
> > + if (!timespec64_valid(&to))
> > + return -EINVAL;
> > +
> > + timeout = timespec64_to_ns(&to);
> > + }
>
> Have you considered just passing the nanosecond value here?
> Since you do not appear to write it back, that would avoid
> the complexities of dealing with timespec layout differences
> and indirection.
That'd be nicer in general. I think there was some documentation that advised
using timespec64 for new ioctl interfaces but it may have been outdated or
misread.
>
> > + ids = kmalloc_array(count, sizeof(*ids), GFP_KERNEL);
> > + if (!ids)
> > + return -ENOMEM;
> > + if (copy_from_user(ids, u64_to_user_ptr(args->objs),
> > + array_size(count, sizeof(*ids)))) {
> > + kfree(ids);
> > + return -EFAULT;
> > + }
>
> This looks like memdup_user() would be slightly simpler.
That's useful, thanks.
^ permalink raw reply
* Re: [PATCH v2 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Gregory Price @ 2024-01-24 18:01 UTC (permalink / raw)
To: Huang, Ying
Cc: Gregory Price, linux-mm, linux-kernel, linux-doc, linux-fsdevel,
linux-api, corbet, akpm, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <87jznzts6f.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Wed, Jan 24, 2024 at 09:51:20AM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
>
> + if (new && (new->mode == MPOL_INTERLEAVE ||
> + new->mode == MPOL_WEIGHTED_INTERLEAVE))
> current->il_prev = MAX_NUMNODES-1;
> task_unlock(current);
> mpol_put(old);
>
> I don't think we need to change this.
>
Ah you're right it's set to MAX_NUMNODES-1 here, but NUMA_NO_NODE can be
passed in as an argument to alloc_pages_bulk_array_mempolicy, like here:
vm_area_alloc_pages()
if (IS_ENABLED(CONFIG_NUMA) && nid == NUMA_NO_NODE)
nr = alloc_pages_bulk_array_mempolicy(bulk_gfp,
nr_pages_request,
pages + nr_allocated);
> > (cur_weight = 0) can happen in two scenarios:
> > - initial setting of mempolicy (NUMA_NO_NODE w/ cur_weight=0)
> > - weighted_interleave_nodes decrements it down to 0
> >
> > Now that i'm looking at it - the second condition should not exist, and
> > we can eliminate it. The logic in weighted_interleave_nodes is actually
> > annoyingly unclear at the moment, so I'm going to re-factor it a bit to
> > be more explicit.
>
> I am OK with either way. Just a reminder, the first condition may be
> true in alloc_pages_bulk_array_weighted_interleave() and perhaps some
> other places.
>
Yeah, the bulk allocator handles it correctly, it's just a matter of
clarity for weighted_interleave_nodes.
What isn't necessarily handled correctly is the rebind code. Rebind due
to a cgroup/mems_allowed change can cause a stale weight to be carried.
Basically cur_weight is not cleared, but the node it applied to may no
longer be the next node when next_node_in() is called.
The race condition is 1) exceedingly rare, and 2) not necessarily harmful,
just inaccurate. The worst case scenario is that a node receives up to 255
additional allocations once after a rebind (but more likely 10-20).
I was considering forcing the interleave forward like this:
@@ -356,6 +361,10 @@ static void mpol_rebind_nodemask(struct mempolicy *pol, const nodemask_t *nodes)
tmp = *nodes;
pol->nodes = tmp;
+
+ /* Weighted interleave policies are forced forward to the next node */
+ if (pol->mode & MPOL_WEIGHTED_INTERLEAVE)
+ pol->wil.cur_weight = 0;
}
But this creates 2 race conditions when we read cur_weight and nodemask
in the allocator path.
Example 1:
1) bulk allocator READ_ONCE(mask), READ_ONCE(cur_weight)
2) rebind changes nodemask and { cur_weight = 0; }
3) bulk allocator sets pol->wil.cur_weight
In this scenario, resume_weight is stale coming out of bulk allocations
if the resume_node has been removed from the node mask.
Example 2:
1) rebind changes nodemask
2) bulk allocator READ_ONCE(mask), READ_ONCE(cur_weight)
3) rebind sets { cur_weight = 0; }
In this scenario, cur_weight is stale going into bulk allocations.
Neither of these can force a violation of mems_allowed, just a
mis-application of a weight.
I'll need to think on this a bit. We can either leave this as-is,
meaning the first allocation after a rebind may apply the wrong weight
to a node, or we can try to track the current-interleave-node and
validate next_node_in(mask) == current-interleave-node before leaving
the allocator path (this may also be just as racey).
turns out concurrent counting is still hard :]
~Gregory
^ permalink raw reply
* Re: [RFC PATCH 2/9] ntsync: Reserve a minor device number and ioctl range.
From: Elizabeth Figura @ 2024-01-24 17:59 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Arnd Bergmann, linux-kernel, linux-api, wine-devel,
André Almeida, Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra
In-Reply-To: <2024012454-cosmos-sprint-7db7@gregkh>
On Wednesday, 24 January 2024 06:32:13 CST Greg Kroah-Hartman wrote:
> On Tue, Jan 23, 2024 at 09:43:09PM -0600, Elizabeth Figura wrote:
> > > Why do you need a fixed minor number? Can't your userspace handle
> > > dynamic numbers? What systems require a static value?
> >
> > I believe I added this because it's necessary for MODULE_ALIAS (and, more
> > broadly, because I was following the example of vaguely comparable devices
> > like /dev/loop-control). I suppose I could instead just remove MODULE_ALIAS
> > (or even remove the ability to compile ntsync as a module entirely).
>
> Do you really need MODULE_ALIAS()? Having it for char devices to be
> auto-loaded is not generally considered a good idea anymore as systems
> should have the module loaded before userspace goes and asks for it.
>
> It also reduces suprises when any random userspace program can cause
> kernel modules to be loaded for no real reason.
I think there's no reason we can't make loading the system's problem. I'll remove it in v2.
^ permalink raw reply
* Re: [RFC PATCH 1/9] ntsync: Introduce the ntsync driver and character device.
From: Elizabeth Figura @ 2024-01-24 17:51 UTC (permalink / raw)
To: Greg Kroah-Hartman, linux-kernel, linux-api, Arnd Bergmann
Cc: wine-devel, André Almeida, Wolfram Sang, Arkadiusz Hiler,
Peter Zijlstra
In-Reply-To: <bd3f6fcb-2239-4fd3-bb9a-c772bbce5a44@app.fastmail.com>
On Wednesday, 24 January 2024 01:38:52 CST Arnd Bergmann wrote:
> On Wed, Jan 24, 2024, at 01:40, Elizabeth Figura wrote:
> > ntsync uses a misc device as the simplest and least intrusive uAPI interface.
> >
> > Each file description on the device represents an isolated NT instance, intended
> > to correspond to a single NT virtual machine.
> >
> > Signed-off-by: Elizabeth Figura <zfigura@codeweavers.com>
>
> I'm looking at the ioctl interface to ensure it's well-formed.
>
> Your patches look ok from that perspective, but there are a
> few minor things I would check for consistency here:
>
> > +
> > +static const struct file_operations ntsync_fops = {
> > + .owner = THIS_MODULE,
> > + .open = ntsync_char_open,
> > + .release = ntsync_char_release,
> > + .unlocked_ioctl = ntsync_char_ioctl,
> > + .compat_ioctl = ntsync_char_ioctl,
> > + .llseek = no_llseek,
> > +};
>
> The .compat_ioctl pointer should point to compat_ptr_ioctl()
> since the actual ioctl commands all take pointers instead
> of interpreting the argument as a number.
>
> On x86 and arm64 this won't make a difference as compat_ptr()
> is a nop.
Thanks; will fix.
^ permalink raw reply
* Re: [net-next 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-24 15:37 UTC (permalink / raw)
To: netdev, linux-kernel
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba
In-Reply-To: <20240124025359.11419-4-jdamato@fastly.com>
On Wed, Jan 24, 2024 at 02:53:59AM +0000, 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>
> ---
> .../userspace-api/ioctl/ioctl-number.rst | 1 +
> fs/eventpoll.c | 41 +++++++++++++++++++
> include/uapi/linux/eventpoll.h | 12 ++++++
> 3 files changed, 54 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..d973147c015c 100644
> --- a/fs/eventpoll.c
> +++ b/fs/eventpoll.c
> @@ -869,6 +869,45 @@ 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;
> +
> + 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 +1014,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..9211103779c4 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 */
> + uint8_t data[118];
Err, just noticed that this should be a u8, instead. Sorry I missed this.
I assume there will be other feedback to address, but if not, I've noted
that I need to fix this in the v2.
^ permalink raw reply
* Re: [net-next 0/3] Per epoll context busy poll support
From: Joe Damato @ 2024-01-24 15:19 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, sridhar.samudrala, kuba, Wei Wang
In-Reply-To: <CANn89i+UiCRpu6M-hDga=dSTk1F5MjkgV=kKS6zC31pvOh78DQ@mail.gmail.com>
On Wed, Jan 24, 2024 at 03:38:19PM +0100, Eric Dumazet wrote:
> On Wed, Jan 24, 2024 at 3:20 PM Joe Damato <jdamato@fastly.com> wrote:
> >
> > On Wed, Jan 24, 2024 at 09:20:09AM +0100, Eric Dumazet wrote:
> > > On Wed, Jan 24, 2024 at 3:54 AM Joe Damato <jdamato@fastly.com> wrote:
> > > >
> > > > Greetings:
> > > >
> > > > 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.
> > > >
> > > > 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. 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.
> > > >
> > > > 4. Lastly, busy poll must be enabled via a sysctl
> > > > (/proc/sys/net/core/busy_poll).
> > > >
> > > > The unfortunate part about step 4 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.
> > > >
> > >
> > > I think this description missed the napi_defer_hard_irqs and
> > > gro_flush_timeout settings ?
> >
> > I'm not sure if those settings are strictly related to the change I am
> > proposing which makes epoll-based busy poll something that can be
> > enabled/disabled on a per-epoll context basis and allows the budget to be
> > set as well, but maybe I am missing something? Sorry for my
> > misunderstanding if so.
> >
> > IMHO: a single system-wide busy poll setting is difficult to use
> > properly and it is unforunate that the packet budget is hardcoded. It would
> > be extremely useful to be able to set both of these on a per-epoll basis
> > and I think my suggested change helps to solve this.
> >
> > Please let me know.
> >
> > Re the two settings you noted:
> >
> > I didn't mention those in the interest of brevity, but yes they can be used
> > instead of or in addition to what I've described above.
> >
> > While those settings are very useful, IMHO, they have their own issues
> > because they are system-wide as well. If they were settable per-NAPI, that
> > would make it much easier to use them because they could be enabled for the
> > NAPIs which are being busy-polled by applications that support busy-poll.
> >
> > Imagine you have 3 types of apps running side-by-side:
> > - A low latency epoll-based busy poll app,
> > - An app where latency doesn't matter as much, and
> > - A latency sensitive legacy app which does not yet support epoll-based
> > busy poll.
> >
> > In the first two cases, the settings you mention would be helpful or not
> > make any difference, but in the third case the system-wide impact might be
> > undesirable because having IRQs fire might be important to keep latency
> > down.
> >
> > If your comment was more that my cover letter should have mentioned these,
> > I can include that in a future cover letter or suggest some kernel
> > documentation which will discuss all of these features and how they relate
> > to each other.
> >
> > >
> > > I would think that if an application really wants to make sure its
> > > thread is the only one
> > > eventually calling napi->poll(), we must make sure NIC interrupts stay masked.
> > >
> > > Current implementations of busy poll always release NAPI_STATE_SCHED bit when
> > > returning to user space.
> > >
> > > It seems you want to make sure the application and only the
> > > application calls the napi->poll()
> > > at chosen times.
> > >
> > > Some kind of contract is needed, and the presence of the hrtimer
> > > (currently only driven from dev->@gro_flush_timeout)
> > > would allow to do that correctly.
> > >
> > > Whenever we 'trust' user space to perform the napi->poll shortly, we
> > > also want to arm the hrtimer to eventually detect
> > > the application took too long, to restart the other mechanisms (NIC irq based)
> >
> > There is another change [1] I've been looking at from a research paper [2]
> > which does something similar to what you've described above -- it keeps
> > IRQs suppressed during busy polling. The paper suggests a performance
> > improvement is measured when using a mechanism like this to keep IRQs off.
> > Please see the paper for more details.
> >
> > I haven't had a chance to reach out to the authors or to tweak this patch
> > to attempt an RFC / submission for it, but it seems fairly promising in my
> > initial synthetic tests.
> >
> > When I tested their patch, as you might expect, no IRQs were generated at
> > all for the NAPIs that were being busy polled, but the rest of the
> > NAPIs and queues were generating IRQs as expected.
> >
> > Regardless of the above patch: I think my proposed change is helpful and
> > the IRQ suppression bit can be handled in a separate change in the future.
> > What do you think?
> >
> > > Note that we added the kthread based napi polling, and we are working
> > > to add a busy polling feature to these kthreads.
> > > allowing to completely mask NIC interrupts and further reduce latencies.
> >
> > I am aware of kthread based NAPI polling, yes, but I was not aware that
> > busy polling was being considered as a feature for them, thanks for the
> > head's up.
> >
> > > Thank you
> >
> > Thanks for your comments - I appreciate your time and attention.
> >
> > Could you let me know if your comments are meant as a n-ack or similar?
>
> Patch #2 needs the 'why' part, and why would we allow user space to
> ask to poll up to 65535 packets...
> There is a reason we have a warning in place when a driver attempts to
> set a budget bigger than 64.
Sure, thanks for pointing this out.
I am happy to cap the budget to 64 if a user app requests a larger amount
and I can add a netdev_warn when this happens, if you'd like.
The 'why' has two reasons:
- Increasing the budget for fast NICs can help improve throughput
under load (i.e. the hardcoded amount might be too low for some users)
- other poll methods have user-configurable budget amounts
(SO_BUSY_POLL_BUDGET), so epoll stands out as an edge case where the
budget is hardcoded.
I hope that reasoning is sufficient and I can include that more explicitly
in the commit message.
FWIW: My reading of SO_BUSY_POLL_BUDGET suggests that any budget amount
up to U16_MAX will be accepted. I probably missed it somewhere, but I
didn't see a warning in this case.
I think in a v2 SO_BUSY_POLL_BUDGET and the epoll ioctl budget should
be capped at the same amount for consistency and I am happy to agree to 64
or 128 or similar as a cap.
Let me know what you think and thanks again for your thoughts and detailed
response.
> You cited recent papers, I wrote this one specific to linux busy
> polling ( https://netdevconf.info/2.1/papers/BusyPollingNextGen.pdf )
Thanks for sending this link, I'll need to take a closer look, but a quick
read surfaced two things:
Their use is very limited, since they enforce busy polling
for all sockets, which is not desirable
We agree on the limited usefulness of system-wide settings ;)
Another big problem is that Busy Polling was not really
deployed in production, because it works well when having
no more than one thread per NIC RX queue.
I've been testing epoll-based busy poll in production with a few different
NICs and application setups and it has been pretty helpful, but I agree
that this is application architecture specific as you allude to in
your next paragraph about the scheduler.
Thanks for linking to the paper.
It would be great if all of this context and information could be put in
one place in the kernel docs. If I have time in the future, I'll propose a
doc change to try to outline all of this.
> Busy polling had been in the pipe, when Wei sent her patches and follow ups.
>
> cb038357937ee4f589aab2469ec3896dce90f317 net: fix race between napi
> kthread mode and busy poll
> 5fdd2f0e5c64846bf3066689b73fc3b8dddd1c74 net: add sysfs attribute to
> control napi threaded mode
> 29863d41bb6e1d969c62fdb15b0961806942960e net: implement threaded-able
> napi poll loop support
Thanks for letting me know. I think I'd seen these in passing, but hadn't
remembered until you mentioned it now.
> I am saying that I am currently working to implement the kthread busy
> polling implementation,
> after fixing two bugs in SCTP and UDP (making me wondering if busy
> polling is really used these days)
Ah, I see. FWIW, I have so far only been trying to use it for TCP and so
far I haven't hit any bugs.
I was planning to use it with UDP in the future, though, once the TCP
epoll-based busy polling stuff I am working on is done... so thanks in
advance for the bug fixes in UDP.
> I am also considering unifying napi_threaded_poll() and the
> napi_busy_loop(), and seeing your patches
> coming make this work more challenging.
Sorry about that. I am happy to make modifications to my patches if there's
anything I could do which would make your work easier in the future.
Thanks,
Joe
^ permalink raw reply
* Re: [net-next 0/3] Per epoll context busy poll support
From: Eric Dumazet @ 2024-01-24 14:38 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, sridhar.samudrala, kuba, Wei Wang
In-Reply-To: <20240124142008.GA1448@fastly.com>
On Wed, Jan 24, 2024 at 3:20 PM Joe Damato <jdamato@fastly.com> wrote:
>
> On Wed, Jan 24, 2024 at 09:20:09AM +0100, Eric Dumazet wrote:
> > On Wed, Jan 24, 2024 at 3:54 AM Joe Damato <jdamato@fastly.com> wrote:
> > >
> > > Greetings:
> > >
> > > 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.
> > >
> > > 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. 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.
> > >
> > > 4. Lastly, busy poll must be enabled via a sysctl
> > > (/proc/sys/net/core/busy_poll).
> > >
> > > The unfortunate part about step 4 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.
> > >
> >
> > I think this description missed the napi_defer_hard_irqs and
> > gro_flush_timeout settings ?
>
> I'm not sure if those settings are strictly related to the change I am
> proposing which makes epoll-based busy poll something that can be
> enabled/disabled on a per-epoll context basis and allows the budget to be
> set as well, but maybe I am missing something? Sorry for my
> misunderstanding if so.
>
> IMHO: a single system-wide busy poll setting is difficult to use
> properly and it is unforunate that the packet budget is hardcoded. It would
> be extremely useful to be able to set both of these on a per-epoll basis
> and I think my suggested change helps to solve this.
>
> Please let me know.
>
> Re the two settings you noted:
>
> I didn't mention those in the interest of brevity, but yes they can be used
> instead of or in addition to what I've described above.
>
> While those settings are very useful, IMHO, they have their own issues
> because they are system-wide as well. If they were settable per-NAPI, that
> would make it much easier to use them because they could be enabled for the
> NAPIs which are being busy-polled by applications that support busy-poll.
>
> Imagine you have 3 types of apps running side-by-side:
> - A low latency epoll-based busy poll app,
> - An app where latency doesn't matter as much, and
> - A latency sensitive legacy app which does not yet support epoll-based
> busy poll.
>
> In the first two cases, the settings you mention would be helpful or not
> make any difference, but in the third case the system-wide impact might be
> undesirable because having IRQs fire might be important to keep latency
> down.
>
> If your comment was more that my cover letter should have mentioned these,
> I can include that in a future cover letter or suggest some kernel
> documentation which will discuss all of these features and how they relate
> to each other.
>
> >
> > I would think that if an application really wants to make sure its
> > thread is the only one
> > eventually calling napi->poll(), we must make sure NIC interrupts stay masked.
> >
> > Current implementations of busy poll always release NAPI_STATE_SCHED bit when
> > returning to user space.
> >
> > It seems you want to make sure the application and only the
> > application calls the napi->poll()
> > at chosen times.
> >
> > Some kind of contract is needed, and the presence of the hrtimer
> > (currently only driven from dev->@gro_flush_timeout)
> > would allow to do that correctly.
> >
> > Whenever we 'trust' user space to perform the napi->poll shortly, we
> > also want to arm the hrtimer to eventually detect
> > the application took too long, to restart the other mechanisms (NIC irq based)
>
> There is another change [1] I've been looking at from a research paper [2]
> which does something similar to what you've described above -- it keeps
> IRQs suppressed during busy polling. The paper suggests a performance
> improvement is measured when using a mechanism like this to keep IRQs off.
> Please see the paper for more details.
>
> I haven't had a chance to reach out to the authors or to tweak this patch
> to attempt an RFC / submission for it, but it seems fairly promising in my
> initial synthetic tests.
>
> When I tested their patch, as you might expect, no IRQs were generated at
> all for the NAPIs that were being busy polled, but the rest of the
> NAPIs and queues were generating IRQs as expected.
>
> Regardless of the above patch: I think my proposed change is helpful and
> the IRQ suppression bit can be handled in a separate change in the future.
> What do you think?
>
> > Note that we added the kthread based napi polling, and we are working
> > to add a busy polling feature to these kthreads.
> > allowing to completely mask NIC interrupts and further reduce latencies.
>
> I am aware of kthread based NAPI polling, yes, but I was not aware that
> busy polling was being considered as a feature for them, thanks for the
> head's up.
>
> > Thank you
>
> Thanks for your comments - I appreciate your time and attention.
>
> Could you let me know if your comments are meant as a n-ack or similar?
Patch #2 needs the 'why' part, and why would we allow user space to
ask to poll up to 65535 packets...
There is a reason we have a warning in place when a driver attempts to
set a budget bigger than 64.
You cited recent papers, I wrote this one specific to linux busy
polling ( https://netdevconf.info/2.1/papers/BusyPollingNextGen.pdf )
Busy polling had been in the pipe, when Wei sent her patches and follow ups.
cb038357937ee4f589aab2469ec3896dce90f317 net: fix race between napi
kthread mode and busy poll
5fdd2f0e5c64846bf3066689b73fc3b8dddd1c74 net: add sysfs attribute to
control napi threaded mode
29863d41bb6e1d969c62fdb15b0961806942960e net: implement threaded-able
napi poll loop support
I am saying that I am currently working to implement the kthread busy
polling implementation,
after fixing two bugs in SCTP and UDP (making me wondering if busy
polling is really used these days)
I am also considering unifying napi_threaded_poll() and the
napi_busy_loop(), and seeing your patches
coming make this work more challenging.
^ permalink raw reply
* Re: [net-next 0/3] Per epoll context busy poll support
From: Joe Damato @ 2024-01-24 14:20 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, sridhar.samudrala, kuba, Wei Wang
In-Reply-To: <CANn89i+YKwrgpt8VnHrw4eeVpqRamLkTSr4u+g1mRDMZa6b+7Q@mail.gmail.com>
On Wed, Jan 24, 2024 at 09:20:09AM +0100, Eric Dumazet wrote:
> On Wed, Jan 24, 2024 at 3:54 AM Joe Damato <jdamato@fastly.com> wrote:
> >
> > Greetings:
> >
> > 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.
> >
> > 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. 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.
> >
> > 4. Lastly, busy poll must be enabled via a sysctl
> > (/proc/sys/net/core/busy_poll).
> >
> > The unfortunate part about step 4 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.
> >
>
> I think this description missed the napi_defer_hard_irqs and
> gro_flush_timeout settings ?
I'm not sure if those settings are strictly related to the change I am
proposing which makes epoll-based busy poll something that can be
enabled/disabled on a per-epoll context basis and allows the budget to be
set as well, but maybe I am missing something? Sorry for my
misunderstanding if so.
IMHO: a single system-wide busy poll setting is difficult to use
properly and it is unforunate that the packet budget is hardcoded. It would
be extremely useful to be able to set both of these on a per-epoll basis
and I think my suggested change helps to solve this.
Please let me know.
Re the two settings you noted:
I didn't mention those in the interest of brevity, but yes they can be used
instead of or in addition to what I've described above.
While those settings are very useful, IMHO, they have their own issues
because they are system-wide as well. If they were settable per-NAPI, that
would make it much easier to use them because they could be enabled for the
NAPIs which are being busy-polled by applications that support busy-poll.
Imagine you have 3 types of apps running side-by-side:
- A low latency epoll-based busy poll app,
- An app where latency doesn't matter as much, and
- A latency sensitive legacy app which does not yet support epoll-based
busy poll.
In the first two cases, the settings you mention would be helpful or not
make any difference, but in the third case the system-wide impact might be
undesirable because having IRQs fire might be important to keep latency
down.
If your comment was more that my cover letter should have mentioned these,
I can include that in a future cover letter or suggest some kernel
documentation which will discuss all of these features and how they relate
to each other.
>
> I would think that if an application really wants to make sure its
> thread is the only one
> eventually calling napi->poll(), we must make sure NIC interrupts stay masked.
>
> Current implementations of busy poll always release NAPI_STATE_SCHED bit when
> returning to user space.
>
> It seems you want to make sure the application and only the
> application calls the napi->poll()
> at chosen times.
>
> Some kind of contract is needed, and the presence of the hrtimer
> (currently only driven from dev->@gro_flush_timeout)
> would allow to do that correctly.
>
> Whenever we 'trust' user space to perform the napi->poll shortly, we
> also want to arm the hrtimer to eventually detect
> the application took too long, to restart the other mechanisms (NIC irq based)
There is another change [1] I've been looking at from a research paper [2]
which does something similar to what you've described above -- it keeps
IRQs suppressed during busy polling. The paper suggests a performance
improvement is measured when using a mechanism like this to keep IRQs off.
Please see the paper for more details.
I haven't had a chance to reach out to the authors or to tweak this patch
to attempt an RFC / submission for it, but it seems fairly promising in my
initial synthetic tests.
When I tested their patch, as you might expect, no IRQs were generated at
all for the NAPIs that were being busy polled, but the rest of the
NAPIs and queues were generating IRQs as expected.
Regardless of the above patch: I think my proposed change is helpful and
the IRQ suppression bit can be handled in a separate change in the future.
What do you think?
> Note that we added the kthread based napi polling, and we are working
> to add a busy polling feature to these kthreads.
> allowing to completely mask NIC interrupts and further reduce latencies.
I am aware of kthread based NAPI polling, yes, but I was not aware that
busy polling was being considered as a feature for them, thanks for the
head's up.
> Thank you
Thanks for your comments - I appreciate your time and attention.
Could you let me know if your comments are meant as a n-ack or similar?
I am unsure if you were suggesting that per-epoll context based busy
polling is unneeded/unnecessary from your perspective - or if it was more
of a hint that I should be including more context somewhere in the kernel
documentation as part of this change :)
Again, IMHO, allowing epoll based busy polling to be configured on a
per-epoll context basis (both the usecs and the packet budget) really help
to make epoll-based busy polling much more usable by user apps.
Thanks,
Joe
[1]: https://gitlab.uwaterloo.ca/p5cai/netstack-exp/-/raw/master/kernel-polling-5.15.79-base.patch?ref_type=heads
[2]: https://dl.acm.org/doi/pdf/10.1145/3626780
^ permalink raw reply
* Re: [PATCH] selftests/landlock:Fix fs_test build issues with old libc
From: Mickaël Salaün @ 2024-01-24 13:13 UTC (permalink / raw)
To: Hu Yadi
Cc: jmorris, serge, shuah, mathieu.desnoyers, amir73il, brauner,
avagin, linux-api, linux-kernel, linux-security-module,
linux-kselftest, 514118380, konstantin.meskhidze
In-Reply-To: <20240124022908.42100-1-hu.yadi@h3c.com>
Thanks, it's merged with some fixes:
https://git.kernel.org/mic/c/82852a3cc2152eb7c7b7007b6430faa979b08fad
On Wed, Jan 24, 2024 at 10:29:08AM +0800, Hu Yadi wrote:
> From: "Hu.Yadi" <hu.yadi@h3c.com>
You might want to fix the extra dot in your name.
>
> Fixes: 04f9070e99a4 ("selftests/landlock: Add tests for pseudo filesystems")
>
> one issues comes up while building selftest/landlock on my side
> (gcc 7.3/glibc-2.28/kernel-4.19)
>
> gcc -Wall -O2 -isystem fs_test.c -lcap -o selftests/landlock/fs_test
> fs_test.c:4575:9: error: initializer element is not constant
> .mnt = mnt_tmp,
> ^~~~~~~
>
> Signed-off-by: Hu.Yadi <hu.yadi@h3c.com>
> Suggested-by: Jiao <jiaoxupo@h3c.com>
> Reviewed-by: Berlin <berlin@h3c.com>
> ---
> tools/testing/selftests/landlock/fs_test.c | 6 +++++-
> 1 file changed, 5 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/landlock/fs_test.c b/tools/testing/selftests/landlock/fs_test.c
> index 18e1f86a6234..1f2584b4dfce 100644
> --- a/tools/testing/selftests/landlock/fs_test.c
> +++ b/tools/testing/selftests/landlock/fs_test.c
> @@ -40,6 +40,7 @@ int renameat2(int olddirfd, const char *oldpath, int newdirfd,
>
> #define TMP_DIR "tmp"
> #define BINARY_PATH "./true"
> +#define MNT_TMP_DATA "size=4m,mode=700"
The idea was to reuse MNT_TMP_DATA for mnt_tmp too. I fixed that in the
applied patch, see my next branch.
>
> /* Paths (sibling number and depth) */
> static const char dir_s1d1[] = TMP_DIR "/s1d1";
> @@ -4572,7 +4573,10 @@ FIXTURE_VARIANT(layout3_fs)
> /* clang-format off */
> FIXTURE_VARIANT_ADD(layout3_fs, tmpfs) {
> /* clang-format on */
> - .mnt = mnt_tmp,
> + .mnt = {
> + .type = "tmpfs",
> + .data = MNT_TMP_DATA,
> + },
> .file_path = file1_s1d1,
> };
>
> --
> 2.23.0
>
>
^ permalink raw reply
* Re: [RFC PATCH 2/9] ntsync: Reserve a minor device number and ioctl range.
From: Greg Kroah-Hartman @ 2024-01-24 12:32 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Arnd Bergmann, linux-kernel, linux-api, wine-devel,
André Almeida, Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra
In-Reply-To: <1875326.tdWV9SEqCh@terabithia>
On Tue, Jan 23, 2024 at 09:43:09PM -0600, Elizabeth Figura wrote:
> > Why do you need a fixed minor number? Can't your userspace handle
> > dynamic numbers? What systems require a static value?
>
> I believe I added this because it's necessary for MODULE_ALIAS (and, more
> broadly, because I was following the example of vaguely comparable devices
> like /dev/loop-control). I suppose I could instead just remove MODULE_ALIAS
> (or even remove the ability to compile ntsync as a module entirely).
Do you really need MODULE_ALIAS()? Having it for char devices to be
auto-loaded is not generally considered a good idea anymore as systems
should have the module loaded before userspace goes and asks for it.
It also reduces suprises when any random userspace program can cause
kernel modules to be loaded for no real reason.
> It's a bit difficult to figure out what's the preferred way to organize things
> like this (there not being a lot of precedent for this kind of driver) so I'd
> appreciate any direction.
For now, I'd just stick to a dynamic id, no module alias, and if it's
ever needed in the future, it can be added. But if you add it now, we
can't ever remove it as it's user-visible functionality.
thanks,
greg k-h
^ permalink raw reply
* Re: [RFC PATCH 0/9] NT synchronization primitive driver
From: Greg Kroah-Hartman @ 2024-01-24 12:29 UTC (permalink / raw)
To: Elizabeth Figura
Cc: Arnd Bergmann, linux-kernel, linux-api, wine-devel,
André Almeida, Wolfram Sang, Arkadiusz Hiler, Peter Zijlstra
In-Reply-To: <12365105.O9o76ZdvQC@camazotz>
On Tue, Jan 23, 2024 at 07:37:01PM -0600, Elizabeth Figura wrote:
> On Tuesday, 23 January 2024 18:59:35 CST Greg Kroah-Hartman wrote:
> > On Tue, Jan 23, 2024 at 06:40:19PM -0600, Elizabeth Figura wrote:
> > > == Patches ==
> > >
> > > This is the first part of a 32-patch series. The series comprises 17 patches
> > > which contain the actual implementation, 13 which provide self-tests, 1 to
> > > update the MAINTAINERS file, and 1 to add API documentation.
> >
> > 32 patches? I only see 9 here, why not submit them all?
>
> Because Documentation/process/submitting-patches.rst makes a point of asking people not to submit large patch series (and it matches the expectation of other projects I've worked with—that patches would be submitted and reviewed a few at a time). I suppose I've misunderstood that advice, though.
32 patches isn't all that "large", we can handle that easily :)
100+ patches is large, I guess it all depends, so I can understand the
confusion. You need to send us enough for us to be able to understand
and review the code, this was a bit short for that.
> I'll resend with the entire series. Sorry for the noise.
Ptches are NOT noise, we want to see them!
thanks,
greg k-h
^ permalink raw reply
* [PATCH v2 2/3] readv.2: Document RWF_ATOMIC flag
From: John Garry @ 2024-01-24 11:27 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: martin.petersen, djwong, david, himanshu.madhani, hch, viro,
brauner, jack, John Garry
In-Reply-To: <20240124112731.28579-1-john.g.garry@oracle.com>
From: Himanshu Madhani <himanshu.madhani@oracle.com>
Add RWF_ATOMIC flag description for pwritev2().
Signed-off-by: Himanshu Madhani <himanshu.madhani@oracle.com>
#jpg: complete rewrite
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
man2/readv.2 | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 72 insertions(+), 1 deletion(-)
diff --git a/man2/readv.2 b/man2/readv.2
index d39f2b9b9..aa5d6576b 100644
--- a/man2/readv.2
+++ b/man2/readv.2
@@ -193,6 +193,61 @@ which provides lower latency, but may use additional resources.
.B O_DIRECT
flag.)
.TP
+.BR RWF_ATOMIC " (since Linux 6.8)"
+Requires that writes to regular files in block-based filesystems be issued with
+torn-write protection. Torn-write protection means that for a power failure or
+any other hardware failure, all or none of the data from the write will be
+stored, but never a mix of old and new data. This flag is meaningful only for
+.BR pwritev2 (),
+and its effect applies only to the data range written by the system call.
+The total write length must be power-of-2 and must be sized between
+.I stx_atomic_write_unit_min
+ and
+.I stx_atomic_write_unit_max
+, both inclusive. The
+write must be at a naturally-aligned offset within the file with respect to the
+total write length - for example, a write of length 32KB at a file offset of
+32KB is permitted, however a write of length 32KB at a file offset of 48KB is
+not permitted. The upper limit of
+.I iovcnt
+for
+.BR pwritev2 ()
+is in
+.I stx_atomic_write_segments_max.
+Torn-write protection only works with
+.B O_DIRECT
+flag, i.e. buffered writes are not supported. To guarantee consistency from
+the write between a file's in-core state with the storage device,
+.BR fdatasync (2),
+or
+.BR fsync (2),
+or
+.BR open (2)
+and either
+.B O_SYNC
+or
+.B O_DSYNC,
+or
+.B pwritev2 ()
+and either
+.B RWF_SYNC
+or
+.B RWF_DSYNC
+is required. Flags
+.B O_SYNC
+or
+.B RWF_SYNC
+provide the strongest guarantees for
+.BR RWF_ATOMIC,
+in that all data and also file metadata updates will be persisted for a
+successfully completed write. Just using either flags
+.B O_DSYNC
+or
+.B RWF_DSYNC
+means that all data and any file updates will be persisted for a successfully
+completed write. Not using any sync flags means that there
+is no guarantee that data or filesystem updates are persisted.
+.TP
.BR RWF_SYNC " (since Linux 4.7)"
.\" commit e864f39569f4092c2b2bc72c773b6e486c7e3bd9
Provide a per-write equivalent of the
@@ -279,10 +334,26 @@ values overflows an
.I ssize_t
value.
.TP
+.B EINVAL
+ For
+.BR RWF_ATOMIC
+set,
+the combination of the sum of the
+.I iov_len
+values and the
+.I offset
+value
+does not comply with the length and offset torn-write protection rules.
+.TP
.B EINVAL
The vector count,
.IR iovcnt ,
-is less than zero or greater than the permitted maximum.
+is less than zero or greater than the permitted maximum. For
+.BR RWF_ATOMIC
+set, this maximum is in
+.I stx_atomic_write_segments_max
+from
+.I statx.
.TP
.B EOPNOTSUPP
An unknown flag is specified in \fIflags\fP.
--
2.31.1
^ permalink raw reply related
* [PATCH v2 1/3] statx.2: Document STATX_WRITE_ATOMIC
From: John Garry @ 2024-01-24 11:27 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: martin.petersen, djwong, david, himanshu.madhani, hch, viro,
brauner, jack, John Garry
In-Reply-To: <20240124112731.28579-1-john.g.garry@oracle.com>
From: Himanshu Madhani <himanshu.madhani@oracle.com>
Add the text to the statx man page.
Signed-off-by: Himanshu Madhani <himanshu.madhani@oracle.com>
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
man2/statx.2 | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)
diff --git a/man2/statx.2 b/man2/statx.2
index 0dcf7e20b..aa056ecdf 100644
--- a/man2/statx.2
+++ b/man2/statx.2
@@ -68,6 +68,10 @@ struct statx {
/* Direct I/O alignment restrictions */
__u32 stx_dio_mem_align;
__u32 stx_dio_offset_align;
+\&
+ __u32 stx_atomic_write_unit_min;
+ __u32 stx_atomic_write_unit_max;
+ __u32 stx_atomic_write_segments_max;
};
.EE
.in
@@ -255,6 +259,9 @@ STATX_ALL The same as STATX_BASIC_STATS | STATX_BTIME.
STATX_MNT_ID Want stx_mnt_id (since Linux 5.8)
STATX_DIOALIGN Want stx_dio_mem_align and stx_dio_offset_align
(since Linux 6.1; support varies by filesystem)
+STATX_WRITE_ATOMIC Want stx_atomic_write_unit_min, stx_atomic_write_unit_max,
+ and stx_atomic_write_segments_max.
+ (since Linux 6.9; support varies by filesystem)
.TE
.in
.P
@@ -439,6 +446,25 @@ or 0 if direct I/O is not supported on this file.
This will only be nonzero if
.I stx_dio_mem_align
is nonzero, and vice versa.
+.TP
+.I stx_atomic_write_unit_min
+The minimum size (in bytes) supported for direct I/O
+.RB ( O_DIRECT )
+on the file to be written with torn-write protection. This value is guaranteed
+to be a power-of-2.
+.TP
+.I stx_atomic_write_unit_max
+The maximum size (in bytes) supported for direct I/O
+.RB ( O_DIRECT )
+on the file to be written with torn-write protection. This value is guaranteed
+to be a power-of-2.
+.TP
+.I stx_atomic_write_segments_max
+The maximum number of elements in an array of vectors for a write with
+torn-write protection enabled. See
+.BR RWF_ATOMIC
+flag for
+.BR pwritev2 (2).
.P
For further information on the above fields, see
.BR inode (7).
@@ -492,6 +518,9 @@ It cannot be written to, and all reads from it will be verified
against a cryptographic hash that covers the
entire file (e.g., via a Merkle tree).
.TP
+.BR STATX_ATTR_WRITE_ATOMIC " (since Linux 6.8)"
+The file supports torn-write protection.
+.TP
.BR STATX_ATTR_DAX " (since Linux 5.8)"
The file is in the DAX (cpu direct access) state.
DAX state attempts to
--
2.31.1
^ permalink raw reply related
* [PATCH v2 3/3] io_submit.2: Document RWF_ATOMIC
From: John Garry @ 2024-01-24 11:27 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: martin.petersen, djwong, david, himanshu.madhani, hch, viro,
brauner, jack, John Garry
In-Reply-To: <20240124112731.28579-1-john.g.garry@oracle.com>
Document RWF_ATOMIC for asynchronous I/O.
Signed-off-by: John Garry <john.g.garry@oracle.com>
---
man2/io_submit.2 | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/man2/io_submit.2 b/man2/io_submit.2
index c53ae9aaf..fb6c4d9bc 100644
--- a/man2/io_submit.2
+++ b/man2/io_submit.2
@@ -140,6 +140,23 @@ as well the description of
.B O_SYNC
in
.BR open (2).
+.TP
+.BR RWF_ATOMIC " (since Linux 6.9)"
+Write a block of data such that a write will never be
+torn from power fail or similar. See the description
+of the flag of the same name in
+.BR pwritev2 (2).
+For usage with
+.BR IOCB_CMD_PWRITEV,
+the upper vector limit is in
+.I stx_atomic_write_segments_max.
+See
+.B STATX_WRITE_ATOMIC
+and
+.I stx_atomic_write_segments_max
+description
+in
+.BR statx (2).
.RE
.TP
.I aio_lio_opcode
--
2.31.1
^ permalink raw reply related
* [PATCH v2 0/3] man2: Document RWF_ATOMIC
From: John Garry @ 2024-01-24 11:27 UTC (permalink / raw)
To: linux-kernel, linux-api
Cc: martin.petersen, djwong, david, himanshu.madhani, hch, viro,
brauner, jack, John Garry
Document RWF_ATOMIC flag for pwritev2().
RWF_ATOMIC atomic is used for enabling torn-write protection.
We use RWF_ATOMIC as this is legacy name for similar feature proposed in
the past.
Differences to v1:
- Add statx max segments param
- Expand readv.2 description
- Document EINVAL
Himanshu Madhani (2):
statx.2: Document STATX_WRITE_ATOMIC
readv.2: Document RWF_ATOMIC flag
John Garry (1):
io_submit.2: Document RWF_ATOMIC
man2/io_submit.2 | 17 +++++++++++
man2/readv.2 | 73 +++++++++++++++++++++++++++++++++++++++++++++++-
man2/statx.2 | 29 +++++++++++++++++++
3 files changed, 118 insertions(+), 1 deletion(-)
--
2.31.1
^ permalink raw reply
* Re: [net-next 0/3] Per epoll context busy poll support
From: Eric Dumazet @ 2024-01-24 8:20 UTC (permalink / raw)
To: Joe Damato
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, sridhar.samudrala, kuba, Wei Wang
In-Reply-To: <20240124025359.11419-1-jdamato@fastly.com>
On Wed, Jan 24, 2024 at 3:54 AM Joe Damato <jdamato@fastly.com> wrote:
>
> Greetings:
>
> 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.
>
> 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. 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.
>
> 4. Lastly, busy poll must be enabled via a sysctl
> (/proc/sys/net/core/busy_poll).
>
> The unfortunate part about step 4 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.
>
I think this description missed the napi_defer_hard_irqs and
gro_flush_timeout settings ?
I would think that if an application really wants to make sure its
thread is the only one
eventually calling napi->poll(), we must make sure NIC interrupts stay masked.
Current implementations of busy poll always release NAPI_STATE_SCHED bit when
returning to user space.
It seems you want to make sure the application and only the
application calls the napi->poll()
at chosen times.
Some kind of contract is needed, and the presence of the hrtimer
(currently only driven from dev->@gro_flush_timeout)
would allow to do that correctly.
Whenever we 'trust' user space to perform the napi->poll shortly, we
also want to arm the hrtimer to eventually detect
the application took too long, to restart the other mechanisms (NIC irq based)
Note that we added the kthread based napi polling, and we are working
to add a busy polling feature to these kthreads.
allowing to completely mask NIC interrupts and further reduce latencies.
Thank you
> Thanks,
> Joe
>
> [1]: https://lore.kernel.org/lkml/20170324170836.15226.87178.stgit@localhost.localdomain/
>
> 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 | 99 ++++++++++++++++++-
> include/uapi/linux/eventpoll.h | 12 +++
> 3 files changed, 107 insertions(+), 5 deletions(-)
>
> --
> 2.25.1
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox