Re: [PATCH v4 3/4] workqueue: Convert the idle_timer to a delayed_work

[Tejun Heo <tj@kernel.org>]

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.

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?

Thanks.

-- 
tejun