* [PATCH net-next v4 3/3] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-01-31 1:47 UTC (permalink / raw)
To: linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, David.Laight, arnd, Joe Damato, Jonathan Corbet,
Alexander Viro, Jan Kara, Michael Ellerman, Greg Kroah-Hartman,
Nathan Lynch, Thomas Zimmermann, Maik Broemme, Steve French,
Julien Panis, Jiri Slaby, Thomas Huth, Andrew Waterman, Albert Ou,
Palmer Dabbelt, open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240131014738.469858-1-jdamato@fastly.com>
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.
Parameters are limited:
- busy_poll_usecs is limited to <= u32_max
- busy_poll_budget is limited to <= NAPI_POLL_WEIGHT by unprivileged
users (!capable(CAP_NET_ADMIN)).
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
fs/eventpoll.c | 65 +++++++++++++++++++
include/uapi/linux/eventpoll.h | 12 ++++
3 files changed, 78 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 3985434df527..afdb91c6faa8 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -37,6 +37,7 @@
#include <linux/seq_file.h>
#include <linux/compat.h>
#include <linux/rculist.h>
+#include <linux/capability.h>
#include <net/busy_poll.h>
/*
@@ -495,6 +496,42 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
ep->napi_id = napi_id;
}
+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct eventpoll *ep;
+ struct epoll_params epoll_params;
+ void __user *uarg = (void __user *) arg;
+
+ ep = file->private_data;
+
+ switch (cmd) {
+ case EPIOCSPARAMS:
+ if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
+ return -EFAULT;
+
+ if (epoll_params.busy_poll_usecs > U32_MAX)
+ return -EINVAL;
+
+ if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
+ !capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
+ ep->busy_poll_budget = epoll_params.busy_poll_budget;
+ 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;
+ default:
+ return -ENOIOCTLCMD;
+ }
+}
+
#else
static inline bool ep_busy_loop(struct eventpoll *ep, int nonblock)
@@ -510,6 +547,12 @@ static inline bool ep_busy_loop_on(struct eventpoll *ep)
{
return false;
}
+
+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return -EOPNOTSUPP;
+}
#endif /* CONFIG_NET_RX_BUSY_POLL */
/*
@@ -869,6 +912,26 @@ static void ep_clear_and_put(struct eventpoll *ep)
ep_free(ep);
}
+static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ int ret;
+
+ if (!is_file_epoll(file))
+ return -EINVAL;
+
+ switch (cmd) {
+ case EPIOCSPARAMS:
+ case EPIOCGPARAMS:
+ ret = ep_eventpoll_bp_ioctl(file, cmd, arg);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
static int ep_eventpoll_release(struct inode *inode, struct file *file)
{
struct eventpoll *ep = file->private_data;
@@ -975,6 +1038,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..98e5ea525dd0 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 {
+ __aligned_u64 busy_poll_usecs;
+ __u16 busy_poll_budget;
+
+ /* pad the struct to a multiple of 64bits for alignment on all arches */
+ __u8 __pad[6];
+};
+
+#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
^ permalink raw reply related
* [PATCH net-next v4 2/3] eventpoll: Add per-epoll busy poll packet budget
From: Joe Damato @ 2024-01-31 1:47 UTC (permalink / raw)
To: linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, David.Laight, arnd, Joe Damato, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240131014738.469858-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 ce75189d46df..3985434df527 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -229,6 +229,8 @@ struct eventpoll {
unsigned int napi_id;
/* busy poll timeout */
u64 busy_poll_usecs;
+ /* busy poll packet budget */
+ u16 busy_poll_budget;
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -437,10 +439,14 @@ static bool ep_busy_loop_end(void *p, unsigned long start_time)
static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
{
unsigned int napi_id = READ_ONCE(ep->napi_id);
+ u16 budget = READ_ONCE(ep->busy_poll_budget);
+
+ if (!budget)
+ budget = BUSY_POLL_BUDGET;
if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
- BUSY_POLL_BUDGET);
+ budget);
if (ep_events_available(ep))
return true;
/*
@@ -2098,6 +2104,7 @@ static int do_epoll_create(int flags)
}
#ifdef CONFIG_NET_RX_BUSY_POLL
ep->busy_poll_usecs = 0;
+ ep->busy_poll_budget = 0;
#endif
ep->file = file;
fd_install(fd, file);
--
2.25.1
^ permalink raw reply related
* [PATCH net-next v4 1/3] eventpoll: support busy poll per epoll instance
From: Joe Damato @ 2024-01-31 1:47 UTC (permalink / raw)
To: linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, David.Laight, arnd, Joe Damato, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240131014738.469858-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..ce75189d46df 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -227,6 +227,8 @@ struct eventpoll {
#ifdef CONFIG_NET_RX_BUSY_POLL
/* used to track busy poll napi_id */
unsigned int napi_id;
+ /* busy poll timeout */
+ u64 busy_poll_usecs;
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -386,12 +388,44 @@ static inline int ep_events_available(struct eventpoll *ep)
READ_ONCE(ep->ovflist) != EP_UNACTIVE_PTR;
}
+/**
+ * busy_loop_ep_timeout - check if busy poll has timed out. The timeout value
+ * from the epoll instance ep is preferred, but if it is not set fallback to
+ * the system-wide global via busy_loop_timeout.
+ *
+ * @start_time: The start time used to compute the remaining time until timeout.
+ * @ep: Pointer to the eventpoll context.
+ *
+ * Return: true if the timeout has expired, false otherwise.
+ */
+static inline bool busy_loop_ep_timeout(unsigned long start_time, struct eventpoll *ep)
+{
+#ifdef CONFIG_NET_RX_BUSY_POLL
+ unsigned long bp_usec = READ_ONCE(ep->busy_poll_usecs);
+
+ if (bp_usec) {
+ unsigned long end_time = start_time + bp_usec;
+ unsigned long now = busy_loop_current_time();
+
+ return time_after(now, end_time);
+ } else {
+ return busy_loop_timeout(start_time);
+ }
+#endif
+ return true;
+}
+
#ifdef CONFIG_NET_RX_BUSY_POLL
+static bool ep_busy_loop_on(struct eventpoll *ep)
+{
+ return !!ep->busy_poll_usecs || net_busy_loop_on();
+}
+
static bool ep_busy_loop_end(void *p, unsigned long start_time)
{
struct eventpoll *ep = p;
- return ep_events_available(ep) || busy_loop_timeout(start_time);
+ return ep_events_available(ep) || busy_loop_ep_timeout(start_time, ep);
}
/*
@@ -404,7 +438,7 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
{
unsigned int napi_id = READ_ONCE(ep->napi_id);
- if ((napi_id >= MIN_NAPI_ID) && net_busy_loop_on()) {
+ if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
BUSY_POLL_BUDGET);
if (ep_events_available(ep))
@@ -430,7 +464,8 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
struct socket *sock;
struct sock *sk;
- if (!net_busy_loop_on())
+ ep = epi->ep;
+ if (!ep_busy_loop_on(ep))
return;
sock = sock_from_file(epi->ffd.file);
@@ -442,7 +477,6 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
return;
napi_id = READ_ONCE(sk->sk_napi_id);
- ep = epi->ep;
/* Non-NAPI IDs can be rejected
* or
@@ -466,6 +500,10 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
{
}
+static inline bool ep_busy_loop_on(struct eventpoll *ep)
+{
+ return false;
+}
#endif /* CONFIG_NET_RX_BUSY_POLL */
/*
@@ -2058,6 +2096,9 @@ static int do_epoll_create(int flags)
error = PTR_ERR(file);
goto out_free_fd;
}
+#ifdef CONFIG_NET_RX_BUSY_POLL
+ ep->busy_poll_usecs = 0;
+#endif
ep->file = file;
fd_install(fd, file);
return fd;
--
2.25.1
^ permalink raw reply related
* [PATCH net-next v4 0/3] Per epoll context busy poll support
From: Joe Damato @ 2024-01-31 1:47 UTC (permalink / raw)
To: linux-kernel, netdev
Cc: chuck.lever, jlayton, linux-api, brauner, edumazet, davem,
alexander.duyck, sridhar.samudrala, kuba, willemdebruijn.kernel,
weiwan, David.Laight, arnd, Joe Damato, Albert Ou, Alexander Viro,
Andrew Waterman, Greg Kroah-Hartman, Jan Kara, Jiri Slaby,
Jonathan Corbet, Julien Panis, open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure), Michael Ellerman,
Namjae Jeon, Nathan Lynch, Palmer Dabbelt, Steve French,
Thomas Huth, Thomas Zimmermann
Greetings:
Welcome to v4. Important functional change to patch 1/4 explained below and
in change list at the end.
TL;DR This builds on commit bf3b9f6372c4 ("epoll: Add busy poll support to
epoll with socket fds.") by allowing user applications to enable
epoll-based busy polling and set a busy poll packet budget on a per epoll
context basis.
This makes epoll-based busy polling much more usable for user
applications than the current system-wide sysctl and hardcoded budget.
To allow for this, two ioctls have been added for epoll contexts for
getting and setting a new struct, struct epoll_params.
ioctl was chosen vs a new syscall after reviewing a suggestion by Willem
de Bruijn [1]. I am open to using a new syscall instead of an ioctl, but it
seemed that:
- Busy poll affects all existing epoll_wait and epoll_pwait variants in
the same way, so new verions of many syscalls might be needed. It
seems much simpler for users to use the correct
epoll_wait/epoll_pwait for their app and add a call to ioctl to enable
or disable busy poll as needed. This also probably means less work to
get an existing epoll app using busy poll.
- previously added epoll_pwait2 helped to bring epoll closer to
existing syscalls (like pselect and ppoll) and this busy poll change
reflected as a new syscall would not have the same effect.
Note: patch 1/4 as of v4 uses an or (||) instead of an xor. I thought about
it some more and I realized that if the user enables both the per-epoll
context setting and the system wide sysctl, then busy poll should be
enabled and not disabled. Using xor doesn't seem to make much sense after
thinking through this a bit.
Longer explanation:
Presently epoll has support for a very useful form of busy poll based on
the incoming NAPI ID (see also: SO_INCOMING_NAPI_ID [2]).
This form of busy poll allows epoll_wait to drive NAPI packet processing
which allows for a few interesting user application designs which can
reduce latency and also potentially improve L2/L3 cache hit rates by
deferring NAPI until userland has finished its work.
The documentation available on this is, IMHO, a bit confusing so please
allow me to explain how one might use this:
1. Ensure each application thread has its own epoll instance mapping
1-to-1 with NIC RX queues. An n-tuple filter would likely be used to
direct connections with specific dest ports to these queues.
2. Optionally: Setup IRQ coalescing for the NIC RX queues where busy
polling will occur. This can help avoid the userland app from being
pre-empted by a hard IRQ while userland is running. Note this means that
userland must take care to call epoll_wait and not take too long in
userland since it now drives NAPI via epoll_wait.
3. Optionally: Consider using napi_defer_hard_irqs and gro_flush_timeout to
further restrict IRQ generation from the NIC. These settings are
system-wide so their impact must be carefully weighed against the running
applications.
4. Ensure that all incoming connections added to an epoll instance
have the same NAPI ID. This can be done with a BPF filter when
SO_REUSEPORT is used or getsockopt + SO_INCOMING_NAPI_ID when a single
accept thread is used which dispatches incoming connections to threads.
5. Lastly, busy poll must be enabled via a sysctl
(/proc/sys/net/core/busy_poll).
Please see Eric Dumazet's paper about busy polling [3] and a recent
academic paper about measured performance improvements of busy polling [4]
(albeit with a modification that is not currently present in the kernel)
for additional context.
The unfortunate part about step 5 above is that this enables busy poll
system-wide which affects all user applications on the system,
including epoll-based network applications which were not intended to
be used this way or applications where increased CPU usage for lower
latency network processing is unnecessary or not desirable.
If the user wants to run one low latency epoll-based server application
with epoll-based busy poll, but would like to run the rest of the
applications on the system (which may also use epoll) without busy poll,
this system-wide sysctl presents a significant problem.
This change preserves the system-wide sysctl, but adds a mechanism (via
ioctl) to enable or disable busy poll for epoll contexts as needed by
individual applications, making epoll-based busy poll more usable.
Note that this change includes an or (as of v4) instead of an xor. If the
user has enabled both the system-wide sysctl and also the per epoll-context
busy poll settings, then epoll should probably busy poll (vs being
disabled).
Thanks,
Joe
v3 -> v4:
- patch 1/3 was updated to include an important functional change:
ep_busy_loop_on was updated to use or (||) instead of xor (^). After
thinking about it a bit more, I thought xor didn't make much sense.
Enabling both the per-epoll context and the system-wide sysctl should
probably enable busy poll, not disable it. So, or (||) makes more
sense, I think.
- patch 3/3 was updated:
- to change the epoll_params fields to be __u64, __u16, and __u8 and
to pad the struct to a multiple of 64bits. Suggested by Greg K-H [5]
and Arnd Bergmann [6].
- remove an unused pr_fmt, left over from the previous revision.
- ioctl now returns -EINVAL when epoll_params.busy_poll_usecs >
U32_MAX.
v2 -> v3:
- cover letter updated to mention why ioctl seems (to me) like a better
choice vs a new syscall.
- patch 3/4 was modified in 3 ways:
- when an unknown ioctl is received, -ENOIOCTLCMD is returned instead
of -EINVAL as the ioctl documentation requires.
- epoll_params.busy_poll_budget can only be set to a value larger than
NAPI_POLL_WEIGHT if code is run by privileged (CAP_NET_ADMIN) users.
Otherwise, -EPERM is returned.
- busy poll specific ioctl code moved out to its own function. On
kernels without busy poll support, -EOPNOTSUPP is returned. This also
makes the kernel build robot happier without littering the code with
more #ifdefs.
- dropped patch 4/4 after Eric Dumazet's review of it when it was sent
independently to the list [7].
v1 -> v2:
- cover letter updated to make a mention of napi_defer_hard_irqs and
gro_flush_timeout as an added step 3 and to cite both Eric Dumazet's
busy polling paper and a paper from University of Waterloo for
additional context. Specifically calling out the xor in patch 1/4
incase it is missed by reviewers.
- Patch 2/4 has its commit message updated, but no functional changes.
Commit message now describes that allowing for a settable budget helps
to improve throughput and is more consistent with other busy poll
mechanisms that allow a settable budget via SO_BUSY_POLL_BUDGET.
- Patch 3/4 was modified to check if the epoll_params.busy_poll_budget
exceeds NAPI_POLL_WEIGHT. The larger value is allowed, but an error is
printed. This was done for consistency with netif_napi_add_weight,
which does the same.
- Patch 3/4 the struct epoll_params was updated to fix the type of the
data field; it was uint8_t and was changed to u8.
- Patch 4/4 added to check if SO_BUSY_POLL_BUDGET exceeds
NAPI_POLL_WEIGHT. The larger value is allowed, but an error is
printed. This was done for consistency with netif_napi_add_weight,
which does the same.
[1]: https://lore.kernel.org/lkml/65b1cb7f73a6a_250560294bd@willemb.c.googlers.com.notmuch/
[2]: https://lore.kernel.org/lkml/20170324170836.15226.87178.stgit@localhost.localdomain/
[3]: https://netdevconf.info/2.1/papers/BusyPollingNextGen.pdf
[4]: https://dl.acm.org/doi/pdf/10.1145/3626780
[5]: https://lore.kernel.org/lkml/2024012551-anyone-demeaning-867b@gregkh/
[6]: https://lore.kernel.org/lkml/57b62135-2159-493d-a6bb-47d5be55154a@app.fastmail.com/
[7]: https://lore.kernel.org/lkml/CANn89i+uXsdSVFiQT9fDfGw+h_5QOcuHwPdWi9J=5U6oLXkQTA@mail.gmail.com/
Joe Damato (3):
eventpoll: support busy poll per epoll instance
eventpoll: Add per-epoll busy poll packet budget
eventpoll: Add epoll ioctl for epoll_params
.../userspace-api/ioctl/ioctl-number.rst | 1 +
fs/eventpoll.c | 123 +++++++++++++++++-
include/uapi/linux/eventpoll.h | 12 ++
3 files changed, 131 insertions(+), 5 deletions(-)
--
2.25.1
^ permalink raw reply
* Re: [PATCH net-next v3 0/3] Per epoll context busy poll support
From: Willem de Bruijn @ 2024-01-30 20:33 UTC (permalink / raw)
To: Joe Damato, Willem de Bruijn
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba, weiwan,
Alexander Viro, Andrew Waterman, Arnd Bergmann, Dominik Brodowski,
Greg Kroah-Hartman, Jan Kara, Jiri Slaby, Jonathan Corbet,
Julien Panis, open list:DOCUMENTATION,
(open list:FILESYSTEMS \(VFS and infrastructure\)),
Michael Ellerman, Nathan Lynch, Palmer Dabbelt, Steve French,
Thomas Huth, Thomas Zimmermann
In-Reply-To: <20240129190922.GA1315@fastly.com>
Joe Damato wrote:
> On Sat, Jan 27, 2024 at 11:20:51AM -0500, Willem de Bruijn wrote:
> > Joe Damato wrote:
> > > Greetings:
> > >
> > > Welcome to v3. Cover letter updated from v2 to explain why ioctl and
> > > adjusted my cc_cmd to try to get the correct people in addition to folks
> > > who were added in v1 & v2. Labeled as net-next because it seems networking
> > > related to me even though it is fs code.
> > >
> > > TL;DR This builds on commit bf3b9f6372c4 ("epoll: Add busy poll support to
> > > epoll with socket fds.") by allowing user applications to enable
> > > epoll-based busy polling and set a busy poll packet budget on a per epoll
> > > context basis.
> > >
> > > This makes epoll-based busy polling much more usable for user
> > > applications than the current system-wide sysctl and hardcoded budget.
> > >
> > > To allow for this, two ioctls have been added for epoll contexts for
> > > getting and setting a new struct, struct epoll_params.
> > >
> > > ioctl was chosen vs a new syscall after reviewing a suggestion by Willem
> > > de Bruijn [1]. I am open to using a new syscall instead of an ioctl, but it
> > > seemed that:
> > > - Busy poll affects all existing epoll_wait and epoll_pwait variants in
> > > the same way, so new verions of many syscalls might be needed. It
> >
> > There is no need to support a new feature on legacy calls. Applications have
> > to be upgraded to the new ioctl, so they can also be upgraded to the latest
> > epoll_wait variant.
>
> Sure, that's a fair point. I think we could probably make reasonable
> arguments in both directions about the pros/cons of each approach.
>
> It's still not clear to me that a new syscall is the best way to go on
> this, and IMO it does not offer a clear advantage. I understand that part
> of the premise of your argument is that ioctls are not recommended, but in
> this particular case it seems like a good use case and there have been
> new ioctls added recently (at least according to git log).
>
> This makes me think that while their use is not recommended, they can serve
> a purpose in specific use cases. To me, this use case seems very fitting.
>
> More of a joke and I hate to mention this, but this setting is changing how
> io is done and it seems fitting that this done via an ioctl ;)
>
> > epoll_pwait extends epoll_wait with a sigmask.
> > epoll_pwait2 extends extends epoll_pwait with nsec resolution timespec.
> > Since they are supersets, nothing is lots by limiting to the most recent API.
> >
> > In the discussion of epoll_pwait2 the addition of a forward looking flags
> > argument was discussed, but eventually dropped. Based on the argument that
> > adding a syscall is not a big task and does not warrant preemptive code.
> > This decision did receive a suitably snarky comment from Jonathan Corbet [1].
> >
> > It is definitely more boilerplate, but essentially it is as feasible to add an
> > epoll_pwait3 that takes an optional busy poll argument. In which case, I also
> > believe that it makes more sense to configure the behavior of the syscall
> > directly, than through another syscall and state stored in the kernel.
>
> I definitely hear what you are saying; I think I'm still not convinced, but
> I am thinking it through.
>
> In my mind, all of the other busy poll settings are configured by setting
> options on the sockets using various SO_* options, which modify some state
> in the kernel. The existing system-wide busy poll sysctl also does this. It
> feels strange to me to diverge from that pattern just for epoll.
I think the stateful approach for read is because there we do want
to support all variants: read, readv, recv, recvfrom, recvmsg,
recvmmsg. So there is no way to pass it directly.
That said, I don't mean to argue strenously for this API or against
yours. Want to make sure the option space is explored. There does not
seem to be much other feedback. I don't hold a strong opinion either.
> In the case of epoll_pwait2 the addition of a new syscall is an approach
> that I think makes a lot of sense. The new system call is also probably
> better from an end-user usability perspective, as well. For busy poll, I
> don't see a clear reasoning why a new system call is better, but maybe I am
> still missing something.
>
> > I don't think that the usec fine grain busy poll argument is all that useful.
> > Documentation always suggests setting it to 50us or 100us, based on limited
> > data. Main point is to set it to exceed the round-trip delay of whatever the
> > process is trying to wait on. Overestimating is not costly, as the call
> > returns as soon as the condition is met. An epoll_pwait3 flag EPOLL_BUSY_POLL
> > with default 100us might be sufficient.
> >
> > [1] https://lwn.net/Articles/837816/
>
> Perhaps I am misunderstanding what you are suggesting, but I am opposed to
> hardcoding a value. If it is currently configurable system-wide and via
> SO_* options for other forms of busy poll, I think it should similarly be
> configurable for epoll busy poll.
>
> I may yet be convinced by the new syscall argument, but I don't think I'd
> agree on imposing a default. The value can be modified by other forms of
> busy poll and the goal of my changes are to:
> - make epoll-based busy poll per context
> - allow applications to configure (within reason) how epoll-based busy
> poll behaves, like they can do now with the existing SO_* options for
> other busy poll methods.
Okay. I expected some push back. Was curious if people would come back
with examples of where the full range is actually being used.
> > > seems much simpler for users to use the correct
> > > epoll_wait/epoll_pwait for their app and add a call to ioctl to enable
> > > or disable busy poll as needed. This also probably means less work to
> > > get an existing epoll app using busy poll.
> >
^ permalink raw reply
* Re: [net-next 0/3] Per epoll context busy poll support
From: Samudrala, Sridhar @ 2024-01-30 18:54 UTC (permalink / raw)
To: Eric Dumazet, Joe Damato
Cc: netdev, linux-kernel, chuck.lever, jlayton, linux-api, brauner,
davem, alexander.duyck, kuba, Wei Wang, Amritha Nambiar
In-Reply-To: <CANn89i+YKwrgpt8VnHrw4eeVpqRamLkTSr4u+g1mRDMZa6b+7Q@mail.gmail.com>
On 1/24/2024 2:20 AM, 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.
Agree. looking forward to see this patch series accepted soon.
>>
>> 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.
Good to know that you are looking into enabling busy polling for napi
kthreads.
We have something similar in our ice OOT driver that is implemented and
we call it 'independent pollers' as in this mode, busy polling will not
be app dependent or triggered by an application.
Here is a link to the slides we presented at netdev 0x16 driver workshop.
https://netdevconf.info/0x16/slides/48/netdev0x16_driver_workshop_ADQ.pdf
We haven't yet submitted the patches upstream as there is no kernel
interface to configure napi specific timeouts.
With the recent per-queue and per-napi netlink APIs that are accepted
upstream
https://lore.kernel.org/netdev/170147307026.5260.9300080745237900261.stgit@anambiarhost.jf.intel.com/
we are thinking of making timeout as a per-napi parameter and can be
used as an interface to enable napi kthread based busy polling.
I think even the per-device napi_defer_hard_irqs and gro_flush_timeout
should become per-napi parameters.
>
> 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
* [PATCH v4 3/3] mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted interleaving
From: Gregory Price @ 2024-01-30 18:20 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linux-fsdevel, linux-api, corbet, akpm,
gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji, mhocko,
ying.huang, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Srinivasulu Thanneeru
In-Reply-To: <20240130182046.74278-1-gregory.price@memverge.com>
When a system has multiple NUMA nodes and it becomes bandwidth hungry,
using the current MPOL_INTERLEAVE could be an wise option.
However, if those NUMA nodes consist of different types of memory such
as socket-attached DRAM and CXL/PCIe attached DRAM, the round-robin
based interleave policy does not optimally distribute data to make use
of their different bandwidth characteristics.
Instead, interleave is more effective when the allocation policy follows
each NUMA nodes' bandwidth weight rather than a simple 1:1 distribution.
This patch introduces a new memory policy, MPOL_WEIGHTED_INTERLEAVE,
enabling weighted interleave between NUMA nodes. Weighted interleave
allows for proportional distribution of memory across multiple numa
nodes, preferably apportioned to match the bandwidth of each node.
For example, if a system has 1 CPU node (0), and 2 memory nodes (0,1),
with bandwidth of (100GB/s, 50GB/s) respectively, the appropriate
weight distribution is (2:1).
Weights for each node can be assigned via the new sysfs extension:
/sys/kernel/mm/mempolicy/weighted_interleave/
For now, the default value of all nodes will be `1`, which matches
the behavior of standard 1:1 round-robin interleave. An extension
will be added in the future to allow default values to be registered
at kernel and device bringup time.
The policy allocates a number of pages equal to the set weights. For
example, if the weights are (2,1), then 2 pages will be allocated on
node0 for every 1 page allocated on node1.
The new flag MPOL_WEIGHTED_INTERLEAVE can be used in set_mempolicy(2)
and mbind(2).
Some high level notes about the pieces of weighted interleave:
current->il_prev:
Default interleave uses this to track the last used node.
Weighted interleave uses this to track the *current* node, and
when weight reaches 0 it will be used to acquire the next node.
current->il_weight:
The active weight of the current node (current->il_prev)
When this reaches 0, current->il_prev is set to the next node
and current->il_weight is set to the next weight.
weighted_interleave_nodes:
Counts the number of allocations as they occur, and applies the
weight for the current node. When the weight reaches 0, switch
to the next node. Operates only on task->mempolicy.
weighted_interleave_nid:
Gets the total weight of the nodemask as well as each individual
node weight, then calculates the node based on the given index.
Operates on VMA policies.
bulk_array_weighted_interleave:
Gets the total weight of the nodemask as well as each individual
node weight, then calculates the number of "interleave rounds" as
well as any delta ("partial round"). Calculates the number of
pages for each node and allocates them.
If a node was scheduled for interleave via interleave_nodes, the
current weight will be allocated first.
Operates only on the task->mempolicy.
One piece of complexity is the interaction between a recent refactor
which split the logic to acquire the "ilx" (interleave index) of an
allocation and the actual application of the interleave. If a call
to alloc_pages_mpol() were made with a weighted-interleave policy and
ilx set to NO_INTERLEAVE_INDEX, weighted_interleave_nodes() would
operate on a VMA policy - violating the description above.
An inspection of all callers of alloc_pages_mpol() shows that all
external callers set ilx to `0`, an index value, or will call
get_vma_policy() to acquire the ilx.
For example, mm/shmem.c may call into alloc_pages_mpol. The call stacks
all set (pgoff_t ilx) or end up in `get_vma_policy()`. This enforces
the `weighted_interleave_nodes()` and `weighted_interleave_nid()`
policy requirements (task/vma respectively).
Suggested-by: Hasan Al Maruf <Hasan.Maruf@amd.com>
Signed-off-by: Gregory Price <gregory.price@memverge.com>
Co-developed-by: Rakie Kim <rakie.kim@sk.com>
Signed-off-by: Rakie Kim <rakie.kim@sk.com>
Co-developed-by: Honggyu Kim <honggyu.kim@sk.com>
Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
Co-developed-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
Signed-off-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
Co-developed-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
Signed-off-by: Srinivasulu Thanneeru <sthanneeru.opensrc@micron.com>
Co-developed-by: Ravi Jonnalagadda <ravis.opensrc@micron.com>
Signed-off-by: Ravi Jonnalagadda <ravis.opensrc@micron.com>
---
.../admin-guide/mm/numa_memory_policy.rst | 9 +
include/linux/sched.h | 1 +
include/uapi/linux/mempolicy.h | 1 +
mm/mempolicy.c | 231 +++++++++++++++++-
4 files changed, 238 insertions(+), 4 deletions(-)
diff --git a/Documentation/admin-guide/mm/numa_memory_policy.rst b/Documentation/admin-guide/mm/numa_memory_policy.rst
index eca38fa81e0f..a70f20ce1ffb 100644
--- a/Documentation/admin-guide/mm/numa_memory_policy.rst
+++ b/Documentation/admin-guide/mm/numa_memory_policy.rst
@@ -250,6 +250,15 @@ MPOL_PREFERRED_MANY
can fall back to all existing numa nodes. This is effectively
MPOL_PREFERRED allowed for a mask rather than a single node.
+MPOL_WEIGHTED_INTERLEAVE
+ This mode operates the same as MPOL_INTERLEAVE, except that
+ interleaving behavior is executed based on weights set in
+ /sys/kernel/mm/mempolicy/weighted_interleave/
+
+ Weighted interleave allocates pages on nodes according to a
+ weight. For example if nodes [0,1] are weighted [5,2], 5 pages
+ will be allocated on node0 for every 2 pages allocated on node1.
+
NUMA memory policy supports the following optional mode flags:
MPOL_F_STATIC_NODES
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ffe8f618ab86..b9ce285d8c9c 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1259,6 +1259,7 @@ struct task_struct {
/* Protected by alloc_lock: */
struct mempolicy *mempolicy;
short il_prev;
+ u8 il_weight;
short pref_node_fork;
#endif
#ifdef CONFIG_NUMA_BALANCING
diff --git a/include/uapi/linux/mempolicy.h b/include/uapi/linux/mempolicy.h
index a8963f7ef4c2..1f9bb10d1a47 100644
--- a/include/uapi/linux/mempolicy.h
+++ b/include/uapi/linux/mempolicy.h
@@ -23,6 +23,7 @@ enum {
MPOL_INTERLEAVE,
MPOL_LOCAL,
MPOL_PREFERRED_MANY,
+ MPOL_WEIGHTED_INTERLEAVE,
MPOL_MAX, /* always last member of enum */
};
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 3bdfaf03b660..7cd92f4ec0d7 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -19,6 +19,13 @@
* for anonymous memory. For process policy an process counter
* is used.
*
+ * weighted interleave
+ * Allocate memory interleaved over a set of nodes based on
+ * a set of weights (per-node), with normal fallback if it
+ * fails. Otherwise operates the same as interleave.
+ * Example: nodeset(0,1) & weights (2,1) - 2 pages allocated
+ * on node 0 for every 1 page allocated on node 1.
+ *
* bind Only allocate memory on a specific set of nodes,
* no fallback.
* FIXME: memory is allocated starting with the first node
@@ -441,6 +448,10 @@ static const struct mempolicy_operations mpol_ops[MPOL_MAX] = {
.create = mpol_new_nodemask,
.rebind = mpol_rebind_preferred,
},
+ [MPOL_WEIGHTED_INTERLEAVE] = {
+ .create = mpol_new_nodemask,
+ .rebind = mpol_rebind_nodemask,
+ },
};
static bool migrate_folio_add(struct folio *folio, struct list_head *foliolist,
@@ -862,8 +873,11 @@ static long do_set_mempolicy(unsigned short mode, unsigned short flags,
old = current->mempolicy;
current->mempolicy = new;
- if (new && new->mode == MPOL_INTERLEAVE)
+ if (new && (new->mode == MPOL_INTERLEAVE ||
+ new->mode == MPOL_WEIGHTED_INTERLEAVE)) {
current->il_prev = MAX_NUMNODES-1;
+ current->il_weight = 0;
+ }
task_unlock(current);
mpol_put(old);
ret = 0;
@@ -888,6 +902,7 @@ static void get_policy_nodemask(struct mempolicy *pol, nodemask_t *nodes)
case MPOL_INTERLEAVE:
case MPOL_PREFERRED:
case MPOL_PREFERRED_MANY:
+ case MPOL_WEIGHTED_INTERLEAVE:
*nodes = pol->nodes;
break;
case MPOL_LOCAL:
@@ -972,6 +987,13 @@ static long do_get_mempolicy(int *policy, nodemask_t *nmask,
} else if (pol == current->mempolicy &&
pol->mode == MPOL_INTERLEAVE) {
*policy = next_node_in(current->il_prev, pol->nodes);
+ } else if (pol == current->mempolicy &&
+ pol->mode == MPOL_WEIGHTED_INTERLEAVE) {
+ if (current->il_weight)
+ *policy = current->il_prev;
+ else
+ *policy = next_node_in(current->il_prev,
+ pol->nodes);
} else {
err = -EINVAL;
goto out;
@@ -1336,7 +1358,8 @@ static long do_mbind(unsigned long start, unsigned long len,
* VMAs, the nodes will still be interleaved from the targeted
* nodemask, but one by one may be selected differently.
*/
- if (new->mode == MPOL_INTERLEAVE) {
+ if (new->mode == MPOL_INTERLEAVE ||
+ new->mode == MPOL_WEIGHTED_INTERLEAVE) {
struct page *page;
unsigned int order;
unsigned long addr = -EFAULT;
@@ -1784,7 +1807,8 @@ struct mempolicy *__get_vma_policy(struct vm_area_struct *vma,
* @vma: virtual memory area whose policy is sought
* @addr: address in @vma for shared policy lookup
* @order: 0, or appropriate huge_page_order for interleaving
- * @ilx: interleave index (output), for use only when MPOL_INTERLEAVE
+ * @ilx: interleave index (output), for use only when MPOL_INTERLEAVE or
+ * MPOL_WEIGHTED_INTERLEAVE
*
* Returns effective policy for a VMA at specified address.
* Falls back to current->mempolicy or system default policy, as necessary.
@@ -1801,7 +1825,8 @@ struct mempolicy *get_vma_policy(struct vm_area_struct *vma,
pol = __get_vma_policy(vma, addr, ilx);
if (!pol)
pol = get_task_policy(current);
- if (pol->mode == MPOL_INTERLEAVE) {
+ if (pol->mode == MPOL_INTERLEAVE ||
+ pol->mode == MPOL_WEIGHTED_INTERLEAVE) {
*ilx += vma->vm_pgoff >> order;
*ilx += (addr - vma->vm_start) >> (PAGE_SHIFT + order);
}
@@ -1851,6 +1876,22 @@ bool apply_policy_zone(struct mempolicy *policy, enum zone_type zone)
return zone >= dynamic_policy_zone;
}
+static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
+{
+ unsigned int node = current->il_prev;
+
+ if (!current->il_weight || !node_isset(node, policy->nodes)) {
+ node = next_node_in(node, policy->nodes);
+ /* can only happen if nodemask is being rebound */
+ if (node == MAX_NUMNODES)
+ return node;
+ current->il_prev = node;
+ current->il_weight = get_il_weight(node);
+ }
+ current->il_weight--;
+ return node;
+}
+
/* Do dynamic interleaving for a process */
static unsigned int interleave_nodes(struct mempolicy *policy)
{
@@ -1885,6 +1926,9 @@ unsigned int mempolicy_slab_node(void)
case MPOL_INTERLEAVE:
return interleave_nodes(policy);
+ case MPOL_WEIGHTED_INTERLEAVE:
+ return weighted_interleave_nodes(policy);
+
case MPOL_BIND:
case MPOL_PREFERRED_MANY:
{
@@ -1923,6 +1967,45 @@ static unsigned int read_once_policy_nodemask(struct mempolicy *pol,
return nodes_weight(*mask);
}
+static unsigned int weighted_interleave_nid(struct mempolicy *pol, pgoff_t ilx)
+{
+ nodemask_t nodemask;
+ unsigned int target, nr_nodes;
+ u8 __rcu *table;
+ unsigned int weight_total = 0;
+ u8 weight;
+ int nid;
+
+ nr_nodes = read_once_policy_nodemask(pol, &nodemask);
+ if (!nr_nodes)
+ return numa_node_id();
+
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ /* calculate the total weight */
+ for_each_node_mask(nid, nodemask) {
+ /* detect system default usage */
+ weight = table ? table[nid] : 1;
+ weight = weight ? weight : 1;
+ weight_total += weight;
+ }
+
+ /* Calculate the node offset based on totals */
+ target = ilx % weight_total;
+ nid = first_node(nodemask);
+ while (target) {
+ /* detect system default usage */
+ weight = table ? table[nid] : 1;
+ weight = weight ? weight : 1;
+ if (target < weight)
+ break;
+ target -= weight;
+ nid = next_node_in(nid, nodemask);
+ }
+ rcu_read_unlock();
+ return nid;
+}
+
/*
* Do static interleaving for interleave index @ilx. Returns the ilx'th
* node in pol->nodes (starting from ilx=0), wrapping around if ilx
@@ -1983,6 +2066,11 @@ static nodemask_t *policy_nodemask(gfp_t gfp, struct mempolicy *pol,
*nid = (ilx == NO_INTERLEAVE_INDEX) ?
interleave_nodes(pol) : interleave_nid(pol, ilx);
break;
+ case MPOL_WEIGHTED_INTERLEAVE:
+ *nid = (ilx == NO_INTERLEAVE_INDEX) ?
+ weighted_interleave_nodes(pol) :
+ weighted_interleave_nid(pol, ilx);
+ break;
}
return nodemask;
@@ -2044,6 +2132,7 @@ bool init_nodemask_of_mempolicy(nodemask_t *mask)
case MPOL_PREFERRED_MANY:
case MPOL_BIND:
case MPOL_INTERLEAVE:
+ case MPOL_WEIGHTED_INTERLEAVE:
*mask = mempolicy->nodes;
break;
@@ -2144,6 +2233,7 @@ struct page *alloc_pages_mpol(gfp_t gfp, unsigned int order,
* node in its nodemask, we allocate the standard way.
*/
if (pol->mode != MPOL_INTERLEAVE &&
+ pol->mode != MPOL_WEIGHTED_INTERLEAVE &&
(!nodemask || node_isset(nid, *nodemask))) {
/*
* First, try to allocate THP only on local node, but
@@ -2279,6 +2369,127 @@ static unsigned long alloc_pages_bulk_array_interleave(gfp_t gfp,
return total_allocated;
}
+static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
+ struct mempolicy *pol, unsigned long nr_pages,
+ struct page **page_array)
+{
+ struct task_struct *me = current;
+ unsigned long total_allocated = 0;
+ unsigned long nr_allocated = 0;
+ unsigned long rounds;
+ unsigned long node_pages, delta;
+ u8 __rcu *table, *weights, weight;
+ unsigned int weight_total = 0;
+ unsigned long rem_pages = nr_pages;
+ nodemask_t nodes;
+ int nnodes, node, next_node;
+ int resume_node = MAX_NUMNODES - 1;
+ u8 resume_weight = 0;
+ int prev_node;
+ int i;
+
+ if (!nr_pages)
+ return 0;
+
+ nnodes = read_once_policy_nodemask(pol, &nodes);
+ if (!nnodes)
+ return 0;
+
+ /* Continue allocating from most recent node and adjust the nr_pages */
+ node = me->il_prev;
+ weight = me->il_weight;
+ if (weight && node_isset(node, nodes)) {
+ node_pages = min(rem_pages, weight);
+ nr_allocated = __alloc_pages_bulk(gfp, node, NULL, node_pages,
+ NULL, page_array);
+ page_array += nr_allocated;
+ total_allocated += nr_allocated;
+ /* if that's all the pages, no need to interleave */
+ if (rem_pages < weight) {
+ /* stay on current node, adjust il_weight */
+ me->il_weight -= rem_pages;
+ return total_allocated;
+ } else if (rem_pages == weight) {
+ /* move to next node / weight */
+ me->il_prev = next_node_in(node, nodes);
+ me->il_weight = get_il_weight(next_node);
+ return total_allocated;
+ }
+ /* Otherwise we adjust remaining pages, continue from there */
+ rem_pages -= weight;
+ }
+ /* clear active weight in case of an allocation failure */
+ me->il_weight = 0;
+ prev_node = node;
+
+ /* create a local copy of node weights to operate on outside rcu */
+ weights = kzalloc(nr_node_ids, GFP_KERNEL);
+ if (!weights)
+ return total_allocated;
+
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ if (table)
+ memcpy(weights, table, nr_node_ids);
+ rcu_read_unlock();
+
+ /* calculate total, detect system default usage */
+ for_each_node_mask(node, nodes) {
+ if (!weights[node])
+ weights[node] = 1;
+ weight_total += weights[node];
+ }
+
+ /*
+ * Calculate rounds/partial rounds to minimize __alloc_pages_bulk calls.
+ * Track which node weighted interleave should resume from.
+ *
+ * if (rounds > 0) and (delta == 0), resume_node will always be
+ * the node following prev_node and its weight.
+ */
+ rounds = rem_pages / weight_total;
+ delta = rem_pages % weight_total;
+ resume_node = next_node_in(prev_node, nodes);
+ resume_weight = weights[resume_node];
+ for (i = 0; i < nnodes; i++) {
+ node = next_node_in(prev_node, nodes);
+ weight = weights[node];
+ node_pages = weight * rounds;
+ /* If a delta exists, add this node's portion of the delta */
+ if (delta > weight) {
+ node_pages += weight;
+ delta -= weight;
+ } else if (delta) {
+ node_pages += delta;
+ /* delta may deplete on a boundary or w/ a remainder */
+ if (delta == weight) {
+ /* boundary: resume from next node/weight */
+ resume_node = next_node_in(node, nodes);
+ resume_weight = weights[resume_node];
+ } else {
+ /* remainder: resume this node w/ remainder */
+ resume_node = node;
+ resume_weight = weight - delta;
+ }
+ delta = 0;
+ }
+ /* node_pages can be 0 if an allocation fails and rounds == 0 */
+ if (!node_pages)
+ break;
+ nr_allocated = __alloc_pages_bulk(gfp, node, NULL, node_pages,
+ NULL, page_array);
+ page_array += nr_allocated;
+ total_allocated += nr_allocated;
+ if (total_allocated == nr_pages)
+ break;
+ prev_node = node;
+ }
+ me->il_prev = resume_node;
+ me->il_weight = resume_weight;
+ kfree(weights);
+ return total_allocated;
+}
+
static unsigned long alloc_pages_bulk_array_preferred_many(gfp_t gfp, int nid,
struct mempolicy *pol, unsigned long nr_pages,
struct page **page_array)
@@ -2319,6 +2530,10 @@ unsigned long alloc_pages_bulk_array_mempolicy(gfp_t gfp,
return alloc_pages_bulk_array_interleave(gfp, pol,
nr_pages, page_array);
+ if (pol->mode == MPOL_WEIGHTED_INTERLEAVE)
+ return alloc_pages_bulk_array_weighted_interleave(
+ gfp, pol, nr_pages, page_array);
+
if (pol->mode == MPOL_PREFERRED_MANY)
return alloc_pages_bulk_array_preferred_many(gfp,
numa_node_id(), pol, nr_pages, page_array);
@@ -2394,6 +2609,7 @@ bool __mpol_equal(struct mempolicy *a, struct mempolicy *b)
case MPOL_INTERLEAVE:
case MPOL_PREFERRED:
case MPOL_PREFERRED_MANY:
+ case MPOL_WEIGHTED_INTERLEAVE:
return !!nodes_equal(a->nodes, b->nodes);
case MPOL_LOCAL:
return true;
@@ -2530,6 +2746,10 @@ int mpol_misplaced(struct folio *folio, struct vm_area_struct *vma,
polnid = interleave_nid(pol, ilx);
break;
+ case MPOL_WEIGHTED_INTERLEAVE:
+ polnid = weighted_interleave_nid(pol, ilx);
+ break;
+
case MPOL_PREFERRED:
if (node_isset(curnid, pol->nodes))
goto out;
@@ -2904,6 +3124,7 @@ static const char * const policy_modes[] =
[MPOL_PREFERRED] = "prefer",
[MPOL_BIND] = "bind",
[MPOL_INTERLEAVE] = "interleave",
+ [MPOL_WEIGHTED_INTERLEAVE] = "weighted interleave",
[MPOL_LOCAL] = "local",
[MPOL_PREFERRED_MANY] = "prefer (many)",
};
@@ -2963,6 +3184,7 @@ int mpol_parse_str(char *str, struct mempolicy **mpol)
}
break;
case MPOL_INTERLEAVE:
+ case MPOL_WEIGHTED_INTERLEAVE:
/*
* Default to online nodes with memory if no nodelist
*/
@@ -3073,6 +3295,7 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
case MPOL_PREFERRED_MANY:
case MPOL_BIND:
case MPOL_INTERLEAVE:
+ case MPOL_WEIGHTED_INTERLEAVE:
nodes = pol->nodes;
break;
default:
--
2.39.1
^ permalink raw reply related
* [PATCH v4 2/3] mm/mempolicy: refactor a read-once mechanism into a function for re-use
From: Gregory Price @ 2024-01-30 18:20 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linux-fsdevel, linux-api, corbet, akpm,
gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji, mhocko,
ying.huang, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams
In-Reply-To: <20240130182046.74278-1-gregory.price@memverge.com>
move the use of barrier() to force policy->nodemask onto the stack into
a function `read_once_policy_nodemask` so that it may be re-used.
Suggested-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Gregory Price <gregory.price@memverge.com>
---
mm/mempolicy.c | 26 ++++++++++++++++----------
1 file changed, 16 insertions(+), 10 deletions(-)
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 440128a398ef..3bdfaf03b660 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -1909,6 +1909,20 @@ unsigned int mempolicy_slab_node(void)
}
}
+static unsigned int read_once_policy_nodemask(struct mempolicy *pol,
+ nodemask_t *mask)
+{
+ /*
+ * barrier stabilizes the nodemask locally so that it can be iterated
+ * over safely without concern for changes. Allocators validate node
+ * selection does not violate mems_allowed, so this is safe.
+ */
+ barrier();
+ memcpy(mask, &pol->nodes, sizeof(nodemask_t));
+ barrier();
+ return nodes_weight(*mask);
+}
+
/*
* Do static interleaving for interleave index @ilx. Returns the ilx'th
* node in pol->nodes (starting from ilx=0), wrapping around if ilx
@@ -1916,20 +1930,12 @@ unsigned int mempolicy_slab_node(void)
*/
static unsigned int interleave_nid(struct mempolicy *pol, pgoff_t ilx)
{
- nodemask_t nodemask = pol->nodes;
+ nodemask_t nodemask;
unsigned int target, nnodes;
int i;
int nid;
- /*
- * The barrier will stabilize the nodemask in a register or on
- * the stack so that it will stop changing under the code.
- *
- * Between first_node() and next_node(), pol->nodes could be changed
- * by other threads. So we put pol->nodes in a local stack.
- */
- barrier();
- nnodes = nodes_weight(nodemask);
+ nnodes = read_once_policy_nodemask(pol, &nodemask);
if (!nnodes)
return numa_node_id();
target = ilx % nnodes;
--
2.39.1
^ permalink raw reply related
* [PATCH v4 1/3] mm/mempolicy: implement the sysfs-based weighted_interleave interface
From: Gregory Price @ 2024-01-30 18:20 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linux-fsdevel, linux-api, corbet, akpm,
gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji, mhocko,
ying.huang, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams
In-Reply-To: <20240130182046.74278-1-gregory.price@memverge.com>
From: Rakie Kim <rakie.kim@sk.com>
This patch provides a way to set interleave weight information under
sysfs at /sys/kernel/mm/mempolicy/weighted_interleave/nodeN
The sysfs structure is designed as follows.
$ tree /sys/kernel/mm/mempolicy/
/sys/kernel/mm/mempolicy/ [1]
└── weighted_interleave [2]
├── node0 [3]
└── node1
Each file above can be explained as follows.
[1] mm/mempolicy: configuration interface for mempolicy subsystem
[2] weighted_interleave/: config interface for weighted interleave policy
[3] weighted_interleave/nodeN: weight for nodeN
If a node value is set to `0`, the system-default value will be used.
As of this patch, the system-default for all nodes is always 1.
Suggested-by: Huang Ying <ying.huang@intel.com>
Signed-off-by: Rakie Kim <rakie.kim@sk.com>
Signed-off-by: Honggyu Kim <honggyu.kim@sk.com>
Co-developed-by: Gregory Price <gregory.price@memverge.com>
Signed-off-by: Gregory Price <gregory.price@memverge.com>
Co-developed-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
Signed-off-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
---
.../ABI/testing/sysfs-kernel-mm-mempolicy | 4 +
...fs-kernel-mm-mempolicy-weighted-interleave | 25 ++
mm/mempolicy.c | 223 ++++++++++++++++++
3 files changed, 252 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-mempolicy
create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy
new file mode 100644
index 000000000000..8ac327fd7fb6
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy
@@ -0,0 +1,4 @@
+What: /sys/kernel/mm/mempolicy/
+Date: January 2024
+Contact: Linux memory management mailing list <linux-mm@kvack.org>
+Description: Interface for Mempolicy
diff --git a/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
new file mode 100644
index 000000000000..0b7972de04e9
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
@@ -0,0 +1,25 @@
+What: /sys/kernel/mm/mempolicy/weighted_interleave/
+Date: January 2024
+Contact: Linux memory management mailing list <linux-mm@kvack.org>
+Description: Configuration Interface for the Weighted Interleave policy
+
+What: /sys/kernel/mm/mempolicy/weighted_interleave/nodeN
+Date: January 2024
+Contact: Linux memory management mailing list <linux-mm@kvack.org>
+Description: Weight configuration interface for nodeN
+
+ The interleave weight for a memory node (N). These weights are
+ utilized by tasks which have set their mempolicy to
+ MPOL_WEIGHTED_INTERLEAVE.
+
+ These weights only affect new allocations, and changes at runtime
+ will not cause migrations on already allocated pages.
+
+ The minimum weight for a node is always 1.
+
+ Minimum weight: 1
+ Maximum weight: 255
+
+ Writing an empty string or `0` will reset the weight to the
+ system default. The system default may be set by the kernel
+ or drivers at boot or during hotplug events.
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 10a590ee1c89..440128a398ef 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -131,6 +131,32 @@ static struct mempolicy default_policy = {
static struct mempolicy preferred_node_policy[MAX_NUMNODES];
+/*
+ * iw_table is the sysfs-set interleave weight table, a value of 0 denotes
+ * system-default value should be used. A NULL iw_table also denotes that
+ * system-default values should be used. Until the system-default table
+ * is implemented, the system-default is always 1.
+ *
+ * iw_table is RCU protected
+ */
+static u8 __rcu *iw_table;
+static DEFINE_MUTEX(iw_table_lock);
+
+static u8 get_il_weight(int node)
+{
+ u8 __rcu *table;
+ u8 weight;
+
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ /* if no iw_table, use system default */
+ weight = table ? table[node] : 1;
+ /* if value in iw_table is 0, use system default */
+ weight = weight ? weight : 1;
+ rcu_read_unlock();
+ return weight;
+}
+
/**
* numa_nearest_node - Find nearest node by state
* @node: Node id to start the search
@@ -3067,3 +3093,200 @@ void mpol_to_str(char *buffer, int maxlen, struct mempolicy *pol)
p += scnprintf(p, buffer + maxlen - p, ":%*pbl",
nodemask_pr_args(&nodes));
}
+
+#ifdef CONFIG_SYSFS
+struct iw_node_attr {
+ struct kobj_attribute kobj_attr;
+ int nid;
+};
+
+static ssize_t node_show(struct kobject *kobj, struct kobj_attribute *attr,
+ char *buf)
+{
+ struct iw_node_attr *node_attr;
+ u8 weight;
+
+ node_attr = container_of(attr, struct iw_node_attr, kobj_attr);
+ weight = get_il_weight(node_attr->nid);
+ return sysfs_emit(buf, "%d\n", weight);
+}
+
+static ssize_t node_store(struct kobject *kobj, struct kobj_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct iw_node_attr *node_attr;
+ u8 __rcu *new;
+ u8 __rcu *old;
+ u8 weight = 0;
+
+ node_attr = container_of(attr, struct iw_node_attr, kobj_attr);
+ if (count == 0 || sysfs_streq(buf, ""))
+ weight = 0;
+ else if (kstrtou8(buf, 0, &weight))
+ return -EINVAL;
+
+ new = kzalloc(nr_node_ids, GFP_KERNEL);
+ if (!new)
+ return -ENOMEM;
+
+ mutex_lock(&iw_table_lock);
+ old = rcu_dereference_protected(iw_table,
+ lockdep_is_held(&iw_table_lock));
+ if (old)
+ memcpy(new, old, nr_node_ids);
+ new[node_attr->nid] = weight;
+ rcu_assign_pointer(iw_table, new);
+ mutex_unlock(&iw_table_lock);
+ synchronize_rcu();
+ kfree(old);
+ return count;
+}
+
+static struct iw_node_attr **node_attrs;
+
+static void sysfs_wi_node_release(struct iw_node_attr *node_attr,
+ struct kobject *parent)
+{
+ if (!node_attr)
+ return;
+ sysfs_remove_file(parent, &node_attr->kobj_attr.attr);
+ kfree(node_attr->kobj_attr.attr.name);
+ kfree(node_attr);
+}
+
+static void sysfs_wi_release(struct kobject *wi_kobj)
+{
+ int i;
+
+ for (i = 0; i < nr_node_ids; i++)
+ sysfs_wi_node_release(node_attrs[i], wi_kobj);
+ kobject_put(wi_kobj);
+}
+
+static const struct kobj_type wi_ktype = {
+ .sysfs_ops = &kobj_sysfs_ops,
+ .release = sysfs_wi_release,
+};
+
+static int add_weight_node(int nid, struct kobject *wi_kobj)
+{
+ struct iw_node_attr *node_attr;
+ char *name;
+
+ node_attr = kzalloc(sizeof(*node_attr), GFP_KERNEL);
+ if (!node_attr)
+ return -ENOMEM;
+
+ name = kasprintf(GFP_KERNEL, "node%d", nid);
+ if (!name) {
+ kfree(node_attr);
+ return -ENOMEM;
+ }
+
+ sysfs_attr_init(&node_attr->kobj_attr.attr);
+ node_attr->kobj_attr.attr.name = name;
+ node_attr->kobj_attr.attr.mode = 0644;
+ node_attr->kobj_attr.show = node_show;
+ node_attr->kobj_attr.store = node_store;
+ node_attr->nid = nid;
+
+ if (sysfs_create_file(wi_kobj, &node_attr->kobj_attr.attr)) {
+ kfree(node_attr->kobj_attr.attr.name);
+ kfree(node_attr);
+ pr_err("failed to add attribute to weighted_interleave\n");
+ return -ENOMEM;
+ }
+
+ node_attrs[nid] = node_attr;
+ return 0;
+}
+
+static int add_weighted_interleave_group(struct kobject *root_kobj)
+{
+ struct kobject *wi_kobj;
+ int nid, err;
+
+ wi_kobj = kzalloc(sizeof(struct kobject), GFP_KERNEL);
+ if (!wi_kobj)
+ return -ENOMEM;
+
+ err = kobject_init_and_add(wi_kobj, &wi_ktype, root_kobj,
+ "weighted_interleave");
+ if (err) {
+ kfree(wi_kobj);
+ return err;
+ }
+
+ for_each_node_state(nid, N_POSSIBLE) {
+ err = add_weight_node(nid, wi_kobj);
+ if (err) {
+ pr_err("failed to add sysfs [node%d]\n", nid);
+ break;
+ }
+ }
+ if (err)
+ kobject_put(wi_kobj);
+ return 0;
+}
+
+static void mempolicy_kobj_release(struct kobject *kobj)
+{
+ u8 __rcu *old;
+
+ mutex_lock(&iw_table_lock);
+ old = rcu_dereference_protected(iw_table,
+ lockdep_is_held(&iw_table_lock));
+ rcu_assign_pointer(iw_table, NULL);
+ mutex_unlock(&iw_table_lock);
+ synchronize_rcu();
+ kfree(old);
+ kfree(node_attrs);
+ kfree(kobj);
+}
+
+static const struct kobj_type mempolicy_ktype = {
+ .release = mempolicy_kobj_release
+};
+
+static int __init mempolicy_sysfs_init(void)
+{
+ int err;
+ static struct kobject *mempolicy_kobj;
+
+ mempolicy_kobj = kzalloc(sizeof(*mempolicy_kobj), GFP_KERNEL);
+ if (!mempolicy_kobj) {
+ err = -ENOMEM;
+ goto err_out;
+ }
+
+ node_attrs = kcalloc(nr_node_ids, sizeof(struct iw_node_attr *),
+ GFP_KERNEL);
+ if (!node_attrs) {
+ err = -ENOMEM;
+ goto mempol_out;
+ }
+
+ err = kobject_init_and_add(mempolicy_kobj, &mempolicy_ktype, mm_kobj,
+ "mempolicy");
+ if (err)
+ goto node_out;
+
+ err = add_weighted_interleave_group(mempolicy_kobj);
+ if (err) {
+ pr_err("mempolicy sysfs structure failed to initialize\n");
+ kobject_put(mempolicy_kobj);
+ return err;
+ }
+
+ return err;
+node_out:
+ kfree(node_attrs);
+mempol_out:
+ kfree(mempolicy_kobj);
+err_out:
+ pr_err("failed to add mempolicy kobject to the system\n");
+ return err;
+}
+
+late_initcall(mempolicy_sysfs_init);
+#endif /* CONFIG_SYSFS */
--
2.39.1
^ permalink raw reply related
* [PATCH v4 0/3] mm/mempolicy: weighted interleave mempolicy and sysfs extension
From: Gregory Price @ 2024-01-30 18:20 UTC (permalink / raw)
To: linux-mm
Cc: linux-kernel, linux-doc, linux-fsdevel, linux-api, corbet, akpm,
gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji, mhocko,
ying.huang, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams,
Hasan Al Maruf, Hao Wang, Michal Hocko, Zhongkun He,
Frank van der Linden, John Groves, Jonathan Cameron, Andi Kleen
Hi Andrew,
This is *hopefully* the final major update to this line.
Full version nodes at the end of initial cover letter chunk.
(v4: style, task->il_weight, uninitialized values, docs)
---
Weighted interleave is a new interleave policy intended to make
use of heterogeneous memory environments appearing with CXL.
The existing interleave mechanism does an even round-robin
distribution of memory across all nodes in a nodemask, while
weighted interleave distributes memory across nodes according
to a provided weight. (Weight = # of page allocations per round)
Weighted interleave is intended to reduce average latency when
bandwidth is pressured - therefore increasing total throughput.
In other words: It allows greater use of the total available
bandwidth in a heterogeneous hardware environment (different
hardware provides different bandwidth capacity).
As bandwidth is pressured, latency increases - first linearly
and then exponentially. By keeping bandwidth usage distributed
according to available bandwidth, we therefore can reduce the
average latency of a cacheline fetch.
A good explanation of the bandwidth vs latency response curve:
https://mahmoudhatem.wordpress.com/2017/11/07/memory-bandwidth-vs-latency-response-curve/
From the article:
```
Constant region:
The latency response is fairly constant for the first 40%
of the sustained bandwidth.
Linear region:
In between 40% to 80% of the sustained bandwidth, the
latency response increases almost linearly with the bandwidth
demand of the system due to contention overhead by numerous
memory requests.
Exponential region:
Between 80% to 100% of the sustained bandwidth, the memory
latency is dominated by the contention latency which can be
as much as twice the idle latency or more.
Maximum sustained bandwidth :
Is 65% to 75% of the theoretical maximum bandwidth.
```
As a general rule of thumb:
* If bandwidth usage is low, latency does not increase. It is
optimal to place data in the nearest (lowest latency) device.
* If bandwidth usage is high, latency increases. It is optimal
to place data such that bandwidth use is optimized per-device.
This is the top line goal: Provide a user a mechanism to target using
the "maximum sustained bandwidth" of each hardware component in a
heterogenous memory system.
For example, the stream benchmark demonstrates that 1:1 (default)
interleave is actively harmful, while weighted interleave can be
beneficial. Default interleave distributes data such that too much
pressure is placed on devices with lower available bandwidth.
Stream Benchmark (vs DRAM, 1 Socket + 1 CXL Device)
Default interleave : -78% (slower than DRAM)
Global weighting : -6% to +4% (workload dependant)
Targeted weights : +2.5% to +4% (consistently better than DRAM)
Global means the task-policy was set (set_mempolicy), while targeted
means VMA policies were set (mbind2). We see weighted interleave
is not always beneficial when applied globally, but is always
beneficial when applied to bandwidth-driving memory regions.
There are 3 patches in this set:
1) Implement system-global interleave weights as sysfs extension
in mm/mempolicy.c. These weights are RCU protected, and a
default weight set is provided (all weights are 1 by default).
In future work, we intend to expose an interface for HMAT/CDAT
code to set reasonable default values based on the memory
configuration of the system discovered at boot/hotplug.
2) A mild refactor of some interleave-logic for re-use in the
new weighted interleave logic.
3) MPOL_WEIGHTED_INTERLEAVE extension for set_mempolicy/mbind
Included below are some performance and LTP test information,
and a sample numactl branch which can be used for testing.
= Performance summary =
(tests may have different configurations, see extended info below)
1) MLC (W2) : +38% over DRAM. +264% over default interleave.
MLC (W5) : +40% over DRAM. +226% over default interleave.
2) Stream : -6% to +4% over DRAM, +430% over default interleave.
3) XSBench : +19% over DRAM. +47% over default interleave.
= LTP Testing Summary =
existing mempolicy & mbind tests: pass
mempolicy & mbind + weighted interleave (global weights): pass
= version history
v4:
- style fixes, code deduplication, simplifications, comments
- moved mempolicy->il_weight to task_struct->il_weight
- changed logic to simply move forward on il_weight=0 rather than
treat il_weight=0 as a special value
- detect when il_prev is no longer in the nodemask and move forward
- missed weighted interleave check in alloc_pages_mpol()
- uninitialized value of nr_allocated in bulk allocator
=====================================================================
Performance tests - MLC
From - Ravi Jonnalagadda <ravis.opensrc@micron.com>
Hardware: Single-socket, multiple CXL memory expanders.
Workload: W2
Data Signature: 2:1 read:write
DRAM only bandwidth (GBps): 298.8
DRAM + CXL (default interleave) (GBps): 113.04
DRAM + CXL (weighted interleave)(GBps): 412.5
Gain over DRAM only: 1.38x
Gain over default interleave: 2.64x
Workload: W5
Data Signature: 1:1 read:write
DRAM only bandwidth (GBps): 273.2
DRAM + CXL (default interleave) (GBps): 117.23
DRAM + CXL (weighted interleave)(GBps): 382.7
Gain over DRAM only: 1.4x
Gain over default interleave: 2.26x
=====================================================================
Performance test - Stream
From - Gregory Price <gregory.price@memverge.com>
Hardware: Single socket, single CXL expander
numactl extension: https://github.com/gmprice/numactl/tree/weighted_interleave_master
Summary: 64 threads, ~18GB workload, 3GB per array, executed 100 times
Default interleave : -78% (slower than DRAM)
Global weighting : -6% to +4% (workload dependant)
mbind2 weights : +2.5% to +4% (consistently better than DRAM)
dram only:
numactl --cpunodebind=1 --membind=1 ./stream_c.exe --ntimes 100 --array-size 400M --malloc
Function Direction BestRateMBs AvgTime MinTime MaxTime
Copy: 0->0 200923.2 0.032662 0.031853 0.033301
Scale: 0->0 202123.0 0.032526 0.031664 0.032970
Add: 0->0 208873.2 0.047322 0.045961 0.047884
Triad: 0->0 208523.8 0.047262 0.046038 0.048414
CXL-only:
numactl --cpunodebind=1 -w --membind=2 ./stream_c.exe --ntimes 100 --array-size 400M --malloc
Copy: 0->0 22209.7 0.288661 0.288162 0.289342
Scale: 0->0 22288.2 0.287549 0.287147 0.288291
Add: 0->0 24419.1 0.393372 0.393135 0.393735
Triad: 0->0 24484.6 0.392337 0.392083 0.394331
Based on the above, the optimal weights are ~9:1
echo 9 > /sys/kernel/mm/mempolicy/weighted_interleave/node1
echo 1 > /sys/kernel/mm/mempolicy/weighted_interleave/node2
default interleave:
numactl --cpunodebind=1 --interleave=1,2 ./stream_c.exe --ntimes 100 --array-size 400M --malloc
Copy: 0->0 44666.2 0.143671 0.143285 0.144174
Scale: 0->0 44781.6 0.143256 0.142916 0.143713
Add: 0->0 48600.7 0.197719 0.197528 0.197858
Triad: 0->0 48727.5 0.197204 0.197014 0.197439
global weighted interleave:
numactl --cpunodebind=1 -w --interleave=1,2 ./stream_c.exe --ntimes 100 --array-size 400M --malloc
Copy: 0->0 190085.9 0.034289 0.033669 0.034645
Scale: 0->0 207677.4 0.031909 0.030817 0.033061
Add: 0->0 202036.8 0.048737 0.047516 0.053409
Triad: 0->0 217671.5 0.045819 0.044103 0.046755
targted regions w/ global weights (modified stream to mbind2 malloc'd regions))
numactl --cpunodebind=1 --membind=1 ./stream_c.exe -b --ntimes 100 --array-size 400M --malloc
Copy: 0->0 205827.0 0.031445 0.031094 0.031984
Scale: 0->0 208171.8 0.031320 0.030744 0.032505
Add: 0->0 217352.0 0.045087 0.044168 0.046515
Triad: 0->0 216884.8 0.045062 0.044263 0.046982
=====================================================================
Performance tests - XSBench
From - Hyeongtak Ji <hyeongtak.ji@sk.com>
Hardware: Single socket, Single CXL memory Expander
NUMA node 0: 56 logical cores, 128 GB memory
NUMA node 2: 96 GB CXL memory
Threads: 56
Lookups: 170,000,000
Summary: +19% over DRAM. +47% over default interleave.
Performance tests - XSBench
1. dram only
$ numactl -m 0 ./XSBench -s XL –p 5000000
Runtime: 36.235 seconds
Lookups/s: 4,691,618
2. default interleave
$ numactl –i 0,2 ./XSBench –s XL –p 5000000
Runtime: 55.243 seconds
Lookups/s: 3,077,293
3. weighted interleave
numactl –w –i 0,2 ./XSBench –s XL –p 5000000
Runtime: 29.262 seconds
Lookups/s: 5,809,513
=====================================================================
LTP Tests: https://github.com/gmprice/ltp/tree/mempolicy2
= Existing tests
set_mempolicy, get_mempolicy, mbind
MPOL_WEIGHTED_INTERLEAVE added manually to test basic functionality
but did not adjust tests for weighting. Basically the weights were
set to 1, which is the default, and it should behave the same as
MPOL_INTERLEAVE if logic is correct.
== set_mempolicy01 : passed 18, failed 0
== set_mempolicy02 : passed 10, failed 0
== set_mempolicy03 : passed 64, failed 0
== set_mempolicy04 : passed 32, failed 0
== set_mempolicy05 - n/a on non-x86
== set_mempolicy06 : passed 10, failed 0
this is set_mempolicy02 + MPOL_WEIGHTED_INTERLEAVE
== set_mempolicy07 : passed 32, failed 0
set_mempolicy04 + MPOL_WEIGHTED_INTERLEAVE
== get_mempolicy01 : passed 12, failed 0
change: added MPOL_WEIGHTED_INTERLEAVE
== get_mempolicy02 : passed 2, failed 0
== mbind01 : passed 15, failed 0
added MPOL_WEIGHTED_INTERLEAVE
== mbind02 : passed 4, failed 0
added MPOL_WEIGHTED_INTERLEAVE
== mbind03 : passed 16, failed 0
added MPOL_WEIGHTED_INTERLEAVE
== mbind04 : passed 48, failed 0
added MPOL_WEIGHTED_INTERLEAVE
=====================================================================
numactl (set_mempolicy) w/ global weighting test
numactl fork: https://github.com/gmprice/numactl/tree/weighted_interleave_master
command: numactl -w --interleave=0,1 ./eatmem
result (weights 1:1):
0176a000 weighted interleave:0-1 heap anon=65793 dirty=65793 active=0 N0=32897 N1=32896 kernelpagesize_kB=4
7fceeb9ff000 weighted interleave:0-1 anon=65537 dirty=65537 active=0 N0=32768 N1=32769 kernelpagesize_kB=4
50% distribution is correct
result (weights 5:1):
01b14000 weighted interleave:0-1 heap anon=65793 dirty=65793 active=0 N0=54828 N1=10965 kernelpagesize_kB=4
7f47a1dff000 weighted interleave:0-1 anon=65537 dirty=65537 active=0 N0=54614 N1=10923 kernelpagesize_kB=4
16.666% distribution is correct
result (weights 1:5):
01f07000 weighted interleave:0-1 heap anon=65793 dirty=65793 active=0 N0=10966 N1=54827 kernelpagesize_kB=4
7f17b1dff000 weighted interleave:0-1 anon=65537 dirty=65537 active=0 N0=10923 N1=54614 kernelpagesize_kB=4
16.666% distribution is correct
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
int main (void)
{
char* mem = malloc(1024*1024*256);
memset(mem, 1, 1024*1024*256);
for (int i = 0; i < ((1024*1024*256)/4096); i++)
{
mem = malloc(4096);
mem[0] = 1;
}
printf("done\n");
getchar();
return 0;
}
=====================================================================
Suggested-by: Gregory Price <gregory.price@memverge.com>
Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
Suggested-by: Hasan Al Maruf <hasanalmaruf@fb.com>
Suggested-by: Hao Wang <haowang3@fb.com>
Suggested-by: Ying Huang <ying.huang@intel.com>
Suggested-by: Dan Williams <dan.j.williams@intel.com>
Suggested-by: Michal Hocko <mhocko@suse.com>
Suggested-by: Zhongkun He <hezhongkun.hzk@bytedance.com>
Suggested-by: Frank van der Linden <fvdl@google.com>
Suggested-by: John Groves <john@jagalactic.com>
Suggested-by: Vinicius Tavares Petrucci <vtavarespetr@micron.com>
Suggested-by: Srinivasulu Thanneeru <sthanneeru@micron.com>
Suggested-by: Ravi Jonnalagadda <ravis.opensrc@micron.com>
Suggested-by: Jonathan Cameron <Jonathan.Cameron@Huawei.com>
Suggested-by: Hyeongtak Ji <hyeongtak.ji@sk.com>
Suggested-by: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Gregory Price <gregory.price@memverge.com>
Gregory Price (2):
mm/mempolicy: refactor a read-once mechanism into a function for
re-use
mm/mempolicy: introduce MPOL_WEIGHTED_INTERLEAVE for weighted
interleaving
Rakie Kim (1):
mm/mempolicy: implement the sysfs-based weighted_interleave interface
.../ABI/testing/sysfs-kernel-mm-mempolicy | 4 +
...fs-kernel-mm-mempolicy-weighted-interleave | 25 +
.../admin-guide/mm/numa_memory_policy.rst | 9 +
include/linux/sched.h | 1 +
include/uapi/linux/mempolicy.h | 1 +
mm/mempolicy.c | 480 +++++++++++++++++-
6 files changed, 506 insertions(+), 14 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-mempolicy
create mode 100644 Documentation/ABI/testing/sysfs-kernel-mm-mempolicy-weighted-interleave
--
2.39.1
^ permalink raw reply
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Gregory Price @ 2024-01-30 16: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
In-Reply-To: <871q9ziel5.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Tue, Jan 30, 2024 at 01:18:30PM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
>
> > For normal interleave, this isn't an issue because it always proceeds to
> > the next node. The same is not true of weighted interleave, which may
> > have a hanging weight in task->il_weight.
>
> So, I added a check as follows,
>
> node_isset(current->il_prev, policy->nodes)
>
> If prev node is removed from nodemask, allocation will proceed to the
> next node. Otherwise, it's safe to use current->il_weight.
>
Funny enough I have this on one of my branches and dropped it, but after
digging through everything - this should be sufficient.
I'll just add il_weight next to il_prev and have a new set of patches
out today. Code is already there, just needs one last cleanup pass.
~Gregory
^ permalink raw reply
* Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()
From: Oleg Nesterov @ 2024-01-30 11:21 UTC (permalink / raw)
To: Christian Brauner
Cc: Tycho Andersen, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240129-plakat-einlud-4903633a5410@brauner>
On 01/29, Christian Brauner wrote:
>
> On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote:
> > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> > prepare_kill_siginfo(sig, &kinfo);
> > }
> >
> > + /* TODO: respect PIDFD_THREAD */
>
> So I've been thinking about this at the end of last week. Do we need to
> give userspace a way to send a thread-group wide signal even when a
> PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> right now and wait until someone needs this?
I don't know. I am fine either way, but I think this needs a separate
patch and another discussion in any case. Anyway should be trivial,
pidfd_send_signal() has the "flags" argument.
On a related note, should copy_process(CLONE_PIDFD | CLONE_THREAD) add
PIDFD_THREAD flag "automatically" depending on CLONE_THREAD? Or do we
want another CLONE_PIDFD_THREAD flag so that PIDFD_THREAD can be used
without CLONE_THREAD? Again, I do not know, needs another discussion.
> Otherwise this looks good to me!
OK, thanks, I'll send v2 in a minute. The patch is the same, I only
updated the comments.
Oleg.
^ permalink raw reply
* Re: Several tst-robust* tests time out with recent Linux kernel
From: Stefan Liebler @ 2024-01-30 10:12 UTC (permalink / raw)
To: Edgecombe, Rick P, peterz@infradead.org
Cc: xry111@xry111.site, hca@linux.ibm.com, andrealmeid@igalia.com,
fweimer@redhat.com, linux-mm@kvack.org, libc-alpha@sourceware.org,
linux-kernel@vger.kernel.org, tglx@linutronix.de,
svens@linux.ibm.com, linux-api@vger.kernel.org,
linux-arch@vger.kernel.org
In-Reply-To: <eeb6d178dff61dfebf5a3ce9675486a3271b748c.camel@intel.com>
On 29.01.24 23:23, Edgecombe, Rick P wrote:
> On Fri, 2024-01-19 at 14:56 +0100, Stefan Liebler wrote:
>> I've reduced the test (see attachement) and now have only one process
>> with three threads.
>
> This tests fails on my setup as well:
> main: start 3 threads.
> #0: started: fct=1
> #1: started: fct=1
> #2: started: fct=1
> #2: mutex_timedlock failed with 22 (round=28772)
>
> But, after this patch:
> https://lore.kernel.org/all/20240116130810.ji1YCxpg@linutronix.de/
>
> ...the attached test hangs.
>
> However, the glibc test that was failing for me "nptl/tst-robustpi8"
> passes with the linked patch applied. So I think that patch fixes the
> issue I hit.
>
> What is passing supposed to look like on the attached test?
kernel commit "futex: Prevent the reuse of stale pi_state"
https://git.kernel.org/pub/scm/linux/kernel/git/tip/tip.git/patch/?id=e626cb02ee8399fd42c415e542d031d185783903
fixes the issue on s390x.
With this commit, the test runs to the end:
main: start 3 threads.
#0: started: fct=1
#1: started: fct=1
#2: started: fct=1
#2: REACHED round 100000000. => exit
#0: REACHED round 100000000. => exit
#1: REACHED round 100000000. => exit
main: end.
If you want you can reduce the number of rounds by compiling with
-DROUNDS=XYZ or manually adjusting the ROUNDS macro define.
^ permalink raw reply
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Huang, Ying @ 2024-01-30 5:18 UTC (permalink / raw)
To: Gregory Price
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
In-Reply-To: <ZbhuJTBp68e8eLRv@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Tue, Jan 30, 2024 at 11:15:35AM +0800, Huang, Ying wrote:
>> Gregory Price <gregory.price@memverge.com> writes:
>>
>> > On Mon, Jan 29, 2024 at 10:48:47AM -0500, Gregory Price wrote:
>> >> On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
>> >> > Gregory Price <gregory.price@memverge.com> writes:
>> >> >
>> >> > But, in contrast, it's bad to put task-local "current weight" in
>> >> > mempolicy. So, I think that it's better to move cur_il_weight to
>> >> > task_struct. And maybe combine it with current->il_prev.
>> >> >
>> >> Style question: is it preferable add an anonymous union into task_struct:
>> >>
>> >> union {
>> >> short il_prev;
>> >> atomic_t wil_node_weight;
>> >> };
>> >>
>> >> Or should I break out that union explicitly in mempolicy.h?
>> >>
>> >
>> > Having attempted this, it looks like including mempolicy.h into sched.h
>> > is a non-starter. There are build issues likely associated from the
>> > nested include of uapi/linux/mempolicy.h
>> >
>> > So I went ahead and did the following. Style-wise If it's better to just
>> > integrate this as an anonymous union in task_struct, let me know, but it
>> > seemed better to add some documentation here.
>> >
>> > I also added static get/set functions to mempolicy.c to touch these
>> > values accordingly.
>> >
>> > As suggested, I changed things to allow 0-weight in il_prev.node_weight
>> > adjusted the logic accordingly. Will be testing this for a day or so
>> > before sending out new patches.
>> >
>>
>> Thanks about this again. It seems that we don't need to touch
>> task->il_prev and task->il_weight during rebinding for weighted
>> interleave too.
>>
>
> It's not clear to me this is the case. cpusets takes the task_lock to
> change mems_allowed and rebind task->mempolicy, but I do not see the
> task lock access blocking allocations.
>
> Comments from cpusets suggest allocations can happen in parallel.
>
> /*
> * cpuset_change_task_nodemask - change task's mems_allowed and mempolicy
> * @tsk: the task to change
> * @newmems: new nodes that the task will be set
> *
> * We use the mems_allowed_seq seqlock to safely update both tsk->mems_allowed
> * and rebind an eventual tasks' mempolicy. If the task is allocating in
> * parallel, it might temporarily see an empty intersection, which results in
> * a seqlock check and retry before OOM or allocation failure.
> */
>
>
> For normal interleave, this isn't an issue because it always proceeds to
> the next node. The same is not true of weighted interleave, which may
> have a hanging weight in task->il_weight.
So, I added a check as follows,
node_isset(current->il_prev, policy->nodes)
If prev node is removed from nodemask, allocation will proceed to the
next node. Otherwise, it's safe to use current->il_weight.
--
Best Regards,
Huang, Ying
> That is why I looked to combine the two, so at least node/weight were
> carried together.
>
>> unsigned int weighted_interleave_nodes(struct mempolicy *policy)
>> {
>> unsigned int nid;
>> struct task_struct *me = current;
>>
>> nid = me->il_prev;
>> if (!me->il_weight || !node_isset(nid, policy->nodes)) {
>> nid = next_node_in(...);
>> me->il_prev = nid;
>> me->il_weight = weights[nid];
>> }
>> me->il_weight--;
>>
>> return nid;
>> }
>
> I ended up with this:
>
> static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
> {
> unsigned int node;
> u8 weight;
>
> get_wil_prev(&node, &weight);
> /* If nodemask was rebound, just fetch the next node */
> if (!weight) {
> node = next_node_in(node, policy->nodes);
> /* can only happen if nodemask has become invalid */
> if (node == MAX_NUMNODES)
> return node;
> weight = get_il_weight(node);
> }
> weight--;
> set_wil_prev(node, weight);
> return node;
> }
>
> ~Gregory
^ permalink raw reply
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Gregory Price @ 2024-01-30 3:33 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
In-Reply-To: <875xzbika0.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Tue, Jan 30, 2024 at 11:15:35AM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
>
> > On Mon, Jan 29, 2024 at 10:48:47AM -0500, Gregory Price wrote:
> >> On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
> >> > Gregory Price <gregory.price@memverge.com> writes:
> >> >
> >> > But, in contrast, it's bad to put task-local "current weight" in
> >> > mempolicy. So, I think that it's better to move cur_il_weight to
> >> > task_struct. And maybe combine it with current->il_prev.
> >> >
> >> Style question: is it preferable add an anonymous union into task_struct:
> >>
> >> union {
> >> short il_prev;
> >> atomic_t wil_node_weight;
> >> };
> >>
> >> Or should I break out that union explicitly in mempolicy.h?
> >>
> >
> > Having attempted this, it looks like including mempolicy.h into sched.h
> > is a non-starter. There are build issues likely associated from the
> > nested include of uapi/linux/mempolicy.h
> >
> > So I went ahead and did the following. Style-wise If it's better to just
> > integrate this as an anonymous union in task_struct, let me know, but it
> > seemed better to add some documentation here.
> >
> > I also added static get/set functions to mempolicy.c to touch these
> > values accordingly.
> >
> > As suggested, I changed things to allow 0-weight in il_prev.node_weight
> > adjusted the logic accordingly. Will be testing this for a day or so
> > before sending out new patches.
> >
>
> Thanks about this again. It seems that we don't need to touch
> task->il_prev and task->il_weight during rebinding for weighted
> interleave too.
>
It's not clear to me this is the case. cpusets takes the task_lock to
change mems_allowed and rebind task->mempolicy, but I do not see the
task lock access blocking allocations.
Comments from cpusets suggest allocations can happen in parallel.
/*
* cpuset_change_task_nodemask - change task's mems_allowed and mempolicy
* @tsk: the task to change
* @newmems: new nodes that the task will be set
*
* We use the mems_allowed_seq seqlock to safely update both tsk->mems_allowed
* and rebind an eventual tasks' mempolicy. If the task is allocating in
* parallel, it might temporarily see an empty intersection, which results in
* a seqlock check and retry before OOM or allocation failure.
*/
For normal interleave, this isn't an issue because it always proceeds to
the next node. The same is not true of weighted interleave, which may
have a hanging weight in task->il_weight.
That is why I looked to combine the two, so at least node/weight were
carried together.
> unsigned int weighted_interleave_nodes(struct mempolicy *policy)
> {
> unsigned int nid;
> struct task_struct *me = current;
>
> nid = me->il_prev;
> if (!me->il_weight || !node_isset(nid, policy->nodes)) {
> nid = next_node_in(...);
> me->il_prev = nid;
> me->il_weight = weights[nid];
> }
> me->il_weight--;
>
> return nid;
> }
I ended up with this:
static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
{
unsigned int node;
u8 weight;
get_wil_prev(&node, &weight);
/* If nodemask was rebound, just fetch the next node */
if (!weight) {
node = next_node_in(node, policy->nodes);
/* can only happen if nodemask has become invalid */
if (node == MAX_NUMNODES)
return node;
weight = get_il_weight(node);
}
weight--;
set_wil_prev(node, weight);
return node;
}
~Gregory
^ permalink raw reply
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Huang, Ying @ 2024-01-30 3:15 UTC (permalink / raw)
To: Gregory Price
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
In-Reply-To: <ZbfqVHA9+38/j3Mq@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Mon, Jan 29, 2024 at 10:48:47AM -0500, Gregory Price wrote:
>> On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
>> > Gregory Price <gregory.price@memverge.com> writes:
>> >
>> > But, in contrast, it's bad to put task-local "current weight" in
>> > mempolicy. So, I think that it's better to move cur_il_weight to
>> > task_struct. And maybe combine it with current->il_prev.
>> >
>> Style question: is it preferable add an anonymous union into task_struct:
>>
>> union {
>> short il_prev;
>> atomic_t wil_node_weight;
>> };
>>
>> Or should I break out that union explicitly in mempolicy.h?
>>
>
> Having attempted this, it looks like including mempolicy.h into sched.h
> is a non-starter. There are build issues likely associated from the
> nested include of uapi/linux/mempolicy.h
>
> So I went ahead and did the following. Style-wise If it's better to just
> integrate this as an anonymous union in task_struct, let me know, but it
> seemed better to add some documentation here.
>
> I also added static get/set functions to mempolicy.c to touch these
> values accordingly.
>
> As suggested, I changed things to allow 0-weight in il_prev.node_weight
> adjusted the logic accordingly. Will be testing this for a day or so
> before sending out new patches.
>
Thanks about this again. It seems that we don't need to touch
task->il_prev and task->il_weight during rebinding for weighted
interleave too.
For weighted interleaving, il_prev is the node used for previous
allocation, il_weight is the weight after previous allocation. So
weighted_interleave_nodes() could be as follows,
unsigned int weighted_interleave_nodes(struct mempolicy *policy)
{
unsigned int nid;
struct task_struct *me = current;
nid = me->il_prev;
if (!me->il_weight || !node_isset(nid, policy->nodes)) {
nid = next_node_in(...);
me->il_prev = nid;
me->il_weight = weights[nid];
}
me->il_weight--;
return nid;
}
If this works, we can just add il_weight into task_struct.
--
Best Regards,
Huang, Ying
^ permalink raw reply
* Re: Several tst-robust* tests time out with recent Linux kernel
From: Edgecombe, Rick P @ 2024-01-29 22:23 UTC (permalink / raw)
To: peterz@infradead.org, stli@linux.ibm.com
Cc: xry111@xry111.site, hca@linux.ibm.com, andrealmeid@igalia.com,
fweimer@redhat.com, linux-mm@kvack.org, libc-alpha@sourceware.org,
linux-kernel@vger.kernel.org, tglx@linutronix.de,
svens@linux.ibm.com, linux-api@vger.kernel.org,
linux-arch@vger.kernel.org
In-Reply-To: <fc3fd07a-218d-406c-918b-e7f701968eb0@linux.ibm.com>
On Fri, 2024-01-19 at 14:56 +0100, Stefan Liebler wrote:
> I've reduced the test (see attachement) and now have only one process
> with three threads.
This tests fails on my setup as well:
main: start 3 threads.
#0: started: fct=1
#1: started: fct=1
#2: started: fct=1
#2: mutex_timedlock failed with 22 (round=28772)
But, after this patch:
https://lore.kernel.org/all/20240116130810.ji1YCxpg@linutronix.de/
...the attached test hangs.
However, the glibc test that was failing for me "nptl/tst-robustpi8"
passes with the linked patch applied. So I think that patch fixes the
issue I hit.
What is passing supposed to look like on the attached test?
^ permalink raw reply
* Re: [PATCH net-next v3 0/3] Per epoll context busy poll support
From: Joe Damato @ 2024-01-29 19:09 UTC (permalink / raw)
To: Willem de Bruijn
Cc: linux-kernel, netdev, chuck.lever, jlayton, linux-api, brauner,
edumazet, davem, alexander.duyck, sridhar.samudrala, kuba, weiwan,
Alexander Viro, Andrew Waterman, Arnd Bergmann, Dominik Brodowski,
Greg Kroah-Hartman, Jan Kara, Jiri Slaby, Jonathan Corbet,
Julien Panis, open list:DOCUMENTATION,
(open list:FILESYSTEMS \(VFS and infrastructure\)),
Michael Ellerman, Nathan Lynch, Palmer Dabbelt, Steve French,
Thomas Huth, Thomas Zimmermann
In-Reply-To: <65b52d6381de7_3a9e0b2943d@willemb.c.googlers.com.notmuch>
On Sat, Jan 27, 2024 at 11:20:51AM -0500, Willem de Bruijn wrote:
> Joe Damato wrote:
> > Greetings:
> >
> > Welcome to v3. Cover letter updated from v2 to explain why ioctl and
> > adjusted my cc_cmd to try to get the correct people in addition to folks
> > who were added in v1 & v2. Labeled as net-next because it seems networking
> > related to me even though it is fs code.
> >
> > TL;DR This builds on commit bf3b9f6372c4 ("epoll: Add busy poll support to
> > epoll with socket fds.") by allowing user applications to enable
> > epoll-based busy polling and set a busy poll packet budget on a per epoll
> > context basis.
> >
> > This makes epoll-based busy polling much more usable for user
> > applications than the current system-wide sysctl and hardcoded budget.
> >
> > To allow for this, two ioctls have been added for epoll contexts for
> > getting and setting a new struct, struct epoll_params.
> >
> > ioctl was chosen vs a new syscall after reviewing a suggestion by Willem
> > de Bruijn [1]. I am open to using a new syscall instead of an ioctl, but it
> > seemed that:
> > - Busy poll affects all existing epoll_wait and epoll_pwait variants in
> > the same way, so new verions of many syscalls might be needed. It
>
> There is no need to support a new feature on legacy calls. Applications have
> to be upgraded to the new ioctl, so they can also be upgraded to the latest
> epoll_wait variant.
Sure, that's a fair point. I think we could probably make reasonable
arguments in both directions about the pros/cons of each approach.
It's still not clear to me that a new syscall is the best way to go on
this, and IMO it does not offer a clear advantage. I understand that part
of the premise of your argument is that ioctls are not recommended, but in
this particular case it seems like a good use case and there have been
new ioctls added recently (at least according to git log).
This makes me think that while their use is not recommended, they can serve
a purpose in specific use cases. To me, this use case seems very fitting.
More of a joke and I hate to mention this, but this setting is changing how
io is done and it seems fitting that this done via an ioctl ;)
> epoll_pwait extends epoll_wait with a sigmask.
> epoll_pwait2 extends extends epoll_pwait with nsec resolution timespec.
> Since they are supersets, nothing is lots by limiting to the most recent API.
>
> In the discussion of epoll_pwait2 the addition of a forward looking flags
> argument was discussed, but eventually dropped. Based on the argument that
> adding a syscall is not a big task and does not warrant preemptive code.
> This decision did receive a suitably snarky comment from Jonathan Corbet [1].
>
> It is definitely more boilerplate, but essentially it is as feasible to add an
> epoll_pwait3 that takes an optional busy poll argument. In which case, I also
> believe that it makes more sense to configure the behavior of the syscall
> directly, than through another syscall and state stored in the kernel.
I definitely hear what you are saying; I think I'm still not convinced, but
I am thinking it through.
In my mind, all of the other busy poll settings are configured by setting
options on the sockets using various SO_* options, which modify some state
in the kernel. The existing system-wide busy poll sysctl also does this. It
feels strange to me to diverge from that pattern just for epoll.
In the case of epoll_pwait2 the addition of a new syscall is an approach
that I think makes a lot of sense. The new system call is also probably
better from an end-user usability perspective, as well. For busy poll, I
don't see a clear reasoning why a new system call is better, but maybe I am
still missing something.
> I don't think that the usec fine grain busy poll argument is all that useful.
> Documentation always suggests setting it to 50us or 100us, based on limited
> data. Main point is to set it to exceed the round-trip delay of whatever the
> process is trying to wait on. Overestimating is not costly, as the call
> returns as soon as the condition is met. An epoll_pwait3 flag EPOLL_BUSY_POLL
> with default 100us might be sufficient.
>
> [1] https://lwn.net/Articles/837816/
Perhaps I am misunderstanding what you are suggesting, but I am opposed to
hardcoding a value. If it is currently configurable system-wide and via
SO_* options for other forms of busy poll, I think it should similarly be
configurable for epoll busy poll.
I may yet be convinced by the new syscall argument, but I don't think I'd
agree on imposing a default. The value can be modified by other forms of
busy poll and the goal of my changes are to:
- make epoll-based busy poll per context
- allow applications to configure (within reason) how epoll-based busy
poll behaves, like they can do now with the existing SO_* options for
other busy poll methods.
> > seems much simpler for users to use the correct
> > epoll_wait/epoll_pwait for their app and add a call to ioctl to enable
> > or disable busy poll as needed. This also probably means less work to
> > get an existing epoll app using busy poll.
>
^ permalink raw reply
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Gregory Price @ 2024-01-29 18:11 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
In-Reply-To: <ZbfI3+nhgQlNKMPG@memverge.com>
On Mon, Jan 29, 2024 at 10:48:47AM -0500, Gregory Price wrote:
> On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
> > Gregory Price <gregory.price@memverge.com> writes:
> >
> > But, in contrast, it's bad to put task-local "current weight" in
> > mempolicy. So, I think that it's better to move cur_il_weight to
> > task_struct. And maybe combine it with current->il_prev.
> >
> Style question: is it preferable add an anonymous union into task_struct:
>
> union {
> short il_prev;
> atomic_t wil_node_weight;
> };
>
> Or should I break out that union explicitly in mempolicy.h?
>
Having attempted this, it looks like including mempolicy.h into sched.h
is a non-starter. There are build issues likely associated from the
nested include of uapi/linux/mempolicy.h
So I went ahead and did the following. Style-wise If it's better to just
integrate this as an anonymous union in task_struct, let me know, but it
seemed better to add some documentation here.
I also added static get/set functions to mempolicy.c to touch these
values accordingly.
As suggested, I changed things to allow 0-weight in il_prev.node_weight
adjusted the logic accordingly. Will be testing this for a day or so
before sending out new patches.
~Gregory
diff --git a/include/linux/sched.h b/include/linux/sched.h
index ffe8f618ab86..f0d2af3bbc69 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -745,6 +745,29 @@ struct kmap_ctrl {
#endif
};
+
+/*
+ * Describes task_struct interleave settings
+ *
+ * Interleave uses mpol_interleave.node
+ * last node allocated from
+ * intended for use in next_node_in() on the next allocation
+ *
+ * Weighted interleave uses mpol_interleave.node_weight
+ * node is the value of the current node to allocate from
+ * weight is the number of allocations left on that node
+ * when weight is 0, next_node_in(node) will be invoked
+ */
+union mpol_interleave {
+ struct {
+ short node;
+ short resv;
+ };
+ /* structure: short node; u8 resv; u8 weight; */
+ atomic_t node_weight;
+};
+
+
struct task_struct {
#ifdef CONFIG_THREAD_INFO_IN_TASK
/*
@@ -1258,7 +1281,7 @@ struct task_struct {
#ifdef CONFIG_NUMA
/* Protected by alloc_lock: */
struct mempolicy *mempolicy;
- short il_prev;
+ union mpol_interleave il_prev;
short pref_node_fork;
#endif
#ifdef CONFIG_NUMA_BALANCING
diff --git a/mm/mempolicy.c b/mm/mempolicy.c
index 92740b8f0eb5..48e365b507b2 100644
--- a/mm/mempolicy.c
+++ b/mm/mempolicy.c
@@ -149,6 +149,66 @@ static struct mempolicy preferred_node_policy[MAX_NUMNODES];
static u8 __rcu *iw_table;
static DEFINE_MUTEX(iw_table_lock);
+static u8 get_il_weight(int node)
+{
+ u8 __rcu *table;
+ u8 weight;
+
+ rcu_read_lock();
+ table = rcu_dereference(iw_table);
+ /* if no iw_table, use system default */
+ weight = table ? table[node] : 1;
+ /* if value in iw_table is 0, use system default */
+ weight = weight ? weight : 1;
+ rcu_read_unlock();
+ return weight;
+}
+
+/* Clear any interleave values from task->il_prev */
+static void clear_il_prev(void)
+{
+ int node_weight;
+
+ node_weight = MAKE_WIL_PREV(MAX_NUMNODES - 1, 0);
+ atomic_set(¤t->il_prev.node_weight, node_weight);
+}
+
+/* get the next value for weighted interleave */
+static void get_wil_prev(int *node, u8 *weight)
+{
+ int node_weight;
+
+ node_weight = atomic_read(¤t->il_prev.node_weight);
+ *node = WIL_NODE(node_weight);
+ *weight = WIL_WEIGHT(node_weight);
+}
+
+/* set the next value for weighted interleave */
+static void set_wil_prev(int node, u8 weight)
+{
+ int node_weight;
+
+ if (node == MAX_NUMNODES)
+ node -= 1;
+ node_weight = MAKE_WIL_PREV(node, weight);
+ atomic_set(¤t->il_prev.node_weight, node_weight);
+}
+
+/* get the previous interleave node */
+static short get_il_prev(void)
+{
+ return current->il_prev.node;
+}
+
+/* set the previous interleave node */
+static void set_il_prev(int node)
+{
+ if (unlikely(node >= MAX_NUMNODES))
+ node = MAX_NUMNODES - 1;
+
+ current->il_prev.node = node;
+}
+
^ permalink raw reply related
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Gregory Price @ 2024-01-29 15:48 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
In-Reply-To: <877cjsk0yd.fsf@yhuang6-desk2.ccr.corp.intel.com>
On Mon, Jan 29, 2024 at 04:17:46PM +0800, Huang, Ying wrote:
> Gregory Price <gregory.price@memverge.com> writes:
>
> > Using current->il_prev between these two policies, is just plain incorrect,
> > so I will need to rethink this, and the existing code will need to be
> > updated such that weighted_interleave does not use current->il_prev.
>
> IIUC, weighted_interleave_nodes() is only used for mempolicy of tasks
> (set_mempolicy()), as in the following code.
>
> + *nid = (ilx == NO_INTERLEAVE_INDEX) ?
> + weighted_interleave_nodes(pol) :
> + weighted_interleave_nid(pol, ilx);
>
Was digging through this the past couple of days. It does look like
this is true - because if (pol) comes from a vma, ilx will not be
NO_INTERLEAVE_INDEX. If this changes in the future, however,
weighted_interleave_nodes may begin to miscount under heavy contention.
It may be worth documenting this explicitly, because this is incredibly
non-obvious. I will add a comment to this chunk here.
> But, in contrast, it's bad to put task-local "current weight" in
> mempolicy. So, I think that it's better to move cur_il_weight to
> task_struct. And maybe combine it with current->il_prev.
>
Given all of this, I think is reasonably. That is effectively what is
happening anyway for anyone that just uses `numactl -w --interleave=...`
Style question: is it preferable add an anonymous union into task_struct:
union {
short il_prev;
atomic_t wil_node_weight;
};
Or should I break out that union explicitly in mempolicy.h?
The latter involves additional code updates in mempolicy.c for the union
name (current->___.il_prev) but it lets us add documentation to mempolicy.h
~Gregory
^ permalink raw reply
* Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()
From: Christian Brauner @ 2024-01-29 15:14 UTC (permalink / raw)
To: Tycho Andersen
Cc: Oleg Nesterov, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <Zbe2x+M8tDBotSNZ@tycho.pizza>
On Mon, Jan 29, 2024 at 07:31:35AM -0700, Tycho Andersen wrote:
> On Mon, Jan 29, 2024 at 02:41:11PM +0100, Christian Brauner wrote:
> > On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote:
> > > --- a/kernel/signal.c
> > > +++ b/kernel/signal.c
> > > @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
> > > WARN_ON_ONCE(!tsk->ptrace &&
> > > (tsk->group_leader != tsk || !thread_group_empty(tsk)));
> > > /*
> > > - * tsk is a group leader and has no threads, wake up the pidfd waiters.
> > > + * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD
> > > + * waiters.
> > > */
> > > if (thread_group_empty(tsk))
> > > do_notify_pidfd(tsk);
> > > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> > > prepare_kill_siginfo(sig, &kinfo);
> > > }
> > >
> > > + /* TODO: respect PIDFD_THREAD */
> >
> > So I've been thinking about this at the end of last week. Do we need to
> > give userspace a way to send a thread-group wide signal even when a
> > PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> > right now and wait until someone needs this?
>
> I don't need it currently, but it would have been handy for some of
> the tests I wrote.
>
> Should I fix those up and send them too on top of Oleg's v2?
Sure, I don't mind.
^ permalink raw reply
* Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()
From: Tycho Andersen @ 2024-01-29 14:31 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240129-plakat-einlud-4903633a5410@brauner>
On Mon, Jan 29, 2024 at 02:41:11PM +0100, Christian Brauner wrote:
> On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote:
> > --- a/kernel/signal.c
> > +++ b/kernel/signal.c
> > @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
> > WARN_ON_ONCE(!tsk->ptrace &&
> > (tsk->group_leader != tsk || !thread_group_empty(tsk)));
> > /*
> > - * tsk is a group leader and has no threads, wake up the pidfd waiters.
> > + * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD
> > + * waiters.
> > */
> > if (thread_group_empty(tsk))
> > do_notify_pidfd(tsk);
> > @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> > prepare_kill_siginfo(sig, &kinfo);
> > }
> >
> > + /* TODO: respect PIDFD_THREAD */
>
> So I've been thinking about this at the end of last week. Do we need to
> give userspace a way to send a thread-group wide signal even when a
> PIDFD_THREAD pidfd is passed? Or should we just not worry about this
> right now and wait until someone needs this?
I don't need it currently, but it would have been handy for some of
the tests I wrote.
Should I fix those up and send them too on top of Oleg's v2?
Tycho
^ permalink raw reply
* Re: [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()
From: Christian Brauner @ 2024-01-29 13:41 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tycho Andersen, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240129112313.GA11635@redhat.com>
On Mon, Jan 29, 2024 at 12:23:15PM +0100, Oleg Nesterov wrote:
> On 01/27, Oleg Nesterov wrote:
> >
> > I'll (hopefully) send v2 on top of
> >
> > pidfd: cleanup the usage of __pidfd_prepare's flags
> > pidfd: don't do_notify_pidfd() if !thread_group_empty()
> >
> > on Monday
>
> Sorry, I don't have time to finish v2 today, I need to update the comments
> and write the changelog.
>
> But the patch itself is ready, I am sending it for review.
>
> Tycho, Christian, any comments?
>
> Oleg.
>
>
> From c31780f6c1136a72048d24701ac6d8401fc1afda Mon Sep 17 00:00:00 2001
> From: Oleg Nesterov <oleg@redhat.com>
> Date: Sat, 27 Jan 2024 16:59:18 +0100
> Subject: [PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()
>
> ---
> include/uapi/linux/pidfd.h | 3 ++-
> kernel/exit.c | 7 +++++++
> kernel/fork.c | 29 +++++++++++++++++++++++++++--
> kernel/pid.c | 2 +-
> kernel/signal.c | 4 +++-
> 5 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
> index 5406fbc13074..2e6461459877 100644
> --- a/include/uapi/linux/pidfd.h
> +++ b/include/uapi/linux/pidfd.h
> @@ -7,6 +7,7 @@
> #include <linux/fcntl.h>
>
> /* Flags for pidfd_open(). */
> -#define PIDFD_NONBLOCK O_NONBLOCK
> +#define PIDFD_NONBLOCK O_NONBLOCK
> +#define PIDFD_THREAD O_EXCL
>
> #endif /* _UAPI_LINUX_PIDFD_H */
> diff --git a/kernel/exit.c b/kernel/exit.c
> index dfb963d2f862..74fe6bfb9577 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -739,6 +739,13 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
> kill_orphaned_pgrp(tsk->group_leader, NULL);
>
> tsk->exit_state = EXIT_ZOMBIE;
> + /*
> + * sub-thread or delay_group_leader(), wake up the PIDFD_THREAD
> + * waiters.
> + */
> + if (!thread_group_empty(tsk))
> + do_notify_pidfd(tsk);
> +
> if (unlikely(tsk->ptrace)) {
> int sig = thread_group_leader(tsk) &&
> thread_group_empty(tsk) &&
> diff --git a/kernel/fork.c b/kernel/fork.c
> index 347641398f9d..977b58c0eac6 100644
> --- a/kernel/fork.c
> +++ b/kernel/fork.c
> @@ -101,6 +101,7 @@
> #include <linux/user_events.h>
> #include <linux/iommu.h>
> #include <linux/rseq.h>
> +#include <uapi/linux/pidfd.h>
>
> #include <asm/pgalloc.h>
> #include <linux/uaccess.h>
> @@ -2050,6 +2051,8 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
>
> seq_put_decimal_ll(m, "Pid:\t", nr);
>
> + /* TODO: report PIDFD_THREAD */
Ah yes, very good point. We should give userspace an indicator whether
something is thread pidfd or not.
> +
> #ifdef CONFIG_PID_NS
> seq_put_decimal_ll(m, "\nNSpid:\t", nr);
> if (nr > 0) {
> @@ -2068,12 +2071,27 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
> }
> #endif
>
> +static bool pidfd_task_exited(struct pid *pid, bool thread)
> +{
> + struct task_struct *task;
> + bool exited;
> +
> + rcu_read_lock();
> + task = pid_task(pid, PIDTYPE_PID);
> + exited = !task ||
> + (READ_ONCE(task->exit_state) && (thread || thread_group_empty(task)));
> + rcu_read_unlock();
> +
> + return exited;
> +}
> +
> /*
> * Poll support for process exit notification.
> */
> static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> {
> struct pid *pid = file->private_data;
> + bool thread = file->f_flags & PIDFD_THREAD;
> __poll_t poll_flags = 0;
>
> poll_wait(file, &pid->wait_pidfd, pts);
> @@ -2083,7 +2101,7 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
> * If the thread group leader exits before all other threads in the
> * group, then poll(2) should block, similar to the wait(2) family.
> */
> - if (thread_group_exited(pid))
> + if (pidfd_task_exited(pid, thread))
> poll_flags = EPOLLIN | EPOLLRDNORM;
>
> return poll_flags;
> @@ -2141,6 +2159,11 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
> return PTR_ERR(pidfd_file);
> }
> get_pid(pid); /* held by pidfd_file now */
> + /*
> + * anon_inode_getfile() ignores everything outside of the
> + * O_ACCMODE | O_NONBLOCK mask, set PIDFD_THREAD manually.
> + */
> + pidfd_file->f_flags |= (flags & PIDFD_THREAD);
> *ret = pidfd_file;
> return pidfd;
> }
> @@ -2173,7 +2196,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
> */
> int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret)
> {
> - if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
> + bool thread = flags & PIDFD_THREAD;
> +
> + if (!pid || !pid_has_task(pid, thread ? PIDTYPE_PID : PIDTYPE_TGID));
> return -EINVAL;
>
> return __pidfd_prepare(pid, flags, ret);
> diff --git a/kernel/pid.c b/kernel/pid.c
> index c7a3e359f8f5..04bdd5ecf183 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -629,7 +629,7 @@ SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
> int fd;
> struct pid *p;
>
> - if (flags & ~PIDFD_NONBLOCK)
> + if (flags & ~(PIDFD_NONBLOCK | PIDFD_THREAD))
> return -EINVAL;
>
> if (pid <= 0)
> diff --git a/kernel/signal.c b/kernel/signal.c
> index 9561a3962ca6..919cd33a0405 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
> WARN_ON_ONCE(!tsk->ptrace &&
> (tsk->group_leader != tsk || !thread_group_empty(tsk)));
> /*
> - * tsk is a group leader and has no threads, wake up the pidfd waiters.
> + * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD
> + * waiters.
> */
> if (thread_group_empty(tsk))
> do_notify_pidfd(tsk);
> @@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
> prepare_kill_siginfo(sig, &kinfo);
> }
>
> + /* TODO: respect PIDFD_THREAD */
So I've been thinking about this at the end of last week. Do we need to
give userspace a way to send a thread-group wide signal even when a
PIDFD_THREAD pidfd is passed? Or should we just not worry about this
right now and wait until someone needs this?
> ret = kill_pid_info(sig, &kinfo, pid);
Otherwise this looks good to me!
^ permalink raw reply
* [RFC PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()
From: Oleg Nesterov @ 2024-01-29 11:23 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, linux-kernel, linux-api, Tycho Andersen,
Eric W. Biederman
In-Reply-To: <20240127210634.GE13787@redhat.com>
On 01/27, Oleg Nesterov wrote:
>
> I'll (hopefully) send v2 on top of
>
> pidfd: cleanup the usage of __pidfd_prepare's flags
> pidfd: don't do_notify_pidfd() if !thread_group_empty()
>
> on Monday
Sorry, I don't have time to finish v2 today, I need to update the comments
and write the changelog.
But the patch itself is ready, I am sending it for review.
Tycho, Christian, any comments?
Oleg.
From c31780f6c1136a72048d24701ac6d8401fc1afda Mon Sep 17 00:00:00 2001
From: Oleg Nesterov <oleg@redhat.com>
Date: Sat, 27 Jan 2024 16:59:18 +0100
Subject: [PATCH] pidfd: implement PIDFD_THREAD flag for pidfd_open()
---
include/uapi/linux/pidfd.h | 3 ++-
kernel/exit.c | 7 +++++++
kernel/fork.c | 29 +++++++++++++++++++++++++++--
kernel/pid.c | 2 +-
kernel/signal.c | 4 +++-
5 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/include/uapi/linux/pidfd.h b/include/uapi/linux/pidfd.h
index 5406fbc13074..2e6461459877 100644
--- a/include/uapi/linux/pidfd.h
+++ b/include/uapi/linux/pidfd.h
@@ -7,6 +7,7 @@
#include <linux/fcntl.h>
/* Flags for pidfd_open(). */
-#define PIDFD_NONBLOCK O_NONBLOCK
+#define PIDFD_NONBLOCK O_NONBLOCK
+#define PIDFD_THREAD O_EXCL
#endif /* _UAPI_LINUX_PIDFD_H */
diff --git a/kernel/exit.c b/kernel/exit.c
index dfb963d2f862..74fe6bfb9577 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -739,6 +739,13 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
kill_orphaned_pgrp(tsk->group_leader, NULL);
tsk->exit_state = EXIT_ZOMBIE;
+ /*
+ * sub-thread or delay_group_leader(), wake up the PIDFD_THREAD
+ * waiters.
+ */
+ if (!thread_group_empty(tsk))
+ do_notify_pidfd(tsk);
+
if (unlikely(tsk->ptrace)) {
int sig = thread_group_leader(tsk) &&
thread_group_empty(tsk) &&
diff --git a/kernel/fork.c b/kernel/fork.c
index 347641398f9d..977b58c0eac6 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -101,6 +101,7 @@
#include <linux/user_events.h>
#include <linux/iommu.h>
#include <linux/rseq.h>
+#include <uapi/linux/pidfd.h>
#include <asm/pgalloc.h>
#include <linux/uaccess.h>
@@ -2050,6 +2051,8 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
seq_put_decimal_ll(m, "Pid:\t", nr);
+ /* TODO: report PIDFD_THREAD */
+
#ifdef CONFIG_PID_NS
seq_put_decimal_ll(m, "\nNSpid:\t", nr);
if (nr > 0) {
@@ -2068,12 +2071,27 @@ static void pidfd_show_fdinfo(struct seq_file *m, struct file *f)
}
#endif
+static bool pidfd_task_exited(struct pid *pid, bool thread)
+{
+ struct task_struct *task;
+ bool exited;
+
+ rcu_read_lock();
+ task = pid_task(pid, PIDTYPE_PID);
+ exited = !task ||
+ (READ_ONCE(task->exit_state) && (thread || thread_group_empty(task)));
+ rcu_read_unlock();
+
+ return exited;
+}
+
/*
* Poll support for process exit notification.
*/
static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
{
struct pid *pid = file->private_data;
+ bool thread = file->f_flags & PIDFD_THREAD;
__poll_t poll_flags = 0;
poll_wait(file, &pid->wait_pidfd, pts);
@@ -2083,7 +2101,7 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
* If the thread group leader exits before all other threads in the
* group, then poll(2) should block, similar to the wait(2) family.
*/
- if (thread_group_exited(pid))
+ if (pidfd_task_exited(pid, thread))
poll_flags = EPOLLIN | EPOLLRDNORM;
return poll_flags;
@@ -2141,6 +2159,11 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
return PTR_ERR(pidfd_file);
}
get_pid(pid); /* held by pidfd_file now */
+ /*
+ * anon_inode_getfile() ignores everything outside of the
+ * O_ACCMODE | O_NONBLOCK mask, set PIDFD_THREAD manually.
+ */
+ pidfd_file->f_flags |= (flags & PIDFD_THREAD);
*ret = pidfd_file;
return pidfd;
}
@@ -2173,7 +2196,9 @@ static int __pidfd_prepare(struct pid *pid, unsigned int flags, struct file **re
*/
int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret)
{
- if (!pid || !pid_has_task(pid, PIDTYPE_TGID))
+ bool thread = flags & PIDFD_THREAD;
+
+ if (!pid || !pid_has_task(pid, thread ? PIDTYPE_PID : PIDTYPE_TGID));
return -EINVAL;
return __pidfd_prepare(pid, flags, ret);
diff --git a/kernel/pid.c b/kernel/pid.c
index c7a3e359f8f5..04bdd5ecf183 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -629,7 +629,7 @@ SYSCALL_DEFINE2(pidfd_open, pid_t, pid, unsigned int, flags)
int fd;
struct pid *p;
- if (flags & ~PIDFD_NONBLOCK)
+ if (flags & ~(PIDFD_NONBLOCK | PIDFD_THREAD))
return -EINVAL;
if (pid <= 0)
diff --git a/kernel/signal.c b/kernel/signal.c
index 9561a3962ca6..919cd33a0405 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2051,7 +2051,8 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
WARN_ON_ONCE(!tsk->ptrace &&
(tsk->group_leader != tsk || !thread_group_empty(tsk)));
/*
- * tsk is a group leader and has no threads, wake up the pidfd waiters.
+ * tsk is a group leader and has no threads, wake up the !PIDFD_THREAD
+ * waiters.
*/
if (thread_group_empty(tsk))
do_notify_pidfd(tsk);
@@ -3926,6 +3927,7 @@ SYSCALL_DEFINE4(pidfd_send_signal, int, pidfd, int, sig,
prepare_kill_siginfo(sig, &kinfo);
}
+ /* TODO: respect PIDFD_THREAD */
ret = kill_pid_info(sig, &kinfo, pid);
err:
--
2.25.1.362.g51ebf55
^ permalink raw reply related
* Re: [PATCH v3 4/4] mm/mempolicy: change cur_il_weight to atomic and carry the node with it
From: Huang, Ying @ 2024-01-29 8:17 UTC (permalink / raw)
To: Gregory Price
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
In-Reply-To: <ZbPf6d2cQykdl3Eb@memverge.com>
Gregory Price <gregory.price@memverge.com> writes:
> On Fri, Jan 26, 2024 at 03:40:27PM +0800, Huang, Ying wrote:
>> Gregory Price <gourry.memverge@gmail.com> writes:
>>
>> > Two special observations:
>> > - if the weight is non-zero, cur_il_weight must *always* have a
>> > valid node number, e.g. it cannot be NUMA_NO_NODE (-1).
>>
>> IIUC, we don't need that, "MAX_NUMNODES-1" is used instead.
>>
>
> Correct, I just thought it pertinent to call this out explicitly since
> I'm stealing the top byte, but the node value has traditionally been a
> full integer.
>
> This may be relevant should anyone try to carry, a random node value
> into this field. For example, if someone tried to copy policy->home_node
> into cur_il_weight for whatever reason.
>
> It's worth breaking out a function to defend against this - plus to hide
> the bit operations directly as you recommend below.
>
>> > /* Weighted interleave settings */
>> > - u8 cur_il_weight;
>> > + atomic_t cur_il_weight;
>>
>> If we use this field for node and weight, why not change the field name?
>> For example, cur_wil_node_weight.
>>
>
> ack.
>
>> > + if (cweight & 0xFF)
>> > + *policy = cweight >> 8;
>>
>> Please define some helper functions or macros instead of operate on bits
>> directly.
>>
>
> ack.
>
>> > else
>> > *policy = next_node_in(current->il_prev,
>> > pol->nodes);
>>
>> If we record current node in pol->cur_il_weight, why do we still need
>> curren->il_prev. Can we only use pol->cur_il_weight? And if so, we can
>> even make current->il_prev a union.
>>
>
> I just realized that there's a problem here for shared memory policies.
>
> from weighted_interleave_nodes, I do this:
>
> cur_weight = atomic_read(&policy->cur_il_weight);
> ...
> weight--;
> ...
> atomic_set(&policy->cur_il_weight, cur_weight);
>
> On a shared memory policy, this is a race condition.
>
>
> I don't think we can combine il_prev and cur_wil_node_weight because
> the task policy may be different than the current policy.
>
> i.e. it's totally valid to do the following:
>
> 1) set_mempolicy(MPOL_INTERLEAVE)
> 2) mbind(..., MPOL_WEIGHTED_INTERLEAVE)
>
> Using current->il_prev between these two policies, is just plain incorrect,
> so I will need to rethink this, and the existing code will need to be
> updated such that weighted_interleave does not use current->il_prev.
IIUC, weighted_interleave_nodes() is only used for mempolicy of tasks
(set_mempolicy()), as in the following code.
+ *nid = (ilx == NO_INTERLEAVE_INDEX) ?
+ weighted_interleave_nodes(pol) :
+ weighted_interleave_nid(pol, ilx);
But, in contrast, it's bad to put task-local "current weight" in
mempolicy. So, I think that it's better to move cur_il_weight to
task_struct. And maybe combine it with current->il_prev.
--
Best Regards,
Huang, Ying
^ 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