| Message ID | 20180903043149.4076-5-peterx@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v7,1/7] qapi: Fix build_params() for empty parameter list | expand |
Peter Xu <peterx@redhat.com> writes: > Now it was not used any more, drop it. We can still do that since > out-of-band is still experimental, and this event is only used when > out-of-band is enabled. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > docs/interop/qmp-spec.txt | 5 +++-- > qapi/misc.json | 40 --------------------------------------- > 2 files changed, 3 insertions(+), 42 deletions(-) > > diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt > index 8f7da0245d..67e44a8120 100644 > --- a/docs/interop/qmp-spec.txt > +++ b/docs/interop/qmp-spec.txt > @@ -130,8 +130,9 @@ 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 eventually drop commands to limit the > -queue length. The sever sends event COMMAND_DROPPED then. > +execute them, the server will stop reading the requests from the QMP > +channel until the request queue length is reduced to an acceptable > +range. The change described by this documentation update is in the previous commit. This commit merely cleans up unused code. Instead of moving the doc update to the previous commit, we can simply squash the two commits. Let's add a hint on managing flow of in-band commands to keep the monitor available for out-of-band commands. Here's my try: 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 its request queue length falls below the limit. To keep the monitor available for out-of-band commands, the client has to manage the flow of in-band commands. Of course, anyone trying to put this hint to use will need to know the actual request queue limit. Options: * Make it ABI by documenting it here * Let the client configure it with a suitable command * Let the introspect it with a suitable command > > Only a few commands support out-of-band execution. The ones that do > have "allow-oob": true in output of query-qmp-schema. > diff --git a/qapi/misc.json b/qapi/misc.json > index d450cfef21..2c1a6119bf 100644 > --- a/qapi/misc.json > +++ b/qapi/misc.json > @@ -3432,46 +3432,6 @@ > ## > { 'command': 'query-sev-capabilities', 'returns': 'SevCapability' } > > -## > -# @CommandDropReason: > -# > -# Reasons that caused one command to be dropped. > -# > -# @queue-full: the command queue is full. This can only occur when > -# the client sends a new non-oob command before the > -# response to the previous non-oob command has been > -# received. > -# > -# Since: 2.12 > -## > -{ 'enum': 'CommandDropReason', > - 'data': [ 'queue-full' ] } > - > -## > -# @COMMAND_DROPPED: > -# > -# Emitted when a command is dropped due to some reason. Commands can > -# only be dropped when the oob capability is enabled. > -# > -# @id: The dropped command's "id" field. > -# FIXME Broken by design. Events are broadcast to all monitors. If > -# another monitor's client has a command with the same ID in flight, > -# the event will incorrectly claim that command was dropped. > -# > -# @reason: The reason why the command is dropped. > -# > -# Since: 2.12 > -# > -# Example: > -# > -# { "event": "COMMAND_DROPPED", > -# "data": {"result": {"id": "libvirt-102", > -# "reason": "queue-full" } } } > -# > -## > -{ 'event': 'COMMAND_DROPPED' , > - 'data': { 'id': 'any', 'reason': 'CommandDropReason' } } > - > ## > # @set-numa-node: > #
On Mon, Sep 03, 2018 at 09:49:13AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > Now it was not used any more, drop it. We can still do that since > > out-of-band is still experimental, and this event is only used when > > out-of-band is enabled. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > docs/interop/qmp-spec.txt | 5 +++-- > > qapi/misc.json | 40 --------------------------------------- > > 2 files changed, 3 insertions(+), 42 deletions(-) > > > > diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt > > index 8f7da0245d..67e44a8120 100644 > > --- a/docs/interop/qmp-spec.txt > > +++ b/docs/interop/qmp-spec.txt > > @@ -130,8 +130,9 @@ 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 eventually drop commands to limit the > > -queue length. The sever sends event COMMAND_DROPPED then. > > +execute them, the server will stop reading the requests from the QMP > > +channel until the request queue length is reduced to an acceptable > > +range. > > The change described by this documentation update is in the previous > commit. This commit merely cleans up unused code. Instead of moving > the doc update to the previous commit, we can simply squash the two > commits. Sure, I'll squash. > > Let's add a hint on managing flow of in-band commands to keep the > monitor available for out-of-band commands. Here's my try: > > 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 its request queue length falls below the limit. To > keep the monitor available for out-of-band commands, the client has > to manage the flow of in-band commands. > > Of course, anyone trying to put this hint to use will need to know the > actual request queue limit. Options: > > * Make it ABI by documenting it here > > * Let the client configure it with a suitable command > > * Let the introspect it with a suitable command I'm not sure whether we should expose this information to the client, considering that the client can after all detect that "full" by monitoring the status of its output buffer used (e.g., poll with POLLOUT) and assuming that should be enough for a client. Or did I miss anything that you were trying to emphasize but I didn't notice (since after all you mentioned this too in the other thread)? Regards,
Cc: Eric & Daniel for the libvirt point of view. Peter Xu <peterx@redhat.com> writes: > On Mon, Sep 03, 2018 at 09:49:13AM +0200, Markus Armbruster wrote: >> Peter Xu <peterx@redhat.com> writes: >> >> > Now it was not used any more, drop it. We can still do that since >> > out-of-band is still experimental, and this event is only used when >> > out-of-band is enabled. >> > >> > Signed-off-by: Peter Xu <peterx@redhat.com> >> > --- >> > docs/interop/qmp-spec.txt | 5 +++-- >> > qapi/misc.json | 40 --------------------------------------- >> > 2 files changed, 3 insertions(+), 42 deletions(-) >> > >> > diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt >> > index 8f7da0245d..67e44a8120 100644 >> > --- a/docs/interop/qmp-spec.txt >> > +++ b/docs/interop/qmp-spec.txt >> > @@ -130,8 +130,9 @@ 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 eventually drop commands to limit the >> > -queue length. The sever sends event COMMAND_DROPPED then. >> > +execute them, the server will stop reading the requests from the QMP >> > +channel until the request queue length is reduced to an acceptable >> > +range. >> >> The change described by this documentation update is in the previous >> commit. This commit merely cleans up unused code. Instead of moving >> the doc update to the previous commit, we can simply squash the two >> commits. > > Sure, I'll squash. > >> >> Let's add a hint on managing flow of in-band commands to keep the >> monitor available for out-of-band commands. Here's my try: >> >> 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 its request queue length falls below the limit. To >> keep the monitor available for out-of-band commands, the client has >> to manage the flow of in-band commands. >> >> Of course, anyone trying to put this hint to use will need to know the >> actual request queue limit. Options: >> >> * Make it ABI by documenting it here >> >> * Let the client configure it with a suitable command >> >> * Let the introspect it with a suitable command > > I'm not sure whether we should expose this information to the client, > considering that the client can after all detect that "full" by > monitoring the status of its output buffer used (e.g., poll with > POLLOUT) and assuming that should be enough for a client. Or did I > miss anything that you were trying to emphasize but I didn't notice > (since after all you mentioned this too in the other thread)? By the time the client gets "write would block", OOB has already become unavailable, and will remain so until the logjam clears sufficiently. 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?
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. 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.
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. The exception to the rule right now are some of the migration APIs which we allow to be invoked to manage the migration process. > 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. In terms of what libvirt would want to parallelize, I think it is reasonable to consider any of the query-XXXX commands desirable. Other stuff is likely to remain serialized from libvirt's side. Regards, Daniel
On Mon, Sep 03, 2018 at 03:41:16PM +0100, Daniel P. Berrangé wrote: > 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. (It's a shame of me to have just noticed that the out-of-band command will be stuck after we dropped the COMMAND_DROP event... so now I agree it's not that ideal any more to drop the event but maybe still preferable) > > > > > > 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. > > The exception to the rule right now are some of the migration APIs which > we allow to be invoked to manage the migration process. > > > 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. > > In terms of what libvirt would want to parallelize, I think it is reasonable > to consider any of the query-XXXX commands desirable. Other stuff is likely > to remain serialized from libvirt's side. IMHO concurrency won't help much now even for query commands, since our current concurrency is still "partly" - the executions of query commands (which is the most time consuming part) will still be done sequentially, so even if we send multiple query commands in parallel (without waiting for a response of any sent commands), the total time used for the list of commands would be mostly the same. My understanding for why we have such a queue length now is that it came from a security concern: after we have a queue, we need that queue length to limit the memory usages for the QMP server. Though that might not help much for real users like Libvirt, it's majorly serving as a way to protect QEMU QMP from being attacked or from being turned down by a buggy QMP client. But I agree now that the queue length information might still be helpful some day. Maybe, we can hide that until we support executing commands in parallel for some of them. Regards,
Daniel P. Berrangé <berrange@redhat.com> 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. > 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? >> 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? > In terms of what libvirt would want to parallelize, I think it is reasonable > to consider any of the query-XXXX commands desirable. Other stuff is likely > to remain serialized from libvirt's side. QEMU doesn't care whether it queues in-band query commands or other in-band commands. [*] With a queue length of 1, the monitor suspends on receipt of an in-band command, and resumes after it completes. This renders out-of-band commands useless: there is no way an out-of-band command can overtake an in-band command.
Peter Xu <peterx@redhat.com> writes: > On Mon, Sep 03, 2018 at 03:41:16PM +0100, Daniel P. Berrangé wrote: >> 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. > > (It's a shame of me to have just noticed that the out-of-band command > will be stuck after we dropped the COMMAND_DROP event... so now I > agree it's not that ideal any more to drop the event but maybe still > preferable) We can queue without limit, we can drop commands, or we can suspend reading. Each of these has drawbacks: * Queuing without limit is simple for the client, but unsafe for QEMU. * Dropping commands requires the client to cope with dropped commands. As currently designed, it's just as unsafe for QEMU: instead of queuing commands without limit, we get to queue their COMMAND_DROPPED events without limit. A better design could avoid this flaw. * Suspending reading requires the client to manage the flow of commands if it wants to keep the monitor available for out-of-band commands. We decided that clients having to manage the flow of commands is no worse than clients having to cope with dropped commands. There's still time to challenge this decision. This series acts upon the decision: it switches from dropping commands to suspending reading. Makes the input direction safe. The output direction remains as unsafe as it's always been. Fixing that is left for later. >> > > >> > > 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. >> >> The exception to the rule right now are some of the migration APIs which >> we allow to be invoked to manage the migration process. >> >> > 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. >> >> In terms of what libvirt would want to parallelize, I think it is reasonable >> to consider any of the query-XXXX commands desirable. Other stuff is likely >> to remain serialized from libvirt's side. > > IMHO concurrency won't help much now even for query commands, since > our current concurrency is still "partly" - the executions of query > commands (which is the most time consuming part) will still be done > sequentially, so even if we send multiple query commands in parallel > (without waiting for a response of any sent commands), the total time > used for the list of commands would be mostly the same. Yes. We execute all in-band commands (regardless of their monitor) in the main thread. Out-of-band commands can execute in @mon_iothread, which provides a modest degree of concurrency. > My understanding for why we have such a queue length now is that it > came from a security concern: after we have a queue, we need that > queue length to limit the memory usages for the QMP server. Though > that might not help much for real users like Libvirt, it's majorly > serving as a way to protect QEMU QMP from being attacked or from being > turned down by a buggy QMP client. Yes. QEMU has to trust its QMP clients, so malice is not a concern, but accidents are. Robust software does not buffer without bounds. > But I agree now that the queue length information might still be > helpful some day. Maybe, we can hide that until we support executing > commands in parallel for some of them. Queue length can become interesting long before we get general concurrency. If you use QMP only synchronously (send command #1; receive reply #1; send command #2; ...), then out-of-band does exactly nothing for you. To make use of it, you have to send an out-of-band command *before* you receive the previous command's reply. That's a form of pipelining. Note there's still no general concurrency. There's a bit of pipelining, and there's a bit of concurrency between one in-band command (executing in main thread) and out-of-band command (executing in @mon_iothread). Since we need to support a bit of pipelining anyway, why not support it more generally? All it takes it raising the queue length limit above the minimum required for the use of OOB I just sketched. Note that "since we need to support a bit of concurrency anyway, why not support it more generally?" would be ludicrously naive :)
On Tue, Sep 04, 2018 at 08:39:27AM +0200, Markus Armbruster wrote: > Daniel P. Berrangé <berrange@redhat.com> 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. > >> 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. Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> writes: > On Tue, Sep 04, 2018 at 08:39:27AM +0200, Markus Armbruster wrote: >> Daniel P. Berrangé <berrange@redhat.com> 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.
On Tue, Sep 04, 2018 at 10:04:00AM +0200, Markus Armbruster wrote: > Peter Xu <peterx@redhat.com> writes: > > > On Mon, Sep 03, 2018 at 03:41:16PM +0100, Daniel P. Berrangé wrote: > >> 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. > > > > (It's a shame of me to have just noticed that the out-of-band command > > will be stuck after we dropped the COMMAND_DROP event... so now I > > agree it's not that ideal any more to drop the event but maybe still > > preferable) > > We can queue without limit, we can drop commands, or we can suspend > reading. Each of these has drawbacks: > > * Queuing without limit is simple for the client, but unsafe for QEMU. > > * Dropping commands requires the client to cope with dropped commands. > As currently designed, it's just as unsafe for QEMU: instead of > queuing commands without limit, we get to queue their COMMAND_DROPPED > events without limit. A better design could avoid this flaw. > > * Suspending reading requires the client to manage the flow of commands > if it wants to keep the monitor available for out-of-band commands. > > We decided that clients having to manage the flow of commands is no > worse than clients having to cope with dropped commands. There's still > time to challenge this decision. > > This series acts upon the decision: it switches from dropping commands > to suspending reading. Makes the input direction safe. The output > direction remains as unsafe as it's always been. Fixing that is left > for later. Yes. Options (1) and (2) seems not really acceptable for me, but my conclusion is based on that I think QEMU should still protect itself from the client. Take the example of QEMU & Libvirt: I think death or bug of either of the program should not affect the other one. But maybe I misunderstood somewhere since I saw that you emphasized it at [1] below... And for (3), I really think a proper client should never trigger that queue full state. Hopefully with that then the client would never lost the out-of-band feature due to a stuck input channel. > > >> > > > >> > > 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. > >> > >> The exception to the rule right now are some of the migration APIs which > >> we allow to be invoked to manage the migration process. > >> > >> > 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. > >> > >> In terms of what libvirt would want to parallelize, I think it is reasonable > >> to consider any of the query-XXXX commands desirable. Other stuff is likely > >> to remain serialized from libvirt's side. > > > > IMHO concurrency won't help much now even for query commands, since > > our current concurrency is still "partly" - the executions of query > > commands (which is the most time consuming part) will still be done > > sequentially, so even if we send multiple query commands in parallel > > (without waiting for a response of any sent commands), the total time > > used for the list of commands would be mostly the same. > > Yes. We execute all in-band commands (regardless of their monitor) in > the main thread. Out-of-band commands can execute in @mon_iothread, > which provides a modest degree of concurrency. > > > My understanding for why we have such a queue length now is that it > > came from a security concern: after we have a queue, we need that > > queue length to limit the memory usages for the QMP server. Though > > that might not help much for real users like Libvirt, it's majorly > > serving as a way to protect QEMU QMP from being attacked or from being > > turned down by a buggy QMP client. > > Yes. > > QEMU has to trust its QMP clients, so malice is not a concern, but > accidents are. Robust software does not buffer without bounds. [1] > > > But I agree now that the queue length information might still be > > helpful some day. Maybe, we can hide that until we support executing > > commands in parallel for some of them. > > Queue length can become interesting long before we get general > concurrency. > > If you use QMP only synchronously (send command #1; receive reply #1; > send command #2; ...), then out-of-band does exactly nothing for you. > To make use of it, you have to send an out-of-band command *before* you > receive the previous command's reply. That's a form of pipelining. Yes, out-of-band should be special here, but as Dave has already mentioned (possibly someone else too) that we may just need a length=1 queue for in-band command and length=1 queue for out-of-band command and that should be enough at least for now (say, oob command will never block, and oob commands will be executed once a time). By that extra length=1 out-of-band queue we gain the ability to talk to QMP any time we want when necessary (though with limited list of cmds). > > Note there's still no general concurrency. There's a bit of pipelining, > and there's a bit of concurrency between one in-band command (executing > in main thread) and out-of-band command (executing in @mon_iothread). > > Since we need to support a bit of pipelining anyway, why not support it > more generally? All it takes it raising the queue length limit above > the minimum required for the use of OOB I just sketched. > > Note that "since we need to support a bit of concurrency anyway, why not > support it more generally?" would be ludicrously naive :) Regards,
* Markus Armbruster (armbru@redhat.com) wrote: > Daniel P. Berrangé <berrange@redhat.com> writes: > > > On Tue, Sep 04, 2018 at 08:39:27AM +0200, Markus Armbruster wrote: > >> Daniel P. Berrangé <berrange@redhat.com> 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? > > 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. 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 / dgilbert@redhat.com / Manchester, UK
"Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > * Markus Armbruster (armbru@redhat.com) wrote: >> Daniel P. Berrangé <berrange@redhat.com> writes: >> >> > On Tue, Sep 04, 2018 at 08:39:27AM +0200, Markus Armbruster wrote: >> >> Daniel P. Berrangé <berrange@redhat.com> 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 / dgilbert@redhat.com / Manchester, UK
* Markus Armbruster (armbru@redhat.com) wrote: > "Dr. David Alan Gilbert" <dgilbert@redhat.com> writes: > > > * Markus Armbruster (armbru@redhat.com) wrote: > >> Daniel P. Berrangé <berrange@redhat.com> writes: > >> > >> > On Tue, Sep 04, 2018 at 08:39:27AM +0200, Markus Armbruster wrote: > >> >> Daniel P. Berrangé <berrange@redhat.com> 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 it could add to version,package, capabilities a 'values' array? > 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? Oh OK, that's a lot less scary; I suggest appending a 'Each QMP channel has it's own independent queue.' Dave > > > > 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 / dgilbert@redhat.com / Manchester, UK -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff --git a/docs/interop/qmp-spec.txt b/docs/interop/qmp-spec.txt index 8f7da0245d..67e44a8120 100644 --- a/docs/interop/qmp-spec.txt +++ b/docs/interop/qmp-spec.txt @@ -130,8 +130,9 @@ 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 eventually drop commands to limit the -queue length. The sever sends event COMMAND_DROPPED then. +execute them, the server will stop reading the requests from the QMP +channel until the request queue length is reduced to an acceptable +range. Only a few commands support out-of-band execution. The ones that do have "allow-oob": true in output of query-qmp-schema. diff --git a/qapi/misc.json b/qapi/misc.json index d450cfef21..2c1a6119bf 100644 --- a/qapi/misc.json +++ b/qapi/misc.json @@ -3432,46 +3432,6 @@ ## { 'command': 'query-sev-capabilities', 'returns': 'SevCapability' } -## -# @CommandDropReason: -# -# Reasons that caused one command to be dropped. -# -# @queue-full: the command queue is full. This can only occur when -# the client sends a new non-oob command before the -# response to the previous non-oob command has been -# received. -# -# Since: 2.12 -## -{ 'enum': 'CommandDropReason', - 'data': [ 'queue-full' ] } - -## -# @COMMAND_DROPPED: -# -# Emitted when a command is dropped due to some reason. Commands can -# only be dropped when the oob capability is enabled. -# -# @id: The dropped command's "id" field. -# FIXME Broken by design. Events are broadcast to all monitors. If -# another monitor's client has a command with the same ID in flight, -# the event will incorrectly claim that command was dropped. -# -# @reason: The reason why the command is dropped. -# -# Since: 2.12 -# -# Example: -# -# { "event": "COMMAND_DROPPED", -# "data": {"result": {"id": "libvirt-102", -# "reason": "queue-full" } } } -# -## -{ 'event': 'COMMAND_DROPPED' , - 'data': { 'id': 'any', 'reason': 'CommandDropReason' } } - ## # @set-numa-node: #
Now it was not used any more, drop it. We can still do that since out-of-band is still experimental, and this event is only used when out-of-band is enabled. Signed-off-by: Peter Xu <peterx@redhat.com> --- docs/interop/qmp-spec.txt | 5 +++-- qapi/misc.json | 40 --------------------------------------- 2 files changed, 3 insertions(+), 42 deletions(-)