Re: [Qemu-devel] [PATCH 2/2] qemu-thread: let cur_mon be per-thread

[Peter Xu]

On Tue, Apr 10, 2018 at 08:54:31AM -0500, Eric Blake wrote:
> On 04/10/2018 07:49 AM, Peter Xu wrote:
> > cur_mon was only used in main loop so we don't really need that to be
> > per-thread variable.  Now it's possible that we have more than one
> > thread to operate on it.  Let's start to let it be per-thread variable.
> > 
> > In case we'll create threads within a valid cur_mon setup, we'd better
> > let the child threads to inherit the cur_mon from parent thread too.  Do
> > that for both posix and win32 threads.
> > 
> > Signed-off-by: Peter Xu <address@hidden>
> > ---
> >  include/monitor/monitor.h   | 2 +-
> >  include/qemu/thread-win32.h | 1 +
> >  monitor.c                   | 2 +-
> >  stubs/monitor.c             | 2 +-
> >  tests/test-util-sockets.c   | 2 +-
> >  util/qemu-thread-posix.c    | 6 ++++++
> >  util/qemu-thread-win32.c    | 6 ++++++
> >  7 files changed, 17 insertions(+), 4 deletions(-)
> > 
> 
> > @@ -494,6 +496,9 @@ static void *qemu_thread_start(void *args)
> >      void *(*start_routine)(void *) = qemu_thread_args->start_routine;
> >      void *arg = qemu_thread_args->arg;
> >  
> > +    /* Inherit the cur_mon pointer from father thread */
> 
> More typical as s/father/parent/

Fixed.

> 
> > +++ b/util/qemu-thread-win32.c
> 
> > @@ -339,6 +341,9 @@ static unsigned __stdcall win32_start_routine(void *arg)
> >      void *(*start_routine)(void *) = data->start_routine;
> >      void *thread_arg = data->arg;
> >  
> > +    /* Inherit the cur_mon pointer from father thread */
> > +    cur_mon = data->current_monitor;
> 
> Otherwise makes sense to me.
> 
> I agree with your analysis that the set of existing OOB commands (just
> 'x-oob-test') has no direct use of cur_mon.  I'm a little fuzzier on
> whether the OOB changes can cause cur_mon to be modified by two threads
> in parallel (monitor_qmp_dispatch_one() is futzing around with 'cur_mon'
> around the call to qmp_dispatch(), and at least
> qmp_human_monitor_command() is also futzing around with it; is there a
> case where handling qmp_human_monitor_command() in the dispatch thread
> in parallel with more input on the main thread could break?)  Thus I'm
> not sure whether this is needed for 2.12 to avoid a regression.

Could I ask what's the "more input on the main thread"?

AFAIU, if we don't take x-oob-test into account, then still only the
main thread will touch (not only modify, even read) the cur_mon
variable.  And as long as that's true, we are keeping the old behavior
as when we are without Out-Of-Band, then IMHO we'll be fine.

qmp_human_monitor_command() is only one QMP command handler, now it
can only be run within main thread.  So even we can have the stack
like monitor_qmp_dispatch_one (which modifies cur_mon) calls
qmp_human_monitor_command (which modifies cur_mon again), still we'll
be fine too as long as they are in the same thread, just like before.

That's why I think it's not a 2.12 regression.  We just need to be
prepared for what might come. Thanks,

-- 
Peter Xu