Messages in this thread Patch in this message | ![/](/images/icornerl.gif) | | From | Valentin Schneider <> | Subject | [RFC PATCH v2 1/2] workqueue: Unbind workers before sending them to exit() | Date | Wed, 27 Jul 2022 12:53:26 +0100 |
| |
It has been reported that isolated CPUs can suffer from interference due to per-CPU kworkers waking up just to die.
A surge of workqueue activity during initial setup of a latency-sensitive application (refresh_vm_stats() being one of the culprits) can cause extra per-CPU kworkers to be spawned. Then, said latency-sensitive task can be running merrily on an isolated CPU only to be interrupted sometime later by a kworker marked for death (cf. IDLE_WORKER_TIMEOUT, 5 minutes after last kworker activity).
Prevent this by affining kworkers to the wq_unbound_cpumask (which doesn't contain isolated CPUs, cf. HK_TYPE_WQ) before waking them up after marking them with WORKER_DIE.
Changing the affinity does require a sleepable context, so get rid of the pool->idle_timer and use a delayed_work instead.
Signed-off-by: Valentin Schneider <vschneid@redhat.com> --- kernel/workqueue.c | 109 +++++++++++++++++++++++++++++++++------------ 1 file changed, 81 insertions(+), 28 deletions(-)
diff --git a/kernel/workqueue.c b/kernel/workqueue.c index 1ea50f6be843..27642166dcc5 100644 --- a/kernel/workqueue.c +++ b/kernel/workqueue.c @@ -167,9 +167,9 @@ struct worker_pool { int nr_workers; /* L: total number of workers */ int nr_idle; /* L: currently idle workers */ - struct list_head idle_list; /* L: list of idle workers */ - struct timer_list idle_timer; /* L: worker idle timeout */ - struct timer_list mayday_timer; /* L: SOS timer for workers */ + struct list_head idle_list; /* L: list of idle workers */ + struct delayed_work idle_reaper_work; /* L: worker idle timeout */ + struct timer_list mayday_timer; /* L: SOS timer for workers */ /* a workers is either on busy_hash or idle_list, or the manager */ DECLARE_HASHTABLE(busy_hash, BUSY_WORKER_HASH_ORDER); @@ -1806,8 +1806,10 @@ static void worker_enter_idle(struct worker *worker) /* idle_list is LIFO */ list_add(&worker->entry, &pool->idle_list); - if (too_many_workers(pool) && !timer_pending(&pool->idle_timer)) - mod_timer(&pool->idle_timer, jiffies + IDLE_WORKER_TIMEOUT); + if (too_many_workers(pool) && !delayed_work_pending(&pool->idle_reaper_work)) + mod_delayed_work(system_unbound_wq, + &pool->idle_reaper_work, + IDLE_WORKER_TIMEOUT); /* Sanity check nr_running. */ WARN_ON_ONCE(pool->nr_workers == pool->nr_idle && pool->nr_running); @@ -1972,9 +1974,29 @@ static struct worker *create_worker(struct worker_pool *pool) return NULL; } +static void unbind_worker(struct worker *worker) +{ + kthread_set_per_cpu(worker->task, -1); + WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, wq_unbound_cpumask) < 0); +} + +static void rebind_worker(struct worker *worker, struct worker_pool *pool) +{ + kthread_set_per_cpu(worker->task, pool->cpu); + WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, pool->attrs->cpumask) < 0); +} + +static void reap_worker(struct worker *worker) +{ + list_del_init(&worker->entry); + unbind_worker(worker); + wake_up_process(worker->task); +} + /** * destroy_worker - destroy a workqueue worker * @worker: worker to be destroyed + * @list: transfer worker away from its pool->idle_list and into list * * Destroy @worker and adjust @pool stats accordingly. The worker should * be idle. @@ -1982,7 +2004,7 @@ static struct worker *create_worker(struct worker_pool *pool) * CONTEXT: * raw_spin_lock_irq(pool->lock). */ -static void destroy_worker(struct worker *worker) +static void destroy_worker(struct worker *worker, struct list_head *list) { struct worker_pool *pool = worker->pool; @@ -1997,34 +2019,64 @@ static void destroy_worker(struct worker *worker) pool->nr_workers--; pool->nr_idle--; - list_del_init(&worker->entry); + list_del(&worker->entry); worker->flags |= WORKER_DIE; - wake_up_process(worker->task); + + list_add(&worker->entry, list); } -static void idle_worker_timeout(struct timer_list *t) +/** + * idle_reaper_fn - reap workers that have been idle for too long. + * + * Unbinding marked-for-destruction workers requires a sleepable context, as + * changing a task's affinity is not an atomic operation, and we don't want + * to disturb isolated CPUs IDLE_WORKER_TIMEOUT in the future just for a kworker + * to do_exit(). + * + * Percpu kworkers should meet the conditions for the affinity change to not + * block (not migration-disabled and not running), but there is no *hard* + * guarantee that they are not running when we get here. + * + * The delayed_work is only ever modified under raw_spin_lock_irq(pool->lock). + */ +static void idle_reaper_fn(struct work_struct *work) { - struct worker_pool *pool = from_timer(pool, t, idle_timer); + struct delayed_work *dwork = to_delayed_work(work); + struct worker_pool *pool = container_of(dwork, struct worker_pool, idle_reaper_work); + struct list_head reaplist; + struct worker *worker, *tmp; + + INIT_LIST_HEAD(&reaplist); raw_spin_lock_irq(&pool->lock); while (too_many_workers(pool)) { - struct worker *worker; unsigned long expires; + unsigned long now = jiffies; /* idle_list is kept in LIFO order, check the last one */ worker = list_entry(pool->idle_list.prev, struct worker, entry); expires = worker->last_active + IDLE_WORKER_TIMEOUT; - if (time_before(jiffies, expires)) { - mod_timer(&pool->idle_timer, expires); + /* + * Careful: queueing a work item from here can and will cause a + * self-deadlock when dealing with an unbound pool. However, + * here the delay *cannot* be zero and *has* to be in the + * future, which works. + */ + if (time_before(now, expires)) { + mod_delayed_work(system_unbound_wq, + &pool->idle_reaper_work, + expires - now); break; } - destroy_worker(worker); + destroy_worker(worker, &reaplist); } - raw_spin_unlock_irq(&pool->lock); + + list_for_each_entry_safe(worker, tmp, &reaplist, entry) + reap_worker(worker); } static void send_mayday(struct work_struct *work) @@ -3454,7 +3506,7 @@ static int init_worker_pool(struct worker_pool *pool) INIT_LIST_HEAD(&pool->idle_list); hash_init(pool->busy_hash); - timer_setup(&pool->idle_timer, idle_worker_timeout, TIMER_DEFERRABLE); + INIT_DEFERRABLE_WORK(&pool->idle_reaper_work, idle_reaper_fn); timer_setup(&pool->mayday_timer, pool_mayday_timeout, 0); @@ -3559,7 +3611,10 @@ static bool wq_manager_inactive(struct worker_pool *pool) static void put_unbound_pool(struct worker_pool *pool) { DECLARE_COMPLETION_ONSTACK(detach_completion); - struct worker *worker; + struct list_head dlist; + struct worker *worker, *tmp; + + INIT_LIST_HEAD(&dlist); lockdep_assert_held(&wq_pool_mutex); @@ -3588,10 +3643,13 @@ static void put_unbound_pool(struct worker_pool *pool) pool->flags |= POOL_MANAGER_ACTIVE; while ((worker = first_idle_worker(pool))) - destroy_worker(worker); + destroy_worker(worker, &dlist); WARN_ON(pool->nr_workers || pool->nr_idle); raw_spin_unlock_irq(&pool->lock); + list_for_each_entry_safe(worker, tmp, &dlist, entry) + reap_worker(worker); + mutex_lock(&wq_pool_attach_mutex); if (!list_empty(&pool->workers)) pool->detach_completion = &detach_completion; @@ -3601,7 +3659,7 @@ static void put_unbound_pool(struct worker_pool *pool) wait_for_completion(pool->detach_completion); /* shut down the timers */ - del_timer_sync(&pool->idle_timer); + cancel_delayed_work_sync(&pool->idle_reaper_work); del_timer_sync(&pool->mayday_timer); /* RCU protected to allow dereferences from get_work_pool() */ @@ -4999,10 +5057,8 @@ static void unbind_workers(int cpu) raw_spin_unlock_irq(&pool->lock); - for_each_pool_worker(worker, pool) { - kthread_set_per_cpu(worker->task, -1); - WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, wq_unbound_cpumask) < 0); - } + for_each_pool_worker(worker, pool) + unbind_worker(worker); mutex_unlock(&wq_pool_attach_mutex); } @@ -5027,11 +5083,8 @@ static void rebind_workers(struct worker_pool *pool) * of all workers first and then clear UNBOUND. As we're called * from CPU_ONLINE, the following shouldn't fail. */ - for_each_pool_worker(worker, pool) { - kthread_set_per_cpu(worker->task, pool->cpu); - WARN_ON_ONCE(set_cpus_allowed_ptr(worker->task, - pool->attrs->cpumask) < 0); - } + for_each_pool_worker(worker, pool) + rebind_worker(worker, pool); raw_spin_lock_irq(&pool->lock); -- 2.31.1
| ![\](/images/icornerr.gif) |