| Message ID | 1505375436-28439-8-git-send-email-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | QMP: out-of-band (OOB) execution support | expand |
On 09/14/2017 02:50 AM, Peter Xu wrote: > There are many places for monitor init its globals, at least: > > - monitor_init_qmp_commands() at the very beginning > - single function to init monitor_lock > - in the first entry of monitor_init() using "is_first_init" > > Unify them a bit. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/monitor/monitor.h | 2 +- > monitor.c | 25 ++++++++++--------------- > vl.c | 3 ++- > 3 files changed, 13 insertions(+), 17 deletions(-) > > > +void monitor_init_globals(void) > +{ > + monitor_init_qmp_commands(); > + monitor_qapi_event_init(); > + sortcmdlist(); > + qemu_mutex_init(&monitor_lock); > +} Are we sure that this new function is called sooner than any access to monitor_lock, > -static void __attribute__((constructor)) monitor_lock_init(void) > -{ > - qemu_mutex_init(&monitor_lock); > -} especially since the old code initialized the lock REALLY early? > diff --git a/vl.c b/vl.c > index fb1f05b..850cf55 100644 > --- a/vl.c > +++ b/vl.c > @@ -3049,7 +3049,6 @@ int main(int argc, char **argv, char **envp) > qemu_init_exec_dir(argv[0]); > > module_call_init(MODULE_INIT_QOM); > - monitor_init_qmp_commands(); > > qemu_add_opts(&qemu_drive_opts); > qemu_add_drive_opts(&qemu_legacy_drive_opts); > @@ -4587,6 +4586,8 @@ int main(int argc, char **argv, char **envp) > > parse_numa_opts(current_machine); > > + monitor_init_globals(); > + Pre-patch, a breakpoint on main() and on monitor_lock_init() fires on monitor_lock_init() first, prior to main. Breakpoint 2, monitor_lock_init () at /home/eblake/qemu/monitor.c:4089 4089 qemu_mutex_init(&monitor_lock); (gdb) c Continuing. [New Thread 0x7fffce225700 (LWP 26380)] Thread 1 "qemu-system-x86" hit Breakpoint 1, main (argc=5, argv=0x7fffffffdc88, envp=0x7fffffffdcb8) at vl.c:3077 3077 { Post-patch, the mutex is not initialized until well after main(). So the real question is what (if anything) is using the lock in between those two points? Hmm - it may be that we needed it back before commit 05875687, when we really did depend on MODULE_INIT_QAPI, but it is something we forgot to cleanup in the meantime? If nothing else, the commit message should call out that dropping __attribute__((constructor)) nonsense is intentional (if it was indeed nonsense).
On 09/19/2017 04:35 PM, Eric Blake wrote: > On 09/14/2017 02:50 AM, Peter Xu wrote: >> There are many places for monitor init its globals, at least: >> > Are we sure that this new function is called sooner than any access to > monitor_lock, > >> -static void __attribute__((constructor)) monitor_lock_init(void) >> -{ >> - qemu_mutex_init(&monitor_lock); >> -} > > especially since the old code initialized the lock REALLY early? (Partially) answering myself: > > Pre-patch, a breakpoint on main() and on monitor_lock_init() fires on > monitor_lock_init() first, prior to main. > > Breakpoint 2, monitor_lock_init () at /home/eblake/qemu/monitor.c:4089 > 4089 qemu_mutex_init(&monitor_lock); > (gdb) c > Continuing. > [New Thread 0x7fffce225700 (LWP 26380)] > > Thread 1 "qemu-system-x86" hit Breakpoint 1, main (argc=5, > argv=0x7fffffffdc88, envp=0x7fffffffdcb8) at vl.c:3077 > 3077 { Also, pre-patch, 'watch monitor_lock.initialized' and 'watch monitor_lock.lock.__data.__lock' show that the lock is first utilized at: (gdb) bt #0 0x00007fffdac59e12 in __GI___pthread_mutex_lock (mutex=0x555556399340 <monitor_lock>) at ../nptl/pthread_mutex_lock.c:80 #1 0x0000555555ce01ed in qemu_mutex_lock (mutex=0x555556399340 <monitor_lock>) at util/qemu-thread-posix.c:65 #2 0x00005555557bc8b8 in monitor_init (chr=0x55555690bf70, flags=4) at /home/eblake/qemu/monitor.c:4126 #3 0x000055555591ae80 in mon_init_func (opaque=0x0, opts=0x55555688e3d0, errp=0x0) at vl.c:2482 #4 0x0000555555cf3e63 in qemu_opts_foreach (list=0x555556225200 <qemu_mon_opts>, func=0x55555591ad33 <mon_init_func>, opaque=0x0, errp=0x0) at util/qemu-option.c:1104 #5 0x0000555555920128 in main (argc=5, argv=0x7fffffffdc88, envp=0x7fffffffdcb8) at vl.c:4670 and double-checking qemu_mutex_lock, our .initialized member provides NICE runtime checking that we don't use an uninitialized lock. So the fact that your patch doesn't assert means your later initialization is still fine. [TIL: the gdb 'watch' command is cool, but it's better if you watch only 4 or 8 bytes at a time; I first tried 'watch monitor_lock', but that's hundreds of times slower as hardware can't watch that much data at once, at which point gdb emulates it by single-stepping the entire program] > > Post-patch, the mutex is not initialized until well after main(). So > the real question is what (if anything) is using the lock in between > those two points? According to gdb watchpoints, no. > > Hmm - it may be that we needed it back before commit 05875687, when we > really did depend on MODULE_INIT_QAPI, but it is something we forgot to > cleanup in the meantime? So what I didn't debug was whether the constructor attribute was mandatory in the past, and if so, which commit made it no longer mandatory (my mention of commit 05875687 is only a guess). > > If nothing else, the commit message should call out that dropping > __attribute__((constructor)) nonsense is intentional (if it was indeed > nonsense). > This part is still true.
On Tue, Sep 19, 2017 at 04:48:35PM -0500, Eric Blake wrote: > On 09/19/2017 04:35 PM, Eric Blake wrote: > > On 09/14/2017 02:50 AM, Peter Xu wrote: > >> There are many places for monitor init its globals, at least: > >> > > > Are we sure that this new function is called sooner than any access to > > monitor_lock, > > > >> -static void __attribute__((constructor)) monitor_lock_init(void) > >> -{ > >> - qemu_mutex_init(&monitor_lock); > >> -} > > > > especially since the old code initialized the lock REALLY early? > > (Partially) answering myself: > > > > > Pre-patch, a breakpoint on main() and on monitor_lock_init() fires on > > monitor_lock_init() first, prior to main. > > > > Breakpoint 2, monitor_lock_init () at /home/eblake/qemu/monitor.c:4089 > > 4089 qemu_mutex_init(&monitor_lock); > > (gdb) c > > Continuing. > > [New Thread 0x7fffce225700 (LWP 26380)] > > > > Thread 1 "qemu-system-x86" hit Breakpoint 1, main (argc=5, > > argv=0x7fffffffdc88, envp=0x7fffffffdcb8) at vl.c:3077 > > 3077 { > > Also, pre-patch, 'watch monitor_lock.initialized' and 'watch > monitor_lock.lock.__data.__lock' show that the lock is first utilized at: > > (gdb) bt > #0 0x00007fffdac59e12 in __GI___pthread_mutex_lock > (mutex=0x555556399340 <monitor_lock>) at ../nptl/pthread_mutex_lock.c:80 > #1 0x0000555555ce01ed in qemu_mutex_lock (mutex=0x555556399340 > <monitor_lock>) > at util/qemu-thread-posix.c:65 > #2 0x00005555557bc8b8 in monitor_init (chr=0x55555690bf70, flags=4) > at /home/eblake/qemu/monitor.c:4126 > #3 0x000055555591ae80 in mon_init_func (opaque=0x0, > opts=0x55555688e3d0, errp=0x0) at vl.c:2482 > #4 0x0000555555cf3e63 in qemu_opts_foreach (list=0x555556225200 > <qemu_mon_opts>, func=0x55555591ad33 <mon_init_func>, opaque=0x0, errp=0x0) > at util/qemu-option.c:1104 > #5 0x0000555555920128 in main (argc=5, argv=0x7fffffffdc88, > envp=0x7fffffffdcb8) at vl.c:4670 > > and double-checking qemu_mutex_lock, our .initialized member provides > NICE runtime checking that we don't use an uninitialized lock. So the > fact that your patch doesn't assert means your later initialization is > still fine. Yeah, that's something I liked as well. > > [TIL: the gdb 'watch' command is cool, but it's better if you watch only > 4 or 8 bytes at a time; I first tried 'watch monitor_lock', but that's > hundreds of times slower as hardware can't watch that much data at once, > at which point gdb emulates it by single-stepping the entire program] Good to learn it! Thanks for digging the whole thing up. > > > > > Post-patch, the mutex is not initialized until well after main(). So > > the real question is what (if anything) is using the lock in between > > those two points? > > According to gdb watchpoints, no. > > > > > Hmm - it may be that we needed it back before commit 05875687, when we > > really did depend on MODULE_INIT_QAPI, but it is something we forgot to > > cleanup in the meantime? > > So what I didn't debug was whether the constructor attribute was > mandatory in the past, and if so, which commit made it no longer > mandatory (my mention of commit 05875687 is only a guess). > > > > > If nothing else, the commit message should call out that dropping > > __attribute__((constructor)) nonsense is intentional (if it was indeed > > nonsense). > > > > This part is still true. If this patch is doable, I'll add explicit reason to commit message. Paolo/Markus, would any of you help confirm this change? (considering Paolo introduced commit d622cb587) One thing I slightly not sure of is that, some device realization has this code path (take fsl_imx25_realize() as example): fsl_imx25_realize qemu_chr_new qemu_chr_new_noreplay char is_mux? monitor_init (note: I never know why we create the monitor in chardev creation... would there be a better place?) Especially considering some integrated devices can be created along with machine init. Anyway, this patch was trying to cleanup the things a bit, and also more convenient for me to add new codes upon. If any of us think it's not safe enough, please say explicitly, and I can drop it and do the rest in "the ugly way". Thanks,
diff --git a/include/monitor/monitor.h b/include/monitor/monitor.h index 83ea4a1..3a5128a 100644 --- a/include/monitor/monitor.h +++ b/include/monitor/monitor.h @@ -16,7 +16,7 @@ extern Monitor *cur_mon; bool monitor_cur_is_qmp(void); -void monitor_init_qmp_commands(void); +void monitor_init_globals(void); void monitor_init(Chardev *chr, int flags); void monitor_cleanup(void); diff --git a/monitor.c b/monitor.c index d7eb3c2..7bd2e90 100644 --- a/monitor.c +++ b/monitor.c @@ -1000,7 +1000,7 @@ static void qmp_unregister_commands_hack(void) #endif } -void monitor_init_qmp_commands(void) +static void monitor_init_qmp_commands(void) { /* * Two command lists: @@ -4043,6 +4043,14 @@ static void sortcmdlist(void) qsort((void *)info_cmds, array_num, elem_size, compare_mon_cmd); } +void monitor_init_globals(void) +{ + monitor_init_qmp_commands(); + monitor_qapi_event_init(); + sortcmdlist(); + qemu_mutex_init(&monitor_lock); +} + /* These functions just adapt the readline interface in a typesafe way. We * could cast function pointers but that discards compiler checks. */ @@ -4083,23 +4091,10 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) } } -static void __attribute__((constructor)) monitor_lock_init(void) -{ - qemu_mutex_init(&monitor_lock); -} - void monitor_init(Chardev *chr, int flags) { - static int is_first_init = 1; - Monitor *mon; - - if (is_first_init) { - monitor_qapi_event_init(); - sortcmdlist(); - is_first_init = 0; - } + Monitor *mon = g_malloc(sizeof(*mon)); - mon = g_malloc(sizeof(*mon)); monitor_data_init(mon, false); qemu_chr_fe_init(&mon->chr, chr, &error_abort); diff --git a/vl.c b/vl.c index fb1f05b..850cf55 100644 --- a/vl.c +++ b/vl.c @@ -3049,7 +3049,6 @@ int main(int argc, char **argv, char **envp) qemu_init_exec_dir(argv[0]); module_call_init(MODULE_INIT_QOM); - monitor_init_qmp_commands(); qemu_add_opts(&qemu_drive_opts); qemu_add_drive_opts(&qemu_legacy_drive_opts); @@ -4587,6 +4586,8 @@ int main(int argc, char **argv, char **envp) parse_numa_opts(current_machine); + monitor_init_globals(); + if (qemu_opts_foreach(qemu_find_opts("mon"), mon_init_func, NULL, NULL)) { exit(1);
There are many places for monitor init its globals, at least: - monitor_init_qmp_commands() at the very beginning - single function to init monitor_lock - in the first entry of monitor_init() using "is_first_init" Unify them a bit. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/monitor/monitor.h | 2 +- monitor.c | 25 ++++++++++--------------- vl.c | 3 ++- 3 files changed, 13 insertions(+), 17 deletions(-)