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

[Markus Armbruster <armbru@redhat.com>]

Peter Xu <peterx@redhat.com> writes:

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

It's not just the actual OOB commands (there are just two), it's also
the monitor code to read, parse, format and send.

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

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

>> > Reviewed-by: Eric Blake <eblake@redhat.com>
>> > Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>> > Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
>> > [peterx: touch up commit message a bit]
>> > Signed-off-by: Peter Xu <peterx@redhat.com>