Messages in this thread Patch in this message | | | Date | Wed, 30 Nov 2022 11:06:20 -1000 | From | Tejun Heo <> | Subject | Re: [PATCH v6 3/4] workqueue: Convert the idle_timer to a timer + work_struct |
| |
On Mon, Nov 28, 2022 at 06:31:08PM +0000, Valentin Schneider wrote: > @@ -1806,7 +1808,9 @@ 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)) > + if (too_many_workers(pool) && > + !timer_pending(&pool->idle_timer) && > + !work_pending(&pool->idle_cull_work))
Just checking the timer should be enough here, I think.
> mod_timer(&pool->idle_timer, jiffies + IDLE_WORKER_TIMEOUT); > > /* Sanity check nr_running. */ > @@ -2019,17 +2023,56 @@ static void destroy_worker(struct worker *worker) > wake_up_process(worker->task); > } > > +/* > + * idle_worker_timeout - check if some idle workers can now be deleted.
Might as well turn it into a proper function comment starting w/ "/**" and with argument list.
> + * > + * The timer is armed in worker_enter_idle(). Note that it isn't disarmed in > + * worker_leave_idle(), as a worker flicking between idle and active while its > + * pool is at the too_many_workers() tipping point would cause too much timer > + * housekeeping overhead. Since IDLE_WORKER_TIMEOUT is long enough, we just let > + * it expire and re-evaluate things from there. > + */ > static void idle_worker_timeout(struct timer_list *t) > { > struct worker_pool *pool = from_timer(pool, t, idle_timer); > + bool do_cull = false; > + > + if (work_pending(&pool->idle_cull_work)) > + return; > > raw_spin_lock_irq(&pool->lock); > > - while (too_many_workers(pool)) { > + if (too_many_workers(pool)) { > struct worker *worker; > unsigned long expires; > > /* 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; > + do_cull = !time_before(jiffies, expires); > + > + if (!do_cull) > + mod_timer(&pool->idle_timer, expires); > + } > + raw_spin_unlock_irq(&pool->lock); > + > + if (do_cull) > + queue_work(system_unbound_wq, &pool->idle_cull_work); > +} > + > +/* > + * idle_cull_fn - cull workers that have been idle for too long. > + */
Please turn it into a full function comment or drop the wings (ie. make it an one-liner).
> +static void idle_cull_fn(struct work_struct *work) > +{ > + struct worker_pool *pool = container_of(work, struct worker_pool, idle_cull_work); > + > + raw_spin_lock_irq(&pool->lock); > + > + while (too_many_workers(pool)) { > + struct worker *worker; > + unsigned long expires; > +
Other than that, looks great to me.
Thanks.
-- tejun
| |