On 3/8/23 4:42?AM, Daniel Dao wrote: > Hi all, > > We encountered EINVAL when enabling cpuset in cgroupv2 when io_uring > worker threads are running. Here are the steps to reproduce the failure > on kernel 6.1.14: > > 1. Remove cpuset from subtree_control > > > for d in $(find /sys/fs/cgroup/ -maxdepth 1 -type d); do echo > '-cpuset' | sudo tee -a $d/cgroup.subtree_control; done > > cat /sys/fs/cgroup/cgroup.subtree_control > cpu io memory pids > > 2. Run any applications that utilize the uring worker thread pool. I used > https://github.com/cloudflare/cloudflare-blog/tree/master/2022-02-io_uring-worker-pool > > > cargo run -- -a -w 2 -t 2 > > 3. Enabling cpuset will return EINVAL > > > echo '+cpuset' | sudo tee -a /sys/fs/cgroup/cgroup.subtree_control > +cpuset > tee: /sys/fs/cgroup/cgroup.subtree_control: Invalid argument > > We traced this down to task_can_attach that will return EINVAL when it > encounters > kthreads with PF_NO_SETAFFINITY, which io_uring worker threads have. > > This seems like an unexpected interaction when enabling cpuset for the subtrees > that contain kthreads. We are currently considering a workaround to try to > enable cpuset in root subtree_control before any io_uring applications > can start, > hence failure to enable cpuset is localized to only cgroup with > io_uring kthreads. > But this is cumbersome. > > Any suggestions would be very much appreciated. One important thing to note here is that io_uring workers are not kthreads, but like kthreads, they set PF_NO_SETAFFINITY which prevents userspace from moving them around. We do have an explicit API for setting the affinity of workers associated with an io_uring context, however. But you are not the first to come across this, and I'm pondering how we can improve this situation. io-wq blocks changing the CPU affinity because it organizes workers within a node, but this is purely an optimization and not integral to how it works. One thing we could do is simply check the cpumask of the worker after it went to sleep, and if it woke up due to a timeout (eg not to handle real work). That'd lazily drop workers that are now not affinitized correctly. With that, I think it'd be sane to drop the PF_NO_SETAFFINITY mask from the worker. Something like the below, would be great if you could test. diff --git a/io_uring/io-wq.c b/io_uring/io-wq.c index 411bb2d1acd4..669f50cb4e90 100644 --- a/io_uring/io-wq.c +++ b/io_uring/io-wq.c @@ -616,7 +616,7 @@ static int io_wqe_worker(void *data) struct io_wqe_acct *acct = io_wqe_get_acct(worker); struct io_wqe *wqe = worker->wqe; struct io_wq *wq = wqe->wq; - bool last_timeout = false; + bool exit_mask = false, last_timeout = false; char buf[TASK_COMM_LEN]; worker->flags |= (IO_WORKER_F_UP | IO_WORKER_F_RUNNING); @@ -632,8 +632,11 @@ static int io_wqe_worker(void *data) io_worker_handle_work(worker); raw_spin_lock(&wqe->lock); - /* timed out, exit unless we're the last worker */ - if (last_timeout && acct->nr_workers > 1) { + /* + * Last sleep timed out. Exit if we're not the last worker, + * or if someone modified our affinity. + */ + if (last_timeout && (exit_mask || acct->nr_workers > 1)) { acct->nr_workers--; raw_spin_unlock(&wqe->lock); __set_current_state(TASK_RUNNING); @@ -652,7 +655,11 @@ static int io_wqe_worker(void *data) continue; break; } - last_timeout = !ret; + if (!ret) { + last_timeout = true; + exit_mask = !cpumask_test_cpu(smp_processor_id(), + wqe->cpu_mask); + } } if (test_bit(IO_WQ_BIT_EXIT, &wq->state)) @@ -704,7 +711,6 @@ static void io_init_new_worker(struct io_wqe *wqe, struct io_worker *worker, tsk->worker_private = worker; worker->task = tsk; set_cpus_allowed_ptr(tsk, wqe->cpu_mask); - tsk->flags |= PF_NO_SETAFFINITY; raw_spin_lock(&wqe->lock); hlist_nulls_add_head_rcu(&worker->nulls_node, &wqe->free_list); -- Jens Axboe
