* [Qemu-devel] [PULL for-2.9 1/3] coroutine: remove GThread implementation
2017-05-12 14:37 [Qemu-devel] [PULL for-2.9 0/3] Block patches Stefan Hajnoczi
@ 2017-05-12 14:37 ` Stefan Hajnoczi
2017-05-12 20:19 ` Eric Blake
2017-05-12 14:37 ` [Qemu-devel] [PULL for-2.9 2/3] block: Simplify BDRV_BLOCK_RAW recursion Stefan Hajnoczi
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Stefan Hajnoczi @ 2017-05-12 14:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Daniel P. Berrange, Stefan Hajnoczi
From: "Daniel P. Berrange" <berrange@redhat.com>
The GThread implementation is not functional enough to actually
run QEMU reliably. While it was potentially useful for debugging,
we have a scripts/qemugdb/coroutine.py to enable tracing of
ucontext coroutines in GDB, so that removes the only reason for
GThread to exist.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Acked-by: Alex Bennée <alex.bennee@linaro.org>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
configure | 19 ++---
util/coroutine-gthread.c | 198 -----------------------------------------------
.travis.yml | 5 +-
3 files changed, 6 insertions(+), 216 deletions(-)
delete mode 100644 util/coroutine-gthread.c
diff --git a/configure b/configure
index 7c020c0..39e727e 100755
--- a/configure
+++ b/configure
@@ -1334,7 +1334,7 @@ Advanced options (experts only):
--oss-lib path to OSS library
--cpu=CPU Build for host CPU [$cpu]
--with-coroutine=BACKEND coroutine backend. Supported options:
- gthread, ucontext, sigaltstack, windows
+ ucontext, sigaltstack, windows
--enable-gcov enable test coverage analysis with gcov
--gcov=GCOV use specified gcov [$gcov_tool]
--disable-blobs disable installing provided firmware blobs
@@ -4418,10 +4418,8 @@ fi
# check and set a backend for coroutine
# We prefer ucontext, but it's not always possible. The fallback
-# is sigcontext. gthread is not selectable except explicitly, because
-# it is not functional enough to run QEMU proper. (It is occasionally
-# useful for debugging purposes.) On Windows the only valid backend
-# is the Windows-specific one.
+# is sigcontext. On Windows the only valid backend is the Windows
+# specific one.
ucontext_works=no
if test "$darwin" != "yes"; then
@@ -4460,7 +4458,7 @@ else
feature_not_found "ucontext"
fi
;;
- gthread|sigaltstack)
+ sigaltstack)
if test "$mingw32" = "yes"; then
error_exit "only the 'windows' coroutine backend is valid for Windows"
fi
@@ -4472,14 +4470,7 @@ else
fi
if test "$coroutine_pool" = ""; then
- if test "$coroutine" = "gthread"; then
- coroutine_pool=no
- else
- coroutine_pool=yes
- fi
-fi
-if test "$coroutine" = "gthread" -a "$coroutine_pool" = "yes"; then
- error_exit "'gthread' coroutine backend does not support pool (use --disable-coroutine-pool)"
+ coroutine_pool=yes
fi
if test "$debug_stack_usage" = "yes"; then
diff --git a/util/coroutine-gthread.c b/util/coroutine-gthread.c
deleted file mode 100644
index 62bfb40..0000000
--- a/util/coroutine-gthread.c
+++ /dev/null
@@ -1,198 +0,0 @@
-/*
- * GThread coroutine initialization code
- *
- * Copyright (C) 2006 Anthony Liguori <anthony@codemonkey.ws>
- * Copyright (C) 2011 Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
- *
- * This library is free software; you can redistribute it and/or
- * modify it under the terms of the GNU Lesser General Public
- * License as published by the Free Software Foundation; either
- * version 2.0 of the License, or (at your option) any later version.
- *
- * This library is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
- * Lesser General Public License for more details.
- *
- * You should have received a copy of the GNU Lesser General Public
- * License along with this library; if not, see <http://www.gnu.org/licenses/>.
- */
-
-#include "qemu/osdep.h"
-#include "qemu-common.h"
-#include "qemu/coroutine_int.h"
-
-typedef struct {
- Coroutine base;
- GThread *thread;
- bool runnable;
- bool free_on_thread_exit;
- CoroutineAction action;
-} CoroutineGThread;
-
-static CompatGMutex coroutine_lock;
-static CompatGCond coroutine_cond;
-
-/* GLib 2.31 and beyond deprecated various parts of the thread API,
- * but the new interfaces are not available in older GLib versions
- * so we have to cope with both.
- */
-#if GLIB_CHECK_VERSION(2, 31, 0)
-/* Awkwardly, the GPrivate API doesn't provide a way to update the
- * GDestroyNotify handler for the coroutine key dynamically. So instead
- * we track whether or not the CoroutineGThread should be freed on
- * thread exit / coroutine key update using the free_on_thread_exit
- * field.
- */
-static void coroutine_destroy_notify(gpointer data)
-{
- CoroutineGThread *co = data;
- if (co && co->free_on_thread_exit) {
- g_free(co);
- }
-}
-
-static GPrivate coroutine_key = G_PRIVATE_INIT(coroutine_destroy_notify);
-
-static inline CoroutineGThread *get_coroutine_key(void)
-{
- return g_private_get(&coroutine_key);
-}
-
-static inline void set_coroutine_key(CoroutineGThread *co,
- bool free_on_thread_exit)
-{
- /* Unlike g_static_private_set() this does not call the GDestroyNotify
- * if the previous value of the key was NULL. Fortunately we only need
- * the GDestroyNotify in the non-NULL key case.
- */
- co->free_on_thread_exit = free_on_thread_exit;
- g_private_replace(&coroutine_key, co);
-}
-
-static inline GThread *create_thread(GThreadFunc func, gpointer data)
-{
- return g_thread_new("coroutine", func, data);
-}
-
-#else
-
-/* Handle older GLib versions */
-
-static GStaticPrivate coroutine_key = G_STATIC_PRIVATE_INIT;
-
-static inline CoroutineGThread *get_coroutine_key(void)
-{
- return g_static_private_get(&coroutine_key);
-}
-
-static inline void set_coroutine_key(CoroutineGThread *co,
- bool free_on_thread_exit)
-{
- g_static_private_set(&coroutine_key, co,
- free_on_thread_exit ? (GDestroyNotify)g_free : NULL);
-}
-
-static inline GThread *create_thread(GThreadFunc func, gpointer data)
-{
- return g_thread_create_full(func, data, 0, TRUE, TRUE,
- G_THREAD_PRIORITY_NORMAL, NULL);
-}
-
-#endif
-
-
-static void __attribute__((constructor)) coroutine_init(void)
-{
-#if !GLIB_CHECK_VERSION(2, 31, 0)
- if (!g_thread_supported()) {
- g_thread_init(NULL);
- }
-#endif
-}
-
-static void coroutine_wait_runnable_locked(CoroutineGThread *co)
-{
- while (!co->runnable) {
- g_cond_wait(&coroutine_cond, &coroutine_lock);
- }
-}
-
-static void coroutine_wait_runnable(CoroutineGThread *co)
-{
- g_mutex_lock(&coroutine_lock);
- coroutine_wait_runnable_locked(co);
- g_mutex_unlock(&coroutine_lock);
-}
-
-static gpointer coroutine_thread(gpointer opaque)
-{
- CoroutineGThread *co = opaque;
-
- set_coroutine_key(co, false);
- coroutine_wait_runnable(co);
- co->base.entry(co->base.entry_arg);
- qemu_coroutine_switch(&co->base, co->base.caller, COROUTINE_TERMINATE);
- return NULL;
-}
-
-Coroutine *qemu_coroutine_new(void)
-{
- CoroutineGThread *co;
-
- co = g_malloc0(sizeof(*co));
- co->thread = create_thread(coroutine_thread, co);
- if (!co->thread) {
- g_free(co);
- return NULL;
- }
- return &co->base;
-}
-
-void qemu_coroutine_delete(Coroutine *co_)
-{
- CoroutineGThread *co = DO_UPCAST(CoroutineGThread, base, co_);
-
- g_thread_join(co->thread);
- g_free(co);
-}
-
-CoroutineAction qemu_coroutine_switch(Coroutine *from_,
- Coroutine *to_,
- CoroutineAction action)
-{
- CoroutineGThread *from = DO_UPCAST(CoroutineGThread, base, from_);
- CoroutineGThread *to = DO_UPCAST(CoroutineGThread, base, to_);
-
- g_mutex_lock(&coroutine_lock);
- from->runnable = false;
- from->action = action;
- to->runnable = true;
- to->action = action;
- g_cond_broadcast(&coroutine_cond);
-
- if (action != COROUTINE_TERMINATE) {
- coroutine_wait_runnable_locked(from);
- }
- g_mutex_unlock(&coroutine_lock);
- return from->action;
-}
-
-Coroutine *qemu_coroutine_self(void)
-{
- CoroutineGThread *co = get_coroutine_key();
- if (!co) {
- co = g_malloc0(sizeof(*co));
- co->runnable = true;
- set_coroutine_key(co, true);
- }
-
- return &co->base;
-}
-
-bool qemu_in_coroutine(void)
-{
- CoroutineGThread *co = get_coroutine_key();
-
- return co && co->base.caller;
-}
diff --git a/.travis.yml b/.travis.yml
index 9008a79..27a2d9c 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -86,9 +86,6 @@ matrix:
- env: CONFIG="--enable-trace-backends=ust"
TEST_CMD=""
compiler: gcc
- - env: CONFIG="--with-coroutine=gthread"
- TEST_CMD=""
- compiler: gcc
- env: CONFIG=""
os: osx
compiler: clang
@@ -191,7 +188,7 @@ matrix:
compiler: none
env:
- COMPILER_NAME=gcc CXX=g++-5 CC=gcc-5
- - CONFIG="--cc=gcc-5 --cxx=g++-5 --disable-pie --disable-linux-user --with-coroutine=gthread"
+ - CONFIG="--cc=gcc-5 --cxx=g++-5 --disable-pie --disable-linux-user"
- TEST_CMD=""
before_script:
- ./configure ${CONFIG} --extra-cflags="-g3 -O0 -fsanitize=thread -fuse-ld=gold" || cat config.log
--
2.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PULL for-2.9 2/3] block: Simplify BDRV_BLOCK_RAW recursion
2017-05-12 14:37 [Qemu-devel] [PULL for-2.9 0/3] Block patches Stefan Hajnoczi
2017-05-12 14:37 ` [Qemu-devel] [PULL for-2.9 1/3] coroutine: remove GThread implementation Stefan Hajnoczi
@ 2017-05-12 14:37 ` Stefan Hajnoczi
2017-05-12 14:37 ` [Qemu-devel] [PULL for-2.9 3/3] aio: add missing aio_notify() to aio_enable_external() Stefan Hajnoczi
2017-05-15 13:38 ` [Qemu-devel] [PULL for-2.9 0/3] Block patches Stefan Hajnoczi
3 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2017-05-12 14:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Eric Blake, Stefan Hajnoczi
From: Eric Blake <eblake@redhat.com>
Since we are already in coroutine context during the body of
bdrv_co_get_block_status(), we can shave off a few layers of
wrappers when recursing to query the protocol when a format driver
returned BDRV_BLOCK_RAW.
Note that we are already using the correct recursion later on in
the same function, when probing whether the protocol layer is sparse
in order to find out if we can add BDRV_BLOCK_ZERO to an existing
BDRV_BLOCK_DATA|BDRV_BLOCK_OFFSET_VALID.
Signed-off-by: Eric Blake <eblake@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Message-id: 20170504173745.27414-1-eblake@redhat.com
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
block/io.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/block/io.c b/block/io.c
index 40bd94f..fdd7485 100644
--- a/block/io.c
+++ b/block/io.c
@@ -1784,8 +1784,8 @@ static int64_t coroutine_fn bdrv_co_get_block_status(BlockDriverState *bs,
if (ret & BDRV_BLOCK_RAW) {
assert(ret & BDRV_BLOCK_OFFSET_VALID);
- ret = bdrv_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
- *pnum, pnum, file);
+ ret = bdrv_co_get_block_status(*file, ret >> BDRV_SECTOR_BITS,
+ *pnum, pnum, file);
goto out;
}
--
2.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread* [Qemu-devel] [PULL for-2.9 3/3] aio: add missing aio_notify() to aio_enable_external()
2017-05-12 14:37 [Qemu-devel] [PULL for-2.9 0/3] Block patches Stefan Hajnoczi
2017-05-12 14:37 ` [Qemu-devel] [PULL for-2.9 1/3] coroutine: remove GThread implementation Stefan Hajnoczi
2017-05-12 14:37 ` [Qemu-devel] [PULL for-2.9 2/3] block: Simplify BDRV_BLOCK_RAW recursion Stefan Hajnoczi
@ 2017-05-12 14:37 ` Stefan Hajnoczi
2017-05-15 13:38 ` [Qemu-devel] [PULL for-2.9 0/3] Block patches Stefan Hajnoczi
3 siblings, 0 replies; 7+ messages in thread
From: Stefan Hajnoczi @ 2017-05-12 14:37 UTC (permalink / raw)
To: qemu-devel; +Cc: Peter Maydell, Stefan Hajnoczi
The main loop uses aio_disable_external()/aio_enable_external() to
temporarily disable processing of external AioContext clients like
device emulation.
This allows monitor commands to quiesce I/O and prevent the guest from
submitting new requests while a monitor command is in progress.
The aio_enable_external() API is currently broken when an IOThread is in
aio_poll() waiting for fd activity when the main loop re-enables
external clients. Incrementing ctx->external_disable_cnt does not wake
the IOThread from ppoll(2) so fd processing remains suspended and leads
to unresponsive emulated devices.
This patch adds an aio_notify() call to aio_enable_external() so the
IOThread is kicked out of ppoll(2) and will re-arm the file descriptors.
The bug can be reproduced as follows:
$ qemu -M accel=kvm -m 1024 \
-object iothread,id=iothread0 \
-device virtio-scsi-pci,iothread=iothread0,id=virtio-scsi-pci0 \
-drive if=none,id=drive0,aio=native,cache=none,format=raw,file=test.img \
-device scsi-hd,id=scsi-hd0,drive=drive0 \
-qmp tcp::5555,server,nowait
$ scripts/qmp/qmp-shell localhost:5555
(qemu) blockdev-snapshot-sync device=drive0 snapshot-file=sn1.qcow2
mode=absolute-paths format=qcow2
After blockdev-snapshot-sync completes the SCSI disk will be
unresponsive. This leads to request timeouts inside the guest.
Reported-by: Qianqian Zhu <qizhu@redhat.com>
Reviewed-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
Message-id: 20170508180705.20609-1-stefanha@redhat.com
Suggested-by: Fam Zheng <famz@redhat.com>
Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
---
include/block/aio.h | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/block/aio.h b/include/block/aio.h
index 406e323..e9aeeae 100644
--- a/include/block/aio.h
+++ b/include/block/aio.h
@@ -454,8 +454,14 @@ static inline void aio_disable_external(AioContext *ctx)
*/
static inline void aio_enable_external(AioContext *ctx)
{
- assert(ctx->external_disable_cnt > 0);
- atomic_dec(&ctx->external_disable_cnt);
+ int old;
+
+ old = atomic_fetch_dec(&ctx->external_disable_cnt);
+ assert(old > 0);
+ if (old == 1) {
+ /* Kick event loop so it re-arms file descriptors */
+ aio_notify(ctx);
+ }
}
/**
--
2.9.3
^ permalink raw reply related [flat|nested] 7+ messages in thread