Re: [Qemu-devel] [PATCH v4] monitor: let cur_mon be per-thread

[Peter Xu]

On Thu, Jul 19, 2018 at 09:20:34AM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
> 
> > On Wed, Jul 18, 2018 at 05:38:11PM +0200, Markus Armbruster wrote:
> >> Peter Xu <address@hidden> writes:
> >> 
> >> > After the Out-Of-Band work, the monitor iothread may be accessing the
> >> > cur_mon as well (via monitor_qmp_dispatch_one()).  Let's convert the
> >> > cur_mon variable to be a per-thread variable to make sure there won't be
> >> > a race between threads when accessing the variable.
> >> 
> >> Hmm... why hasn't the OOB work created such a race already?
> >> 
> >> A monitor reads, parses, dispatches and executes commands, formats and
> >> sends replies.
> >> 
> >> Before OOB, all of that ran in the main thread.  Any access of cur_mon
> >> should therefore be from the main thread.  No races.
> >> 
> >> OOB moves read, parse, format and send to an I/O thread.  Dispatch and
> >> execute remain in the main thread.  *Except* for commands executed OOB,
> >> dispatch and execute move to the I/O thread, too.
> >> 
> >> Why is this not racy?  I guess it relies on careful non-use of cur_mon
> >> in any part that may now execute in the I/O thread.  Scary...
> >
> > I think it's because cur_mon is not really used in out-of-band command
> > executions - now we only have a few out-of-band enabled commands, and
> > IIUC none of them is using cur_mon (for example, in
> > qmp_migrate_recover() we don't even call error_report, and the code
> > path is quite straight forward to make sure of that).  So IIUC cur_mon
> > variable is still only touched by main thread for now hence we should
> > be safe.  However that condition might change in the future when we
> > add more out-of-band capable commands.
> >
> > (not to mention that I don't even know whether there are real users of
> >  out-of-band if we haven't yet started to support that for libvirt...)
> 
> It's not just the actual OOB commands (there are just two), it's also
> the monitor code to read, parse, format and send.

My understanding is that read, parse, format, send will not touch
cur_mon (it was touched before but some patches in the out-of-band
series should have removed the last users when parsing).  So IIUC only
the dispatcher would touch that now.  I didn't consider the callers
like net_init_socket() and I'm only considering the monitor code (and
those callers should be only in the main thread too after all).

> 
> >> Should this go into 3.0 to reduce the risk of bugs?
> >
> > Yes I think it would be good to have that even for 3.0, since it still
> > can be seen as a bug fix of existing code.
> 
> Agreed.
> 
> > Regards,
> >
> >> > Note that thread variables are not initialized to a valid value when new
> >> > thread is created.
> 
> Confusing.  It sounds like @cur_mon's initial value would be
> indeterminate, like an automatic variable's.  Not true.  Variables with
> thread storage duration are initialized when the thread is created.
> Since @cur_mon's declaration lacks an initializer, it'll be initialized
> to a null pointer.  Your sentence is correct when you consider that null
> pointer not a valid value.

Yes that's what I meant.  So how about this?

  Note that the per-thread @cur_mon variable is not initialized to
  point to a valid Monitor struct when a new thread is created (the
  default value will be NULL).

Please feel free to tune it up.

> 
> >> >                     However for our case we don't need to set it up,
> >> > since the cur_mon variable is only used in such a pattern:
> >> > 
> >> >   old_mon = cur_mon;
> >> >   cur_mon = xxx;
> >> >   (do something, read cur_mon if necessary in the stack)

[1]

> >> >   cur_mon = old_mon;
> >> > 
> >> > It plays a role as stack variable, so no need to be initialized at all.
> >> > We only need to make sure the variable won't be changed unexpectedly by
> >> > other threads.
> 
> Do we need this paragraph?  The commit doesn't mess with @cur_mon's
> initial value at all...

I was trying to explain why we don't need to initialize that variable
for each thread.  A common idea (at least that's what I have had in
mind) is that when we create a new thread we should possibly inherit
that @cur_mon variable in a copy-on-write fashion for that new thread.
But that's not really necessary for the use case like above (as long
as we don't create thread during [1], and that's what we do).

If you think the patch explains itself better without these lines,
please feel free to drop it.

> 
> >> > Reviewed-by: Eric Blake <address@hidden>
> >> > Reviewed-by: Marc-André Lureau <address@hidden>
> >> > Reviewed-by: Stefan Hajnoczi <address@hidden>
> >> > [peterx: touch up commit message a bit]
> >> > Signed-off-by: Peter Xu <address@hidden>

Thanks,

-- 
Peter Xu