linux-kernel - Re: [PATCH workqueue/for-4.14-fixes] workqueue: replace pool->manager

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20171009144702.GB3301751@devbig577.frc2.facebook.com>
Date:   Mon, 9 Oct 2017 07:47:02 -0700
From:   Tejun Heo <tj@...nel.org>
To:     Boqun Feng <boqun.feng@...il.com>
Cc:     linux-kernel@...r.kernel.org, Josef Bacik <josef@...icpanda.com>,
        Peter Zijlstra <peterz@...radead.org>,
        Lai Jiangshan <jiangshanlai@...il.com>
Subject: Re: [PATCH workqueue/for-4.14-fixes] workqueue: replace
 pool->manager_arb mutex with a flag

Hello, Boqun.

On Mon, Oct 09, 2017 at 10:21:17PM +0800, Boqun Feng wrote:
> > +static DECLARE_WAIT_QUEUE_HEAD(wq_manager_wait); /* wait for manager to go away */
> 
> I think this wait_queue_head better be a per-pool one rather than shared
> among pools?

It should be fine either way.  All the involved operations are pretty
low frequency.

> > @@ -3338,7 +3332,10 @@ static void put_unbound_pool(struct worker_pool *pool)
> >  	if (pool->detach_completion)
> >  		wait_for_completion(pool->detach_completion);
> >  
> > -	mutex_unlock(&pool->manager_arb);
> > +	spin_lock_irq(&pool->lock);
> > +	pool->flags &= ~POOL_MANAGER_ACTIVE;
> > +	wake_up(&wq_manager_wait);
> > +	spin_unlock_irq(&pool->lock);
> >  
> 
> Is the above code necesarry? IIUC, we are going to free the pool
> entirely, so whether manager is active is pointless here and no one is
> waiting for the ->flags of *this* pool to be !POOL_MANAGER_ACTIVE.
> 
> Am I missing something subtle here?

Ah, true.  I'll drop the above chunk.

Thanks.

-- 
tejun