Mike Christie <michael.christie at oracle.com> writes:> The following patches made over Linus's tree, 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.I read through the code and I don't see why you want to make these almost threads of a process not actually threads of that process (like the io_uring threads are). As a separate process there are many things that will continue to be disjoint. If the thread changes cgroups for example your new process won't follow. If you want them to be actually processes with an lifetime independent of the creating process I expect you want to reparent them to the local init process. Just so they don't confuse the process tree. Plus init processes know how to handle unexpected children. What are the semantics you are aiming for? I can see sense in generalizing some of the pieces of create_io_thread but I think generalizing create_io_thread itself is premature. The code lives in kernel/fork.c because it is a very special thing that we want to keep our eyes on. Some of your generalization makes it much more difficult to tell what your code is going to use because you remove hard codes that are there to simplify the analysis of the situation. My gut says adding a new create_vhost_worker and putting that in kernel/fork.c is a lot safer and will allow much better code analysis. If there a really are commonalities between creating a userspace process that runs completely in the kernel and creating an additional userspace thread we refactor the code and simplify things. I am especially nervous about generalizing the io_uring code as it's signal handling just barely works, and any generalization will cause it to break. So you are in the process of generalizing code that simply can not handle the general case. That scares me. Eric> > 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
michael.christie at oracle.com
2021-Dec-17 22:08 UTC
[PATCH V6 01/10] Use copy_process in vhost layer
On 12/17/21 1:26 PM, Eric W. Biederman wrote:> Mike Christie <michael.christie at oracle.com> writes: > >> The following patches made over Linus's tree, 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://urldefense.com/v3/__https://lkml.org/lkml/2021/6/23/1233__;!!ACWV5N9M2RV99hQ!eIaEe9V8mCgGU6vyvaWTKGi3Zlnz0rgk5Y-0nsBXRbsuVZsM8lYfHr8I8GRuoLYPYrOB$ >> >> 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. > > I read through the code and I don't see why you want to make these > almost threads of a process not actually threads of that process > (like the io_uring threads are). > > As a separate process there are many things that will continue to be > disjoint. If the thread changes cgroups for example your new process > won't follow. > > If you want them to be actually processes with an lifetime independent > of the creating process I expect you want to reparent them to the local > init process. Just so they don't confuse the process tree. Plus init > processes know how to handle unexpected children. > > What are the semantics you are aiming for? >Hi Eric, Right now, for vhost we need the thread being created: 1. added to the caller's cgroup. 2. to share the mm struct with the caller. 3. to be accounted for under the caller's nproc rlimit value. For 1 and 2, we have cgroup_attach_task_all and get_task_mm already. This patchset started with me just trying to handle #3 by modifying kthreads like here: https://lkml.org/lkml/2021/6/23/1234 So we can use kthreads and the existing helpers and add: A. a ucounts version of the above patches in the link or B. a helper that does something like copy_process's use of is_ucounts_overlimit and vhost can call that. instead of this patchset. Before we even get to the next section below, do you consider items 1 - 3 something we need an API based on copy_process for? Do you think I should just do A or B above, or do you have another idea? If so can we get agreement on that from everyone? I thought my patches in that link were a little hacky in how they passed around the user/creds info. I thought maybe it shouldn't be passed around like that, so switched to the copy_process based approach which did everything for me. And I thought io_uring needed something similar as us so I made it generic. I don't have a preference. You and Christian are the experts, so I'll leave it to you guys.> I can see sense in generalizing some of the pieces of create_io_thread > but I think generalizing create_io_thread itself is premature. The code > lives in kernel/fork.c because it is a very special thing that we want > to keep our eyes on. > > Some of your generalization makes it much more difficult to tell what > your code is going to use because you remove hard codes that are there > to simplify the analysis of the situation. > > My gut says adding a new create_vhost_worker and putting that in > kernel/fork.c is a lot safer and will allow much better code analysis. > > If there a really are commonalities between creating a userspace process > that runs completely in the kernel and creating an additional userspace > thread we refactor the code and simplify things. > > I am especially nervous about generalizing the io_uring code as it's > signal handling just barely works, and any generalization will cause it > to break. So you are in the process of generalizing code that simply > can not handle the general case. That scares me