Re: [PATCH 1/2] workqueue: pin the pool while it is managing

[Tejun Heo <tj@kernel.org>]

Hello,

On Thu, May 28, 2020 at 03:06:55AM +0000, Lai Jiangshan wrote:
> @@ -2129,10 +2128,21 @@ __acquires(&pool->lock)
>  static bool manage_workers(struct worker *worker)
>  {
>  	struct worker_pool *pool = worker->pool;
> +	struct work_struct *work = list_first_entry(&pool->worklist,
> +					struct work_struct, entry);

I'm not sure about this. It's depending on an external condition (active
work item) which isn't obvious and when that condition breaks the resulting
bug will be one which is difficult to reproduce. Adding to that, pwq isn't
even the object this code path is interested in, which is the cause of the
previous problem too.

> @@ -2140,7 +2150,7 @@ static bool manage_workers(struct worker *worker)
>  
>  	pool->manager = NULL;
>  	pool->flags &= ~POOL_MANAGER_ACTIVE;
> -	wake_up(&wq_manager_wait);
> +	put_pwq(pwq);

So, this works only because pwq release bounces through another work item,
so even if a worker of the pool which is currently being destroyed initiates
the release of the containing pool, it still works out, because by the time
the async release path kicks in and grabs the pool lock, everything should
be idle.

I get that this can work but it's sitting on top of a bunch of subtleties.
The current code is more verbose but also significantly more explicit and
straight-forward. I'd rather keep the current behavior unless we can get rid
of the subtleties.

Thanks.

-- 
tejun