xfs
[Top] [All Lists]

Re: [PATCH workqueue wq/for-3.19-fixes] workqueue: fix subtle pool manag

To: Tejun Heo <tj@xxxxxxxxxx>, Eric Sandeen <sandeen@xxxxxxxxxxx>
Subject: Re: [PATCH workqueue wq/for-3.19-fixes] workqueue: fix subtle pool management issue which can stall whole worker_pool
From: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>
Date: Mon, 19 Jan 2015 10:15:41 +0800
Cc: Eric Sandeen <sandeen@xxxxxxxxxx>, xfs-oss <xfs@xxxxxxxxxxx>, <linux-kernel@xxxxxxxxxxxxxxx>
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20150116193239.GA3715@xxxxxxxxxxxxxx>
References: <54B429EB.9050807@xxxxxxxxxxx> <20150112225314.GC22156@xxxxxxxxxxxxxx> <54B454E2.70707@xxxxxxxxxxx> <20150112233755.GD22156@xxxxxxxxxxxxxx> <54B56D2B.6090401@xxxxxxxxxxx> <20150113201900.GA9489@xxxxxxxxxxxxxx> <54B58041.9070502@xxxxxxxxxxx> <20150113204633.GC9489@xxxxxxxxxxxxxx> <54B5A313.2030300@xxxxxxxxxxx> <20150113233552.GH9489@xxxxxxxxxxxxxx> <20150116193239.GA3715@xxxxxxxxxxxxxx>
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.3.0
On 01/17/2015 03:32 AM, Tejun Heo wrote:
>>From 29187a9eeaf362d8422e62e17a22a6e115277a49 Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@xxxxxxxxxx>
> Date: Fri, 16 Jan 2015 14:21:16 -0500
> 
> A worker_pool's forward progress is guaranteed by the fact that the
> last idle worker assumes the manager role to create more workers and
> summon the rescuers if creating workers doesn't succeed in timely
> manner before proceeding to execute work items.
> 
> This manager role is implemented in manage_workers(), which indicates
> whether the worker may proceed to work item execution with its return
> value.  This is necessary because multiple workers may contend for the
> manager role, and, if there already is a manager, others should
> proceed to work item execution.
> 
> Unfortunately, the function also indicates that the worker may proceed
> to work item execution if need_to_create_worker() is false at the head
> of the function.  need_to_create_worker() tests the following
> conditions.
> 
>       pending work items && !nr_running && !nr_idle

> 
> The first and third conditions are protected by pool->lock and thus
> won't change while holding pool->lock; however, nr_running can change
> asynchronously as other workers block and resume and while it's likely
> to be zero, as someone woke this worker up in the first place, some
> other workers could have become runnable inbetween making it non-zero.

I had sent a patch similar:
https://lkml.org/lkml/2014/7/10/446

It was shame for me that I did not think deep enough that time.

> 
> If this happens, manage_worker() could return false even with zero
> nr_idle making the worker, the last idle one, proceed to execute work
> items.  If then all workers of the pool end up blocking on a resource
> which can only be released by a work item which is pending on that
> pool, the whole pool can deadlock as there's no one to create more
> workers or summon the rescuers.

How nr_running is decreased to zero in this case?

( The decreasing of nr_running is also protected by "X" )

( I just checked the cpu-hotplug code again ... find no suspect)

> -static bool maybe_create_worker(struct worker_pool *pool)
> +static void maybe_create_worker(struct worker_pool *pool)
>  __releases(&pool->lock)
>  __acquires(&pool->lock)
>  {
> -     if (!need_to_create_worker(pool))
> -             return false;


It only returns false here, if there ware bug, the bug would be here.
But it still holds the pool->lock and no releasing form the beginning to here)

My doubt might be wrong, but at least it is a good cleanup
Acked-by: Lai Jiangshan <laijs@xxxxxxxxxxxxxx>

Thanks
Lai

>  restart:
>       spin_unlock_irq(&pool->lock);
>  
> @@ -1877,7 +1871,6 @@ restart:
>        */
>       if (need_to_create_worker(pool))
>               goto restart;
> -     return true;
>  }
>  
>  /**
> @@ -1897,16 +1890,14 @@ restart:
>   * multiple times.  Does GFP_KERNEL allocations.
>   *
>   * Return:
> - * %false if the pool don't need management and the caller can safely start
> - * processing works, %true indicates that the function released pool->lock
> - * and reacquired it to perform some management function and that the
> - * conditions that the caller verified while holding the lock before
> - * calling the function might no longer be true.
> + * %false if the pool doesn't need management and the caller can safely
> + * start processing works, %true if management function was performed and
> + * the conditions that the caller verified before calling the function may
> + * no longer be true.
>   */
>  static bool manage_workers(struct worker *worker)
>  {
>       struct worker_pool *pool = worker->pool;
> -     bool ret = false;
>  
>       /*
>        * Anyone who successfully grabs manager_arb wins the arbitration
> @@ -1919,12 +1910,12 @@ static bool manage_workers(struct worker *worker)
>        * actual management, the pool may stall indefinitely.
>        */
>       if (!mutex_trylock(&pool->manager_arb))
> -             return ret;
> +             return false;
>  
> -     ret |= maybe_create_worker(pool);
> +     maybe_create_worker(pool);
>  
>       mutex_unlock(&pool->manager_arb);
> -     return ret;
> +     return true;
>  }
>  
>  /**
> 

<Prev in Thread] Current Thread [Next in Thread>