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

[Markus Armbruster]

Peter Xu <address@hidden> writes:

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

Sounds like the commit message needs tweaking then.

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

Paraphrasing, to make sure I understand:

0. We want to test an OOB command can "overtake" commands that hog the
   monitor.

1. We need a command to *reliably* take long enough for us to issue an
   OOB command.  Since OOB commands must not hog the monitor, this
   should be a non-OOB command.

2. We need an OOB command to do the overtaking.

3. We'd very much like to make the overaken command complete quickly
   after the OOB command overtook it, to keep the test quick.

Correct?

Your solution uses x-oob-test both for 1. and 2.  Problematic, because
x-oob-test is OOB, yet does something an OOB command must not do: block
on a lock.  Fixable.

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

Hmm, using migration is actually a cute idea.  But making it take long
enough (1.) is unreliable, as you found.  Also, we can't make it
complete quickly (3.).

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

The bit we actually need is having a command block on something we
control.  You picked a QemuSemaphore, internal to QEMU.  What about some
blocking system call?  Here's my idea:

0. Create a named pipe with mkfifo(1)

1. Issue some (non-OOB) QMP command that opens this pipe.  open()
   blocks.

2. Issue the OOB-command, verify it completes

3. If the non-OOB command writes to the pipe, suck the pipe dry, say
   with cat pipe | >/dev/null.  If it reads, give it something to read,
   say echo >pipe.

To make 3. complete quickly with a command that writes, you need one
that writes only a small amount of data.  Candidates: memsave, pmemsave,
screendump.

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

That one's a hack.  We can make exceptions for hacks.  Where else do we
switch?

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