| Message ID | 20005a15c708fbda983f9be602c55fc0b1979a18.1590008051.git.lukasstraub2@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce 'yank' oob qmp command to recover from hanging qemu | expand |
On 20/05/20 23:05, Lukas Straub wrote: > + > +void yank_init(void) > +{ > + qemu_mutex_init(&lock); > +} You can use __constructor__ for this to avoid the call in vl.c. See job.c for an example. Thanks, Paolo
On Wed, May 20, 2020 at 11:05:39PM +0200, Lukas Straub wrote: > +void yank_generic_iochannel(void *opaque) > +{ > + QIOChannel *ioc = QIO_CHANNEL(opaque); > + > + qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL); > +} > + > +void qmp_yank(strList *instances, Error **errp) > +{ > + strList *tmp; > + struct YankInstance *instance; > + struct YankFuncAndParam *entry; > + > + qemu_mutex_lock(&lock); > + tmp = instances; > + for (; tmp; tmp = tmp->next) { > + instance = yank_find_instance(tmp->value); > + if (!instance) { > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > + "Instance '%s' not found", tmp->value); > + qemu_mutex_unlock(&lock); > + return; > + } > + } > + tmp = instances; > + for (; tmp; tmp = tmp->next) { > + instance = yank_find_instance(tmp->value); > + assert(instance); > + QLIST_FOREACH(entry, &instance->yankfns, next) { > + entry->func(entry->opaque); > + } > + } > + qemu_mutex_unlock(&lock); > +} From docs/devel/qapi-code-gen.txt: An OOB-capable command handler must satisfy the following conditions: - It terminates quickly. - It does not invoke system calls that may block. - It does not access guest RAM that may block when userfaultfd is enabled for postcopy live migration. - It takes only "fast" locks, i.e. all critical sections protected by any lock it takes also satisfy the conditions for OOB command handler code. This patch series violates these rules and calls existing functions that were not designed for OOB execution. Please explain why it is safe to do this. Stefan
On Thu, 21 May 2020 16:03:35 +0100 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Wed, May 20, 2020 at 11:05:39PM +0200, Lukas Straub wrote: > > +void yank_generic_iochannel(void *opaque) > > +{ > > + QIOChannel *ioc = QIO_CHANNEL(opaque); > > + > > + qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL); > > +} > > + > > +void qmp_yank(strList *instances, Error **errp) > > +{ > > + strList *tmp; > > + struct YankInstance *instance; > > + struct YankFuncAndParam *entry; > > + > > + qemu_mutex_lock(&lock); > > + tmp = instances; > > + for (; tmp; tmp = tmp->next) { > > + instance = yank_find_instance(tmp->value); > > + if (!instance) { > > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > > + "Instance '%s' not found", tmp->value); > > + qemu_mutex_unlock(&lock); > > + return; > > + } > > + } > > + tmp = instances; > > + for (; tmp; tmp = tmp->next) { > > + instance = yank_find_instance(tmp->value); > > + assert(instance); > > + QLIST_FOREACH(entry, &instance->yankfns, next) { > > + entry->func(entry->opaque); > > + } > > + } > > + qemu_mutex_unlock(&lock); > > +} > > From docs/devel/qapi-code-gen.txt: > > An OOB-capable command handler must satisfy the following conditions: > > - It terminates quickly. Check. > - It does not invoke system calls that may block. brk/sbrk (malloc and friends): The manpage doesn't say anything about blocking, but malloc is already used while handling the qmp command. shutdown(): The manpage doesn't say anything about blocking, but this is already used in migration oob qmp commands. There are no other syscalls involved to my knowledge. > - It does not access guest RAM that may block when userfaultfd is > enabled for postcopy live migration. Check. > - It takes only "fast" locks, i.e. all critical sections protected by > any lock it takes also satisfy the conditions for OOB command > handler code. The lock in yank.c satisfies this requirement. qio_channel_shutdown doesn't take any locks. Regards, Lukas Straub > This patch series violates these rules and calls existing functions that > were not designed for OOB execution. > > Please explain why it is safe to do this. > > Stefan
On Thu, May 21, 2020 at 05:42:41PM +0200, Lukas Straub wrote: > On Thu, 21 May 2020 16:03:35 +0100 > Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > On Wed, May 20, 2020 at 11:05:39PM +0200, Lukas Straub wrote: > > > +void yank_generic_iochannel(void *opaque) > > > +{ > > > + QIOChannel *ioc = QIO_CHANNEL(opaque); > > > + > > > + qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL); > > > +} > > > + > > > +void qmp_yank(strList *instances, Error **errp) > > > +{ > > > + strList *tmp; > > > + struct YankInstance *instance; > > > + struct YankFuncAndParam *entry; > > > + > > > + qemu_mutex_lock(&lock); > > > + tmp = instances; > > > + for (; tmp; tmp = tmp->next) { > > > + instance = yank_find_instance(tmp->value); > > > + if (!instance) { > > > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > > > + "Instance '%s' not found", tmp->value); > > > + qemu_mutex_unlock(&lock); > > > + return; > > > + } > > > + } > > > + tmp = instances; > > > + for (; tmp; tmp = tmp->next) { > > > + instance = yank_find_instance(tmp->value); > > > + assert(instance); > > > + QLIST_FOREACH(entry, &instance->yankfns, next) { > > > + entry->func(entry->opaque); > > > + } > > > + } > > > + qemu_mutex_unlock(&lock); > > > +} > > > > From docs/devel/qapi-code-gen.txt: > > > > An OOB-capable command handler must satisfy the following conditions: > > > > - It terminates quickly. > Check. > > > - It does not invoke system calls that may block. > brk/sbrk (malloc and friends): > The manpage doesn't say anything about blocking, but malloc is already used while handling the qmp command. > > shutdown(): > The manpage doesn't say anything about blocking, but this is already used in migration oob qmp commands. It just marks the socket state in local kernel side. It doesn't involve any blocking roundtrips over the wire, so this is fine. > > There are no other syscalls involved to my knowledge. > > > - It does not access guest RAM that may block when userfaultfd is > > enabled for postcopy live migration. > Check. > > > - It takes only "fast" locks, i.e. all critical sections protected by > > any lock it takes also satisfy the conditions for OOB command > > handler code. > > The lock in yank.c satisfies this requirement. > > qio_channel_shutdown doesn't take any locks. Agreed, I think the yank code is compliant with all the points listed above. Regards, Daniel
On Thu, May 21, 2020 at 04:48:06PM +0100, Daniel P. Berrangé wrote: > On Thu, May 21, 2020 at 05:42:41PM +0200, Lukas Straub wrote: > > On Thu, 21 May 2020 16:03:35 +0100 > > Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > > > On Wed, May 20, 2020 at 11:05:39PM +0200, Lukas Straub wrote: > > > > +void yank_generic_iochannel(void *opaque) > > > > +{ > > > > + QIOChannel *ioc = QIO_CHANNEL(opaque); > > > > + > > > > + qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL); > > > > +} > > > > + > > > > +void qmp_yank(strList *instances, Error **errp) > > > > +{ > > > > + strList *tmp; > > > > + struct YankInstance *instance; > > > > + struct YankFuncAndParam *entry; > > > > + > > > > + qemu_mutex_lock(&lock); > > > > + tmp = instances; > > > > + for (; tmp; tmp = tmp->next) { > > > > + instance = yank_find_instance(tmp->value); > > > > + if (!instance) { > > > > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > > > > + "Instance '%s' not found", tmp->value); > > > > + qemu_mutex_unlock(&lock); > > > > + return; > > > > + } > > > > + } > > > > + tmp = instances; > > > > + for (; tmp; tmp = tmp->next) { > > > > + instance = yank_find_instance(tmp->value); > > > > + assert(instance); > > > > + QLIST_FOREACH(entry, &instance->yankfns, next) { > > > > + entry->func(entry->opaque); > > > > + } > > > > + } > > > > + qemu_mutex_unlock(&lock); > > > > +} > > > > > > From docs/devel/qapi-code-gen.txt: > > > > > > An OOB-capable command handler must satisfy the following conditions: > > > > > > - It terminates quickly. > > Check. > > > > > - It does not invoke system calls that may block. > > brk/sbrk (malloc and friends): > > The manpage doesn't say anything about blocking, but malloc is already used while handling the qmp command. > > > > shutdown(): > > The manpage doesn't say anything about blocking, but this is already used in migration oob qmp commands. > > It just marks the socket state in local kernel side. It doesn't involve > any blocking roundtrips over the wire, so this is fine. > > > > > There are no other syscalls involved to my knowledge. > > > > > - It does not access guest RAM that may block when userfaultfd is > > > enabled for postcopy live migration. > > Check. > > > > > - It takes only "fast" locks, i.e. all critical sections protected by > > > any lock it takes also satisfy the conditions for OOB command > > > handler code. > > > > The lock in yank.c satisfies this requirement. > > > > qio_channel_shutdown doesn't take any locks. > > Agreed, I think the yank code is compliant with all the points > listed above. The code does not document this in any way. In fact, it's an interface for registering arbitrary callback functions which execute in this special environment. If you don't document this explicitly it will break when someone changes the code, even if it works right now. Please document this in the yank APIs and also in the implementation. For example, QemuMutex yank has the priority inversion problem: no other function may violate the oob rules while holding the mutex, otherwise the oob function will hang while waiting for the lock when the other function is blocked. Stefan
On Wed, 17 Jun 2020 15:39:36 +0100 Stefan Hajnoczi <stefanha@gmail.com> wrote: > On Thu, May 21, 2020 at 04:48:06PM +0100, Daniel P. Berrangé wrote: > > On Thu, May 21, 2020 at 05:42:41PM +0200, Lukas Straub wrote: > > > On Thu, 21 May 2020 16:03:35 +0100 > > > Stefan Hajnoczi <stefanha@gmail.com> wrote: > > > > > > > On Wed, May 20, 2020 at 11:05:39PM +0200, Lukas Straub wrote: > > > > > +void yank_generic_iochannel(void *opaque) > > > > > +{ > > > > > + QIOChannel *ioc = QIO_CHANNEL(opaque); > > > > > + > > > > > + qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL); > > > > > +} > > > > > + > > > > > +void qmp_yank(strList *instances, Error **errp) > > > > > +{ > > > > > + strList *tmp; > > > > > + struct YankInstance *instance; > > > > > + struct YankFuncAndParam *entry; > > > > > + > > > > > + qemu_mutex_lock(&lock); > > > > > + tmp = instances; > > > > > + for (; tmp; tmp = tmp->next) { > > > > > + instance = yank_find_instance(tmp->value); > > > > > + if (!instance) { > > > > > + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, > > > > > + "Instance '%s' not found", tmp->value); > > > > > + qemu_mutex_unlock(&lock); > > > > > + return; > > > > > + } > > > > > + } > > > > > + tmp = instances; > > > > > + for (; tmp; tmp = tmp->next) { > > > > > + instance = yank_find_instance(tmp->value); > > > > > + assert(instance); > > > > > + QLIST_FOREACH(entry, &instance->yankfns, next) { > > > > > + entry->func(entry->opaque); > > > > > + } > > > > > + } > > > > > + qemu_mutex_unlock(&lock); > > > > > +} > > > > > > > > From docs/devel/qapi-code-gen.txt: > > > > > > > > An OOB-capable command handler must satisfy the following conditions: > > > > > > > > - It terminates quickly. > > > Check. > > > > > > > - It does not invoke system calls that may block. > > > brk/sbrk (malloc and friends): > > > The manpage doesn't say anything about blocking, but malloc is already used while handling the qmp command. > > > > > > shutdown(): > > > The manpage doesn't say anything about blocking, but this is already used in migration oob qmp commands. > > > > It just marks the socket state in local kernel side. It doesn't involve > > any blocking roundtrips over the wire, so this is fine. > > > > > > > > There are no other syscalls involved to my knowledge. > > > > > > > - It does not access guest RAM that may block when userfaultfd is > > > > enabled for postcopy live migration. > > > Check. > > > > > > > - It takes only "fast" locks, i.e. all critical sections protected by > > > > any lock it takes also satisfy the conditions for OOB command > > > > handler code. > > > > > > The lock in yank.c satisfies this requirement. > > > > > > qio_channel_shutdown doesn't take any locks. > > > > Agreed, I think the yank code is compliant with all the points > > listed above. > > The code does not document this in any way. In fact, it's an interface > for registering arbitrary callback functions which execute in this > special environment. > > If you don't document this explicitly it will break when someone changes > the code, even if it works right now. > > Please document this in the yank APIs and also in the implementation. Hi, It is documented in yank.h: /** * yank_register_function: Register a yank function * * This registers a yank function. All limitations of qmp oob commands apply * to the yank function as well. * * This function is thread-safe. * * @instance_name: The name of the instance * @func: The yank function * @opaque: Will be passed to the yank function */ Thanks, Lukas Straub > For example, QemuMutex yank has the priority inversion problem: no other > function may violate the oob rules while holding the mutex, otherwise > the oob function will hang while waiting for the lock when the other > function is blocked. > > Stefan
diff --git a/qapi/misc.json b/qapi/misc.json index 99b90ac80b..f5228b2502 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -1550,3 +1550,48 @@ ## { 'command': 'query-vm-generation-id', 'returns': 'GuidInfo' } +## +# @YankInstances: +# +# @instances: List of yank instances. +# +# Yank instances are named after the following schema: +# "blockdev:<node-name>", "chardev:<chardev-name>" and "migration" +# +# Since: 5.1 +## +{ 'struct': 'YankInstances', 'data': {'instances': ['str'] } } + +## +# @yank: +# +# Recover from hanging qemu by yanking the specified instances. +# +# Takes @YankInstances as argument. +# +# Returns: nothing. +# +# Example: +# +# -> { "execute": "yank", "arguments": { "instances": ["blockdev:nbd0"] } } +# <- { "return": {} } +# +# Since: 5.1 +## +{ 'command': 'yank', 'data': 'YankInstances', 'allow-oob': true } + +## +# @query-yank: +# +# Query yank instances. +# +# Returns: @YankInstances +# +# Example: +# +# -> { "execute": "query-yank" } +# <- { "return": { "instances": ["blockdev:nbd0"] } } +# +# Since: 5.1 +## +{ 'command': 'query-yank', 'returns': 'YankInstances', 'allow-oob': true } diff --git a/softmmu/vl.c b/softmmu/vl.c index 32c0047889..5d99749d29 100644 --- a/softmmu/vl.c +++ b/softmmu/vl.c @@ -112,6 +112,7 @@ #include "qapi/qmp/qerror.h" #include "sysemu/iothread.h" #include "qemu/guest-random.h" +#include "yank.h" #define MAX_VIRTIO_CONSOLES 1 @@ -2906,6 +2907,7 @@ void qemu_init(int argc, char **argv, char **envp) precopy_infrastructure_init(); postcopy_infrastructure_init(); monitor_init_globals(); + yank_init(); if (qcrypto_init(&err) < 0) { error_reportf_err(err, "cannot initialize crypto: "); diff --git a/yank.c b/yank.c new file mode 100644 index 0000000000..bfce19185e --- /dev/null +++ b/yank.c @@ -0,0 +1,174 @@ +/* + * QEMU yank feature + * + * Copyright (c) Lukas Straub <lukasstraub2@web.de> + * + * This work is licensed under the terms of the GNU GPL, version 2 or later. + * See the COPYING file in the top-level directory. + */ + +#include "qemu/osdep.h" +#include "qapi/error.h" +#include "qemu/thread.h" +#include "qemu/queue.h" +#include "qapi/qapi-commands-misc.h" +#include "io/channel.h" +#include "yank.h" + +struct YankFuncAndParam { + YankFn *func; + void *opaque; + QLIST_ENTRY(YankFuncAndParam) next; +}; + +struct YankInstance { + char *name; + QLIST_HEAD(, YankFuncAndParam) yankfns; + QLIST_ENTRY(YankInstance) next; +}; + +static QemuMutex lock; +static QLIST_HEAD(yankinst_list, YankInstance) head + = QLIST_HEAD_INITIALIZER(head); + +static struct YankInstance *yank_find_instance(char *name) +{ + struct YankInstance *tmp, *instance; + instance = NULL; + QLIST_FOREACH(tmp, &head, next) { + if (!strcmp(tmp->name, name)) { + instance = tmp; + } + } + return instance; +} + +void yank_register_instance(char *instance_name) +{ + struct YankInstance *instance; + + qemu_mutex_lock(&lock); + assert(!yank_find_instance(instance_name)); + + instance = g_slice_new(struct YankInstance); + instance->name = g_strdup(instance_name); + QLIST_INIT(&instance->yankfns); + QLIST_INSERT_HEAD(&head, instance, next); + + qemu_mutex_unlock(&lock); +} + +void yank_unregister_instance(char *instance_name) +{ + struct YankInstance *instance; + + qemu_mutex_lock(&lock); + instance = yank_find_instance(instance_name); + assert(instance); + + assert(QLIST_EMPTY(&instance->yankfns)); + QLIST_REMOVE(instance, next); + g_free(instance->name); + g_slice_free(struct YankInstance, instance); + + qemu_mutex_unlock(&lock); +} + +void yank_register_function(char *instance_name, YankFn *func, void *opaque) +{ + struct YankInstance *instance; + struct YankFuncAndParam *entry; + + qemu_mutex_lock(&lock); + instance = yank_find_instance(instance_name); + assert(instance); + + entry = g_slice_new(struct YankFuncAndParam); + entry->func = func; + entry->opaque = opaque; + + QLIST_INSERT_HEAD(&instance->yankfns, entry, next); + qemu_mutex_unlock(&lock); +} + +void yank_unregister_function(char *instance_name, YankFn *func, void *opaque) +{ + struct YankInstance *instance; + struct YankFuncAndParam *entry; + + qemu_mutex_lock(&lock); + instance = yank_find_instance(instance_name); + assert(instance); + + QLIST_FOREACH(entry, &instance->yankfns, next) { + if (entry->func == func && entry->opaque == opaque) { + QLIST_REMOVE(entry, next); + g_slice_free(struct YankFuncAndParam, entry); + qemu_mutex_unlock(&lock); + return; + } + } + + abort(); +} + +void yank_generic_iochannel(void *opaque) +{ + QIOChannel *ioc = QIO_CHANNEL(opaque); + + qio_channel_shutdown(ioc, QIO_CHANNEL_SHUTDOWN_BOTH, NULL); +} + +void qmp_yank(strList *instances, Error **errp) +{ + strList *tmp; + struct YankInstance *instance; + struct YankFuncAndParam *entry; + + qemu_mutex_lock(&lock); + tmp = instances; + for (; tmp; tmp = tmp->next) { + instance = yank_find_instance(tmp->value); + if (!instance) { + error_set(errp, ERROR_CLASS_DEVICE_NOT_FOUND, + "Instance '%s' not found", tmp->value); + qemu_mutex_unlock(&lock); + return; + } + } + tmp = instances; + for (; tmp; tmp = tmp->next) { + instance = yank_find_instance(tmp->value); + assert(instance); + QLIST_FOREACH(entry, &instance->yankfns, next) { + entry->func(entry->opaque); + } + } + qemu_mutex_unlock(&lock); +} + +YankInstances *qmp_query_yank(Error **errp) +{ + struct YankInstance *instance; + YankInstances *ret; + + ret = g_new0(YankInstances, 1); + ret->instances = NULL; + + qemu_mutex_lock(&lock); + QLIST_FOREACH(instance, &head, next) { + strList *entry; + entry = g_new0(strList, 1); + entry->value = g_strdup(instance->name); + entry->next = ret->instances; + ret->instances = entry; + } + qemu_mutex_unlock(&lock); + + return ret; +} + +void yank_init(void) +{ + qemu_mutex_init(&lock); +} diff --git a/yank.h b/yank.h new file mode 100644 index 0000000000..f01b370999 --- /dev/null +++ b/yank.h @@ -0,0 +1,69 @@ + +#ifndef YANK_H +#define YANK_H + +typedef void (YankFn) (void *opaque); + +/** + * yank_register_instance: Register a new instance. + * + * This registers a new instance for yanking. Must be called before any yank + * function is registered for this instance. + * + * This function is thread-safe. + * + * @instance_name: The globally unique name of the instance. + */ +void yank_register_instance(char *instance_name); + +/** + * yank_unregister_instance: Unregister a instance. + * + * This unregisters a instance. Must be called only after every yank function + * of the instance has been unregistered. + * + * This function is thread-safe. + * + * @instance_name: The name of the instance. + */ +void yank_unregister_instance(char *instance_name); + +/** + * yank_register_function: Register a yank function + * + * This registers a yank function. All limitations of qmp oob commands apply + * to the yank function as well. + * + * This function is thread-safe. + * + * @instance_name: The name of the instance + * @func: The yank function + * @opaque: Will be passed to the yank function + */ +void yank_register_function(char *instance_name, YankFn *func, void *opaque); + +/** + * yank_unregister_function: Unregister a yank function + * + * This unregisters a yank function. + * + * This function is thread-safe. + * + * @instance_name: The name of the instance + * @func: func that was passed to yank_register_function + * @opaque: opaque that was passed to yank_register_function + */ +void yank_unregister_function(char *instance_name, YankFn *func, void *opaque); + +/** + * yank_unregister_function: Generic yank function for iochannel + * + * This is a generic yank function which will call qio_channel_shutdown on the + * provided QIOChannel. + * + * @opaque: QIOChannel to shutdown + */ +void yank_generic_iochannel(void *opaque); + +void yank_init(void); +#endif
The yank feature allows to recover from hanging qemu by "yanking" at various parts. Other qemu systems can register themselves and multiple yank functions. Then all yank functions for selected instances can be called by the 'yank' out-of-band qmp command. Available instances can be queried by a 'query-yank' oob command. Signed-off-by: Lukas Straub <lukasstraub2@web.de> --- qapi/misc.json | 45 +++++++++++++ softmmu/vl.c | 2 + yank.c | 174 +++++++++++++++++++++++++++++++++++++++++++++++++ yank.h | 69 ++++++++++++++++++++ 4 files changed, 290 insertions(+) create mode 100644 yank.c create mode 100644 yank.h -- 2.20.1