linux-kernel - Re: [PATCH v4 3/4] workqueue: Convert the idle_timer to a delayed

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 31 Oct 2022 08:49:16 -1000
From:   Tejun Heo <tj@...nel.org>
To:     Valentin Schneider <vschneid@...hat.com>
Cc:     linux-kernel@...r.kernel.org,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Frederic Weisbecker <frederic@...nel.org>,
        Juri Lelli <juri.lelli@...hat.com>,
        Phil Auld <pauld@...hat.com>,
        Marcelo Tosatti <mtosatti@...hat.com>
Subject: Re: [PATCH v4 3/4] workqueue: Convert the idle_timer to a
 delayed_work

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