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

[Peter Xu]

On Wed, May 23, 2018 at 05:15:26PM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
> 
> > On Wed, May 23, 2018 at 10:29:37AM +0200, Markus Armbruster wrote:
> >> Peter Xu <address@hidden> writes:
> >> 
> >> > On Thu, May 17, 2018 at 02:46:36PM +0200, Markus Armbruster wrote:
> >> >> Peter Xu <address@hidden> 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 <address@hidden>
> >> >> > ---
> >> >> >  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.

-- 
Peter Xu