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

[Peter Xu]

On Wed, May 23, 2018 at 10:23:22AM +0200, Markus Armbruster wrote:
> Peter Xu <address@hidden> writes:
> 
> > In the future the monitor iothread may be accessing the cur_mon as
> > well (via monitor_qmp_dispatch_one()).  Before we introduce a real
> > Out-Of-Band command,
> 
> Uh, inhowfar are the commands marked allow-oob: true now not real?
> These are migrate-recover, migrate-pause, x-oob-test.

x-oob-test is unreal; the rest are real.

> 
> Aside: having x-oob-test in QEMU proper is awful.  Is there really no
> way around it?

I tried hard to think of a way but failed.  I need to let the
dispatcher stuck for a while, nothing can guarantee that but a lock. I
was using migration before to make sure dispatcher won't quite very
soon, but we have problems there since:

- some architectures may not even have RAM, so snapshot of those
  platforms can still be super fast;

- even if we make sure RAM is big, the time will depend on the speed
  of system, say, what if the test environment is running on RAM-disk
  which is still super fast even to dump the whole RAM onto disk?  As
  a conclusion - time based command won't work, because time is always
  a relative value, which can be affected by OS scheduler and system
  environments.

When there is better suggestion we can remove x-oob-test, but I can't
see any so far.

> 
> >                      let's convert the cur_mon variable to be a
> > per-thread variable to make sure there won't be a race between threads.
> >
> > 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.
> 
> Do we plan to keep switching cur_mon forever?  Or do we intend to work
> towards a 1:1 association between Monitor struct and monitor thread?

I still don't see a good way to remove the cur_mon switching... E.g.,
in qmp_human_monitor_command() we'll switch no matter what.

> 
> Even if we want the latter, I'm okay with this patch as an intermediate
> step.

That'll be appreciated.  Thanks,

> 
> > Signed-off-by: Peter Xu <address@hidden>

-- 
Peter Xu