Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] vhost: missing cond_resched() and loop in vhost_task_fn() after task_work --
commit 97d56a24fed4c711b0eae24aaadc9595a3a04199 Author: Aamir Ahmed <[email protected]> vhost: Run TIF_NOTIFY_RESUME work in the vhost_task loop This patch runs pending TIF_NOTIFY_RESUME work via resume_user_mode_work() at the top of the vhost_task_fn() worker loop so that TWA_RESUME task_work, such as CFS bandwidth throttling, is processed for vhost worker threads. > diff --git a/kernel/vhost_task.c b/kernel/vhost_task.c > index 3717885a5992a..7fa96e1b85b66 100644 > --- a/kernel/vhost_task.c > +++ b/kernel/vhost_task.c > @@ -6,6 +6,7 @@ > #include <linux/completion.h> > #include <linux/sched/task.h> > #include <linux/sched/vhost_task.h> > +#include <linux/resume_user_mode.h> > #include <linux/sched/signal.h> > > enum vhost_task_flags { > @@ -31,6 +32,14 @@ static int vhost_task_fn(void *data) > for (;;) { > bool did_work; > > + /* > + * We never return to user mode, so TWA_RESUME task_work (e.g. > CFS > + * bandwidth throttling) would otherwise never run. Run it here, > + * where no locks are held, like io-wq workers do. > + */ > + if (test_thread_flag(TIF_NOTIFY_RESUME)) > + resume_user_mode_work(NULL); [Severity: High] Should this loop while TIF_NOTIFY_RESUME is set and call cond_resched() after resume_user_mode_work(NULL), similar to io_assign_current_work() in io_uring/io-wq.c: if (work) { io_run_task_work(); cond_resched(); } When resume_user_mode_work() runs task_work_run() -> throttle_cfs_rq_work() in kernel/sched/fair.c, throttle_cfs_rq_work() dequeues current from its cfs_rq (clearing p->se.on_rq), places it on cfs_rq->throttled_limbo_list, sets p->throttled = true, and only sets TIF_NEED_RESCHED via resched_curr(rq) without calling schedule(): dequeue_task_fair(rq, p, DEQUEUE_SLEEP | DEQUEUE_THROTTLE); list_add(&p->throttle_node, &cfs_rq->throttled_limbo_list); p->throttled = true; resched_curr(rq); Without a cond_resched() and loop here, can several problems occur on CONFIG_PREEMPT_NONE or CONFIG_PREEMPT_VOLUNTARY kernels (or when a newly unthrottled task is immediately re-throttled on schedule-in)? 1. Because unlocking task_rq_lock at the end of throttle_cfs_rq_work() does not preempt current under CONFIG_PREEMPT_NONE or CONFIG_PREEMPT_VOLUNTARY, vhost_task_fn() proceeds directly into vtsk->fn(vtsk->data) while current is already on throttled_limbo_list with TIF_NEED_RESCHED pending. For example, when vtsk->fn is kvm_nx_huge_page_recovery_worker() calling kvm_recover_nx_huge_pages() in arch/x86/kvm/mmu/mmu.c, it acquires srcu_read_lock(&kvm->srcu) and kvm->mmu_lock and then checks need_resched(): rcu_idx = srcu_read_lock(&kvm->srcu); if (is_tdp_mmu) read_lock(&kvm->mmu_lock); else write_lock(&kvm->mmu_lock); ... if (need_resched() || rwlock_needbreak(&kvm->mmu_lock)) { ... if (is_tdp_mmu) cond_resched_rwlock_read(&kvm->mmu_lock); else cond_resched_rwlock_write(&kvm->mmu_lock); At that point it schedules out into throttled_limbo_list for the remainder of the CFS bandwidth period while holding srcu_read_lock(&kvm->srcu), stalling synchronize_srcu(&kvm->srcu). Similarly, on CONFIG_PREEMPT_NONE, vhost_run_work_list() executes work->fn(work) (which acquires vq->mutex) before reaching its own cond_resched() at the end of the work loop, and any CPU time spent before rescheduling is not charged by update_curr() because p->se.on_rq is already 0. 2. If vtsk->fn(vtsk->data) returns false (!did_work) while current is on throttled_limbo_list, vhost_task_fn() calls schedule() with state set to TASK_INTERRUPTIBLE. In __schedule() -> try_to_block_task() -> block_task() -> dequeue_task_fair() -> dequeue_throttled_task(): list_del_init(&p->throttle_node); /* task blocked after throttled */ if (flags & DEQUEUE_SLEEP) { p->throttled = false; return; } This removes the task from throttled_limbo_list and clears p->throttled, so a subsequent vhost_task_wake() while the cgroup is still throttled will enqueue the worker back onto the throttled cfs_rq. 3. Even if cond_resched() schedules the throttled task out, when the task is later unthrottled and scheduled back in via set_next_task_fair(), if sibling tasks have already consumed the newly replenished quota, set_next_task_fair() calls task_throttle_setup_work(p) to set TIF_NOTIFY_RESUME again without setting TIF_NEED_RESCHED: if (throttled) task_throttle_setup_work(p); If vhost_task_fn() only checks TIF_NOTIFY_RESUME with an if statement instead of a while loop around resume_user_mode_work(NULL) and cond_resched(), returning from cond_resched() will proceed into vtsk->fn() with the new TIF_NOTIFY_RESUME work still pending. -- Sashiko AI review ยท https://sashiko.dev/#/patchset/as8p251mb00012469e6ecbea2b7d87bb0c8...@as8p251mb0001.eurp251.prod.outlook.com?part=1
