Commit 05e514b1 authored by Paolo Bonzini's avatar Paolo Bonzini Committed by Stefan Hajnoczi
Browse files

AioContext: optimize clearing the EventNotifier 



It is pretty rare for aio_notify to actually set the EventNotifier.  It
can happen with worker threads such as thread-pool.c's, but otherwise it
should never be set thanks to the ctx->notify_me optimization.  The
previous patch, unfortunately, added an unconditional call to
event_notifier_test_and_clear; now add a userspace fast path that
avoids the call.

Note that it is not possible to do the same with event_notifier_set;
it would break, as proved (again) by the included formal model.

This patch survived over 3000 reboots on aarch64 KVM.

Signed-off-by: default avatarPaolo Bonzini <pbonzini@redhat.com>
Reviewed-by: default avatarFam Zheng <famz@redhat.com>
Tested-by: default avatarRichard W.M. Jones <rjones@redhat.com>
Message-id: 1437487673-23740-7-git-send-email-pbonzini@redhat.com
Signed-off-by: default avatarStefan Hajnoczi <stefanha@redhat.com>
parent 21a03d17

aio-posix.c

+1 −1
Original line number Diff line number Diff line
@@ -276,7 +276,7 @@ bool aio_poll(AioContext *ctx, bool blocking) 
        aio_context_acquire(ctx);

    }



    event_notifier_test_and_clear(&ctx->notifier);

    aio_notify_accept(ctx);



    /* if we have any readable fds, dispatch event */

    if (ret > 0) {

aio-win32.c

+1 −1
Original line number Diff line number Diff line
@@ -338,7 +338,7 @@ bool aio_poll(AioContext *ctx, bool blocking) 
        }



        if (first) {

            event_notifier_test_and_clear(&ctx->notifier);

            aio_notify_accept(ctx);

            progress |= aio_bh_poll(ctx);

            first = false;

        }

async.c

+9 −1
Original line number Diff line number Diff line
@@ -203,7 +203,7 @@ aio_ctx_check(GSource *source) 
    QEMUBH *bh;



    atomic_and(&ctx->notify_me, ~1);

    event_notifier_test_and_clear(&ctx->notifier);

    aio_notify_accept(ctx);



    for (bh = ctx->first_bh; bh; bh = bh->next) {

        if (!bh->deleted && bh->scheduled) {
@@ -267,6 +267,14 @@ void aio_notify(AioContext *ctx) 
    smp_mb();

    if (ctx->notify_me) {

        event_notifier_set(&ctx->notifier);

        atomic_mb_set(&ctx->notified, true);

    }

}



void aio_notify_accept(AioContext *ctx)

{

    if (atomic_xchg(&ctx->notified, false)) {

        event_notifier_test_and_clear(&ctx->notifier);

    }

}

docs/aio_notify_accept.promela

0 → 100644
+152 −0
Original line number Diff line number Diff line 
/*

 * This model describes the interaction between ctx->notified

 * and ctx->notifier.

 *

 * Author: Paolo Bonzini <pbonzini@redhat.com>

 *

 * This file is in the public domain.  If you really want a license,

 * the WTFPL will do.

 *

 * To verify the buggy version:

 *     spin -a -DBUG1 docs/aio_notify_bug.promela

 *     gcc -O2 pan.c

 *     ./a.out -a -f

 * (or -DBUG2)

 *

 * To verify the fixed version:

 *     spin -a docs/aio_notify_bug.promela

 *     gcc -O2 pan.c

 *     ./a.out -a -f

 *

 * Add -DCHECK_REQ to test an alternative invariant and the

 * "notify_me" optimization.

 */



int notify_me;

bool notified;

bool event;

bool req;

bool notifier_done;



#ifdef CHECK_REQ

#define USE_NOTIFY_ME 1

#else

#define USE_NOTIFY_ME 0

#endif



#ifdef BUG

#error Please define BUG1 or BUG2 instead.

#endif



active proctype notifier()

{

    do

        :: true -> {

            req = 1;

            if

               :: !USE_NOTIFY_ME || notify_me ->

#if defined BUG1

                   /* CHECK_REQ does not detect this bug! */

                   notified = 1;

                   event = 1;

#elif defined BUG2

                   if

                      :: !notified -> event = 1;

                      :: else -> skip;

                   fi;

                   notified = 1;

#else

                   event = 1;

                   notified = 1;

#endif

               :: else -> skip;

            fi

        }

        :: true -> break;

    od;

    notifier_done = 1;

}



#define AIO_POLL                                                    \

    notify_me++;                                                    \

    if                                                              \

        :: !req -> {                                                \

            if                                                      \

                :: event -> skip;                                   \

            fi;                                                     \

        }                                                           \

        :: else -> skip;                                            \

    fi;                                                             \

    notify_me--;                                                    \

                                                                    \

    atomic { old = notified; notified = 0; }                        \

    if                                                              \

       :: old -> event = 0;                                         \

       :: else -> skip;                                             \

    fi;                                                             \

                                                                    \

    req = 0;



active proctype waiter()

{

    bool old;



    do

       :: true -> AIO_POLL;

    od;

}



/* Same as waiter(), but disappears after a while.  */

active proctype temporary_waiter()

{

    bool old;



    do

       :: true -> AIO_POLL;

       :: true -> break;

    od;

}



#ifdef CHECK_REQ

never {

    do

        :: req -> goto accept_if_req_not_eventually_false;

        :: true -> skip;

    od;



accept_if_req_not_eventually_false:

    if

        :: req -> goto accept_if_req_not_eventually_false;

    fi;

    assert(0);

}



#else

/* There must be infinitely many transitions of event as long

 * as the notifier does not exit.

 *

 * If event stayed always true, the waiters would be busy looping.

 * If event stayed always false, the waiters would be sleeping

 * forever.

 */

never {

    do

        :: !event    -> goto accept_if_event_not_eventually_true;

        :: event     -> goto accept_if_event_not_eventually_false;

        :: true      -> skip;

    od;



accept_if_event_not_eventually_true:

    if

        :: !event && notifier_done  -> do :: true -> skip; od;

        :: !event && !notifier_done -> goto accept_if_event_not_eventually_true;

    fi;

    assert(0);



accept_if_event_not_eventually_false:

    if

        :: event     -> goto accept_if_event_not_eventually_false;

    fi;

    assert(0);

}

#endif

include/block/aio.h

+31 −1
Original line number Diff line number Diff line
@@ -99,7 +99,19 @@ struct AioContext { 
     */

    int walking_bh;



    /* Used for aio_notify.  */

    /* Used by aio_notify.

     *

     * "notified" is used to avoid expensive event_notifier_test_and_clear

     * calls.  When it is clear, the EventNotifier is clear, or one thread

     * is going to clear "notified" before processing more events.  False

     * positives are possible, i.e. "notified" could be set even though the

     * EventNotifier is clear.

     *

     * Note that event_notifier_set *cannot* be optimized the same way.  For

     * more information on the problem that would result, see "#ifdef BUG2"

     * in the docs/aio_notify_accept.promela formal model.

     */

    bool notified;

    EventNotifier notifier;



    /* Thread pool for performing work and receiving completion callbacks */
@@ -173,6 +185,24 @@ QEMUBH *aio_bh_new(AioContext *ctx, QEMUBHFunc *cb, void *opaque); 
 */

void aio_notify(AioContext *ctx);



/**

 * aio_notify_accept: Acknowledge receiving an aio_notify.

 *

 * aio_notify() uses an EventNotifier in order to wake up a sleeping

 * aio_poll() or g_main_context_iteration().  Calls to aio_notify() are

 * usually rare, but the AioContext has to clear the EventNotifier on

 * every aio_poll() or g_main_context_iteration() in order to avoid

 * busy waiting.  This event_notifier_test_and_clear() cannot be done

 * using the usual aio_context_set_event_notifier(), because it must

 * be done before processing all events (file descriptors, bottom halves,

 * timers).

 *

 * aio_notify_accept() is an optimized event_notifier_test_and_clear()

 * that is specific to an AioContext's notifier; it is used internally

 * to clear the EventNotifier only if aio_notify() had been called.

 */

void aio_notify_accept(AioContext *ctx);



/**

 * aio_bh_poll: Poll bottom halves for an AioContext.

 *