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

[Markus Armbruster]

"Dr. David Alan Gilbert" <address@hidden> writes:

> * Markus Armbruster (address@hidden) wrote:
>> 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:
>
>> >> >> 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.
>
> Could you return that size in the response to qmp_capabilities
> at the start of the connection?

Awkward.

qmp_capabilities returns version, package and capabilities.  The latter
fits.  Except it's an array, not a struct, and therefore can only say "I
can do capability 'oob'", but can't add "and by the way, 'oob' has
property 'queue-size': 8".

So far, we've used query commands for such stuff.

>> 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.
>
> If I understand what you're saying then this is a shared limit; i.e.
> if you've got two QMP connections then you have to be aware of how many
> the other connection is queuing, which is tricky.

No, the queue is per monitor.  Care to suggest a better wording?

>
> Dave
>
>>  Only a few commands support out-of-band execution.  The ones that do
>>  have "allow-oob": true in output of query-qmp-schema.
> --
> Dr. David Alan Gilbert / address@hidden / Manchester, UK