On Thu, Sep 07, 2023 at 01:06:39AM +0000, Dr. David Alan Gilbert wrote: > * Stefan Hajnoczi (stefa...@redhat.com) wrote: > > Coroutine HMP commands currently run to completion in a nested event > > loop with the Big QEMU Lock (BQL) held. The call_rcu thread also uses > > the BQL and cannot process work while the coroutine monitor command is > > running. A deadlock occurs when monitor commands attempt to wait for > > call_rcu work to finish. > > I hate to think if there's anywhere else that ends up doing that > other than the monitors.
Luckily drain_call_rcu() has few callers: just
xen_block_device_destroy() and qmp_device_add(). We only need to worry
about their call stacks.
I haven't looked at the Xen code.
>
> But, not knowing the semantics of the rcu code, it looks kind of OK to
> me from the monitor.
>
> (Do you ever get anything like qemu quitting from one of the other
> monitors while this coroutine hasn't been run?)
Not sure what you mean?
Stefan
>
> Dave
>
> > This patch refactors the HMP monitor to use the existing event loop
> > instead of creating a nested event loop. This will allow the next
> > patches to rely on draining call_rcu work.
> >
> > Signed-off-by: Stefan Hajnoczi <stefa...@redhat.com>
> > ---
> > monitor/hmp.c | 28 +++++++++++++++-------------
> > 1 file changed, 15 insertions(+), 13 deletions(-)
> >
> > diff --git a/monitor/hmp.c b/monitor/hmp.c
> > index 69c1b7e98a..6cff2810aa 100644
> > --- a/monitor/hmp.c
> > +++ b/monitor/hmp.c
> > @@ -1111,15 +1111,17 @@ typedef struct HandleHmpCommandCo {
> > Monitor *mon;
> > const HMPCommand *cmd;
> > QDict *qdict;
> > - bool done;
> > } HandleHmpCommandCo;
> >
> > -static void handle_hmp_command_co(void *opaque)
> > +static void coroutine_fn handle_hmp_command_co(void *opaque)
> > {
> > HandleHmpCommandCo *data = opaque;
> > +
> > handle_hmp_command_exec(data->mon, data->cmd, data->qdict);
> > monitor_set_cur(qemu_coroutine_self(), NULL);
> > - data->done = true;
> > + qobject_unref(data->qdict);
> > + monitor_resume(data->mon);
> > + g_free(data);
> > }
> >
> > void handle_hmp_command(MonitorHMP *mon, const char *cmdline)
> > @@ -1157,20 +1159,20 @@ void handle_hmp_command(MonitorHMP *mon, const char
> > *cmdline)
> > Monitor *old_mon = monitor_set_cur(qemu_coroutine_self(),
> > &mon->common);
> > handle_hmp_command_exec(&mon->common, cmd, qdict);
> > monitor_set_cur(qemu_coroutine_self(), old_mon);
> > + qobject_unref(qdict);
> > } else {
> > - HandleHmpCommandCo data = {
> > - .mon = &mon->common,
> > - .cmd = cmd,
> > - .qdict = qdict,
> > - .done = false,
> > - };
> > - Coroutine *co = qemu_coroutine_create(handle_hmp_command_co,
> > &data);
> > + HandleHmpCommandCo *data; /* freed by handle_hmp_command_co() */
> > +
> > + data = g_new(HandleHmpCommandCo, 1);
> > + data->mon = &mon->common;
> > + data->cmd = cmd;
> > + data->qdict = qdict; /* freed by handle_hmp_command_co() */
> > +
> > + Coroutine *co = qemu_coroutine_create(handle_hmp_command_co, data);
> > + monitor_suspend(&mon->common); /* resumed by
> > handle_hmp_command_co() */
> > monitor_set_cur(co, &mon->common);
> > aio_co_enter(qemu_get_aio_context(), co);
> > - AIO_WAIT_WHILE_UNLOCKED(NULL, !data.done);
> > }
> > -
> > - qobject_unref(qdict);
> > }
> >
> > static void cmd_completion(MonitorHMP *mon, const char *name, const char
> > *list)
> > --
> > 2.41.0
> >
> >
> --
> -----Open up your eyes, open up your mind, open up your code -------
> / Dr. David Alan Gilbert | Running GNU/Linux | Happy \
> \ dave @ treblig.org | | In Hex /
> \ _________________________|_____ http://www.treblig.org |_______/
>
signature.asc
Description: PGP signature
