Messages in this thread | | | From | Valentin Schneider <> | Subject | Re: [PATCH v4 3/4] workqueue: Convert the idle_timer to a delayed_work | Date | Wed, 02 Nov 2022 17:13:22 +0000 |
| |
On 31/10/22 08:49, Tejun Heo wrote: > Hello, > > On Tue, Oct 04, 2022 at 04:05:20PM +0100, Valentin Schneider wrote: >> +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); >> >> 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 */ >> + /* idle_list is kept in LIFO order, check the oldest entry */ >> 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); > > So, one thing which bothers me is that the idle timer is supposed to go off > spuriously periodically. The idea being that letting it expire spuriously > should usually be cheaper than trying to update it constantly as workers > wake up and sleep. Converting the timer to a delayed work makes spurious > wakeups significantly more expensive tho as it's now a full scheduling > event. >
Right.
> Can we separate the timer and work item out so that we can kick off the work > item iff there actually are tasks to kill? >
One thing I can try to have a DIY delayed_work where the timer callback doesn't just queue the work but first checks for too_many_workers(). This will mostly likely result in a different behaviour as worker deletion will then involve two pool->lock regions, but this will still catch long-idling workers.
> Thanks. > > -- > tejun
| |