* Re: [PATCH RFT v3 1/5] mm: Introduce ARCH_HAS_USER_SHADOW_STACK
From: Mike Rapoport @ 2024-02-04 11:56 UTC (permalink / raw)
To: Mark Brown
Cc: Rick P. Edgecombe, Deepak Gupta, Szabolcs Nagy, H.J. Lu,
Florian Weimer, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Peter Zijlstra, Juri Lelli,
Vincent Guittot, Dietmar Eggemann, Steven Rostedt, Ben Segall,
Mel Gorman, Daniel Bristot de Oliveira, Valentin Schneider,
Christian Brauner, Shuah Khan, linux-kernel, Catalin Marinas,
Will Deacon, Kees Cook, jannh, linux-kselftest, linux-api,
David Hildenbrand
In-Reply-To: <20231120-clone3-shadow-stack-v3-1-a7b8ed3e2acc@kernel.org>
On Mon, Nov 20, 2023 at 11:54:29PM +0000, Mark Brown wrote:
> Since multiple architectures have support for shadow stacks and we need to
> select support for this feature in several places in the generic code
> provide a generic config option that the architectures can select.
>
> Suggested-by: David Hildenbrand <david@redhat.com>
> Acked-by: David Hildenbrand <david@redhat.com>
> Signed-off-by: Mark Brown <broonie@kernel.org>
> ---
> arch/x86/Kconfig | 1 +
> fs/proc/task_mmu.c | 2 +-
> include/linux/mm.h | 2 +-
> mm/Kconfig | 6 ++++++
> 4 files changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index 3762f41bb092..14b7703a9a2b 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -1952,6 +1952,7 @@ config X86_USER_SHADOW_STACK
> depends on AS_WRUSS
> depends on X86_64
> select ARCH_USES_HIGH_VMA_FLAGS
> + select ARCH_HAS_USER_SHADOW_STACK
> select X86_CET
> help
> Shadow stack protection is a hardware feature that detects function
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index ef2eb12906da..f0a904aeee8e 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -699,7 +699,7 @@ static void show_smap_vma_flags(struct seq_file *m, struct vm_area_struct *vma)
> #ifdef CONFIG_HAVE_ARCH_USERFAULTFD_MINOR
> [ilog2(VM_UFFD_MINOR)] = "ui",
> #endif /* CONFIG_HAVE_ARCH_USERFAULTFD_MINOR */
> -#ifdef CONFIG_X86_USER_SHADOW_STACK
> +#ifdef CONFIG_ARCH_HAS_USER_SHADOW_STACK
> [ilog2(VM_SHADOW_STACK)] = "ss",
> #endif
> };
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 418d26608ece..10462f354614 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -341,7 +341,7 @@ extern unsigned int kobjsize(const void *objp);
> #endif
> #endif /* CONFIG_ARCH_HAS_PKEYS */
>
> -#ifdef CONFIG_X86_USER_SHADOW_STACK
> +#ifdef CONFIG_ARCH_HAS_USER_SHADOW_STACK
> /*
> * VM_SHADOW_STACK should not be set with VM_SHARED because of lack of
> * support core mm.
> diff --git a/mm/Kconfig b/mm/Kconfig
> index 89971a894b60..6713bb3b0b48 100644
> --- a/mm/Kconfig
> +++ b/mm/Kconfig
> @@ -1270,6 +1270,12 @@ config LOCK_MM_AND_FIND_VMA
> bool
> depends on !STACK_GROWSUP
>
> +config ARCH_HAS_USER_SHADOW_STACK
> + bool
> + help
> + The architecture has hardware support for userspace shadow call
> + stacks (eg, x86 CET, arm64 GCS or RISC-V Zicfiss).
The whitespace looks suspicious, I think there should be a leading tab.
Otherwise
Reviewed-by: Mike Rapoport (IBM) <rppt@kernel.org>
> +
> source "mm/damon/Kconfig"
>
> endmenu
>
> --
> 2.30.2
>
--
Sincerely yours,
Mike.
^ permalink raw reply
* Re: [PATCH v5 4/4] mm/mempolicy: protect task interleave functions with tsk->mems_allowed_seq
From: Huang, Ying @ 2024-02-05 5:48 UTC (permalink / raw)
To: Gregory Price
Cc: linux-mm, linux-kernel, linux-doc, linux-fsdevel, linux-api,
corbet, akpm, gregory.price, honggyu.kim, rakie.kim, hyeongtak.ji,
mhocko, vtavarespetr, jgroves, ravis.opensrc, sthanneeru,
emirakhur, Hasan.Maruf, seungjun.ha, hannes, dan.j.williams
In-Reply-To: <20240202170238.90004-5-gregory.price@memverge.com>
Gregory Price <gourry.memverge@gmail.com> writes:
> In the event of rebind, pol->nodemask can change at the same time as an
> allocation occurs. We can detect this with tsk->mems_allowed_seq and
> prevent a miscount or an allocation failure from occurring.
>
> The same thing happens in the allocators to detect failure, but this
> can prevent spurious failures in a much smaller critical section.
>
> Suggested-by: "Huang, Ying" <ying.huang@intel.com>
> Signed-off-by: Gregory Price <gregory.price@memverge.com>
> ---
> mm/mempolicy.c | 31 +++++++++++++++++++++++++------
> 1 file changed, 25 insertions(+), 6 deletions(-)
>
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index d8cc3a577986..ed0d5d2d456a 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
> @@ -1878,11 +1878,17 @@ bool apply_policy_zone(struct mempolicy *policy, enum zone_type 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)) {
> + unsigned int node;
> + unsigned int cpuset_mems_cookie;
> +
> +retry:
> + /* to prevent miscount use tsk->mems_allowed_seq to detect rebind */
> + cpuset_mems_cookie = read_mems_allowed_begin();
> + node = current->il_prev;
> + if (!node || !node_isset(node, policy->nodes)) {
~~~~~
!current->il_weight ?
--
Best Regards,
Huang, Ying
> node = next_node_in(node, policy->nodes);
> - /* can only happen if nodemask is being rebound */
> + if (read_mems_allowed_retry(cpuset_mems_cookie))
> + goto retry;
> if (node == MAX_NUMNODES)
> return node;
> current->il_prev = node;
> @@ -1896,8 +1902,14 @@ static unsigned int weighted_interleave_nodes(struct mempolicy *policy)
> static unsigned int interleave_nodes(struct mempolicy *policy)
> {
> unsigned int nid;
> + unsigned int cpuset_mems_cookie;
> +
> + /* to prevent miscount, use tsk->mems_allowed_seq to detect rebind */
> + do {
> + cpuset_mems_cookie = read_mems_allowed_begin();
> + nid = next_node_in(current->il_prev, policy->nodes);
> + } while (read_mems_allowed_retry(cpuset_mems_cookie));
>
> - nid = next_node_in(current->il_prev, policy->nodes);
> if (nid < MAX_NUMNODES)
> current->il_prev = nid;
> return nid;
> @@ -2374,6 +2386,7 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
> struct page **page_array)
> {
> struct task_struct *me = current;
> + unsigned int cpuset_mems_cookie;
> unsigned long total_allocated = 0;
> unsigned long nr_allocated = 0;
> unsigned long rounds;
> @@ -2391,7 +2404,13 @@ static unsigned long alloc_pages_bulk_array_weighted_interleave(gfp_t gfp,
> if (!nr_pages)
> return 0;
>
> - nnodes = read_once_policy_nodemask(pol, &nodes);
> + /* read the nodes onto the stack, retry if done during rebind */
> + do {
> + cpuset_mems_cookie = read_mems_allowed_begin();
> + nnodes = read_once_policy_nodemask(pol, &nodes);
> + } while (read_mems_allowed_retry(cpuset_mems_cookie));
> +
> + /* if the nodemask has become invalid, we cannot do anything */
> if (!nnodes)
> return 0;
^ permalink raw reply
* [PATCH v4] ELF: AT_PAGE_SHIFT_MASK -- supply userspace with available page shifts
From: Alexey Dobriyan @ 2024-02-05 9:51 UTC (permalink / raw)
To: Kees Cook, Andrew Morton
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
Florian Weimer, linux-kernel, linux-arch, linux-api, x86,
Eric Biederman, linux-mm
Report available page shifts in arch independent manner, so that
userspace developers won't have to parse /proc/cpuinfo hunting
for arch specific strings.
Main users are supposed to be libhugetlbfs-like libraries which try
to abstract huge mappings across multiple architectures. Regular code
which queries hugepage support before using them benefits too because
it doesn't have to deal with descriptors and parsing sysfs hierarchies
while enjoying the simplicity and speed of getauxval(3).
Note 1!
This is strictly for userspace, if some page size is shutdown due
to kernel command line option or CPU bug workaround, than it must
not be reported in aux vector!
Note 2!
getauxval(AT_PAGE_SHIFT_MASK) output is a function of CPU capabilities
only, it is not changed by system memory size, hugepage availability at
any given moment, hugetlbfs being mounted, etc.
Example output:
x86_64 machine with 1 GiB pages:
00000030 06 00 00 00 00 00 00 00 00 10 00 00 00 00 00 00
00000040 1d 00 00 00 00 00 00 00 00 10 20 40 00 00 00 00
x86_64 machine with 2 MiB pages only:
00000030 06 00 00 00 00 00 00 00 00 10 00 00 00 00 00 00
00000040 1d 00 00 00 00 00 00 00 00 10 20 00 00 00 00 00
AT_PAGESZ always reports one smallest page size which is not interesting.
Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---
changes since v3 -- even better changelog
arch/x86/include/asm/elf.h | 12 ++++++++++++
fs/binfmt_elf.c | 3 +++
include/uapi/linux/auxvec.h | 13 +++++++++++++
3 files changed, 28 insertions(+)
--- a/arch/x86/include/asm/elf.h
+++ b/arch/x86/include/asm/elf.h
@@ -358,6 +358,18 @@ else if (IS_ENABLED(CONFIG_IA32_EMULATION)) \
#define COMPAT_ELF_ET_DYN_BASE (TASK_UNMAPPED_BASE + 0x1000000)
+#define ARCH_AT_PAGE_SHIFT_MASK \
+ do { \
+ u32 val = 1 << 12; \
+ if (boot_cpu_has(X86_FEATURE_PSE)) { \
+ val |= 1 << 21; \
+ } \
+ if (boot_cpu_has(X86_FEATURE_GBPAGES)) { \
+ val |= 1 << 30; \
+ } \
+ NEW_AUX_ENT(AT_PAGE_SHIFT_MASK, val); \
+ } while (0)
+
#endif /* !CONFIG_X86_32 */
#define VDSO_CURRENT_BASE ((unsigned long)current->mm->context.vdso)
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -240,6 +240,9 @@ create_elf_tables(struct linux_binprm *bprm, const struct elfhdr *exec,
#endif
NEW_AUX_ENT(AT_HWCAP, ELF_HWCAP);
NEW_AUX_ENT(AT_PAGESZ, ELF_EXEC_PAGESIZE);
+#ifdef ARCH_AT_PAGE_SHIFT_MASK
+ ARCH_AT_PAGE_SHIFT_MASK;
+#endif
NEW_AUX_ENT(AT_CLKTCK, CLOCKS_PER_SEC);
NEW_AUX_ENT(AT_PHDR, phdr_addr);
NEW_AUX_ENT(AT_PHENT, sizeof(struct elf_phdr));
--- a/include/uapi/linux/auxvec.h
+++ b/include/uapi/linux/auxvec.h
@@ -33,6 +33,19 @@
#define AT_RSEQ_FEATURE_SIZE 27 /* rseq supported feature size */
#define AT_RSEQ_ALIGN 28 /* rseq allocation alignment */
+/*
+ * All page sizes supported by CPU encoded as bitmask.
+ *
+ * Example: x86_64 system with pse, pdpe1gb /proc/cpuinfo flags
+ * reports 4 KiB, 2 MiB and 1 GiB page support.
+ *
+ * $ LD_SHOW_AUXV=1 $(which true) | grep -e AT_PAGE_SHIFT_MASK
+ * AT_PAGE_SHIFT_MASK: 0x40201000
+ *
+ * For 2^64 hugepage support please contact your Universe sales representative.
+ */
+#define AT_PAGE_SHIFT_MASK 29
+
#define AT_EXECFN 31 /* filename of program */
#ifndef AT_MINSIGSTKSZ
^ permalink raw reply
* Re: [PATCH v4] ELF: AT_PAGE_SHIFT_MASK -- supply userspace with available page shifts
From: Kees Cook @ 2024-02-05 12:48 UTC (permalink / raw)
To: Alexey Dobriyan
Cc: Andrew Morton, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, Florian Weimer, linux-kernel, linux-arch, linux-api,
x86, Eric Biederman, linux-mm
In-Reply-To: <ecb049aa-bcac-45c7-bbb1-4612d094935a@p183>
On Mon, Feb 05, 2024 at 12:51:43PM +0300, Alexey Dobriyan wrote:
> +#define ARCH_AT_PAGE_SHIFT_MASK \
> + do { \
> + u32 val = 1 << 12; \
> + if (boot_cpu_has(X86_FEATURE_PSE)) { \
> + val |= 1 << 21; \
> + } \
> + if (boot_cpu_has(X86_FEATURE_GBPAGES)) { \
> + val |= 1 << 30; \
> + } \
Can we use something besides literal "12", "21", and "30" values here?
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH 1/3] pidfd_poll: report POLLHUP when pid_task() == NULL
From: Oleg Nesterov @ 2024-02-05 14:08 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
In-Reply-To: <20240203-freuden-frucht-b598f8cca27d@brauner>
On 02/03, Christian Brauner wrote:
>
> On Sat, Feb 03, 2024 at 01:04:26PM +0100, Oleg Nesterov wrote:
> >
> > - wake_up_all(&pid->wait_pidfd);
> > + __wake_up(&pid->wait_pidfd, TASK_NORMAL, 0,
> > + poll_to_key(EPOLLIN | EPOLLRDNORM));
>
>
> Ok, care to just send me a full patch for this?
Will do in a minute,
> Second, I agree that this looks ugly. ;/
Agreed ;)
> So here's a very likely a stupid idea. To make that clean we essentially
> need kernel private information that can't be set in userspace (Btw,
> look at EPOLL_URING_WAKE which is similar in that it cannot be set from
> userspace. It's not the same thing ofc but it is a private bit.). Which
> is the gist of your proposal in a way.
>
> So we would have to grab a new private bit in the epoll flag space.
Agreed, but just in case:
- EPOLLMSG (ab)used by this patch can't "leak" to userspace even
if it was (erroneously) set in pollfd.events
- If EPOLLMSG was set by userspace nothing bad can happen, just
poll(non-PIDFD_THREAD-pidfd) will get the spurious wakeups.
So. I am attaching the patch for the record, in case we return to this
later.
It seems to work fine, but when I look into fs/eventpoll.c I suspect
it is not epoll friendly. I _think_ that the neccessary fix is trivial,
ep_item_poll() should just copy pt->_key to epi->event.events after
vfs_poll(), but I am not sure. So lets forget it for now.
Oleg.
---
diff --git a/include/linux/pid.h b/include/linux/pid.h
index 8124d57752b9..7467cdb9735b 100644
--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -74,7 +74,7 @@ struct pid *pidfd_pid(const struct file *file);
struct pid *pidfd_get_pid(unsigned int fd, unsigned int *flags);
struct task_struct *pidfd_get_task(int pidfd, unsigned int *flags);
int pidfd_prepare(struct pid *pid, unsigned int flags, struct file **ret);
-void do_notify_pidfd(struct task_struct *task);
+void do_notify_pidfd(struct task_struct *task, bool thread);
static inline struct pid *get_pid(struct pid *pid)
{
diff --git a/kernel/exit.c b/kernel/exit.c
index 493647fd7c07..c31f36d3a1ed 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -744,7 +744,7 @@ static void exit_notify(struct task_struct *tsk, int group_dead)
* PIDFD_THREAD waiters.
*/
if (!thread_group_empty(tsk))
- do_notify_pidfd(tsk);
+ do_notify_pidfd(tsk, true);
if (unlikely(tsk->ptrace)) {
int sig = thread_group_leader(tsk) &&
diff --git a/kernel/fork.c b/kernel/fork.c
index 8d08a2d1b095..3b4474ff6f4a 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2081,6 +2081,15 @@ static __poll_t pidfd_poll(struct file *file, struct poll_table_struct *pts)
struct task_struct *task;
__poll_t poll_flags = 0;
+ if (pts && pts->_qproc) {
+ /*
+ * We are not registered yet. Update pts->_key to mark us as
+ * a non POLLHUP-only PIDFD_THREAD waiter for do_notify_pidfd,
+ * _pollwait() will copy this _key to poll_table_entry->key.
+ */
+ if (thread && (pts->_key & (EPOLLIN | EPOLLRDNORM)))
+ pts->_key |= EPOLLMSG;
+ }
poll_wait(file, &pid->wait_pidfd, pts);
/*
* Depending on PIDFD_THREAD, inform pollers when the thread
diff --git a/kernel/signal.c b/kernel/signal.c
index c3fac06937e2..f51070dec132 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2019,14 +2019,15 @@ int send_sigqueue(struct sigqueue *q, struct pid *pid, enum pid_type type)
return ret;
}
-void do_notify_pidfd(struct task_struct *task)
+void do_notify_pidfd(struct task_struct *task, bool thread)
{
+ /* see the usage of EPOLLMSG in pidfd_poll() */
+ __poll_t m = thread ? EPOLLMSG : EPOLLIN | EPOLLRDNORM;
struct pid *pid = task_pid(task);
WARN_ON(task->exit_state == 0);
- __wake_up(&pid->wait_pidfd, TASK_NORMAL, 0,
- poll_to_key(EPOLLIN | EPOLLRDNORM));
+ __wake_up(&pid->wait_pidfd, TASK_NORMAL, 0, poll_to_key(m));
}
/*
@@ -2056,7 +2057,7 @@ bool do_notify_parent(struct task_struct *tsk, int sig)
* non-PIDFD_THREAD waiters.
*/
if (thread_group_empty(tsk))
- do_notify_pidfd(tsk);
+ do_notify_pidfd(tsk, false);
if (sig != SIGCHLD) {
/*
^ permalink raw reply related
* [PATCH] pidfd: clone: allow CLONE_THREAD | CLONE_PIDFD together
From: Oleg Nesterov @ 2024-02-05 14:55 UTC (permalink / raw)
To: Christian Brauner
Cc: Andy Lutomirski, Eric W. Biederman, Tycho Andersen, linux-api,
linux-kernel
copy_process() just needs to pass PIDFD_THREAD to __pidfd_prepare()
if clone_flags & CLONE_THREAD.
We can also add another CLONE_ flag (or perhaps reuse CLONE_DETACHED)
to enforce PIDFD_THREAD without CLONE_THREAD.
Originally-from: Tycho Andersen <tycho@tycho.pizza>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
kernel/fork.c | 7 ++++---
1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/kernel/fork.c b/kernel/fork.c
index 8d08a2d1b095..cd61ca87d0e6 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2311,9 +2311,8 @@ __latent_entropy struct task_struct *copy_process(
/*
* - CLONE_DETACHED is blocked so that we can potentially
* reuse it later for CLONE_PIDFD.
- * - CLONE_THREAD is blocked until someone really needs it.
*/
- if (clone_flags & (CLONE_DETACHED | CLONE_THREAD))
+ if (clone_flags & CLONE_DETACHED)
return ERR_PTR(-EINVAL);
}
@@ -2536,8 +2535,10 @@ __latent_entropy struct task_struct *copy_process(
* if the fd table isn't shared).
*/
if (clone_flags & CLONE_PIDFD) {
+ int flags = (clone_flags & CLONE_THREAD) ? PIDFD_THREAD : 0;
+
/* Note that no task has been attached to @pid yet. */
- retval = __pidfd_prepare(pid, 0, &pidfile);
+ retval = __pidfd_prepare(pid, flags, &pidfile);
if (retval < 0)
goto bad_fork_free_pid;
pidfd = retval;
--
2.25.1.362.g51ebf55
^ permalink raw reply related
* Re: [net-next 0/3] Per epoll context busy poll support
From: Stanislav Fomichev @ 2024-02-05 18:17 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Joe Damato, sridhar.samudrala, Eric Dumazet, netdev, linux-kernel,
chuck.lever, jlayton, linux-api, brauner, davem, alexander.duyck,
Wei Wang, Amritha Nambiar
In-Reply-To: <20240202171539.7347cb01@kernel.org>
On 02/02, Jakub Kicinski wrote:
> On Fri, 2 Feb 2024 12:23:44 -0800 Joe Damato wrote:
> > > Did you see SO_PREFER_BUSY_POLL by any chance? (In combination with
> > > gro_flush_timeout IIRC). We added it a while back with Bjorn, it seems
> > > like a great idea to me at the time but I'm unclear if anyone uses it
> > > in production..
> >
> > I have seen it while reading the code, yes. I think maybe I missed
> > something about its interaction with gro_flush_timeout. In my use case,
> > the machine has no traffic until after the app is started.
> >
> > In this case, I haven't needed to worry about regular NAPI monopolizing the
> > CPU and preventing busy poll from working.
> >
> > Maybe I am missing something more nuanced, though? I'll have another look
> > at the code, just incase.
>
> We reused the gro_flush_timeout as an existing "user doesn't care if
> packets get delayed by this much in worst case" value. If you set
> SO_PREFER_BUSY_POLL the next time you busy pool the NAPI will be marked
> as "already scheduled" and a timer is set (to gro_flush_timeout).
> If NIC IRQ fires before gro_flush_timeout it gets ignored, because NAPI
> is already marked as scheduled.
> If you busy poll again the timer gets postponed for another
> gro_flush_timeout nsec.
> If timer fires we go back to normal NAPI processing.
>
> The idea is that you set gro_flush_timeout to some high value, like
> 10 msec, and expect your app to poll more often than every 10 msec.
>
> Then the normal NAPI processing will never kick in, and there will
> be only 1 NIC IRQ after which the HW IRQ remains masked.
> With high coalescing timer you technically still get an IRQ every
> so often and interrupt the app. Worst case (UDP flood) you may even
> get into an overload where the app gets starved out completely..
Should we also add prefer_busy_poll parameter to EPIOCSPARAMS?
napi_busy_loop in ep_busy_loop has its prefer_busy_poll argument
hard-coded as false.
^ permalink raw reply
* Re: [PATCH net-next v5 1/3] eventpoll: support busy poll per epoll instance
From: Joe Damato @ 2024-02-05 18:25 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, Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240131180811.23566-2-jdamato@fastly.com>
On Wed, Jan 31, 2024 at 06:08:03PM +0000, Joe Damato wrote:
> Allow busy polling on a per-epoll context basis. The per-epoll context
> usec timeout value is preferred, but the pre-existing system wide sysctl
> value is still supported if it specified.
>
> 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.
I just realized that I updated the code below to use a an or (||) instead
of xor (^), but forgot to update the commit message.
I can fix this and send a v6.
>
> 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
* Re: [net-next 0/3] Per epoll context busy poll support
From: Joe Damato @ 2024-02-05 18:51 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Samudrala, Sridhar, Eric Dumazet, netdev, linux-kernel,
chuck.lever, jlayton, linux-api, brauner, davem, alexander.duyck,
Wei Wang, Amritha Nambiar
In-Reply-To: <20240202171539.7347cb01@kernel.org>
On Fri, Feb 02, 2024 at 05:15:39PM -0800, Jakub Kicinski wrote:
> On Fri, 2 Feb 2024 12:23:44 -0800 Joe Damato wrote:
> > > Did you see SO_PREFER_BUSY_POLL by any chance? (In combination with
> > > gro_flush_timeout IIRC). We added it a while back with Bjorn, it seems
> > > like a great idea to me at the time but I'm unclear if anyone uses it
> > > in production..
> >
> > I have seen it while reading the code, yes. I think maybe I missed
> > something about its interaction with gro_flush_timeout. In my use case,
> > the machine has no traffic until after the app is started.
> >
> > In this case, I haven't needed to worry about regular NAPI monopolizing the
> > CPU and preventing busy poll from working.
> >
> > Maybe I am missing something more nuanced, though? I'll have another look
> > at the code, just incase.
>
> We reused the gro_flush_timeout as an existing "user doesn't care if
> packets get delayed by this much in worst case" value. If you set
> SO_PREFER_BUSY_POLL the next time you busy pool the NAPI will be marked
> as "already scheduled" and a timer is set (to gro_flush_timeout).
> If NIC IRQ fires before gro_flush_timeout it gets ignored, because NAPI
> is already marked as scheduled.
> If you busy poll again the timer gets postponed for another
> gro_flush_timeout nsec.
> If timer fires we go back to normal NAPI processing.
Ah, I see. From my reading of the code in busy_poll_stop (which could be
wrong), defer_hard_irqs_count must also be non-zero to postpone the timer.
Is that right?
If so, I think the tricky thing with this is that these settings are
system-wide, so they'd affect non-busy poll apps, too.
I think in the ideal case being able to set these on a per-NAPI basis would
be very helpful. Maybe something for me to try working on next.
> The idea is that you set gro_flush_timeout to some high value, like
> 10 msec, and expect your app to poll more often than every 10 msec.
Yea, that makes sense.
> Then the normal NAPI processing will never kick in, and there will
> be only 1 NIC IRQ after which the HW IRQ remains masked.
> With high coalescing timer you technically still get an IRQ every
> so often and interrupt the app. Worst case (UDP flood) you may even
> get into an overload where the app gets starved out completely..
Yup, this is true. I had been using a modified version of a patch from a
research paper to avoid enabling NIC IRQs [1][2], but I think making
defer_hard_irqs_count and gro_flush_timeout per NAPI parameters would make
more sense.
[1]: https://gitlab.uwaterloo.ca/p5cai/netstack-exp/-/raw/master/kernel-polling-5.15.79-base.patch?ref_type=heads
[2]: https://dl.acm.org/doi/pdf/10.1145/3626780
^ permalink raw reply
* Re: [net-next 0/3] Per epoll context busy poll support
From: Joe Damato @ 2024-02-05 18:52 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Jakub Kicinski, sridhar.samudrala, Eric Dumazet, netdev,
linux-kernel, chuck.lever, jlayton, linux-api, brauner, davem,
alexander.duyck, Wei Wang, Amritha Nambiar
In-Reply-To: <ZcEmDz3UvAksHFFG@google.com>
On Mon, Feb 05, 2024 at 10:17:03AM -0800, Stanislav Fomichev wrote:
> On 02/02, Jakub Kicinski wrote:
> > On Fri, 2 Feb 2024 12:23:44 -0800 Joe Damato wrote:
> > > > Did you see SO_PREFER_BUSY_POLL by any chance? (In combination with
> > > > gro_flush_timeout IIRC). We added it a while back with Bjorn, it seems
> > > > like a great idea to me at the time but I'm unclear if anyone uses it
> > > > in production..
> > >
> > > I have seen it while reading the code, yes. I think maybe I missed
> > > something about its interaction with gro_flush_timeout. In my use case,
> > > the machine has no traffic until after the app is started.
> > >
> > > In this case, I haven't needed to worry about regular NAPI monopolizing the
> > > CPU and preventing busy poll from working.
> > >
> > > Maybe I am missing something more nuanced, though? I'll have another look
> > > at the code, just incase.
> >
> > We reused the gro_flush_timeout as an existing "user doesn't care if
> > packets get delayed by this much in worst case" value. If you set
> > SO_PREFER_BUSY_POLL the next time you busy pool the NAPI will be marked
> > as "already scheduled" and a timer is set (to gro_flush_timeout).
> > If NIC IRQ fires before gro_flush_timeout it gets ignored, because NAPI
> > is already marked as scheduled.
> > If you busy poll again the timer gets postponed for another
> > gro_flush_timeout nsec.
> > If timer fires we go back to normal NAPI processing.
> >
> > The idea is that you set gro_flush_timeout to some high value, like
> > 10 msec, and expect your app to poll more often than every 10 msec.
> >
> > Then the normal NAPI processing will never kick in, and there will
> > be only 1 NIC IRQ after which the HW IRQ remains masked.
> > With high coalescing timer you technically still get an IRQ every
> > so often and interrupt the app. Worst case (UDP flood) you may even
> > get into an overload where the app gets starved out completely..
>
> Should we also add prefer_busy_poll parameter to EPIOCSPARAMS?
> napi_busy_loop in ep_busy_loop has its prefer_busy_poll argument
> hard-coded as false.
I think making this configurable is a good idea. I can add that in the v6
in addition to fixing the incorrect commit message in patch 1/3.
What do you think, Jakub?
^ permalink raw reply
* Re: [net-next 0/3] Per epoll context busy poll support
From: Jakub Kicinski @ 2024-02-05 19:48 UTC (permalink / raw)
To: Joe Damato
Cc: Stanislav Fomichev, sridhar.samudrala, Eric Dumazet, netdev,
linux-kernel, chuck.lever, jlayton, linux-api, brauner, davem,
alexander.duyck, Wei Wang, Amritha Nambiar
In-Reply-To: <20240205185217.GC10463@fastly.com>
On Mon, 5 Feb 2024 10:52:18 -0800 Joe Damato wrote:
> > Should we also add prefer_busy_poll parameter to EPIOCSPARAMS?
> > napi_busy_loop in ep_busy_loop has its prefer_busy_poll argument
> > hard-coded as false.
Good catch! We only plumbed it thru to socket read busy poll.
> I think making this configurable is a good idea. I can add that in the v6
> in addition to fixing the incorrect commit message in patch 1/3.
>
> What do you think, Jakub?
SGTM (with the caveat that I'm not epoll expert)
^ permalink raw reply
* Re: [net-next 0/3] Per epoll context busy poll support
From: Jakub Kicinski @ 2024-02-05 19:59 UTC (permalink / raw)
To: Joe Damato
Cc: Samudrala, Sridhar, Eric Dumazet, netdev, linux-kernel,
chuck.lever, jlayton, linux-api, brauner, davem, alexander.duyck,
Wei Wang, Amritha Nambiar
In-Reply-To: <20240205185130.GB10463@fastly.com>
On Mon, 5 Feb 2024 10:51:30 -0800 Joe Damato wrote:
> > We reused the gro_flush_timeout as an existing "user doesn't care if
> > packets get delayed by this much in worst case" value. If you set
> > SO_PREFER_BUSY_POLL the next time you busy pool the NAPI will be marked
> > as "already scheduled" and a timer is set (to gro_flush_timeout).
> > If NIC IRQ fires before gro_flush_timeout it gets ignored, because NAPI
> > is already marked as scheduled.
> > If you busy poll again the timer gets postponed for another
> > gro_flush_timeout nsec.
> > If timer fires we go back to normal NAPI processing.
>
> Ah, I see. From my reading of the code in busy_poll_stop (which could be
> wrong), defer_hard_irqs_count must also be non-zero to postpone the timer.
>
> Is that right?
>
> If so, I think the tricky thing with this is that these settings are
> system-wide, so they'd affect non-busy poll apps, too.
>
> I think in the ideal case being able to set these on a per-NAPI basis would
> be very helpful. Maybe something for me to try working on next.
If wonder if it'd be good enough to do:
min(defer_hard_irqs_count, 1)
there. If caller asked to prefer busy poll they clearly want to poll.
An explicit per-NAPI API works too, but it's a bit more work.
If I was doing the work I'd try min(..., 1) with the workload.
If there's value in having the full config - go for it.
^ permalink raw reply
* [PATCH net-next v6 0/4] Per epoll context busy poll support
From: Joe Damato @ 2024-02-05 21:04 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, sdf, amritha.nambiar, 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), Maik Broemme,
Michael Ellerman, Namjae Jeon, Nathan Lynch, Palmer Dabbelt,
Steve French, Thomas Huth, Thomas Zimmermann
Greetings:
Welcome to v6.
TL;DR This builds on commit bf3b9f6372c4 ("epoll: Add busy poll support to
epoll with socket fds.") by allowing user applications to enable
epoll-based busy polling, set a busy poll packet budget, and enable or
disable prefer busy poll on a per epoll context basis.
This makes epoll-based busy polling much more usable for user
applications than the current system-wide sysctl and hardcoded budget.
To allow for this, two ioctls have been added for epoll contexts for
getting and setting a new struct, struct epoll_params.
ioctl was chosen vs a new syscall after reviewing a suggestion by Willem
de Bruijn [1]. I am open to using a new syscall instead of an ioctl, but it
seemed that:
- Busy poll affects all existing epoll_wait and epoll_pwait variants in
the same way, so new verions of many syscalls might be needed. It
seems much simpler for users to use the correct
epoll_wait/epoll_pwait for their app and add a call to ioctl to enable
or disable busy poll as needed. This also probably means less work to
get an existing epoll app using busy poll.
- previously added epoll_pwait2 helped to bring epoll closer to
existing syscalls (like pselect and ppoll) and this busy poll change
reflected as a new syscall would not have the same effect.
Note: patch 1/4 as of v4 uses an or (||) instead of an xor. I thought about
it some more and I realized that if the user enables both the per-epoll
context setting and the system wide sysctl, then busy poll should be
enabled and not disabled. Using xor doesn't seem to make much sense after
thinking through this a bit.
Longer explanation:
Presently epoll has support for a very useful form of busy poll based on
the incoming NAPI ID (see also: SO_INCOMING_NAPI_ID [2]).
This form of busy poll allows epoll_wait to drive NAPI packet processing
which allows for a few interesting user application designs which can
reduce latency and also potentially improve L2/L3 cache hit rates by
deferring NAPI until userland has finished its work.
The documentation available on this is, IMHO, a bit confusing so please
allow me to explain how one might use this:
1. Ensure each application thread has its own epoll instance mapping
1-to-1 with NIC RX queues. An n-tuple filter would likely be used to
direct connections with specific dest ports to these queues.
2. Optionally: Setup IRQ coalescing for the NIC RX queues where busy
polling will occur. This can help avoid the userland app from being
pre-empted by a hard IRQ while userland is running. Note this means that
userland must take care to call epoll_wait and not take too long in
userland since it now drives NAPI via epoll_wait.
3. Optionally: Consider using napi_defer_hard_irqs and gro_flush_timeout to
further restrict IRQ generation from the NIC. These settings are
system-wide so their impact must be carefully weighed against the running
applications.
4. Ensure that all incoming connections added to an epoll instance
have the same NAPI ID. This can be done with a BPF filter when
SO_REUSEPORT is used or getsockopt + SO_INCOMING_NAPI_ID when a single
accept thread is used which dispatches incoming connections to threads.
5. Lastly, busy poll must be enabled via a sysctl
(/proc/sys/net/core/busy_poll).
Please see Eric Dumazet's paper about busy polling [3] and a recent
academic paper about measured performance improvements of busy polling [4]
(albeit with a modification that is not currently present in the kernel)
for additional context.
The unfortunate part about step 5 above is that this enables busy poll
system-wide which affects all user applications on the system,
including epoll-based network applications which were not intended to
be used this way or applications where increased CPU usage for lower
latency network processing is unnecessary or not desirable.
If the user wants to run one low latency epoll-based server application
with epoll-based busy poll, but would like to run the rest of the
applications on the system (which may also use epoll) without busy poll,
this system-wide sysctl presents a significant problem.
This change preserves the system-wide sysctl, but adds a mechanism (via
ioctl) to enable or disable busy poll for epoll contexts as needed by
individual applications, making epoll-based busy poll more usable.
Note that this change includes an or (as of v4) instead of an xor. If the
user has enabled both the system-wide sysctl and also the per epoll-context
busy poll settings, then epoll should probably busy poll (vs being
disabled).
Thanks,
Joe
v5 -> v6:
- patch 1/3 no functional change, but commit message corrected to explain
that an or (||) is being used instead of xor.
- patch 3/4 is a new patch which adds support for per epoll context
prefer busy poll setting.
- patch 4/4 updated to allow getting/setting per epoll context prefer
busy poll setting; this setting is limited to either 0 or 1.
v4 -> v5:
- patch 3/3 updated to use memchr_inv to ensure that __pad is zero for
the EPIOCSPARAMS ioctl. Recommended by Greg K-H [5], Dave Chinner [6],
and Jiri Slaby [7].
v3 -> v4:
- patch 1/3 was updated to include an important functional change:
ep_busy_loop_on was updated to use or (||) instead of xor (^). After
thinking about it a bit more, I thought xor didn't make much sense.
Enabling both the per-epoll context and the system-wide sysctl should
probably enable busy poll, not disable it. So, or (||) makes more
sense, I think.
- patch 3/3 was updated:
- to change the epoll_params fields to be __u64, __u16, and __u8 and
to pad the struct to a multiple of 64bits. Suggested by Greg K-H [8]
and Arnd Bergmann [9].
- remove an unused pr_fmt, left over from the previous revision.
- ioctl now returns -EINVAL when epoll_params.busy_poll_usecs >
U32_MAX.
v2 -> v3:
- cover letter updated to mention why ioctl seems (to me) like a better
choice vs a new syscall.
- patch 3/4 was modified in 3 ways:
- when an unknown ioctl is received, -ENOIOCTLCMD is returned instead
of -EINVAL as the ioctl documentation requires.
- epoll_params.busy_poll_budget can only be set to a value larger than
NAPI_POLL_WEIGHT if code is run by privileged (CAP_NET_ADMIN) users.
Otherwise, -EPERM is returned.
- busy poll specific ioctl code moved out to its own function. On
kernels without busy poll support, -EOPNOTSUPP is returned. This also
makes the kernel build robot happier without littering the code with
more #ifdefs.
- dropped patch 4/4 after Eric Dumazet's review of it when it was sent
independently to the list [10].
v1 -> v2:
- cover letter updated to make a mention of napi_defer_hard_irqs and
gro_flush_timeout as an added step 3 and to cite both Eric Dumazet's
busy polling paper and a paper from University of Waterloo for
additional context. Specifically calling out the xor in patch 1/4
incase it is missed by reviewers.
- Patch 2/4 has its commit message updated, but no functional changes.
Commit message now describes that allowing for a settable budget helps
to improve throughput and is more consistent with other busy poll
mechanisms that allow a settable budget via SO_BUSY_POLL_BUDGET.
- Patch 3/4 was modified to check if the epoll_params.busy_poll_budget
exceeds NAPI_POLL_WEIGHT. The larger value is allowed, but an error is
printed. This was done for consistency with netif_napi_add_weight,
which does the same.
- Patch 3/4 the struct epoll_params was updated to fix the type of the
data field; it was uint8_t and was changed to u8.
- Patch 4/4 added to check if SO_BUSY_POLL_BUDGET exceeds
NAPI_POLL_WEIGHT. The larger value is allowed, but an error is
printed. This was done for consistency with netif_napi_add_weight,
which does the same.
[1]: https://lore.kernel.org/lkml/65b1cb7f73a6a_250560294bd@willemb.c.googlers.com.notmuch/
[2]: https://lore.kernel.org/lkml/20170324170836.15226.87178.stgit@localhost.localdomain/
[3]: https://netdevconf.info/2.1/papers/BusyPollingNextGen.pdf
[4]: https://dl.acm.org/doi/pdf/10.1145/3626780
[5]: https://lore.kernel.org/lkml/2024013001-prison-strum-899d@gregkh/
[6]: https://lore.kernel.org/lkml/Zbm3AXgcwL9D6TNM@dread.disaster.area/
[7]: https://lore.kernel.org/lkml/efee9789-4f05-4202-9a95-21d88f6307b0@kernel.org/
[8]: https://lore.kernel.org/lkml/2024012551-anyone-demeaning-867b@gregkh/
[9]: https://lore.kernel.org/lkml/57b62135-2159-493d-a6bb-47d5be55154a@app.fastmail.com/
[10]: https://lore.kernel.org/lkml/CANn89i+uXsdSVFiQT9fDfGw+h_5QOcuHwPdWi9J=5U6oLXkQTA@mail.gmail.com/
Joe Damato (4):
eventpoll: support busy poll per epoll instance
eventpoll: Add per-epoll busy poll packet budget
eventpoll: Add per-epoll prefer busy poll option
eventpoll: Add epoll ioctl for epoll_params
.../userspace-api/ioctl/ioctl-number.rst | 1 +
fs/eventpoll.c | 136 +++++++++++++++++-
include/uapi/linux/eventpoll.h | 13 ++
3 files changed, 144 insertions(+), 6 deletions(-)
--
2.25.1
^ permalink raw reply
* [PATCH net-next v6 1/4] eventpoll: support busy poll per epoll instance
From: Joe Damato @ 2024-02-05 21:04 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, sdf, amritha.nambiar, Joe Damato,
Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240205210453.11301-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.
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 v6 2/4] eventpoll: Add per-epoll busy poll packet budget
From: Joe Damato @ 2024-02-05 21:04 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, sdf, amritha.nambiar, Joe Damato,
Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240205210453.11301-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 v6 3/4] eventpoll: Add per-epoll prefer busy poll option
From: Joe Damato @ 2024-02-05 21:04 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, sdf, amritha.nambiar, Joe Damato,
Alexander Viro, Jan Kara,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240205210453.11301-1-jdamato@fastly.com>
When using epoll-based busy poll, the prefer_busy_poll option is hardcoded
to false. Users may want to enable prefer_busy_poll to be used in
conjunction with gro_flush_timeout and defer_hard_irqs_count to keep device
IRQs masked.
Other busy poll methods allow enabling or disabling prefer busy poll via
SO_PREFER_BUSY_POLL, but epoll-based busy polling uses a hardcoded value.
Fix this edge case by adding support for a per-epoll context
prefer_busy_poll option. The default is false, as it was hardcoded before
this change.
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
fs/eventpoll.c | 7 +++++--
1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 3985434df527..a69ee11682b9 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -231,6 +231,7 @@ struct eventpoll {
u64 busy_poll_usecs;
/* busy poll packet budget */
u16 busy_poll_budget;
+ bool prefer_busy_poll;
#endif
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -440,13 +441,14 @@ static bool ep_busy_loop(struct eventpoll *ep, int nonblock)
{
unsigned int napi_id = READ_ONCE(ep->napi_id);
u16 budget = READ_ONCE(ep->busy_poll_budget);
+ bool prefer_busy_poll = READ_ONCE(ep->prefer_busy_poll);
if (!budget)
budget = BUSY_POLL_BUDGET;
if ((napi_id >= MIN_NAPI_ID) && ep_busy_loop_on(ep)) {
- napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end, ep, false,
- budget);
+ napi_busy_loop(napi_id, nonblock ? NULL : ep_busy_loop_end,
+ ep, prefer_busy_poll, budget);
if (ep_events_available(ep))
return true;
/*
@@ -2105,6 +2107,7 @@ static int do_epoll_create(int flags)
#ifdef CONFIG_NET_RX_BUSY_POLL
ep->busy_poll_usecs = 0;
ep->busy_poll_budget = 0;
+ ep->prefer_busy_poll = false;
#endif
ep->file = file;
fd_install(fd, file);
--
2.25.1
^ permalink raw reply related
* [PATCH net-next v6 4/4] eventpoll: Add epoll ioctl for epoll_params
From: Joe Damato @ 2024-02-05 21:04 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, sdf, amritha.nambiar, Joe Damato,
Jonathan Corbet, Alexander Viro, Jan Kara, Nathan Lynch,
Michael Ellerman, Greg Kroah-Hartman, Namjae Jeon, Steve French,
Thomas Zimmermann, Jiri Slaby, Julien Panis, Andrew Waterman,
Palmer Dabbelt, Albert Ou, open list:DOCUMENTATION,
open list:FILESYSTEMS (VFS and infrastructure)
In-Reply-To: <20240205210453.11301-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, packet budget, and
prefer busy poll params for a specific epoll context.
Parameters are limited:
- busy_poll_usecs is limited to <= u32_max
- busy_poll_budget is limited to <= NAPI_POLL_WEIGHT by unprivileged
users (!capable(CAP_NET_ADMIN))
- prefer_busy_poll must be 0 or 1
- __pad must be 0
Signed-off-by: Joe Damato <jdamato@fastly.com>
---
.../userspace-api/ioctl/ioctl-number.rst | 1 +
fs/eventpoll.c | 73 +++++++++++++++++++
include/uapi/linux/eventpoll.h | 13 ++++
3 files changed, 87 insertions(+)
diff --git a/Documentation/userspace-api/ioctl/ioctl-number.rst b/Documentation/userspace-api/ioctl/ioctl-number.rst
index 457e16f06e04..b33918232f78 100644
--- a/Documentation/userspace-api/ioctl/ioctl-number.rst
+++ b/Documentation/userspace-api/ioctl/ioctl-number.rst
@@ -309,6 +309,7 @@ Code Seq# Include File Comments
0x89 0B-DF linux/sockios.h
0x89 E0-EF linux/sockios.h SIOCPROTOPRIVATE range
0x89 F0-FF linux/sockios.h SIOCDEVPRIVATE range
+0x8A 00-1F linux/eventpoll.h
0x8B all linux/wireless.h
0x8C 00-3F WiNRADiO driver
<http://www.winradio.com.au/>
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index a69ee11682b9..8eb4ea2557af 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -37,6 +37,7 @@
#include <linux/seq_file.h>
#include <linux/compat.h>
#include <linux/rculist.h>
+#include <linux/capability.h>
#include <net/busy_poll.h>
/*
@@ -497,6 +498,50 @@ static inline void ep_set_busy_poll_napi_id(struct epitem *epi)
ep->napi_id = napi_id;
}
+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ struct eventpoll *ep;
+ struct epoll_params epoll_params;
+ void __user *uarg = (void __user *) arg;
+
+ ep = file->private_data;
+
+ switch (cmd) {
+ case EPIOCSPARAMS:
+ if (copy_from_user(&epoll_params, uarg, sizeof(epoll_params)))
+ return -EFAULT;
+
+ if (memchr_inv(epoll_params.__pad, 0, sizeof(epoll_params.__pad)))
+ return -EINVAL;
+
+ if (epoll_params.busy_poll_usecs > U32_MAX)
+ return -EINVAL;
+
+ if (epoll_params.prefer_busy_poll > 1)
+ return -EINVAL;
+
+ if (epoll_params.busy_poll_budget > NAPI_POLL_WEIGHT &&
+ !capable(CAP_NET_ADMIN))
+ return -EPERM;
+
+ ep->busy_poll_usecs = epoll_params.busy_poll_usecs;
+ ep->busy_poll_budget = epoll_params.busy_poll_budget;
+ ep->prefer_busy_poll = !!epoll_params.prefer_busy_poll;
+ return 0;
+ case EPIOCGPARAMS:
+ memset(&epoll_params, 0, sizeof(epoll_params));
+ epoll_params.busy_poll_usecs = ep->busy_poll_usecs;
+ epoll_params.busy_poll_budget = ep->busy_poll_budget;
+ epoll_params.prefer_busy_poll = ep->prefer_busy_poll;
+ if (copy_to_user(uarg, &epoll_params, sizeof(epoll_params)))
+ return -EFAULT;
+ return 0;
+ default:
+ return -ENOIOCTLCMD;
+ }
+}
+
#else
static inline bool ep_busy_loop(struct eventpoll *ep, int nonblock)
@@ -512,6 +557,12 @@ static inline bool ep_busy_loop_on(struct eventpoll *ep)
{
return false;
}
+
+static long ep_eventpoll_bp_ioctl(struct file *file, unsigned int cmd,
+ unsigned long arg)
+{
+ return -EOPNOTSUPP;
+}
#endif /* CONFIG_NET_RX_BUSY_POLL */
/*
@@ -871,6 +922,26 @@ static void ep_clear_and_put(struct eventpoll *ep)
ep_free(ep);
}
+static long ep_eventpoll_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+ int ret;
+
+ if (!is_file_epoll(file))
+ return -EINVAL;
+
+ switch (cmd) {
+ case EPIOCSPARAMS:
+ case EPIOCGPARAMS:
+ ret = ep_eventpoll_bp_ioctl(file, cmd, arg);
+ break;
+ default:
+ ret = -EINVAL;
+ break;
+ }
+
+ return ret;
+}
+
static int ep_eventpoll_release(struct inode *inode, struct file *file)
{
struct eventpoll *ep = file->private_data;
@@ -977,6 +1048,8 @@ static const struct file_operations eventpoll_fops = {
.release = ep_eventpoll_release,
.poll = ep_eventpoll_poll,
.llseek = noop_llseek,
+ .unlocked_ioctl = ep_eventpoll_ioctl,
+ .compat_ioctl = compat_ptr_ioctl,
};
/*
diff --git a/include/uapi/linux/eventpoll.h b/include/uapi/linux/eventpoll.h
index cfbcc4cc49ac..36a002660955 100644
--- a/include/uapi/linux/eventpoll.h
+++ b/include/uapi/linux/eventpoll.h
@@ -85,4 +85,17 @@ struct epoll_event {
__u64 data;
} EPOLL_PACKED;
+struct epoll_params {
+ __aligned_u64 busy_poll_usecs;
+ __u16 busy_poll_budget;
+ __u8 prefer_busy_poll;
+
+ /* pad the struct to a multiple of 64bits for alignment on all arches */
+ __u8 __pad[5];
+};
+
+#define EPOLL_IOC_TYPE 0x8A
+#define EPIOCSPARAMS _IOW(EPOLL_IOC_TYPE, 0x01, struct epoll_params)
+#define EPIOCGPARAMS _IOR(EPOLL_IOC_TYPE, 0x02, struct epoll_params)
+
#endif /* _UAPI_LINUX_EVENTPOLL_H */
--
2.25.1
^ permalink raw reply related
* The sk_err mechanism is infuriating in userspace
From: Andy Lutomirski @ 2024-02-05 23:03 UTC (permalink / raw)
To: Network Development; +Cc: Linux API
Hi all-
I encounter this issue every couple of years, and it still seems to be
an issue, and it drives me nuts every time I see it.
I write software that uses unconnected datagram-style sockets. Errors
happen for all kinds of reasons, and my software knows it. My
software even handles the errors and moves on with its life. I use
MSG_ERRQUEUE to understand the errors. But the kernel fights back:
struct sk_buff *__skb_try_recv_datagram(struct sock *sk,
struct sk_buff_head *queue,
unsigned int flags, int *off, int *err,
struct sk_buff **last)
{
struct sk_buff *skb;
unsigned long cpu_flags;
/*
* Caller is allowed not to check sk->sk_err before skb_recv_datagram()
*/
int error = sock_error(sk);
if (error)
goto no_packet;
^^^^^^^^^^ <----- EXCUSE ME?
The kernel even fights back on the *send* path?!?
static long sock_wait_for_wmem(struct sock *sk, long timeo)
{
DEFINE_WAIT(wait);
sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
for (;;) {
if (!timeo)
break;
if (signal_pending(current))
break;
set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
...
if (READ_ONCE(sk->sk_err))
break; <-- KERNEL HATES UNCONNECTED SOCKETS!
This is IMO just broken. I realize it's legacy behavior, but it's
BROKEN legacy behavior. sk_err does not (at least for an unconnected
socket) indicate that anything is wrong with the socket. It indicates
that something is worthy of notice, and it wants to tell me.
So:
1. sock_wait_for_wmem should IMO just not do that on an unconnected
socket. AFAICS it's simply a bug.
2. How, exactly, am I supposed to call recvmsg() and, unambiguously,
find out whether recvmsg() actually failed? There are actual errors
(something that indicates that the kernel malfunctioned or the socket
is broken), errors indicating that the packet being received is busted
(skb_copy_datagram_msg, for example), and also errors indicating that
there's an error queued up.
I would like to know that there's an error queued up. That's what
poll and epoll are for, right? Or a hint from recvmsg() that I should
call MSG_RECVERR too. Or it could have a mode where it returns a
normal datagram *or* an error as appropriate. But the current state
of affairs is just brittle and racy.
Are there any reasonably implementable, non-breaking ways to improve
the API so that programs that understand socket errors can actually
function fully correctly without gnarly retry loops in userspace and
silly heuristics about what errors are actually errors?
Grumpily,
Andy
^ permalink raw reply
* Re: The sk_err mechanism is infuriating in userspace
From: Andy Lutomirski @ 2024-02-05 23:22 UTC (permalink / raw)
To: Network Development; +Cc: Linux API
In-Reply-To: <CALCETrUe23P_3YAUMT2dmqq62xAc7zN0PVYrcChm4cHGJMDmbg@mail.gmail.com>
> On Feb 5, 2024, at 3:03 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>
> Hi all-
>
> I encounter this issue every couple of years, and it still seems to be
> an issue, and it drives me nuts every time I see it.
>
> I write software that uses unconnected datagram-style sockets. Errors
> happen for all kinds of reasons, and my software knows it. My
> software even handles the errors and moves on with its life. I use
> MSG_ERRQUEUE to understand the errors. But the kernel fights back:
>
> struct sk_buff *__skb_try_recv_datagram(struct sock *sk,
> struct sk_buff_head *queue,
> unsigned int flags, int *off, int *err,
> struct sk_buff **last)
> {
> struct sk_buff *skb;
> unsigned long cpu_flags;
> /*
> * Caller is allowed not to check sk->sk_err before skb_recv_datagram()
> */
> int error = sock_error(sk);
>
> if (error)
> goto no_packet;
> ^^^^^^^^^^ <----- EXCUSE ME?
>
> The kernel even fights back on the *send* path?!?
>
> static long sock_wait_for_wmem(struct sock *sk, long timeo)
> {
> DEFINE_WAIT(wait);
>
> sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
> for (;;) {
> if (!timeo)
> break;
> if (signal_pending(current))
> break;
> set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> ...
> if (READ_ONCE(sk->sk_err))
> break; <-- KERNEL HATES UNCONNECTED SOCKETS!
>
> This is IMO just broken. I realize it's legacy behavior, but it's
> BROKEN legacy behavior. sk_err does not (at least for an unconnected
> socket) indicate that anything is wrong with the socket. It indicates
> that something is worthy of notice, and it wants to tell me.
>
> So:
>
> 1. sock_wait_for_wmem should IMO just not do that on an unconnected
> socket. AFAICS it's simply a bug.
>
> 2. How, exactly, am I supposed to call recvmsg() and, unambiguously,
> find out whether recvmsg() actually failed? There are actual errors
> (something that indicates that the kernel malfunctioned or the socket
> is broken), errors indicating that the packet being received is busted
> (skb_copy_datagram_msg, for example), and also errors indicating that
> there's an error queued up.
>
> I would like to know that there's an error queued up. That's what
> poll and epoll are for, right? Or a hint from recvmsg() that I should
> call MSG_RECVERR too. Or it could have a mode where it returns a
> normal datagram *or* an error as appropriate. But the current state
> of affairs is just brittle and racy.
>
> Are there any reasonably implementable, non-breaking ways to improve
> the API so that programs that understand socket errors can actually
> function fully correctly without gnarly retry loops in userspace and
> silly heuristics about what errors are actually errors?
Contemplating this, recvmsg() can sent status information back via msg_flags. Maybe we could characterize a recvmsg() call as doing one of the following things:
1. Actually fails, via -EFAULT or otherwise. Userspace can get an errno but doesn’t know beyond that what actually went wrong. Should never happen in a correct program. ENOMEM is not in this category.
2. There is nothing to receive. This is -EAGAIN.
3. Received an sk_err error. This is a *success*, and it comes with an error code. Users of RECVERR can’t reliably correlate this with an ERRQUEUE message. Maybe they don’t care.
4. Received a datagram.
5. Received a queued error message a la ERRQUEUE.
6. Dequeued a datagram (or ERRQUEUE) but did *not* receive it due to a checksum error or other error. (And there should be a clear indication of whether the call succeeded but something was wrong with the message or whether the call *failed* for an unexpected reason but the offending message was nonetheless removed from the socket’s queue).
Maybe 7: Received a message (or ERRQUEUE), and the checksum was wrong, but the data is being returned anyway.
I suppose that a flag could enable this mode and then all but #1 would return a *success* code from the syscall. And msg_flags would contain an indication as to what actually happened.
Thoughts? Does io_uring affect any of this?
>
> Grumpily,
> Andy
^ permalink raw reply
* Re: The sk_err mechanism is infuriating in userspace
From: Paolo Abeni @ 2024-02-06 8:43 UTC (permalink / raw)
To: Andy Lutomirski, Network Development; +Cc: Linux API
In-Reply-To: <CALCETrUe23P_3YAUMT2dmqq62xAc7zN0PVYrcChm4cHGJMDmbg@mail.gmail.com>
On Mon, 2024-02-05 at 15:03 -0800, Andy Lutomirski wrote:
> Hi all-
>
> I encounter this issue every couple of years, and it still seems to be
> an issue, and it drives me nuts every time I see it.
>
> I write software that uses unconnected datagram-style sockets. Errors
> happen for all kinds of reasons, and my software knows it. My
> software even handles the errors and moves on with its life. I use
> MSG_ERRQUEUE to understand the errors. But the kernel fights back:
>
> struct sk_buff *__skb_try_recv_datagram(struct sock *sk,
> struct sk_buff_head *queue,
> unsigned int flags, int *off, int *err,
> struct sk_buff **last)
> {
> struct sk_buff *skb;
> unsigned long cpu_flags;
> /*
> * Caller is allowed not to check sk->sk_err before skb_recv_datagram()
> */
> int error = sock_error(sk);
>
> if (error)
> goto no_packet;
> ^^^^^^^^^^ <----- EXCUSE ME?
>
> The kernel even fights back on the *send* path?!?
>
> static long sock_wait_for_wmem(struct sock *sk, long timeo)
> {
> DEFINE_WAIT(wait);
>
> sk_clear_bit(SOCKWQ_ASYNC_NOSPACE, sk);
> for (;;) {
> if (!timeo)
> break;
> if (signal_pending(current))
> break;
> set_bit(SOCK_NOSPACE, &sk->sk_socket->flags);
> ...
> if (READ_ONCE(sk->sk_err))
> break; <-- KERNEL HATES UNCONNECTED SOCKETS!
>
> This is IMO just broken. I realize it's legacy behavior, but it's
> BROKEN legacy behavior.
As you noted this is an established behaviour exposed to the user-
space, and we can't simply change it, regardless of it's own (eventual
lack of) merit.
> sk_err does not (at least for an unconnected
> socket) indicate that anything is wrong with the socket.
What about 'destination/port unreachable' and many other similar errors
reported by sk_err? Which specific errors reported by sk_err does not
indicate that anything is wrong with the socket ?
I guess that if you really want to ignore socket error for datagram
sockets at recvmsg()/sendmsg() time you could implement some new socket
option to conditionally enable such behaviour on a per socket basis.
Cheers,
Paolo
^ permalink raw reply
* Re: [PATCH] pidfd: clone: allow CLONE_THREAD | CLONE_PIDFD together
From: Christian Brauner @ 2024-02-06 13:40 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, Andy Lutomirski, Eric W. Biederman,
Tycho Andersen, linux-api, linux-kernel
In-Reply-To: <20240205145532.GA28823@redhat.com>
On Mon, 05 Feb 2024 15:55:32 +0100, Oleg Nesterov wrote:
> copy_process() just needs to pass PIDFD_THREAD to __pidfd_prepare()
> if clone_flags & CLONE_THREAD.
>
> We can also add another CLONE_ flag (or perhaps reuse CLONE_DETACHED)
> to enforce PIDFD_THREAD without CLONE_THREAD.
>
>
> [...]
Applied to the vfs.pidfd branch of the vfs/vfs.git tree.
Patches in the vfs.pidfd branch should appear in linux-next soon.
Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.
It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.
Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.
tree: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.pidfd
[1/1] pidfd: clone: allow CLONE_THREAD | CLONE_PIDFD together
https://git.kernel.org/vfs/vfs/c/572a94c5290e
^ permalink raw reply
* Re: [PATCH] pidfd: clone: allow CLONE_THREAD | CLONE_PIDFD together
From: Tycho Andersen @ 2024-02-06 14:52 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Christian Brauner, Andy Lutomirski, Eric W. Biederman, linux-api,
linux-kernel
In-Reply-To: <20240205145532.GA28823@redhat.com>
On Mon, Feb 05, 2024 at 03:55:32PM +0100, Oleg Nesterov wrote:
> copy_process() just needs to pass PIDFD_THREAD to __pidfd_prepare()
> if clone_flags & CLONE_THREAD.
>
> We can also add another CLONE_ flag (or perhaps reuse CLONE_DETACHED)
> to enforce PIDFD_THREAD without CLONE_THREAD.
>
> Originally-from: Tycho Andersen <tycho@tycho.pizza>
Goes without saying I suppose, but,
Reviewed-by: Tycho Andersen <tandersen@netflix.com>
as well.
Tycho
^ permalink raw reply
* [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Tycho Andersen @ 2024-02-06 16:43 UTC (permalink / raw)
To: Christian Brauner
Cc: Oleg Nesterov, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen, Tycho Andersen
From: Tycho Andersen <tandersen@netflix.com>
We can get EBADF from __pidfd_fget() if a task is currently exiting, which
might be confusing. Let's check PF_EXITING, and just report ESRCH if so.
I chose PF_EXITING, because it is set in exit_signals(), which is called
before exit_files(). Since ->exit_status is mostly set after exit_files()
in exit_notify(), using that still leaves a window open for the race.
Signed-off-by: Tycho Andersen <tandersen@netflix.com>
---
kernel/pid.c | 2 +-
.../selftests/pidfd/pidfd_getfd_test.c | 31 ++++++++++++++++++-
2 files changed, 31 insertions(+), 2 deletions(-)
diff --git a/kernel/pid.c b/kernel/pid.c
index de0bf2f8d18b..db8731f0ee45 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -688,7 +688,7 @@ static int pidfd_getfd(struct pid *pid, int fd)
int ret;
task = get_pid_task(pid, PIDTYPE_PID);
- if (!task)
+ if (!task || task->flags & PF_EXITING)
return -ESRCH;
file = __pidfd_fget(task, fd);
diff --git a/tools/testing/selftests/pidfd/pidfd_getfd_test.c b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
index 0930e2411dfb..cd51d547b751 100644
--- a/tools/testing/selftests/pidfd/pidfd_getfd_test.c
+++ b/tools/testing/selftests/pidfd/pidfd_getfd_test.c
@@ -5,6 +5,7 @@
#include <fcntl.h>
#include <limits.h>
#include <linux/types.h>
+#include <poll.h>
#include <sched.h>
#include <signal.h>
#include <stdio.h>
@@ -129,6 +130,7 @@ FIXTURE(child)
* When it is closed, the child will exit.
*/
int sk;
+ bool ignore_child_result;
};
FIXTURE_SETUP(child)
@@ -165,10 +167,14 @@ FIXTURE_SETUP(child)
FIXTURE_TEARDOWN(child)
{
+ int ret;
+
EXPECT_EQ(0, close(self->pidfd));
EXPECT_EQ(0, close(self->sk));
- EXPECT_EQ(0, wait_for_pid(self->pid));
+ ret = wait_for_pid(self->pid);
+ if (!self->ignore_child_result)
+ EXPECT_EQ(0, ret);
}
TEST_F(child, disable_ptrace)
@@ -235,6 +241,29 @@ TEST(flags_set)
EXPECT_EQ(errno, EINVAL);
}
+TEST_F(child, no_strange_EBADF)
+{
+ struct pollfd fds;
+
+ self->ignore_child_result = true;
+
+ fds.fd = self->pidfd;
+ fds.events = POLLIN;
+
+ ASSERT_EQ(kill(self->pid, SIGKILL), 0);
+ ASSERT_EQ(poll(&fds, 1, 5000), 1);
+
+ /*
+ * It used to be that pidfd_getfd() could race with the exiting thread
+ * between exit_files() and release_task(), and get a non-null task
+ * with a NULL files struct, and you'd get EBADF, which was slightly
+ * confusing.
+ */
+ errno = 0;
+ EXPECT_EQ(sys_pidfd_getfd(self->pidfd, self->remote_fd, 0), -1);
+ EXPECT_EQ(errno, ESRCH);
+}
+
#if __NR_pidfd_getfd == -1
int main(void)
{
base-commit: 082d11c164aef02e51bcd9c7cbf1554a8e42d9b5
--
2.34.1
^ permalink raw reply related
* Re: The sk_err mechanism is infuriating in userspace
From: Andy Lutomirski @ 2024-02-06 17:24 UTC (permalink / raw)
To: Paolo Abeni; +Cc: Network Development, Linux API
In-Reply-To: <852606cd9cbc8da9c6735b4ad6216ba55408b767.camel@redhat.com>
On Tue, Feb 6, 2024 at 12:43 AM Paolo Abeni <pabeni@redhat.com> wrote:
>
> What about 'destination/port unreachable' and many other similar errors
> reported by sk_err? Which specific errors reported by sk_err does not
> indicate that anything is wrong with the socket ?
Destination/port unreachable are *exactly* the primary offenders. Consider:
1. TCP socket. If the peer becomes unreachable, the connection is
unusable. Maybe reading previously queued data is reasonable; maybe
it's not, but one way or another the connection isn't working any
more. The current API seems okay.
2. UDP peer-to-peer connection. I have a socket and it's connected to
a peer. The peer sends an ICMP error or a route changes and the
kernel can't route to the peer. The connection is at least
temporarily dead. If we accept that temporarily dead equals
permanently dead, then returning errors codes makes sense. Even if we
expect the application to try to recover without making a new socket,
telling the application seems fine. The application will understand
that an error occurred communicating with its peer and can do
something about it.
3. UDP *server* with multiple clients. (Or unconnected UDP socket
communicating with multiple peers, etc.) Imagine a DNS server or a
QUIC server -- I hear QUIC is cool lately. A userspace server has a
socket, and it does sendto() or sendmsg() to a whole bunch of
addresses. One of them sends an ICMP error. There are multiple
things the server might do. It might ignore the error entirely and
treat it just like a timeout, because it probably already has
perfectly nice timeout handling. Or it might want to know that there
was an error communicating with a *specific* peer and release
resources sooner than it would for a timeout. Or it might want to
collect the entire ICMP error (via RECVERR) and do something useful
with it. But it gets no value whatsoever from knowing that an
unspecified peer sent an ICMP error, and it gets negative value from
having a call to recvfrom() or recvmsg() fail and needing to look up
in some hopefully-correct table whether the failure indicates an
actual problem (EFAULT, for example) or a completely useless return
value that should be ignored (EHOSTUNREACH).
(#3 is probably worse if the application uses one-shot notifications
-- the application needs to make a decision as to whether to call
recvfrom/recvmsg again or go back to polling.)
--Andy
^ permalink raw reply
* Re: [PATCH] pidfd: getfd should always report ESRCH if a task is exiting
From: Oleg Nesterov @ 2024-02-06 17:37 UTC (permalink / raw)
To: Tycho Andersen
Cc: Christian Brauner, Eric W . Biederman, linux-kernel, linux-api,
Tycho Andersen
In-Reply-To: <20240206164308.62620-1-tycho@tycho.pizza>
On 02/06, Tycho Andersen wrote:
>
> From: Tycho Andersen <tandersen@netflix.com>
>
> We can get EBADF from __pidfd_fget() if a task is currently exiting, which
> might be confusing.
agreed, because EBADF looks as if the "fd" argument was wrong,
> Let's check PF_EXITING, and just report ESRCH if so.
agreed, we can pretend that the task has already exited,
But:
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -688,7 +688,7 @@ static int pidfd_getfd(struct pid *pid, int fd)
> int ret;
>
> task = get_pid_task(pid, PIDTYPE_PID);
> - if (!task)
> + if (!task || task->flags & PF_EXITING)
> return -ESRCH;
This looks racy. Suppose that pidfd_getfd() races with the exiting task.
It is possible that this task sets PF_EXITING and does exit_files()
after the "task->flags & PF_EXITING" check above and before pidfd_getfd()
does __pidfd_fget(), in this case pidfd_getfd() still returns the same
EBADF we want to avoid.
Perhaps we can change pidfd_getfd() to do
if (IS_ERR(file))
return (task->flags & PF_EXITING) ? -ESRCH : PTR_ERR(file);
instead?
This needs a comment to explain the PF_EXITING check. And perhaps another
comment to explain that we can't miss PF_EXITING if the target task has
already passed exit_files, both exit_files() and fget_task() take the same
task_lock(task).
What do you think?
Oleg.
^ 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