On Thu, 7 Sept 2023 at 16:53, Dr. David Alan Gilbert <d...@treblig.org> wrote: > > * Stefan Hajnoczi (stefa...@gmail.com) wrote: > > On Thu, 7 Sept 2023 at 10:07, Dr. David Alan Gilbert <d...@treblig.org> > > wrote: > > > > > > * Stefan Hajnoczi (stefa...@redhat.com) wrote: > > > > 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? > > > > > > Imagine that just after you create your coroutine, a vCPU does a > > > shutdown and qemu is configured to quit, or on another monitor someone > > > does a quit; does your coroutine get executed or not? > > > > I think the answer is that it depends. > > > > A coroutine can run for a while and then yield while waiting for a > > timer, BH, fd handler, etc. If the coroutine has yielded then I think > > QEMU could terminate. > > > > The behavior of entering a coroutine for the first time depends on the > > API that is used (e.g. qemu_coroutine_enter()/aio_co_enter()/etc). > > qemu_coroutine_enter() is immediate but aio_co_enter() contains > > indirect code paths like scheduling a BH. > > > > To summarize: ¯\_(ツ)_/¯ > > That does mean you leave your g_new'd data and qdict allocated at > exit - meh > > I'm not sure if it means you're making any other assumptions about what > happens if the coroutine gets run during the exit path; although I guess > there are plenty of other cases like that.
Yes, I think QEMU has some resources (memory, file descriptors, etc) that are not freed on exit. Stefan > > Dave > > > Stefan > > > > > > > > Dave > > > > > > > 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 |_______/ > > > > > > > > > > > > > > -- > > > -----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 |_______/ > > > > -- > -----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 |_______/