The following patches were made over Linus's tree. Eric and Christian, the vhost maintainer, Michael Tsirkin has ACK'd the patches. I haven't got any more comments from you guys for a couple postings now (Jan 8 was the last reply). Are you guys ok to merge them? For everyone else that hasn't see this before, the patches allow the vhost layer to do a copy_process on the thread that does the VHOST_SET_OWNER ioctl like how io_uring does a copy_process against its userspace app. This allows the vhost layer's worker threads to inherit cgroups, namespaces, address space, etc and this worker thread will also be accounted for against that owner/parent process's RLIMIT_NPROC limit. If you are not familiar with qemu and vhost here is more detailed problem description: Qemu will create vhost devices in the kernel which perform network, SCSI, etc IO and management operations from worker threads created by the kthread API. Because the kthread API does a copy_process on the kthreadd thread, the vhost layer has to use kthread_use_mm to access the Qemu thread's memory and cgroup_attach_task_all to add itself to the Qemu thread's cgroups. The problem with this approach is that we then have to add new functions/ args/functionality for every thing we want to inherit. I started doing that here: https://lkml.org/lkml/2021/6/23/1233 for the RLIMIT_NPROC check, but it seems it might be easier to just inherit everything from the beginning, becuase I'd need to do something like that patch several times. V10: - Eric's cleanup patches my vhost flush cleanup patches are merged upstream, so rebase against Linus's tree which has everything. V9: - Rebase against Eric's kthread-cleanups-for-v5.19 branch. Drop patches no longer needed due to kernel clone arg and pf io worker patches in that branch. V8: - Fix kzalloc GFP use. - Fix email subject version number. V7: - Drop generic user_worker_* helpers and replace with vhost_task specific ones. - Drop autoreap patch. Use kernel_wait4 instead. - Fix issue where vhost.ko could be removed while the worker function is still running. V6: - Rename kernel_worker to user_worker and fix prefixes. - Add better patch descriptions. V5: - Handle kbuild errors by building patchset against current kernel that has all deps merged. Also add patch to remove create_io_thread code as it's not used anymore. - Rebase patchset against current kernel and handle a new vm PF_IO_WORKER case added in 5.16-rc1. - Add PF_USER_WORKER flag so we can check it later after the initial thread creation for the wake up, vm and singal cses. - Added patch to auto reap the worker thread. V4: - Drop NO_SIG patch and replaced with Christian's SIG_IGN patch. - Merged Christian's kernel_worker_flags_valid helpers into patch 5 that added the new kernel worker functions. - Fixed extra "i" issue. - Added PF_USER_WORKER flag and added check that kernel_worker_start users had that flag set. Also dropped patches that passed worker flags to copy_thread and replaced with PF_USER_WORKER check. V3: - Add parentheses in p->flag and work_flags check in copy_thread. - Fix check in arm/arm64 which was doing the reverse of other archs where it did likely(!flags) instead of unlikely(flags). V2: - Rename kernel_copy_process to kernel_worker. - Instead of exporting functions, make kernel_worker() a proper function/API that does common work for the caller. - Instead of adding new fields to kernel_clone_args for each option make it flag based similar to CLONE_*. - Drop unused completion struct in vhost. - Fix compile warnings by merging vhost cgroup cleanup patch and vhost conversion patch.
Mike Christie
2022-Jun-20 01:13 UTC
[PATCH v10 1/8] fork: Make IO worker options flag based
This patchset adds a couple new options to kernel_clone_args for the vhost layer which is going to work like PF_IO_WORKER but will differ enough that we will need to add several fields to kernel_clone_args. This patch moves us to a flags based approach for these types of users. Signed-off-by: Mike Christie <michael.christie at oracle.com> Suggested-by: Christian Brauner <brauner at kernel.org> Acked-by: Christian Brauner <brauner at kernel.org> Reviewed-by: Christoph Hellwig <hch at lst.de> Acked-by: Michael S. Tsirkin <mst at redhat.com> --- include/linux/sched/task.h | 4 +++- kernel/fork.c | 4 ++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h index 505aaf9fe477..b0a9d6c75bcc 100644 --- a/include/linux/sched/task.h +++ b/include/linux/sched/task.h @@ -18,8 +18,11 @@ struct css_set; /* All the bits taken by the old clone syscall. */ #define CLONE_LEGACY_FLAGS 0xffffffffULL +#define USER_WORKER_IO BIT(0) + struct kernel_clone_args { u64 flags; + u32 worker_flags; int __user *pidfd; int __user *child_tid; int __user *parent_tid; @@ -31,7 +34,6 @@ struct kernel_clone_args { /* Number of elements in *set_tid */ size_t set_tid_size; int cgroup; - int io_thread; int kthread; int idle; int (*fn)(void *); diff --git a/kernel/fork.c b/kernel/fork.c index 9d44f2d46c69..49fedb79925b 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2074,7 +2074,7 @@ static __latent_entropy struct task_struct *copy_process( p->flags &= ~PF_KTHREAD; if (args->kthread) p->flags |= PF_KTHREAD; - if (args->io_thread) { + if (args->worker_flags & USER_WORKER_IO) { /* * Mark us an IO worker, and block any signal that isn't * fatal or STOP @@ -2597,7 +2597,7 @@ struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node) .exit_signal = (lower_32_bits(flags) & CSIGNAL), .fn = fn, .fn_arg = arg, - .io_thread = 1, + .worker_flags = USER_WORKER_IO, }; return copy_process(NULL, 0, node, &args); -- 2.25.1
Mike Christie
2022-Jun-20 01:13 UTC
[PATCH v10 2/8] fork/vm: Move common PF_IO_WORKER behavior to new flag
This adds a new flag, PF_USER_WORKER, that's used for behavior common to to both PF_IO_WORKER and users like vhost which will use a new helper instead of create_io_thread because they require different behavior for operations like signal handling. The common behavior PF_USER_WORKER covers is the vm reclaim handling. Signed-off-by: Mike Christie <michael.christie at oracle.com> Acked-by: Michael S. Tsirkin <mst at redhat.com> --- include/linux/sched.h | 1 + include/linux/sched/task.h | 3 ++- kernel/fork.c | 4 ++++ mm/vmscan.c | 4 ++-- 4 files changed, 9 insertions(+), 3 deletions(-) diff --git a/include/linux/sched.h b/include/linux/sched.h index c46f3a63b758..d7cd2037430b 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1701,6 +1701,7 @@ extern struct pid *cad_pid; #define PF_MEMALLOC 0x00000800 /* Allocating memory */ #define PF_NPROC_EXCEEDED 0x00001000 /* set_user() noticed that RLIMIT_NPROC was exceeded */ #define PF_USED_MATH 0x00002000 /* If unset the fpu must be initialized before use */ +#define PF_USER_WORKER 0x00004000 /* Kernel thread cloned from userspace thread */ #define PF_NOFREEZE 0x00008000 /* This thread should not be frozen */ #define PF_FROZEN 0x00010000 /* Frozen for system suspend */ #define PF_KSWAPD 0x00020000 /* I am kswapd */ diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h index b0a9d6c75bcc..9e20fa18c41f 100644 --- a/include/linux/sched/task.h +++ b/include/linux/sched/task.h @@ -18,7 +18,8 @@ struct css_set; /* All the bits taken by the old clone syscall. */ #define CLONE_LEGACY_FLAGS 0xffffffffULL -#define USER_WORKER_IO BIT(0) +#define USER_WORKER BIT(0) +#define USER_WORKER_IO BIT(1) struct kernel_clone_args { u64 flags; diff --git a/kernel/fork.c b/kernel/fork.c index 49fedb79925b..604c7f3cdde8 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2074,6 +2074,10 @@ static __latent_entropy struct task_struct *copy_process( p->flags &= ~PF_KTHREAD; if (args->kthread) p->flags |= PF_KTHREAD; + + if (args->worker_flags & USER_WORKER) + p->flags |= PF_USER_WORKER; + if (args->worker_flags & USER_WORKER_IO) { /* * Mark us an IO worker, and block any signal that isn't diff --git a/mm/vmscan.c b/mm/vmscan.c index f7d9a683e3a7..6d771d8a3a4a 100644 --- a/mm/vmscan.c +++ b/mm/vmscan.c @@ -1047,12 +1047,12 @@ void reclaim_throttle(pg_data_t *pgdat, enum vmscan_throttle_state reason) DEFINE_WAIT(wait); /* - * Do not throttle IO workers, kthreads other than kswapd or + * Do not throttle user workers, kthreads other than kswapd or * workqueues. They may be required for reclaim to make * forward progress (e.g. journalling workqueues or kthreads). */ if (!current_is_kswapd() && - current->flags & (PF_IO_WORKER|PF_KTHREAD)) { + current->flags & (PF_USER_WORKER|PF_KTHREAD)) { cond_resched(); return; } -- 2.25.1
Mike Christie
2022-Jun-20 01:13 UTC
[PATCH v10 3/8] fork: add USER_WORKER flag to not dup/clone files
Each vhost device gets a thread that is used to perform IO and management operations. Instead of a thread that is accessing a device, the thread is part of the device, so when it creates a thread using a helper based on copy_process we can't dup or clone the parent's files/FDS because it would do an extra increment on ourself. Later, when we do: Qemu process exits: do_exit -> exit_files -> put_files_struct -> close_files we would leak the device's resources because of that extra refcount on the fd or file_struct. This patch adds a no_files option so these worker threads can prevent taking an extra refcount on themselves. Signed-off-by: Mike Christie <michael.christie at oracle.com> Acked-by: Christian Brauner <brauner at kernel.org> Reviewed-by: Christoph Hellwig <hch at lst.de> Acked-by: Michael S. Tsirkin <mst at redhat.com> --- include/linux/sched/task.h | 1 + kernel/fork.c | 11 +++++++++-- 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h index 9e20fa18c41f..7f6364e5aa1a 100644 --- a/include/linux/sched/task.h +++ b/include/linux/sched/task.h @@ -20,6 +20,7 @@ struct css_set; #define USER_WORKER BIT(0) #define USER_WORKER_IO BIT(1) +#define USER_WORKER_NO_FILES BIT(2) struct kernel_clone_args { u64 flags; diff --git a/kernel/fork.c b/kernel/fork.c index 604c7f3cdde8..0313aa9ead89 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1603,7 +1603,8 @@ static int copy_fs(unsigned long clone_flags, struct task_struct *tsk) return 0; } -static int copy_files(unsigned long clone_flags, struct task_struct *tsk) +static int copy_files(unsigned long clone_flags, struct task_struct *tsk, + int no_files) { struct files_struct *oldf, *newf; int error = 0; @@ -1615,6 +1616,11 @@ static int copy_files(unsigned long clone_flags, struct task_struct *tsk) if (!oldf) goto out; + if (no_files) { + tsk->files = NULL; + goto out; + } + if (clone_flags & CLONE_FILES) { atomic_inc(&oldf->count); goto out; @@ -2229,7 +2235,8 @@ static __latent_entropy struct task_struct *copy_process( retval = copy_semundo(clone_flags, p); if (retval) goto bad_fork_cleanup_security; - retval = copy_files(clone_flags, p); + retval = copy_files(clone_flags, p, + args->worker_flags & USER_WORKER_NO_FILES); if (retval) goto bad_fork_cleanup_semundo; retval = copy_fs(clone_flags, p); -- 2.25.1
Mike Christie
2022-Jun-20 01:13 UTC
[PATCH v10 4/8] fork: Add USER_WORKER flag to ignore signals
From: Christian Brauner <brauner at kernel.org> Since: commit 10ab825bdef8 ("change kernel threads to ignore signals instead of blocking them") kthreads have been ignoring signals by default, and the vhost layer has never had a need to change that. This patch adds an option flag, USER_WORKER_SIG_IGN, handled in copy_process() after copy_sighand() and copy_signals() so vhost_tasks added in the next patches can continue to ignore singals. Signed-off-by: Christian Brauner <brauner at kernel.org> Signed-off-by: Mike Christie <michael.christie at oracle.com> Reviewed-by: Christoph Hellwig <hch at lst.de> Acked-by: Michael S. Tsirkin <mst at redhat.com> --- include/linux/sched/task.h | 1 + kernel/fork.c | 3 +++ 2 files changed, 4 insertions(+) diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h index 7f6364e5aa1a..9fbb489b2512 100644 --- a/include/linux/sched/task.h +++ b/include/linux/sched/task.h @@ -21,6 +21,7 @@ struct css_set; #define USER_WORKER BIT(0) #define USER_WORKER_IO BIT(1) #define USER_WORKER_NO_FILES BIT(2) +#define USER_WORKER_SIG_IGN BIT(3) struct kernel_clone_args { u64 flags; diff --git a/kernel/fork.c b/kernel/fork.c index 0313aa9ead89..c479b8419a6e 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2261,6 +2261,9 @@ static __latent_entropy struct task_struct *copy_process( if (retval) goto bad_fork_cleanup_io; + if (args->worker_flags & USER_WORKER_SIG_IGN) + ignore_signals(p); + stackleak_task_init(p); if (pid != &init_struct_pid) { -- 2.25.1
Mike Christie
2022-Jun-20 01:13 UTC
[PATCH v10 5/8] fork: allow kernel code to call copy_process
The next patch adds helpers like create_io_thread, but for use by the vhost layer. There are several functions, so they are in their own file instead of cluttering up fork.c. This patch allows that new file to call copy_process. Signed-off-by: Mike Christie <michael.christie at oracle.com> Acked-by: Michael S. Tsirkin <mst at redhat.com> --- include/linux/sched/task.h | 2 ++ kernel/fork.c | 2 +- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h index 9fbb489b2512..ec4a0097fe4c 100644 --- a/include/linux/sched/task.h +++ b/include/linux/sched/task.h @@ -93,6 +93,8 @@ extern void exit_files(struct task_struct *); extern void exit_itimers(struct signal_struct *); extern pid_t kernel_clone(struct kernel_clone_args *kargs); +struct task_struct *copy_process(struct pid *pid, int trace, int node, + struct kernel_clone_args *args); struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node); struct task_struct *fork_idle(int); struct mm_struct *copy_init_mm(void); diff --git a/kernel/fork.c b/kernel/fork.c index c479b8419a6e..7b6caf46187e 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1978,7 +1978,7 @@ static void copy_oom_score_adj(u64 clone_flags, struct task_struct *tsk) * parts of the process environment (as per the clone * flags). The actual kick-off is left to the caller. */ -static __latent_entropy struct task_struct *copy_process( +__latent_entropy struct task_struct *copy_process( struct pid *pid, int trace, int node, -- 2.25.1
Mike Christie
2022-Jun-20 01:13 UTC
[PATCH v10 6/8] vhost_task: Allow vhost layer to use copy_process
Qemu will create vhost devices in the kernel which perform network, SCSI, etc IO and management operations from worker threads created by the kthread API. Because the kthread API does a copy_process on the kthreadd thread, the vhost layer has to use kthread_use_mm to access the Qemu thread's memory and cgroup_attach_task_all to add itself to the Qemu thread's cgroups, and it bypasses the RLIMIT_NPROC limit which can result in VMs creating more threads than the admin expected. This patch adds a new struct vhost_task which can be used instead of kthreads. They allow the vhost layer to use copy_process and inherit the userspace process's mm and cgroups, the task is accounted for under the userspace's nproc count and can be seen in its process tree, and other features like namespaces work and are inherited by default. Signed-off-by: Mike Christie <michael.christie at oracle.com> Acked-by: Michael S. Tsirkin <mst at redhat.com> --- MAINTAINERS | 2 + drivers/vhost/Kconfig | 5 ++ include/linux/sched/vhost_task.h | 23 ++++++ kernel/Makefile | 1 + kernel/vhost_task.c | 122 +++++++++++++++++++++++++++++++ 5 files changed, 153 insertions(+) create mode 100644 include/linux/sched/vhost_task.h create mode 100644 kernel/vhost_task.c diff --git a/MAINTAINERS b/MAINTAINERS index f52543aedd61..8efd697499fd 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -21147,7 +21147,9 @@ L: virtualization at lists.linux-foundation.org L: netdev at vger.kernel.org S: Maintained T: git git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost.git +F: kernel/vhost_task.c F: drivers/vhost/ +F: include/linux/sched/vhost_task.h F: include/linux/vhost_iotlb.h F: include/uapi/linux/vhost.h diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig index 587fbae06182..b455d9ab6f3d 100644 --- a/drivers/vhost/Kconfig +++ b/drivers/vhost/Kconfig @@ -13,9 +13,14 @@ config VHOST_RING This option is selected by any driver which needs to access the host side of a virtio ring. +config VHOST_TASK + bool + default n + config VHOST tristate select VHOST_IOTLB + select VHOST_TASK help This option is selected by any driver which needs to access the core of vhost. diff --git a/include/linux/sched/vhost_task.h b/include/linux/sched/vhost_task.h new file mode 100644 index 000000000000..50d02a25d37b --- /dev/null +++ b/include/linux/sched/vhost_task.h @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +#ifndef _LINUX_VHOST_TASK_H +#define _LINUX_VHOST_TASK_H + +#include <linux/completion.h> + +struct task_struct; + +struct vhost_task { + int (*fn)(void *data); + void *data; + struct completion exited; + unsigned long flags; + struct task_struct *task; +}; + +struct vhost_task *vhost_task_create(int (*fn)(void *), void *arg, int node); +__printf(2, 3) +void vhost_task_start(struct vhost_task *vtsk, const char namefmt[], ...); +void vhost_task_stop(struct vhost_task *vtsk); +bool vhost_task_should_stop(struct vhost_task *vtsk); + +#endif diff --git a/kernel/Makefile b/kernel/Makefile index a7e1f49ab2b3..c00409e526ad 100644 --- a/kernel/Makefile +++ b/kernel/Makefile @@ -15,6 +15,7 @@ obj-y = fork.o exec_domain.o panic.o \ obj-$(CONFIG_USERMODE_DRIVER) += usermode_driver.o obj-$(CONFIG_MODULES) += kmod.o obj-$(CONFIG_MULTIUSER) += groups.o +obj-$(CONFIG_VHOST_TASK) += vhost_task.o ifdef CONFIG_FUNCTION_TRACER # Do not trace internal ftrace files diff --git a/kernel/vhost_task.c b/kernel/vhost_task.c new file mode 100644 index 000000000000..517dd166bb2b --- /dev/null +++ b/kernel/vhost_task.c @@ -0,0 +1,122 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2021 Oracle Corporation + */ +#include <linux/slab.h> +#include <linux/completion.h> +#include <linux/sched/task.h> +#include <linux/sched/vhost_task.h> +#include <linux/sched/signal.h> + +enum vhost_task_flags { + VHOST_TASK_FLAGS_STOP, +}; + +static int vhost_task_fn(void *data) +{ + struct vhost_task *vtsk = data; + int ret; + + ret = vtsk->fn(vtsk->data); + complete(&vtsk->exited); + do_exit(ret); +} + +/** + * vhost_task_stop - stop a vhost_task + * @vtsk: vhost_task to stop + * + * Callers must call vhost_task_should_stop and return from their worker + * function when it returns true; + */ +void vhost_task_stop(struct vhost_task *vtsk) +{ + pid_t pid = vtsk->task->pid; + + set_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags); + wake_up_process(vtsk->task); + /* + * Make sure vhost_task_fn is no longer accessing the vhost_task before + * freeing it below. If userspace crashed or exited without closing, + * then the vhost_task->task could already be marked dead so + * kernel_wait will return early. + */ + wait_for_completion(&vtsk->exited); + /* + * If we are just closing/removing a device and the parent process is + * not exiting then reap the task. + */ + kernel_wait4(pid, NULL, __WCLONE, NULL); + kfree(vtsk); +} +EXPORT_SYMBOL_GPL(vhost_task_stop); + +/** + * vhost_task_should_stop - should the vhost task return from the work function + */ +bool vhost_task_should_stop(struct vhost_task *vtsk) +{ + return test_bit(VHOST_TASK_FLAGS_STOP, &vtsk->flags); +} +EXPORT_SYMBOL_GPL(vhost_task_should_stop); + +/** + * vhost_task_create - create a copy of a process to be used by the kernel + * @fn: thread stack + * @arg: data to be passed to fn + * @node: numa node to allocate task from + * + * This returns a specialized task for use by the vhost layer or NULL on + * failure. The returned task is inactive, and the caller must fire it up + * through vhost_task_start(). + */ +struct vhost_task *vhost_task_create(int (*fn)(void *), void *arg, int node) +{ + struct kernel_clone_args args = { + .flags = CLONE_FS | CLONE_UNTRACED | CLONE_VM, + .exit_signal = 0, + .worker_flags = USER_WORKER | USER_WORKER_NO_FILES | + USER_WORKER_SIG_IGN, + .fn = vhost_task_fn, + }; + struct vhost_task *vtsk; + struct task_struct *tsk; + + vtsk = kzalloc(sizeof(*vtsk), GFP_KERNEL); + if (!vtsk) + return ERR_PTR(-ENOMEM); + init_completion(&vtsk->exited); + vtsk->data = arg; + vtsk->fn = fn; + + args.fn_arg = vtsk; + + tsk = copy_process(NULL, 0, node, &args); + if (IS_ERR(tsk)) { + kfree(vtsk); + return NULL; + } + + vtsk->task = tsk; + return vtsk; +} +EXPORT_SYMBOL_GPL(vhost_task_create); + +/** + * vhost_task_start - start a vhost_task created with vhost_task_create + * @vtsk: vhost_task to wake up + * @namefmt: printf-style format string for the thread name + */ +void vhost_task_start(struct vhost_task *vtsk, const char namefmt[], ...) +{ + char name[TASK_COMM_LEN]; + va_list args; + + va_start(args, namefmt); + vsnprintf(name, sizeof(name), namefmt, args); + set_task_comm(vtsk->task, name); + va_end(args); + + wake_up_new_task(vtsk->task); +} +EXPORT_SYMBOL_GPL(vhost_task_start); -- 2.25.1
Mike Christie
2022-Jun-20 01:13 UTC
[PATCH v10 7/8] vhost: move worker thread fields to new struct
This is just a prep patch. It moves the worker related fields to a new vhost_worker struct and moves the code around to create some helpers that will be used in the next patch. Signed-off-by: Mike Christie <michael.christie at oracle.com> Reviewed-by: Stefan Hajnoczi <stefanha at redhat.com> Acked-by: Michael S. Tsirkin <mst at redhat.com> Reviewed-by: Christoph Hellwig <hch at lst.de> --- drivers/vhost/vhost.c | 98 ++++++++++++++++++++++++++++--------------- drivers/vhost/vhost.h | 11 +++-- 2 files changed, 72 insertions(+), 37 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 40097826cff0..4e17ac40c08c 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -255,8 +255,8 @@ void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) * sure it was not in the list. * test_and_set_bit() implies a memory barrier. */ - llist_add(&work->node, &dev->work_list); - wake_up_process(dev->worker); + llist_add(&work->node, &dev->worker->work_list); + wake_up_process(dev->worker->task); } } EXPORT_SYMBOL_GPL(vhost_work_queue); @@ -264,7 +264,7 @@ EXPORT_SYMBOL_GPL(vhost_work_queue); /* A lockless hint for busy polling code to exit the loop */ bool vhost_has_work(struct vhost_dev *dev) { - return !llist_empty(&dev->work_list); + return dev->worker && !llist_empty(&dev->worker->work_list); } EXPORT_SYMBOL_GPL(vhost_has_work); @@ -335,7 +335,8 @@ static void vhost_vq_reset(struct vhost_dev *dev, static int vhost_worker(void *data) { - struct vhost_dev *dev = data; + struct vhost_worker *worker = data; + struct vhost_dev *dev = worker->dev; struct vhost_work *work, *work_next; struct llist_node *node; @@ -350,7 +351,7 @@ static int vhost_worker(void *data) break; } - node = llist_del_all(&dev->work_list); + node = llist_del_all(&worker->work_list); if (!node) schedule(); @@ -360,7 +361,7 @@ static int vhost_worker(void *data) llist_for_each_entry_safe(work, work_next, node, node) { clear_bit(VHOST_WORK_QUEUED, &work->flags); __set_current_state(TASK_RUNNING); - kcov_remote_start_common(dev->kcov_handle); + kcov_remote_start_common(worker->kcov_handle); work->fn(work); kcov_remote_stop(); if (need_resched()) @@ -479,7 +480,6 @@ void vhost_dev_init(struct vhost_dev *dev, dev->byte_weight = byte_weight; dev->use_worker = use_worker; dev->msg_handler = msg_handler; - init_llist_head(&dev->work_list); init_waitqueue_head(&dev->wait); INIT_LIST_HEAD(&dev->read_list); INIT_LIST_HEAD(&dev->pending_list); @@ -571,10 +571,60 @@ static void vhost_detach_mm(struct vhost_dev *dev) dev->mm = NULL; } +static void vhost_worker_free(struct vhost_dev *dev) +{ + struct vhost_worker *worker = dev->worker; + + if (!worker) + return; + + dev->worker = NULL; + WARN_ON(!llist_empty(&worker->work_list)); + kthread_stop(worker->task); + kfree(worker); +} + +static int vhost_worker_create(struct vhost_dev *dev) +{ + struct vhost_worker *worker; + struct task_struct *task; + int ret; + + worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT); + if (!worker) + return -ENOMEM; + + dev->worker = worker; + worker->dev = dev; + worker->kcov_handle = kcov_common_handle(); + init_llist_head(&worker->work_list); + + task = kthread_create(vhost_worker, worker, "vhost-%d", current->pid); + if (IS_ERR(task)) { + ret = PTR_ERR(task); + goto free_worker; + } + + worker->task = task; + wake_up_process(task); /* avoid contributing to loadavg */ + + ret = vhost_attach_cgroups(dev); + if (ret) + goto stop_worker; + + return 0; + +stop_worker: + kthread_stop(worker->task); +free_worker: + kfree(worker); + dev->worker = NULL; + return ret; +} + /* Caller should have device mutex */ long vhost_dev_set_owner(struct vhost_dev *dev) { - struct task_struct *worker; int err; /* Is there an owner already? */ @@ -585,36 +635,21 @@ long vhost_dev_set_owner(struct vhost_dev *dev) vhost_attach_mm(dev); - dev->kcov_handle = kcov_common_handle(); if (dev->use_worker) { - worker = kthread_create(vhost_worker, dev, - "vhost-%d", current->pid); - if (IS_ERR(worker)) { - err = PTR_ERR(worker); - goto err_worker; - } - - dev->worker = worker; - wake_up_process(worker); /* avoid contributing to loadavg */ - - err = vhost_attach_cgroups(dev); + err = vhost_worker_create(dev); if (err) - goto err_cgroup; + goto err_worker; } err = vhost_dev_alloc_iovecs(dev); if (err) - goto err_cgroup; + goto err_iovecs; return 0; -err_cgroup: - if (dev->worker) { - kthread_stop(dev->worker); - dev->worker = NULL; - } +err_iovecs: + vhost_worker_free(dev); err_worker: vhost_detach_mm(dev); - dev->kcov_handle = 0; err_mm: return err; } @@ -704,12 +739,7 @@ void vhost_dev_cleanup(struct vhost_dev *dev) dev->iotlb = NULL; vhost_clear_msg(dev); wake_up_interruptible_poll(&dev->wait, EPOLLIN | EPOLLRDNORM); - WARN_ON(!llist_empty(&dev->work_list)); - if (dev->worker) { - kthread_stop(dev->worker); - dev->worker = NULL; - dev->kcov_handle = 0; - } + vhost_worker_free(dev); vhost_detach_mm(dev); } EXPORT_SYMBOL_GPL(vhost_dev_cleanup); diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index d9109107af08..2f6beab93784 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -25,6 +25,13 @@ struct vhost_work { unsigned long flags; }; +struct vhost_worker { + struct task_struct *task; + struct llist_head work_list; + struct vhost_dev *dev; + u64 kcov_handle; +}; + /* Poll a file (eventfd or socket) */ /* Note: there's nothing vhost specific about this structure. */ struct vhost_poll { @@ -147,8 +154,7 @@ struct vhost_dev { struct vhost_virtqueue **vqs; int nvqs; struct eventfd_ctx *log_ctx; - struct llist_head work_list; - struct task_struct *worker; + struct vhost_worker *worker; struct vhost_iotlb *umem; struct vhost_iotlb *iotlb; spinlock_t iotlb_lock; @@ -158,7 +164,6 @@ struct vhost_dev { int iov_limit; int weight; int byte_weight; - u64 kcov_handle; bool use_worker; int (*msg_handler)(struct vhost_dev *dev, u32 asid, struct vhost_iotlb_msg *msg); -- 2.25.1
Mike Christie
2022-Jun-20 01:13 UTC
[PATCH v10 8/8] vhost: use vhost_tasks for worker threads
For vhost workers we use the kthread API which inherit's its values from and checks against the kthreadd thread. This results in the wrong RLIMITs being checked, so while tools like libvirt try to control the number of threads based on the nproc rlimit setting we can end up creating more threads than the user wanted. This patch has us use the vhost_task helpers which will inherit its values/checks from the thread that owns the device similar to if we did a clone in userspace. The vhost threads will now be counted in the nproc rlimits. And we get features like cgroups and mm sharing automatically, so we can remove those calls. Signed-off-by: Mike Christie <michael.christie at oracle.com> Acked-by: Michael S. Tsirkin <mst at redhat.com> --- drivers/vhost/vhost.c | 58 ++++++++----------------------------------- drivers/vhost/vhost.h | 4 +-- 2 files changed, 13 insertions(+), 49 deletions(-) diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c index 4e17ac40c08c..c6aa0a45357d 100644 --- a/drivers/vhost/vhost.c +++ b/drivers/vhost/vhost.c @@ -22,11 +22,11 @@ #include <linux/slab.h> #include <linux/vmalloc.h> #include <linux/kthread.h> -#include <linux/cgroup.h> #include <linux/module.h> #include <linux/sort.h> #include <linux/sched/mm.h> #include <linux/sched/signal.h> +#include <linux/sched/vhost_task.h> #include <linux/interval_tree_generic.h> #include <linux/nospec.h> #include <linux/kcov.h> @@ -256,7 +256,7 @@ void vhost_work_queue(struct vhost_dev *dev, struct vhost_work *work) * test_and_set_bit() implies a memory barrier. */ llist_add(&work->node, &dev->worker->work_list); - wake_up_process(dev->worker->task); + wake_up_process(dev->worker->vtsk->task); } } EXPORT_SYMBOL_GPL(vhost_work_queue); @@ -336,17 +336,14 @@ static void vhost_vq_reset(struct vhost_dev *dev, static int vhost_worker(void *data) { struct vhost_worker *worker = data; - struct vhost_dev *dev = worker->dev; struct vhost_work *work, *work_next; struct llist_node *node; - kthread_use_mm(dev->mm); - for (;;) { /* mb paired w/ kthread_stop */ set_current_state(TASK_INTERRUPTIBLE); - if (kthread_should_stop()) { + if (vhost_task_should_stop(worker->vtsk)) { __set_current_state(TASK_RUNNING); break; } @@ -368,7 +365,7 @@ static int vhost_worker(void *data) schedule(); } } - kthread_unuse_mm(dev->mm); + return 0; } @@ -509,31 +506,6 @@ long vhost_dev_check_owner(struct vhost_dev *dev) } EXPORT_SYMBOL_GPL(vhost_dev_check_owner); -struct vhost_attach_cgroups_struct { - struct vhost_work work; - struct task_struct *owner; - int ret; -}; - -static void vhost_attach_cgroups_work(struct vhost_work *work) -{ - struct vhost_attach_cgroups_struct *s; - - s = container_of(work, struct vhost_attach_cgroups_struct, work); - s->ret = cgroup_attach_task_all(s->owner, current); -} - -static int vhost_attach_cgroups(struct vhost_dev *dev) -{ - struct vhost_attach_cgroups_struct attach; - - attach.owner = current; - vhost_work_init(&attach.work, vhost_attach_cgroups_work); - vhost_work_queue(dev, &attach.work); - vhost_dev_flush(dev); - return attach.ret; -} - /* Caller should have device mutex */ bool vhost_dev_has_owner(struct vhost_dev *dev) { @@ -580,14 +552,14 @@ static void vhost_worker_free(struct vhost_dev *dev) dev->worker = NULL; WARN_ON(!llist_empty(&worker->work_list)); - kthread_stop(worker->task); + vhost_task_stop(worker->vtsk); kfree(worker); } static int vhost_worker_create(struct vhost_dev *dev) { struct vhost_worker *worker; - struct task_struct *task; + struct vhost_task *vtsk; int ret; worker = kzalloc(sizeof(*worker), GFP_KERNEL_ACCOUNT); @@ -595,27 +567,19 @@ static int vhost_worker_create(struct vhost_dev *dev) return -ENOMEM; dev->worker = worker; - worker->dev = dev; worker->kcov_handle = kcov_common_handle(); init_llist_head(&worker->work_list); - task = kthread_create(vhost_worker, worker, "vhost-%d", current->pid); - if (IS_ERR(task)) { - ret = PTR_ERR(task); + vtsk = vhost_task_create(vhost_worker, worker, NUMA_NO_NODE); + if (!vtsk) { + ret = -ENOMEM; goto free_worker; } - worker->task = task; - wake_up_process(task); /* avoid contributing to loadavg */ - - ret = vhost_attach_cgroups(dev); - if (ret) - goto stop_worker; - + worker->vtsk = vtsk; + vhost_task_start(vtsk, "vhost-%d", current->pid); return 0; -stop_worker: - kthread_stop(worker->task); free_worker: kfree(worker); dev->worker = NULL; diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h index 2f6beab93784..3af59c65025e 100644 --- a/drivers/vhost/vhost.h +++ b/drivers/vhost/vhost.h @@ -16,6 +16,7 @@ #include <linux/irqbypass.h> struct vhost_work; +struct vhost_task; typedef void (*vhost_work_fn_t)(struct vhost_work *work); #define VHOST_WORK_QUEUED 1 @@ -26,9 +27,8 @@ struct vhost_work { }; struct vhost_worker { - struct task_struct *task; + struct vhost_task *vtsk; struct llist_head work_list; - struct vhost_dev *dev; u64 kcov_handle; }; -- 2.25.1
Eric and Christian, Ping? If you guys don't like these patches anymore what about something simple like just exporting some helpers to update and check a task's nproc limit. Something like this: diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h index 81cab4b01edc..71b5946be792 100644 --- a/include/linux/sched/task.h +++ b/include/linux/sched/task.h @@ -98,6 +98,10 @@ int kernel_wait(pid_t pid, int *stat); extern void free_task(struct task_struct *tsk); +extern bool task_is_over_nproc_limit(struct task_struct *tsk); +extern void task_inc_nproc(struct task_struct *tsk); +extern void task_dec_nproc(struct task_struct *tsk); + /* sched_exec is called by processes performing an exec */ #ifdef CONFIG_SMP extern void sched_exec(void); diff --git a/kernel/cred.c b/kernel/cred.c index e10c15f51c1f..c15e7b926013 100644 --- a/kernel/cred.c +++ b/kernel/cred.c @@ -358,7 +358,7 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags) kdebug("share_creds(%p{%d,%d})", p->cred, atomic_read(&p->cred->usage), read_cred_subscribers(p->cred)); - inc_rlimit_ucounts(task_ucounts(p), UCOUNT_RLIMIT_NPROC, 1); + task_inc_nproc(p); return 0; } @@ -395,7 +395,7 @@ int copy_creds(struct task_struct *p, unsigned long clone_flags) #endif p->cred = p->real_cred = get_cred(new); - inc_rlimit_ucounts(task_ucounts(p), UCOUNT_RLIMIT_NPROC, 1); + task_inc_nproc(p); alter_cred_subscribers(new, 2); validate_creds(new); return 0; diff --git a/kernel/fork.c b/kernel/fork.c index 9d44f2d46c69..88dbe3458d7d 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1964,6 +1964,32 @@ static void copy_oom_score_adj(u64 clone_flags, struct task_struct *tsk) mutex_unlock(&oom_adj_mutex); } +bool task_is_over_nproc_limit(struct task_struct *tsk) +{ + if (!is_ucounts_overlimit(task_ucounts(tsk), UCOUNT_RLIMIT_NPROC, + task_rlimit(tsk, RLIMIT_NPROC))) + return false; + + if (tsk->real_cred->user != INIT_USER && !capable(CAP_SYS_RESOURCE) && + !capable(CAP_SYS_ADMIN)) + return true; + + return false; +} +EXPORT_SYMBOL_GPL(task_is_over_nproc_limit); + +void task_inc_nproc(struct task_struct *tsk) +{ + inc_rlimit_ucounts(task_ucounts(tsk), UCOUNT_RLIMIT_NPROC, 1); +} +EXPORT_SYMBOL_GPL(task_inc_nproc); + +void task_dec_nproc(struct task_struct *tsk) +{ + dec_rlimit_ucounts(task_ucounts(tsk), UCOUNT_RLIMIT_NPROC, 1); +} +EXPORT_SYMBOL_GPL(task_dec_nproc); + /* * This creates a new process as a copy of the old one, * but does not actually start it yet. @@ -2102,11 +2128,8 @@ static __latent_entropy struct task_struct *copy_process( goto bad_fork_free; retval = -EAGAIN; - if (is_ucounts_overlimit(task_ucounts(p), UCOUNT_RLIMIT_NPROC, rlimit(RLIMIT_NPROC))) { - if (p->real_cred->user != INIT_USER && - !capable(CAP_SYS_RESOURCE) && !capable(CAP_SYS_ADMIN)) - goto bad_fork_cleanup_count; - } + if (task_is_over_nproc_limit(p)) + goto bad_fork_cleanup_count; current->flags &= ~PF_NPROC_EXCEEDED; /* @@ -2526,7 +2549,7 @@ static __latent_entropy struct task_struct *copy_process( bad_fork_cleanup_delayacct: delayacct_tsk_free(p); bad_fork_cleanup_count: - dec_rlimit_ucounts(task_ucounts(p), UCOUNT_RLIMIT_NPROC, 1); + task_dec_nproc(p); exit_creds(p); bad_fork_free: WRITE_ONCE(p->__state, TASK_DEAD); On 6/19/22 8:13 PM, Mike Christie wrote:> The following patches were made over Linus's tree. > > Eric and Christian, the vhost maintainer, Michael Tsirkin has ACK'd the > patches. I haven't got any more comments from you guys for a couple > postings now (Jan 8 was the last reply). Are you guys ok to merge them? > > For everyone else that hasn't see this before, the patches allow the > vhost layer to do a copy_process on the thread that does the > VHOST_SET_OWNER ioctl like how io_uring does a copy_process against its > userspace app. This allows the vhost layer's worker threads to inherit > cgroups, namespaces, address space, etc and this worker thread will also > be accounted for against that owner/parent process's RLIMIT_NPROC limit. > > If you are not familiar with qemu and vhost here is more detailed > problem description: > > Qemu will create vhost devices in the kernel which perform network, SCSI, > etc IO and management operations from worker threads created by the > kthread API. Because the kthread API does a copy_process on the kthreadd > thread, the vhost layer has to use kthread_use_mm to access the Qemu > thread's memory and cgroup_attach_task_all to add itself to the Qemu > thread's cgroups. > > The problem with this approach is that we then have to add new functions/ > args/functionality for every thing we want to inherit. I started doing > that here: > > https://lkml.org/lkml/2021/6/23/1233 > > for the RLIMIT_NPROC check, but it seems it might be easier to just > inherit everything from the beginning, becuase I'd need to do something > like that patch several times. > > V10: > - Eric's cleanup patches my vhost flush cleanup patches are merged > upstream, so rebase against Linus's tree which has everything. > V9: > - Rebase against Eric's kthread-cleanups-for-v5.19 branch. Drop patches > no longer needed due to kernel clone arg and pf io worker patches in that > branch. > V8: > - Fix kzalloc GFP use. > - Fix email subject version number. > V7: > - Drop generic user_worker_* helpers and replace with vhost_task specific > ones. > - Drop autoreap patch. Use kernel_wait4 instead. > - Fix issue where vhost.ko could be removed while the worker function is > still running. > V6: > - Rename kernel_worker to user_worker and fix prefixes. > - Add better patch descriptions. > V5: > - Handle kbuild errors by building patchset against current kernel that > has all deps merged. Also add patch to remove create_io_thread code as > it's not used anymore. > - Rebase patchset against current kernel and handle a new vm PF_IO_WORKER > case added in 5.16-rc1. > - Add PF_USER_WORKER flag so we can check it later after the initial > thread creation for the wake up, vm and singal cses. > - Added patch to auto reap the worker thread. > V4: > - Drop NO_SIG patch and replaced with Christian's SIG_IGN patch. > - Merged Christian's kernel_worker_flags_valid helpers into patch 5 that > added the new kernel worker functions. > - Fixed extra "i" issue. > - Added PF_USER_WORKER flag and added check that kernel_worker_start users > had that flag set. Also dropped patches that passed worker flags to > copy_thread and replaced with PF_USER_WORKER check. > V3: > - Add parentheses in p->flag and work_flags check in copy_thread. > - Fix check in arm/arm64 which was doing the reverse of other archs > where it did likely(!flags) instead of unlikely(flags). > V2: > - Rename kernel_copy_process to kernel_worker. > - Instead of exporting functions, make kernel_worker() a proper > function/API that does common work for the caller. > - Instead of adding new fields to kernel_clone_args for each option > make it flag based similar to CLONE_*. > - Drop unused completion struct in vhost. > - Fix compile warnings by merging vhost cgroup cleanup patch and > vhost conversion patch. > > > > _______________________________________________ > Virtualization mailing list > Virtualization at lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/virtualization