lkml.org 
[lkml]   [2015]   [Feb]   [17]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    From
    Subject[PATCH 3.12 056/122] workqueue: fix subtle pool management issue which can stall whole worker_pool
    Date
    From: Tejun Heo <tj@kernel.org>

    3.12-stable review patch. If anyone has any objections, please let me know.

    ===============

    commit 29187a9eeaf362d8422e62e17a22a6e115277a49 upstream.

    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.

    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.

    This patch fixes the problem by removing the early exit condition from
    maybe_create_worker() and making manage_workers() return false iff
    there's already another manager, which ensures that the last worker
    doesn't start executing work items.

    We can leave the early exit condition alone and just ignore the return
    value but the only reason it was put there is because the
    manage_workers() used to perform both creations and destructions of
    workers and thus the function may be invoked while the pool is trying
    to reduce the number of workers. Now that manage_workers() is called
    only when more workers are needed, the only case this early exit
    condition is triggered is rare race conditions rendering it pointless.

    Tested with simulated workload and modified workqueue code which
    trigger the pool deadlock reliably without this patch.

    tj: Updated to v3.14 where manage_workers() is responsible not only
    for creating more workers but also destroying surplus ones.
    maybe_create_worker() needs to keep its early exit condition to
    avoid creating a new worker when manage_workers() is called to
    destroy surplus ones. Other than that, the adaptabion is
    straight-forward. Both maybe_{create|destroy}_worker() functions
    are converted to return void and manage_workers() returns %false
    iff it lost manager arbitration.

    Signed-off-by: Tejun Heo <tj@kernel.org>
    Reported-by: Eric Sandeen <sandeen@sandeen.net>
    Link: http://lkml.kernel.org/g/54B019F4.8030009@sandeen.net
    Cc: Dave Chinner <david@fromorbit.com>
    Cc: Lai Jiangshan <laijs@cn.fujitsu.com>
    Signed-off-by: Jiri Slaby <jslaby@suse.cz>
    ---
    kernel/workqueue.c | 42 +++++++++++++-----------------------------
    1 file changed, 13 insertions(+), 29 deletions(-)

    diff --git a/kernel/workqueue.c b/kernel/workqueue.c
    index 3fafbbb31927..d10cc05bfbc6 100644
    --- a/kernel/workqueue.c
    +++ b/kernel/workqueue.c
    @@ -1954,17 +1954,13 @@ static void pool_mayday_timeout(unsigned long __pool)
    * spin_lock_irq(pool->lock) which may be released and regrabbed
    * multiple times. Does GFP_KERNEL allocations. Called only from
    * manager.
    - *
    - * Return:
    - * %false if no action was taken and pool->lock stayed locked, %true
    - * otherwise.
    */
    -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;
    + return;
    restart:
    spin_unlock_irq(&pool->lock);

    @@ -1981,7 +1977,7 @@ restart:
    start_worker(worker);
    if (WARN_ON_ONCE(need_to_create_worker(pool)))
    goto restart;
    - return true;
    + return;
    }

    if (!need_to_create_worker(pool))
    @@ -1998,7 +1994,7 @@ restart:
    spin_lock_irq(&pool->lock);
    if (need_to_create_worker(pool))
    goto restart;
    - return true;
    + return;
    }

    /**
    @@ -2011,15 +2007,9 @@ restart:
    * LOCKING:
    * spin_lock_irq(pool->lock) which may be released and regrabbed
    * multiple times. Called only from manager.
    - *
    - * Return:
    - * %false if no action was taken and pool->lock stayed locked, %true
    - * otherwise.
    */
    -static bool maybe_destroy_workers(struct worker_pool *pool)
    +static void maybe_destroy_workers(struct worker_pool *pool)
    {
    - bool ret = false;
    -
    while (too_many_workers(pool)) {
    struct worker *worker;
    unsigned long expires;
    @@ -2033,10 +2023,7 @@ static bool maybe_destroy_workers(struct worker_pool *pool)
    }

    destroy_worker(worker);
    - ret = true;
    }
    -
    - return ret;
    }

    /**
    @@ -2056,16 +2043,14 @@ static bool maybe_destroy_workers(struct worker_pool *pool)
    * 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;

    /*
    * Managership is governed by two mutexes - manager_arb and
    @@ -2089,7 +2074,7 @@ static bool manage_workers(struct worker *worker)
    * manager_mutex.
    */
    if (!mutex_trylock(&pool->manager_arb))
    - return ret;
    + return false;

    /*
    * With manager arbitration won, manager_mutex would be free in
    @@ -2099,7 +2084,6 @@ static bool manage_workers(struct worker *worker)
    spin_unlock_irq(&pool->lock);
    mutex_lock(&pool->manager_mutex);
    spin_lock_irq(&pool->lock);
    - ret = true;
    }

    pool->flags &= ~POOL_MANAGE_WORKERS;
    @@ -2108,12 +2092,12 @@ static bool manage_workers(struct worker *worker)
    * Destroy and then create so that may_start_working() is true
    * on return.
    */
    - ret |= maybe_destroy_workers(pool);
    - ret |= maybe_create_worker(pool);
    + maybe_destroy_workers(pool);
    + maybe_create_worker(pool);

    mutex_unlock(&pool->manager_mutex);
    mutex_unlock(&pool->manager_arb);
    - return ret;
    + return true;
    }

    /**
    --
    2.2.2


    \
     
     \ /
      Last update: 2015-02-17 13:21    [W:4.418 / U:0.052 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site