[Qemu-devel] [PATCH] main-loop: drop spin_counter

[Qemu-devel] [PATCH] main-loop: drop spin_counter

[Stefan Hajnoczi]

Commit d759c951f3287fad04210a52f2dc93f94cf58c7f ("replay: push
replay_mutex_lock up the call tree") removed the !timeout lock
optimization in the main loop.

The idea of the optimization was to avoid ping-pongs between threads by
keeping the Big QEMU Lock held across non-blocking (!timeout) main loop
iterations.

A warning is printed when the main loop spins without releasing BQL for
long periods of time.  These warnings were supposed to aid debugging but
in practice they just alarm users.  They are considered noise because
the cause of spinning is not shown and is hard to find.

Now that the lock optimization has been removed, there is no danger of
hogging the BQL.  Drop the spin counter and the infamous warning.

Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
 util/main-loop.c                 | 25 -------------------------
 tests/qemu-iotests/common.filter |  1 -
 2 files changed, 26 deletions(-)

diff --git a/util/main-loop.c b/util/main-loop.c
index 992f9b0f34..affe0403c5 100644
--- a/util/main-loop.c
+++ b/util/main-loop.c
@@ -222,36 +222,11 @@ static int os_host_main_loop_wait(int64_t timeout)
 {
     GMainContext *context = g_main_context_default();
     int ret;
-    static int spin_counter;
 
     g_main_context_acquire(context);
 
     glib_pollfds_fill(&timeout);
 
-    /* If the I/O thread is very busy or we are incorrectly busy waiting in
-     * the I/O thread, this can lead to starvation of the BQL such that the
-     * VCPU threads never run.  To make sure we can detect the later case,
-     * print a message to the screen.  If we run into this condition, create
-     * a fake timeout in order to give the VCPU threads a chance to run.
-     */
-    if (!timeout && (spin_counter > MAX_MAIN_LOOP_SPIN)) {
-        static bool notified;
-
-        if (!notified && !qtest_enabled() && !qtest_driver()) {
-            warn_report("I/O thread spun for %d iterations",
-                        MAX_MAIN_LOOP_SPIN);
-            notified = true;
-        }
-
-        timeout = SCALE_MS;
-    }
-
-
-    if (timeout) {
-        spin_counter = 0;
-    } else {
-        spin_counter++;
-    }
     qemu_mutex_unlock_iothread();
     replay_mutex_unlock();
 
diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
index f08ee55046..2031e353a5 100644
--- a/tests/qemu-iotests/common.filter
+++ b/tests/qemu-iotests/common.filter
@@ -77,7 +77,6 @@ _filter_qemu()
 {
     sed -e "s#\\(^\\|(qemu) \\)$(basename $QEMU_PROG):#\1QEMU_PROG:#" \
         -e 's#^QEMU [0-9]\+\.[0-9]\+\.[0-9]\+ monitor#QEMU X.Y.Z monitor#' \
-        -e '/main-loop: WARNING: I\/O thread spun for [0-9]\+ iterations/d' \
         -e $'s#\r##' # QEMU monitor uses \r\n line endings
 }
 
-- 
2.17.0

Re: [Qemu-devel] [PATCH] main-loop: drop spin_counter

[Jeff Cody]

On Wed, May 30, 2018 at 08:42:38PM +0100, Stefan Hajnoczi wrote:
> Commit d759c951f3287fad04210a52f2dc93f94cf58c7f ("replay: push
> replay_mutex_lock up the call tree") removed the !timeout lock
> optimization in the main loop.
> 
> The idea of the optimization was to avoid ping-pongs between threads by
> keeping the Big QEMU Lock held across non-blocking (!timeout) main loop
> iterations.
> 
> A warning is printed when the main loop spins without releasing BQL for
> long periods of time.  These warnings were supposed to aid debugging but
> in practice they just alarm users.  They are considered noise because
> the cause of spinning is not shown and is hard to find.
> 
> Now that the lock optimization has been removed, there is no danger of
> hogging the BQL.  Drop the spin counter and the infamous warning.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/main-loop.c                 | 25 -------------------------
>  tests/qemu-iotests/common.filter |  1 -
>  2 files changed, 26 deletions(-)
> 
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 992f9b0f34..affe0403c5 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -222,36 +222,11 @@ static int os_host_main_loop_wait(int64_t timeout)
>  {
>      GMainContext *context = g_main_context_default();
>      int ret;
> -    static int spin_counter;
>  
>      g_main_context_acquire(context);
>  
>      glib_pollfds_fill(&timeout);
>  
> -    /* If the I/O thread is very busy or we are incorrectly busy waiting in
> -     * the I/O thread, this can lead to starvation of the BQL such that the
> -     * VCPU threads never run.  To make sure we can detect the later case,
> -     * print a message to the screen.  If we run into this condition, create
> -     * a fake timeout in order to give the VCPU threads a chance to run.
> -     */
> -    if (!timeout && (spin_counter > MAX_MAIN_LOOP_SPIN)) {
> -        static bool notified;
> -
> -        if (!notified && !qtest_enabled() && !qtest_driver()) {
> -            warn_report("I/O thread spun for %d iterations",
> -                        MAX_MAIN_LOOP_SPIN);
> -            notified = true;
> -        }
> -
> -        timeout = SCALE_MS;
> -    }
> -
> -
> -    if (timeout) {
> -        spin_counter = 0;
> -    } else {
> -        spin_counter++;
> -    }
>      qemu_mutex_unlock_iothread();
>      replay_mutex_unlock();
>  
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index f08ee55046..2031e353a5 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -77,7 +77,6 @@ _filter_qemu()
>  {
>      sed -e "s#\\(^\\|(qemu) \\)$(basename $QEMU_PROG):#\1QEMU_PROG:#" \
>          -e 's#^QEMU [0-9]\+\.[0-9]\+\.[0-9]\+ monitor#QEMU X.Y.Z monitor#' \
> -        -e '/main-loop: WARNING: I\/O thread spun for [0-9]\+ iterations/d' \
>          -e $'s#\r##' # QEMU monitor uses \r\n line endings
>  }
>  
> -- 
> 2.17.0
> 
> 

Happy to see this go!

Reviewed-by: Jeff Cody <jcody@redhat.com>

Re: [Qemu-devel] [PATCH] main-loop: drop spin_counter

[Paolo Bonzini]

On 30/05/2018 21:42, Stefan Hajnoczi wrote:
> Commit d759c951f3287fad04210a52f2dc93f94cf58c7f ("replay: push
> replay_mutex_lock up the call tree") removed the !timeout lock
> optimization in the main loop.
> 
> The idea of the optimization was to avoid ping-pongs between threads by
> keeping the Big QEMU Lock held across non-blocking (!timeout) main loop
> iterations.
> 
> A warning is printed when the main loop spins without releasing BQL for
> long periods of time.  These warnings were supposed to aid debugging but
> in practice they just alarm users.  They are considered noise because
> the cause of spinning is not shown and is hard to find.
> 
> Now that the lock optimization has been removed, there is no danger of
> hogging the BQL.  Drop the spin counter and the infamous warning.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>

Very good idea, at last! :)

Paolo

> ---
>  util/main-loop.c                 | 25 -------------------------
>  tests/qemu-iotests/common.filter |  1 -
>  2 files changed, 26 deletions(-)
> 
> diff --git a/util/main-loop.c b/util/main-loop.c
> index 992f9b0f34..affe0403c5 100644
> --- a/util/main-loop.c
> +++ b/util/main-loop.c
> @@ -222,36 +222,11 @@ static int os_host_main_loop_wait(int64_t timeout)
>  {
>      GMainContext *context = g_main_context_default();
>      int ret;
> -    static int spin_counter;
>  
>      g_main_context_acquire(context);
>  
>      glib_pollfds_fill(&timeout);
>  
> -    /* If the I/O thread is very busy or we are incorrectly busy waiting in
> -     * the I/O thread, this can lead to starvation of the BQL such that the
> -     * VCPU threads never run.  To make sure we can detect the later case,
> -     * print a message to the screen.  If we run into this condition, create
> -     * a fake timeout in order to give the VCPU threads a chance to run.
> -     */
> -    if (!timeout && (spin_counter > MAX_MAIN_LOOP_SPIN)) {
> -        static bool notified;
> -
> -        if (!notified && !qtest_enabled() && !qtest_driver()) {
> -            warn_report("I/O thread spun for %d iterations",
> -                        MAX_MAIN_LOOP_SPIN);
> -            notified = true;
> -        }
> -
> -        timeout = SCALE_MS;
> -    }
> -
> -
> -    if (timeout) {
> -        spin_counter = 0;
> -    } else {
> -        spin_counter++;
> -    }
>      qemu_mutex_unlock_iothread();
>      replay_mutex_unlock();
>  
> diff --git a/tests/qemu-iotests/common.filter b/tests/qemu-iotests/common.filter
> index f08ee55046..2031e353a5 100644
> --- a/tests/qemu-iotests/common.filter
> +++ b/tests/qemu-iotests/common.filter
> @@ -77,7 +77,6 @@ _filter_qemu()
>  {
>      sed -e "s#\\(^\\|(qemu) \\)$(basename $QEMU_PROG):#\1QEMU_PROG:#" \
>          -e 's#^QEMU [0-9]\+\.[0-9]\+\.[0-9]\+ monitor#QEMU X.Y.Z monitor#' \
> -        -e '/main-loop: WARNING: I\/O thread spun for [0-9]\+ iterations/d' \
>          -e $'s#\r##' # QEMU monitor uses \r\n line endings
>  }
>  
> 

Re: [Qemu-devel] [PATCH] main-loop: drop spin_counter

[Stefan Hajnoczi]

[-- Attachment #1: Type: text/plain, Size: 1146 bytes --]

On Wed, May 30, 2018 at 08:42:38PM +0100, Stefan Hajnoczi wrote:
> Commit d759c951f3287fad04210a52f2dc93f94cf58c7f ("replay: push
> replay_mutex_lock up the call tree") removed the !timeout lock
> optimization in the main loop.
> 
> The idea of the optimization was to avoid ping-pongs between threads by
> keeping the Big QEMU Lock held across non-blocking (!timeout) main loop
> iterations.
> 
> A warning is printed when the main loop spins without releasing BQL for
> long periods of time.  These warnings were supposed to aid debugging but
> in practice they just alarm users.  They are considered noise because
> the cause of spinning is not shown and is hard to find.
> 
> Now that the lock optimization has been removed, there is no danger of
> hogging the BQL.  Drop the spin counter and the infamous warning.
> 
> Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> ---
>  util/main-loop.c                 | 25 -------------------------
>  tests/qemu-iotests/common.filter |  1 -
>  2 files changed, 26 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 455 bytes --]