| Message ID | 20180524043952.11245-4-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | monitor: let Monitor be thread safe | expand |
On Thu, May 24, 2018 at 12:39:51PM +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 | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) Aside from the checkpatch.pl error that needs to be fixed (I like to use a git hook to check automatically, see http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html): Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Regarding the subject: what are "fleids"? 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. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > monitor.c | 12 +++++++++++- > 1 file changed, 11 insertions(+), 1 deletion(-) > > diff --git a/monitor.c b/monitor.c > index d6c3c08932..f01589f945 100644 > --- a/monitor.c > +++ b/monitor.c > @@ -207,7 +207,16 @@ struct Monitor { > int suspend_cnt; /* Needs to be accessed atomically */ > bool skip_flush; > bool use_io_thr; > + > + /* > + * 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 Whoops, misunderstanding! I meant this line as a placeholder, to further illustrate my intent. It should not be committed. If we need a comment here, it should use /* traditional comment syntax */. > + > MonitorQMP qmp; > gchar *mon_cpu_path; > BlockCompletionFunc *password_completion_cb; > @@ -1313,7 +1322,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. Callers must hold BQL. */ > int monitor_set_cpu(int cpu_index) > { > CPUState *cpu; > @@ -1327,6 +1336,7 @@ int monitor_set_cpu(int cpu_index) > return 0; > } > > +/* Callers must hold BQL. */ > static CPUState *mon_get_cpu_sync(bool synchronize) > { > CPUState *cpu;
On Thu, May 24, 2018 at 10:41:09AM +0200, Markus Armbruster wrote: > Regarding the subject: what are "fleids"? Ouch. :( I meant the word "fields". > > 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. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > monitor.c | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > > > diff --git a/monitor.c b/monitor.c > > index d6c3c08932..f01589f945 100644 > > --- a/monitor.c > > +++ b/monitor.c > > @@ -207,7 +207,16 @@ struct Monitor { > > int suspend_cnt; /* Needs to be accessed atomically */ > > bool skip_flush; > > bool use_io_thr; > > + > > + /* > > + * 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 > > Whoops, misunderstanding! I meant this line as a placeholder, to > further illustrate my intent. It should not be committed. If we need a > comment here, it should use /* traditional comment syntax */. My fault. Please let me know if you want me to repost... Thanks,
On Thu, May 24, 2018 at 09:08:04AM +0100, Stefan Hajnoczi wrote: > On Thu, May 24, 2018 at 12:39:51PM +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 | 12 +++++++++++- > > 1 file changed, 11 insertions(+), 1 deletion(-) > > Aside from the checkpatch.pl error that needs to be fixed (I like to use > a git hook to check automatically, see > http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html): > > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com> Thanks Stefan. I start to use it now!
Peter Xu <peterx@redhat.com> writes: > On Thu, May 24, 2018 at 10:41:09AM +0200, Markus Armbruster wrote: >> Regarding the subject: what are "fleids"? > > Ouch. :( I meant the word "fields". Can touch that up in my tree. >> 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. >> > >> > Signed-off-by: Peter Xu <peterx@redhat.com> >> > --- >> > monitor.c | 12 +++++++++++- >> > 1 file changed, 11 insertions(+), 1 deletion(-) >> > >> > diff --git a/monitor.c b/monitor.c >> > index d6c3c08932..f01589f945 100644 >> > --- a/monitor.c >> > +++ b/monitor.c >> > @@ -207,7 +207,16 @@ struct Monitor { >> > int suspend_cnt; /* Needs to be accessed atomically */ >> > bool skip_flush; >> > bool use_io_thr; >> > + >> > + /* >> > + * 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 >> >> Whoops, misunderstanding! I meant this line as a placeholder, to >> further illustrate my intent. It should not be committed. If we need a >> comment here, it should use /* traditional comment syntax */. > > My fault. > > Please let me know if you want me to repost... Thanks, Can touch that up, too. No respin needed unless something more complex comes up.
On Thu, May 24, 2018 at 01:16:11PM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Thu, May 24, 2018 at 10:41:09AM +0200, Markus Armbruster wrote: > >> Regarding the subject: what are "fleids"? > > > > Ouch. :( I meant the word "fields". > > Can touch that up in my tree. [...] > >> > + /* > >> > + * 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 > >> > >> Whoops, misunderstanding! I meant this line as a placeholder, to > >> further illustrate my intent. It should not be committed. If we need a > >> comment here, it should use /* traditional comment syntax */. > > > > My fault. > > > > Please let me know if you want me to repost... Thanks, > > Can touch that up, too. No respin needed unless something more complex > comes up. I'll modify these two parts in my next post. Regards,
diff --git a/monitor.c b/monitor.c index d6c3c08932..f01589f945 100644 --- a/monitor.c +++ b/monitor.c @@ -207,7 +207,16 @@ struct Monitor { int suspend_cnt; /* Needs to be accessed atomically */ bool skip_flush; bool use_io_thr; + + /* + * 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 + MonitorQMP qmp; gchar *mon_cpu_path; BlockCompletionFunc *password_completion_cb; @@ -1313,7 +1322,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. Callers must hold BQL. */ int monitor_set_cpu(int cpu_index) { CPUState *cpu; @@ -1327,6 +1336,7 @@ int monitor_set_cpu(int cpu_index) return 0; } +/* Callers must hold BQL. */ 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 | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-)