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