lkml.org 
[lkml]   [2013]   [Jan]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    Subject[PATCH v2] async: fix __lowest_in_progress()
    083b804c4d3e1e3d0eace56bdbc0f674946d2847 ("async: use workqueue for
    worker pool") made it possible that async jobs are moved from pending
    to running out-of-order. While pending async jobs will be queued and
    dispatched for execution in the same order, nothing guarantees they'll
    enter "1) move self to the running queue" of async_run_entry_fn() in
    the same order.

    Before the conversion, async implemented its own worker pool. An
    async worker, upon being woken up, fetches the first item from the
    pending list, which kept the executing lists sorted. The conversion
    to workqueue was done by adding work_struct to each async_entry and
    async just schedules the work item. The queueing and dispatching of
    such work items are still in order but now each worker thread is
    associated with a specific async_entry and moves that specific
    async_entry to the executing list. So, depending on which worker
    reaches that point earlier, which is non-deterministic, we may end up
    moving an async_entry with larger cookie before one with smaller one.

    This broke __lowest_in_progress(). running->domain may not be
    properly sorted and is not guaranteed to contain lower cookies than
    pending list when not empty. Fix it by ensuring sort-inserting to the
    running list and always looking at both pending and running when
    trying to determine the lowest cookie.

    Over time, the async synchronization implementation became quite
    messy. We better restructure it such that each async_entry is linked
    to two lists - one global and one per domain - and not move it when
    execution starts. There's no reason to distinguish pending and
    running. They behave the same for synchronization purposes.

    v2: Description updated to better explain why it's broken.

    Signed-off-by: Tejun Heo <tj@kernel.org>
    Cc: Arjan van de Ven <arjan@linux.intel.com>
    Cc: stable@vger.kernel.org
    ---
    Linus, I've updated the description to better explain why it's broken.
    The code is ugly but cleanup patches are already ready, so it will be
    cleaned up during 3.9-rc1. How should this be routed?

    Thanks.

    kernel/async.c | 27 ++++++++++++++++++++-------
    1 file changed, 20 insertions(+), 7 deletions(-)

    --- a/kernel/async.c
    +++ b/kernel/async.c
    @@ -86,18 +86,27 @@ static atomic_t entry_count;
    */
    static async_cookie_t __lowest_in_progress(struct async_domain *running)
    {
    + async_cookie_t first_running = next_cookie; /* infinity value */
    + async_cookie_t first_pending = next_cookie; /* ditto */
    struct async_entry *entry;

    + /*
    + * Both running and pending lists are sorted but not disjoint.
    + * Take the first cookies from both and return the min.
    + */
    if (!list_empty(&running->domain)) {
    entry = list_first_entry(&running->domain, typeof(*entry), list);
    - return entry->cookie;
    + first_running = entry->cookie;
    }

    - list_for_each_entry(entry, &async_pending, list)
    - if (entry->running == running)
    - return entry->cookie;
    + list_for_each_entry(entry, &async_pending, list) {
    + if (entry->running == running) {
    + first_pending = entry->cookie;
    + break;
    + }
    + }

    - return next_cookie; /* "infinity" value */
    + return min(first_running, first_pending);
    }

    static async_cookie_t lowest_in_progress(struct async_domain *running)
    @@ -118,13 +127,17 @@ static void async_run_entry_fn(struct wo
    {
    struct async_entry *entry =
    container_of(work, struct async_entry, work);
    + struct async_entry *pos;
    unsigned long flags;
    ktime_t uninitialized_var(calltime), delta, rettime;
    struct async_domain *running = entry->running;

    - /* 1) move self to the running queue */
    + /* 1) move self to the running queue, make sure it stays sorted */
    spin_lock_irqsave(&async_lock, flags);
    - list_move_tail(&entry->list, &running->domain);
    + list_for_each_entry_reverse(pos, &running->domain, list)
    + if (entry->cookie < pos->cookie)
    + break;
    + list_move_tail(&entry->list, &pos->list);
    spin_unlock_irqrestore(&async_lock, flags);

    /* 2) run (and print duration) */

    \
     
     \ /
      Last update: 2013-01-23 02:02    [W:4.456 / U:0.896 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site