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

[Peter Xu <peterx@redhat.com>]

On Wed, Jul 18, 2018 at 05:38:11PM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> 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...)

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

Regards,

-- 
Peter Xu