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

[Markus Armbruster <armbru@redhat.com>]

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

Should this go into 3.0 to reduce the risk of bugs?

> Note that thread variables are not initialized to a valid value when new
> thread is created.  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.
>
> 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>