Re: [Qemu-devel] [PATCH v7 4/7] qapi: remove COMMAND_DROPPED event

[Markus Armbruster]

Daniel P. Berrangé <address@hidden> writes:

> On Tue, Sep 04, 2018 at 08:39:27AM +0200, Markus Armbruster wrote:
>> Daniel P. Berrangé <address@hidden> writes:
>> 
>> > On Mon, Sep 03, 2018 at 09:30:52AM -0500, Eric Blake wrote:
>> >> On 09/03/2018 08:31 AM, Markus Armbruster wrote:
>> >> 
>> >> > Example:
>> >> > 
>> >> >      client sends in-band command #1
>> >> >      QEMU reads and queues
>> >> >      QEMU dequeues in-band command #1
>> >> >      in-band command #1 starts executing, but it's slooow
>> >> >      client sends in-band command #2
>> >> >      QEMU reads and queues
>> >> >      ...
>> >> >      client sends in-band command #8
>> >> >      QEMU reads, queues and suspends the monitor
>> >> >      client sends out-of-band command
>> >> > --> time passes...
>> >> >      in-band command #1 completes, QEMU sends reply
>> >> >      QEMU dequeues in-band command #2, resumes the monitor
>> >> >      in-band command #2 starts executing
>> >> >      QEMU reads and executes out-of-band command
>> >> >      out-of-band command completes, QEMU sends reply
>> >> >      in-band command #2 completes, QEMU sends reply
>> >> >      ... same for remaining in-band commands ...
>> >> > 
>> >> > The out-of-band command gets stuck behind the in-band commands.
>> >> > 
>> >> > The client can avoid this by managing the flow of in-band commands: have
>> >> > no more than 7 in flight, so the monitor never gets suspended.
>> >> > 
>> >> > This is a potentially useful thing to do for clients, isn't it?
>> >> > 
>> >> > Eric, Daniel, is it something libvirt would do?
>> >> 
>> >> Right now, libvirt serializes commands - it never sends a QMP command 
>> >> until
>> >> the previous command's response has been processed. But that may not help
>> >> much, since libvirt does not send OOB commands.
>> >
>> > Note that is not merely due to the QMP monitor restriction either.
>> >
>> > Libvirt serializes all its public APIs that can change state of a running
>> > domain.  It usually aims to allow read-only APIs to be run in parallel with
>> > APIs that change state.
>> 
>> Makes sense.  State changes are complex enough without concurrency.
>> Even permitting just concurrent queries can add non-trivial complexity.
>> 
>> However, pipelineing != concurrency.  "Serializing" as I understand it
>> implies no concurrency, it doesn't imply no pipelining.
>> 
>> Mind, I'm not telling you to pipeline, I'm just trying to understand the
>> constraints.
>
> I can only think of one place where libvirt would be likely to use
> pipelining. We have an API that is used for collecting bulk stats
> of many types, across al VMs. We do a couple of QMP queries per-VM,
> so we could pipeline those queries. Even then, I'm not sure we would
> go to the bother, as the bigger burden for performance is that we
> round-robin across every VM. A bigger bang for our buck would be
> to parallelize across VMs, but still serialize within VMs, as that
> would have less complexity.
>
>> > The exception to the rule right now are some of the migration APIs which
>> > we allow to be invoked to manage the migration process. 
>> 
>> Can you give an example?
>
> We have a job that triggers the migration and sits in a thread
> monitoring its progress.
>
> While this is happening we allow a few other API calls such as
> "pause", and of course things like switching to post-copy mode,
> changin max downtime, etc. None of this gets in to parallel
> or pipelined monitor execution though.

At the QMP command level, these are all in-band commands issued
synchronously, except for out-of-band migrate-recover and migrate-pause.

>> >> I guess when we are designing what libvirt should do, and deciding WHEN it
>> >> should send OOB commands, we have the luxury of designing libvirt to 
>> >> enforce
>> >> how many in-flight in-band commands it will ever have pending at once
>> >> (whether the current 'at most 1', or even if we make it more parallel to 
>> >> 'at
>> >> most 7'), so that we can still be ensured that the OOB command will be
>> >> processed without being stuck in the queue of suspended in-band commands.
>> >> If we never send more than one in-band at a time, then it's not a concern
>> >> how deep the qemu queue is; but if we do want libvirt to start parallel
>> >> in-band commands, then you are right that having a way to learn the qemu
>> >> queue depth is programmatically more precise than just guessing the 
>> >> maximum
>> >> depth.  But it's also hard to argue we need that complexity if we don't 
>> >> have
>> >> an immediate use envisioned for it.
>> 
>> True.
>> 
>> Options for the initial interface:
>> 
>> (1) Provide means for the client to determine the queue length limit
>>     (introspection or configuration).  Clients that need the monitory to
>>     remain available for out-of-band commands can keep limit - 1 in-band
>>     commands in flight.
>> 
>> (2) Make the queue length limit part of the documented interface.
>>     Clients that need the monitory to remain available for out-of-band
>>     commands can keep limit - 1 in-band commands in flight.  We can
>>     increase the limit later, but not decrease it.  We can also switch
>>     to (1) as needed.
>> 
>> (3) Treat the queue length limit as implementation detail (but tacitly
>>     assume its at least 2, since less makes no sense[*]).  Clients that
>>     need the monitory to remain available for out-of-band commands
>>     cannot safely keep more than one in-band command in flight.  We can
>>     switch to (2) or (1) as needed.
>> 
>> Opinions?
>
> If you did (3), effectively apps will be behaving as if you had done
> (2) with a documented queue limit of 2, so why not just do (2) right
> away.

Yup, that's what I thought, too.

I append a proposed replacement for the patch to qmp-spec.txt.  It
assumes the current queue size 8.  That value is of course open to
debate.


diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt
index 67e44a8120..1fc6a507e2 100644
--- a/docs/interop/qmp-spec.txt
+++ b/docs/interop/qmp-spec.txt
@@ -130,9 +130,11 @@ to pass "id" with out-of-band commands.  Passing it with 
all commands
 is recommended for clients that accept capability "oob".
 
 If the client sends in-band commands faster than the server can
-execute them, the server will stop reading the requests from the QMP
-channel until the request queue length is reduced to an acceptable
-range.
+execute them, the server's queue will fill up, and the server will
+stop reading commands from the QMP channel until the queue has space
+again.  Clients that need the server to remain responsive to
+out-of-band commands should avoid filling up the queue by limiting the
+number of in-band commands in flight to seven.
 
 Only a few commands support out-of-band execution.  The ones that do
 have "allow-oob": true in output of query-qmp-schema.