Re: [Qemu-devel] [PATCH v7 3/4] monitor: more comments on lock-free fleids/funcs

[Peter Xu]

On Thu, May 24, 2018 at 01:16:11PM +0200, Markus Armbruster wrote:
> Peter Xu  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,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v7 3/4] monitor: more comments on lock-free fleids/funcs

[Markus Armbruster]

Peter Xu  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  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 
>> > ---
>> >  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.

Re: [Qemu-devel] [PATCH v7 3/4] monitor: more comments on lock-free fleids/funcs

[Peter Xu]

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  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 
> > ---
> >  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,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v7 3/4] monitor: more comments on lock-free fleids/funcs

[Peter Xu]

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 
> > ---
> >  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 

Thanks Stefan.  I start to use it now!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v7 3/4] monitor: more comments on lock-free fleids/funcs

[Markus Armbruster]

Regarding the subject: what are "fleids"?

Peter Xu  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 
> ---
>  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 = _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;

Re: [Qemu-devel] [PATCH v7 3/4] monitor: more comments on lock-free fleids/funcs

[Stefan Hajnoczi]

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 
> ---
>  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 


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH v7 3/4] monitor: more comments on lock-free fleids/funcs

[Peter Xu]

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 
---
 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
+
 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 = _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;
-- 
2.17.0