| Message ID | 20180509041734.14135-4-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, May 09, 2018 at 12:17:33PM +0800, Peter Xu wrote: > Add some explicit comment for both Readline and cpu_set/cpu_get helpers > that they do not need the mon_lock protection. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > monitor.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Peter Xu <peterx@redhat.com> writes: > Add some explicit comment for both Readline and cpu_set/cpu_get helpers > that they do not need the mon_lock protection. Appreciated! > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > monitor.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/monitor.c b/monitor.c > index d6c3c08932..ae5bca9d7c 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -207,7 +207,7 @@ struct Monitor { > int suspend_cnt; /* Needs to be accessed atomically */ > bool skip_flush; > bool use_io_thr; > - ReadLineState *rs; > + ReadLineState *rs; /* Only used in parser, so no lock needed. */ Pardon the ignorant question: why does "only used in parser" imply "no lock needed"? > MonitorQMP qmp; > gchar *mon_cpu_path; > BlockCompletionFunc *password_completion_cb; > @@ -1313,7 +1313,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable, > cur_mon->qmp.commands = &qmp_commands; > } > > -/* set the current CPU defined by the user */ > +/* set the current CPU defined by the user. BQL needed. */ It's okay to start a comment containing a phrase with a lower case letter, but you're turning this one into two sentences, and sentences start in upper case. Can touch up on commit. "BQL needed" is okay, just a bit terse; I'd write "Caller must hold BQL". Could change that, too. > int monitor_set_cpu(int cpu_index) > { > CPUState *cpu; > @@ -1327,6 +1327,7 @@ int monitor_set_cpu(int cpu_index) > return 0; > } > > +/* BQL neeeded. */ Likewise. > static CPUState *mon_get_cpu_sync(bool synchronize) > { > CPUState *cpu;
On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > Add some explicit comment for both Readline and cpu_set/cpu_get helpers > > that they do not need the mon_lock protection. > > Appreciated! > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > monitor.c | 5 +++-- > > 1 file changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/monitor.c b/monitor.c > > index d6c3c08932..ae5bca9d7c 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -207,7 +207,7 @@ struct Monitor { > > int suspend_cnt; /* Needs to be accessed atomically */ > > bool skip_flush; > > bool use_io_thr; > > - ReadLineState *rs; > > + ReadLineState *rs; /* Only used in parser, so no lock needed. */ > > Pardon the ignorant question: why does "only used in parser" imply "no > lock needed"? Since even if the monitors can be run in multiple threads now, the monitor parser of a specific Monitor will still only be run in either the main thread or the monitor iothread. My fault to be unclear on the comment. Maybe this one is better: It is only used in parser, and the parser of a monitor will only be run either in main thread or monitor IOThread but never both, so no lock is needed when accessing ReadLineState. > > > MonitorQMP qmp; > > gchar *mon_cpu_path; > > BlockCompletionFunc *password_completion_cb; > > @@ -1313,7 +1313,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable, > > cur_mon->qmp.commands = &qmp_commands; > > } > > > > -/* set the current CPU defined by the user */ > > +/* set the current CPU defined by the user. BQL needed. */ > > It's okay to start a comment containing a phrase with a lower case > letter, but you're turning this one into two sentences, and sentences > start in upper case. Can touch up on commit. > > "BQL needed" is okay, just a bit terse; I'd write "Caller must hold > BQL". Could change that, too. I'll do that. > > > int monitor_set_cpu(int cpu_index) > > { > > CPUState *cpu; > > @@ -1327,6 +1327,7 @@ int monitor_set_cpu(int cpu_index) > > return 0; > > } > > > > +/* BQL neeeded. */ > > Likewise. Will do. Thanks,
Peter Xu <peterx@redhat.com> writes: > On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> >> > Add some explicit comment for both Readline and cpu_set/cpu_get helpers >> > that they do not need the mon_lock protection. >> >> Appreciated! >> >> > Signed-off-by: Peter Xu <peterx@redhat.com> >> > --- >> > monitor.c | 5 +++-- >> > 1 file changed, 3 insertions(+), 2 deletions(-) >> > >> > diff --git a/monitor.c b/monitor.c >> > index d6c3c08932..ae5bca9d7c 100644 >> > --- a/monitor.c >> > +++ b/monitor.c >> > @@ -207,7 +207,7 @@ struct Monitor { >> > int suspend_cnt; /* Needs to be accessed atomically */ >> > bool skip_flush; >> > bool use_io_thr; >> > - ReadLineState *rs; >> > + ReadLineState *rs; /* Only used in parser, so no lock needed. */ >> >> Pardon the ignorant question: why does "only used in parser" imply "no >> lock needed"? > > Since even if the monitors can be run in multiple threads now, the > monitor parser of a specific Monitor will still only be run in either > the main thread or the monitor iothread. My fault to be unclear on > the comment. Maybe this one is better: > > It is only used in parser, and the parser of a monitor will only be > run either in main thread or monitor IOThread but never both, so no > lock is needed when accessing ReadLineState. One further question, just to help me understand how this stuff works: what are the conditions for the parser running in the main thread, and what are the conditions for it running in the monitor IOThread? [...]
On Wed, May 23, 2018 at 10:29:37AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote: > >> Peter Xu <peterx@redhat.com> writes: > >> > >> > Add some explicit comment for both Readline and cpu_set/cpu_get helpers > >> > that they do not need the mon_lock protection. > >> > >> Appreciated! > >> > >> > Signed-off-by: Peter Xu <peterx@redhat.com> > >> > --- > >> > monitor.c | 5 +++-- > >> > 1 file changed, 3 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/monitor.c b/monitor.c > >> > index d6c3c08932..ae5bca9d7c 100644 > >> > --- a/monitor.c > >> > +++ b/monitor.c > >> > @@ -207,7 +207,7 @@ struct Monitor { > >> > int suspend_cnt; /* Needs to be accessed atomically */ > >> > bool skip_flush; > >> > bool use_io_thr; > >> > - ReadLineState *rs; > >> > + ReadLineState *rs; /* Only used in parser, so no lock needed. */ > >> > >> Pardon the ignorant question: why does "only used in parser" imply "no > >> lock needed"? > > > > Since even if the monitors can be run in multiple threads now, the > > monitor parser of a specific Monitor will still only be run in either > > the main thread or the monitor iothread. My fault to be unclear on > > the comment. Maybe this one is better: > > > > It is only used in parser, and the parser of a monitor will only be > > run either in main thread or monitor IOThread but never both, so no > > lock is needed when accessing ReadLineState. > > One further question, just to help me understand how this stuff works: > what are the conditions for the parser running in the main thread, and > what are the conditions for it running in the monitor IOThread? For QMP parsers, the place is decided by Monitor.use_io_thr. If set, the parser runs in monitor IOThread; otherwise it still runs in main thread. For HMP parsers, they should always been run in the main thread. After replying I just noticed that ReadLineState should only be used by HMP, or to be more explicit, when MONITOR_USE_READLINE is set. So maybe the comment is not really accurate above - actually it never runs in monitor iothread! However the conclusion is still the same - we don't need to protect it. Thanks,
Peter Xu <peterx@redhat.com> writes: > On Wed, May 23, 2018 at 10:29:37AM +0200, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> >> > On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote: >> >> Peter Xu <peterx@redhat.com> writes: >> >> >> >> > Add some explicit comment for both Readline and cpu_set/cpu_get helpers >> >> > that they do not need the mon_lock protection. >> >> >> >> Appreciated! >> >> >> >> > Signed-off-by: Peter Xu <peterx@redhat.com> >> >> > --- >> >> > monitor.c | 5 +++-- >> >> > 1 file changed, 3 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/monitor.c b/monitor.c >> >> > index d6c3c08932..ae5bca9d7c 100644 >> >> > --- a/monitor.c >> >> > +++ b/monitor.c >> >> > @@ -207,7 +207,7 @@ struct Monitor { >> >> > int suspend_cnt; /* Needs to be accessed atomically */ >> >> > bool skip_flush; >> >> > bool use_io_thr; >> >> > - ReadLineState *rs; >> >> > + ReadLineState *rs; /* Only used in parser, so no lock needed. */ >> >> >> >> Pardon the ignorant question: why does "only used in parser" imply "no >> >> lock needed"? >> > >> > Since even if the monitors can be run in multiple threads now, the >> > monitor parser of a specific Monitor will still only be run in either >> > the main thread or the monitor iothread. My fault to be unclear on >> > the comment. Maybe this one is better: >> > >> > It is only used in parser, and the parser of a monitor will only be >> > run either in main thread or monitor IOThread but never both, so no >> > lock is needed when accessing ReadLineState. >> >> One further question, just to help me understand how this stuff works: >> what are the conditions for the parser running in the main thread, and >> what are the conditions for it running in the monitor IOThread? > > For QMP parsers, the place is decided by Monitor.use_io_thr. If set, Aside: spelling it use_io_thread would buy us a bit of readability at a total cost of some 30 characters :) > the parser runs in monitor IOThread; otherwise it still runs in main > thread. Commit a5ed352596a and 3fd2457d18e. I see. > For HMP parsers, they should always been run in the main thread. > > After replying I just noticed that ReadLineState should only be used > by HMP, or to be more explicit, when MONITOR_USE_READLINE is set. So > maybe the comment is not really accurate above - actually it never > runs in monitor iothread! However the conclusion is still the same - > we don't need to protect it. Flags MONITOR_USE_READLINE and MONITOR_USE_CONTROL are independent. However, our CLI currently supports mode=readline (MONITOR_USE_READLINE) and mode=control (MONITOR_USE_CONTROL). If relying on "MONITOR_USE_CONTROL implies !MONITOR_USE_READLINE" makes things simpler, no objections from me, but we should add an assertion. Back to the comment on member @rs. What about /* * State used only in the thread "owning" the monitor. * If @use_io_thr, this is mon_global.mon_iothread. * Else, it's the main thread. * These members can be safely accessed without locks. */ ReadLineState *rs; // other members that aren't shared, if any
On Wed, May 23, 2018 at 05:15:26PM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Wed, May 23, 2018 at 10:29:37AM +0200, Markus Armbruster wrote: > >> Peter Xu <peterx@redhat.com> writes: > >> > >> > On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote: > >> >> Peter Xu <peterx@redhat.com> writes: > >> >> > >> >> > Add some explicit comment for both Readline and cpu_set/cpu_get helpers > >> >> > that they do not need the mon_lock protection. > >> >> > >> >> Appreciated! > >> >> > >> >> > Signed-off-by: Peter Xu <peterx@redhat.com> > >> >> > --- > >> >> > monitor.c | 5 +++-- > >> >> > 1 file changed, 3 insertions(+), 2 deletions(-) > >> >> > > >> >> > diff --git a/monitor.c b/monitor.c > >> >> > index d6c3c08932..ae5bca9d7c 100644 > >> >> > --- a/monitor.c > >> >> > +++ b/monitor.c > >> >> > @@ -207,7 +207,7 @@ struct Monitor { > >> >> > int suspend_cnt; /* Needs to be accessed atomically */ > >> >> > bool skip_flush; > >> >> > bool use_io_thr; > >> >> > - ReadLineState *rs; > >> >> > + ReadLineState *rs; /* Only used in parser, so no lock needed. */ > >> >> > >> >> Pardon the ignorant question: why does "only used in parser" imply "no > >> >> lock needed"? > >> > > >> > Since even if the monitors can be run in multiple threads now, the > >> > monitor parser of a specific Monitor will still only be run in either > >> > the main thread or the monitor iothread. My fault to be unclear on > >> > the comment. Maybe this one is better: > >> > > >> > It is only used in parser, and the parser of a monitor will only be > >> > run either in main thread or monitor IOThread but never both, so no > >> > lock is needed when accessing ReadLineState. > >> > >> One further question, just to help me understand how this stuff works: > >> what are the conditions for the parser running in the main thread, and > >> what are the conditions for it running in the monitor IOThread? > > > > For QMP parsers, the place is decided by Monitor.use_io_thr. If set, > > Aside: spelling it use_io_thread would buy us a bit of readability at a > total cost of some 30 characters :) Sorry for the bad names... Please feel free to change that as follow up patches on any of the namings. I am never good at that. :( > > > the parser runs in monitor IOThread; otherwise it still runs in main > > thread. > > Commit a5ed352596a and 3fd2457d18e. I see. > > > For HMP parsers, they should always been run in the main thread. > > > > After replying I just noticed that ReadLineState should only be used > > by HMP, or to be more explicit, when MONITOR_USE_READLINE is set. So > > maybe the comment is not really accurate above - actually it never > > runs in monitor iothread! However the conclusion is still the same - > > we don't need to protect it. > > Flags MONITOR_USE_READLINE and MONITOR_USE_CONTROL are independent. > However, our CLI currently supports mode=readline (MONITOR_USE_READLINE) > and mode=control (MONITOR_USE_CONTROL). Yeah, so it seems to me the truth is that they are dependent no matter how we implemented the flags. > > If relying on "MONITOR_USE_CONTROL implies !MONITOR_USE_READLINE" makes > things simpler, no objections from me, but we should add an assertion. > > Back to the comment on member @rs. What about > > /* > * State used only in the thread "owning" the monitor. > * If @use_io_thr, this is mon_global.mon_iothread. > * Else, it's the main thread. > * These members can be safely accessed without locks. > */ > ReadLineState *rs; > // other members that aren't shared, if any Sure! Thanks for offering.
diff --git a/monitor.c b/monitor.c index d6c3c08932..ae5bca9d7c 100644 --- a/monitor.c +++ b/monitor.c @@ -207,7 +207,7 @@ struct Monitor { int suspend_cnt; /* Needs to be accessed atomically */ bool skip_flush; bool use_io_thr; - ReadLineState *rs; + ReadLineState *rs; /* Only used in parser, so no lock needed. */ MonitorQMP qmp; gchar *mon_cpu_path; BlockCompletionFunc *password_completion_cb; @@ -1313,7 +1313,7 @@ void qmp_qmp_capabilities(bool has_enable, QMPCapabilityList *enable, cur_mon->qmp.commands = &qmp_commands; } -/* set the current CPU defined by the user */ +/* set the current CPU defined by the user. BQL needed. */ int monitor_set_cpu(int cpu_index) { CPUState *cpu; @@ -1327,6 +1327,7 @@ int monitor_set_cpu(int cpu_index) return 0; } +/* BQL neeeded. */ static CPUState *mon_get_cpu_sync(bool synchronize) { CPUState *cpu;
Add some explicit comment for both Readline and cpu_set/cpu_get helpers that they do not need the mon_lock protection. Signed-off-by: Peter Xu <peterx@redhat.com> --- monitor.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)