QMP: out-of-band (OOB) execution support
v7 cover QMP: out-of-band (OOB) execution support
This version should have addressed all comments in previous one, also fixed another race condition after I addressed all the comments (a new race condition introduced by addressing the comments...). For some more details of the race condition, please see the last entry of change log, and please refer to patch 9 for the code change.
I removed RFC tag from this version. Please review. Thanks.
v7:
- add some r-bs, and remove some.
- remove the chardev fix since already queued by Paolo
- use local var in qemu_chr_fe_add_watch [Stefan]
- move doc patch to front, mention it in some patches [Eric]
- Quite a few of English fixes [Eric]
- fix unlock missing in handle_qmp_command [Stefan]
- squash some patches according to the review comments
- don't break gdbserver usage on HMP non-interactive mode by fixing up the suspend/resume logic [Fam, Stefan]
- move the qemu_chr_fe_set_handlers() call in monitor_init() into a bottom half to avoid race between the call itself and iothread. [Stefan]
- spent quite a lot of time debugging another assertion failure in io_watch_poll_finalize() after above change is made (ouch! I really hoped we always have the latest glib): when QEMU inits chardevs in chardev_init_func() it's possible that QEMU registers the chardev handlers there, even before CharBackend is connected to that chardev in monitor_init(). Then, when we reach monitor_init() we must make sure we unregister that old one first, or there can have one orphan GSource still in default gcontext (note that this can really happen when we start to use QEMUBH to setup chardev frontends, which is above change).
v6:
- add r-bs
- s/negociate/negotiate/ [Dave]
- let mon_global be anonymous struct [Stefan]
- in monitor_init(): call qemu_chr_fe_set_handlers() later than init parser, then it's safe [Stefan]
- drop patch "qjson: add "opaque" field to JSONMessageParser", re-write the following one to use container_of(). [Stefan]
- keep get_qmp_greeting() the old way, add cap list [Stefan, Fam]
- fix the iothread_stop() comment since that's not really related to the glib bug [Stefan]
- when do qmp greeting, don't expose oob capability if iothread not used [Stefan, Fam]
- squash "qmp: introduce some capability helpers" into patch "monitor:
separate QMP parser and dispatcher" [Fam]
- tune comments for monitor_qmp_respond(). [Stefan]
- use atomic_mb_{read|set}() where proper instead of no-mb ones [Stefan]
- add patch to let monitor suspend/resume really support QMP. Notify iothread when needed, in both suspend/resume. [Stefan]
- add comment for qmp_queue_lock on lock taking ordering. [Stefan]
- refactor monitor_qmp_bh_dispatcher() to not loop, instead queue itself after each request. [Stefan]
- rename "request" into "command" in the new qmp event [Stefan]
- in handle_qmp_command() protect the queue length fetch with qmp_queue_lock [Stefan]
- one more s/2.11/2.12/. [Fam]
- when isolating response queue, not only flush the queue, but also flush the monitor out buffer [Stefan]
- document rewrite [Stefan]
- some other touch-ups that I forgot to mention and some tiny new patches... anyway, rbs will be gone if any of the patch is touched up. Please have a look!
v5
- rename "monitor_iothread" to "mon_iothread" [Dave]
- add comment in monitor_cleanup(), note that when the hacks can be removed. [Dan]
- add a note section in qmp-spec.txt, mentioning about how to migrate existing QMP command to oob-capable command. [Dave]
- drop patch "qmp: let migrate-incoming allow out-of-band". All migration related changes will all be put into postcopy-recovery series.
v4:
- drop first patch to fix IOWatchPool [Stefan, Dan]
- add s-o-b where missing, and newly got r-bs
- fix English error in commit msg [Fam]
- some tunes on request-dropped event: [Stefan]
- firstly let 'id' be any type, meanwhile make sure "id" is there as long as OOB is enabled for the monitor.
- some comments fix
- add new command "x-oob-test" for testing oob commands [Stefan]
- simplify the test codes to use new x-oob-test
- flush response queue before cleanup monitors
This series was born from this one:
https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg04310.html
The idea comes from Markus Armbruster and the discussion we had in the thread. It's not a super ideal solution (I believe Markus had been thinking hard to keep everything in order meanwhile trying to satisfy the migration requirement), but AFAIU it's currently the best.
What is OOB?
============
It's the shortcut of Out-Of-Band execution, its name is given by Markus. It's a way to quickly execute a QMP request. Say, originally QMP is going throw these steps:
JSON Parser --> QMP Dispatcher --> Respond
/|\ (2) (3) |
(1) | \|/ (4)
+--------- main thread --------+The requests are executed by the so-called QMP-dispatcher after the JSON is parsed. If OOB is on, we run the command directly in the parser and quickly returns.
This series changed the QMP handling logic by moving the command parsing and responding phases into IOThreads, so to be more accurate, after the series the above graph would change into this:
queue/kick queue/kick
JSON Parser ======> QMP Dispatcher =====> Responder
/|\ | (3) /|\ | (4) | /|\
(1) | | (2) | | | |
| | | \|/ (6)| |(5)
| | main thread | |
| | | |
| +--------> monitor IO thread <-------+ |
+-----------/ \----------+New Interfaces
==============
QMP Introspection Changes
-------------------------
When do query-qmp-schema, we were getting something like:
{"name": "migrate-incoming", "ret-type": "17",
"meta-type": "command", "arg-type": "89"}
Now we will get a new key named "allow-oob":
{"name": "migrate-incoming", "ret-type": "17", "allow-oob": true,
"meta-type": "command", "arg-type": "89"}
Which shows whether a command supports OOB.
QMP Negociation Changes
-----------------------
We were running "qmp_capabilities" always without parameters like:
{"execute": "qmp_capabilities"}
Now we can enable capabilities (we don't have any capability before this series) like OOB using:
{"execute": "qmp_capabilities", "arguments": {"enable": ["oob"]}}
Only after we explicitly enable OOB capability can we send requests in OOB manner. Otherwise we'll have exactly the same QMP session as before, just like when OOB is not there.
When OOB is enabled, it's possible that OOB reply reaches faster than previous command, so clients should be prepared.
Trigger OOB execution
---------------------
Let's take migrate-incoming as example. The old command looks like:
{"execute": "migrate-incoming", "arguments" : {"uri": "xxxxxx"}}
To execute a command with OOB execution, we need to specify it in the QMP request in the extra "control" key:
{"execute": "migrate-incoming", "arguments" : {"uri": "xxxxxx"},
"control" { "run-oob": true } }
Then the command will be run in parser, and it can preempt other commands.
Others
=================
The last patch of OOB test may need some attention. I used dump-guest-memory as a time-consuming command to test whether OOB is working, and the only command I can test now is "migrate-incoming". I hope that is a "okay" solution for unit tests. Any other suggestions on that would be welcomed.
Please review. Thanks.
Peter Xu (23):
docs: update QMP documents for OOB commands
qobject: introduce qstring_get_try_str()
qobject: introduce qobject_get_try_str()
qobject: let object_property_get_str() use new API
monitor: move skip_flush into monitor_data_init
monitor: move the cur_mon hack deeper for QMP
monitor: unify global init
monitor: let mon_list be tail queue
monitor: allow using IO thread for parsing
qmp: introduce QMPCapability
monitor: introduce monitor_qmp_respond()
monitor: let suspend_cnt be thread safe
monitor: let suspend/resume work even with QMPs
monitor: separate QMP parser and dispatcher
qmp: add new event "command-dropped"
monitor: send event when command queue full
qapi: introduce new cmd option "allow-oob"
qmp: support out-of-band (oob) execution
qmp: isolate responses into io thread
monitor: enable IO thread for (qmp & !mux) typed
qmp: add command "x-oob-test"
tests: qmp-test: verify command batching
tests: qmp-test: add oob test
docs/devel/qapi-code-gen.txt | 68 +++- docs/interop/qmp-spec.txt | 30 +- include/monitor/monitor.h | 2 +- include/qapi/qmp/dispatch.h | 3 + include/qapi/qmp/qstring.h | 2 + monitor.c | 699 ++++++++++++++++++++++++++++++++++++----- qapi-schema.json | 87 ++++- qapi/introspect.json | 6 +- qapi/qmp-dispatch.c | 32 +- qmp.c | 16 + qobject/qstring.c | 21 ++ qom/object.c | 9 +- scripts/qapi-commands.py | 19 +- scripts/qapi-introspect.py | 10 +- scripts/qapi.py | 15 +- scripts/qapi2texi.py | 2 +- tests/qapi-schema/test-qapi.py | 2 +- tests/qmp-test.c | 97 +++++- trace-events | 3 + vl.c | 7 +- 20 files changed, 1016 insertions(+), 114 deletions(-)
Signature · 1 line
2.14.3
PATCH v7 1/23 docs: update QMP documents for OOB commands
Update both the developer and spec for the new QMP OOB (Out-Of-Band) command.
--- docs/devel/qapi-code-gen.txt | 68 ++++++++++++++++++++++++++++++++++++++++---- docs/interop/qmp-spec.txt | 30 ++++++++++++++++--- 2 files changed, 89 insertions(+), 9 deletions(-)
docs/devel/qapi-code-gen.txt +63 -5
@@ -554,9 +554,12 @@ following example objects: === Commands === +--- General Command Layout --- + Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, '*returns': TYPE-NAME, '*boxed': true, - '*gen': false, '*success-response': false } + '*gen': false, '*success-response': false, + '*allow-oob': false } Commands are defined by using a dictionary containing several members, where three members are most common. The 'command' member is a
@@ -636,6 +639,59 @@ possible, the command expression should include the optional key 'success-response' with boolean value false. So far, only QGA makes use of this member. +A command can be declared to support Out-Of-Band (OOB) execution. By +default, commands do not support OOB. To declare a command to support +it, we need an extra 'allow-oob' field. For example: + + { 'command': 'migrate_recover', + 'data': { 'uri': 'str' }, 'allow-oob': true } + +To execute a command in Out-Of-Band way, we need to specify the +"control" field in the request, with "run-oob" set to true. Example: + + => { "execute": "command-support-oob", + "arguments": { ... }, + "control": { "run-oob": true } } + <= { "return": { } } + +Without it, even the commands that support out-of-band execution will +still be run In-Band. + +Please read the "Out-Of-Band Command Execution" section below for more +information on how OOB execution works. + +--- About Out-Of-Band (OOB) Command Execution --- + +Out-Of-Band does not mean a special kind of command. Instead, it's a +special way to execute the command. One normal command can be +declared to support Out-Of-Band execution when 'allow-oob' field is +set to true when defining the command. With that, it can be run in an +Out-Of-Band way if 'run-oob' is specified in 'control' field of +command request. + +When we say normal QMP command executions, it means basically the +following: + +- They are executed in order, +- They run only in main thread of QEMU, +- They have the BQL taken during execution. + +For OOB command executions, they differ in the following: + +- They can be executed before an existing command, +- They run in a monitor dedicated thread, +- They do not take the BQL during execution. + +OOB command handlers must satisfy the following conditions: + +- It executes extremely fast, +- It does not take any lock, or, it can take very small locks if all + critical regions also follow the rules for OOB command handler code, +- It does not invoke system calls that may block, +- It does not access guest RAM that may block when userfaultfd is + enabled for postcopy live migration. + +If in doubt, do not implement OOB execution support. === Events ===
@@ -739,10 +795,12 @@ references by name. QAPI schema definitions not reachable that way are omitted. The SchemaInfo for a command has meta-type "command", and variant -members "arg-type" and "ret-type". On the wire, the "arguments" -member of a client's "execute" command must conform to the object type -named by "arg-type". The "return" member that the server passes in a -success response conforms to the type named by "ret-type". +members "arg-type", "ret-type" and "allow-oob". On the wire, the +"arguments" member of a client's "execute" command must conform to the +object type named by "arg-type". The "return" member that the server +passes in a success response conforms to the type named by +"ret-type". When "allow-oob" is set, it means the command supports +out-of-band execution. If the command takes no arguments, "arg-type" names an object type without members. Likewise, if the command returns nothing, "ret-type"
docs/interop/qmp-spec.txt +26 -4
@@ -83,16 +83,27 @@ The greeting message format is: 2.2.1 Capabilities ------------------ -As of the date this document was last revised, no server or client -capability strings have been defined. +Currently supported capabilities are: +- "oob": it means the QMP server supports "Out-Of-Band" command + execution. For more detail, please see "run-oob" parameter in + "Issuing Commands" section below. Not all commands allow this "oob" + execution. One can know whether one command supports "oob" by + "query-qmp-schema" command. + +QMP clients can get a list of supported QMP capabilities of the QMP +server in the greeting message mentioned above. By default, all the +capabilities are off. To enable a specific or multiple of QMP +capabilities, QMP client needs to send "qmp_capabilities" command with +extra parameter for the capabilities. 2.3 Issuing Commands -------------------- The format for command execution is: -{ "execute": json-string, "arguments": json-object, "id": json-value } +{ "execute": json-string, "arguments": json-object, "id": json-value, + "control": json-dict } Where,
@@ -102,10 +113,16 @@ The format for command execution is: required. Each command documents what contents will be considered valid when handling the json-argument - The "id" member is a transaction identification associated with the - command execution, it is optional and will be part of the response if + command execution. It is required if OOB is enabled, and optional + if not. The same "id" field will be part of the response if provided. The "id" member can be any json-value, although most clients merely use a json-number incremented for each successive command +- The "control" member is optional, and currently only used for + "out-of-band" execution ("oob" as shortcut). The handling or + response of an "oob" command can overtake prior in-band commands. + To enable "oob" feature, just provide a control field with: { + "control": { "run-oob": true } } 2.4 Commands Responses ----------------------
@@ -113,6 +130,11 @@ The format for command execution is: There are two possible responses which the Server will issue as the result of a command execution: success or error. +As long as the commands were issued with a proper "id" field, then the +same "id" field will be attached in the corresponding response message +so that requests and responses can match. Clients should drop all the +responses that are with unknown "id" field. + 2.4.1 success -------------
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 01/23] docs: update QMP documents for OOB commands
Quoted 15 lines · Peter Xu
Update both the developer and spec for the new QMP OOB (Out-Of-Band) command.
Signed-off-by Peter Xu <[email protected]>--- docs/devel/qapi-code-gen.txt | 68 ++++++++++++++++++++++++++++++++++++++++---- docs/interop/qmp-spec.txt | 30 ++++++++++++++++--- 2 files changed, 89 insertions(+), 9 deletions(-)docs/devel/qapi-code-gen.txt +0
-0@@ -554,9 +554,12 @@ following example objects:
docs/devel/qapi-code-gen.txt following example objects: · line 563
Quoted 10 lines
docs/devel/qapi-code-gen.txt +4
-1=== Commands === +--- General Command Layout --- + Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, '*returns': TYPE-NAME, '*boxed': true, - '*gen': false, '*success-response': false } + '*gen': false, '*success-response': false, + '*allow-oob': false }
Shouldn't this be '*allow-oob': true, as the only time you add the field is if you turn oob on (as it already defaults to off)?
docs/devel/qapi-code-gen.txt possible, the command expression should include the optional key · line 643
Quoted 8 lines
docs/devel/qapi-code-gen.txt +2
-0Commands are defined by using a dictionary containing several members, where three members are most common. The 'command' member is a@@ -636,6 +639,59 @@ possible, the command expression should include the optional key 'success-response' with boolean value false. So far, only QGA makes use of this member. +A command can be declared to support Out-Of-Band (OOB) execution. By +default, commands do not support OOB. To declare a command to support
s/to support/that supports/
docs/devel/qapi-code-gen.txt possible, the command expression should include the optional key · line 655
Quoted 12 lines
docs/devel/qapi-code-gen.txt +12
-0+it, we need an extra 'allow-oob' field. For example: + + { 'command': 'migrate_recover', + 'data': { 'uri': 'str' }, 'allow-oob': true } + +To execute a command in Out-Of-Band way, we need to specify the +"control" field in the request, with "run-oob" set to true. Example: + + => { "execute": "command-support-oob", + "arguments": { ... }, + "control": { "run-oob": true } } + <= { "return": { } }
This talks more about the QMP user protocol, while the rest of the document is about QAPI constructs. But we do have an example of 'my-first-command' in the document, so I guess this is okay.
docs/devel/qapi-code-gen.txt possible, the command expression should include the optional key · line 663
Quoted 8 lines
docs/devel/qapi-code-gen.txt +8
-0+ +Without it, even the commands that support out-of-band execution will +still be run In-Band. + +Please read the "Out-Of-Band Command Execution" section below for more +information on how OOB execution works. + +--- About Out-Of-Band (OOB) Command Execution ---
Do we really need the paragraph mentioning a forward reference, when the very next thing is the item that was referenced?
docs/devel/qapi-code-gen.txt possible, the command expression should include the optional key · line 673
Quoted 10 lines
docs/devel/qapi-code-gen.txt +10
-0+ +Out-Of-Band does not mean a special kind of command. Instead, it's a +special way to execute the command. One normal command can be +declared to support Out-Of-Band execution when 'allow-oob' field is +set to true when defining the command. With that, it can be run in an +Out-Of-Band way if 'run-oob' is specified in 'control' field of +command request. + +When we say normal QMP command executions, it means basically the +following:
Under normal QMP command execution, the following apply to each command:
docs/devel/qapi-code-gen.txt possible, the command expression should include the optional key · line 679
Quoted 6 lines
docs/devel/qapi-code-gen.txt +6
-0+ +- They are executed in order, +- They run only in main thread of QEMU, +- They have the BQL taken during execution. + +For OOB command executions, they differ in the following:
When a command is executed with OOB, the following changes occur:
docs/devel/qapi-code-gen.txt possible, the command expression should include the optional key · line 681
Quoted 2 lines
docs/devel/qapi-code-gen.txt +2
-0+ +- They can be executed before an existing command,
They can be completed before a pending in-band command,
Quoted 38 lines
docs/devel/qapi-code-gen.txt +19
-4+- They run in a monitor dedicated thread, +- They do not take the BQL during execution. + +OOB command handlers must satisfy the following conditions: + +- It executes extremely fast, +- It does not take any lock, or, it can take very small locks if all + critical regions also follow the rules for OOB command handler code, +- It does not invoke system calls that may block, +- It does not access guest RAM that may block when userfaultfd is + enabled for postcopy live migration. + +If in doubt, do not implement OOB execution support. === Events ===@@ -739,10 +795,12 @@ references by name. QAPI schema definitions not reachable that way are omitted. The SchemaInfo for a command has meta-type "command", and variant -members "arg-type" and "ret-type". On the wire, the "arguments" -member of a client's "execute" command must conform to the object type -named by "arg-type". The "return" member that the server passes in a -success response conforms to the type named by "ret-type". +members "arg-type", "ret-type" and "allow-oob". On the wire, the +"arguments" member of a client's "execute" command must conform to the +object type named by "arg-type". The "return" member that the server +passes in a success response conforms to the type named by +"ret-type". When "allow-oob" is set, it means the command supports +out-of-band execution. If the command takes no arguments, "arg-type" names an object type without members. Likewise, if the command returns nothing, "ret-type"docs/interop/qmp-spec.txt +0
-0
docs/interop/qmp-spec.txt The greeting message format is: · line 89
Quoted 9 lines
docs/interop/qmp-spec.txt +3
-2@@ -83,16 +83,27 @@ The greeting message format is: 2.2.1 Capabilities ------------------ -As of the date this document was last revised, no server or client -capability strings have been defined. +Currently supported capabilities are: +- "oob": it means the QMP server supports "Out-Of-Band" command + execution. For more detail, please see "run-oob" parameter in
s/detail/details/
docs/interop/qmp-spec.txt The greeting message format is: · line 92
Quoted 3 lines
docs/interop/qmp-spec.txt +3
-0+ "Issuing Commands" section below. Not all commands allow this "oob" + execution. One can know whether one command supports "oob" by + "query-qmp-schema" command.
The "query-qmp-schema" command can be used to inspect which commands support "oob" execution.
docs/interop/qmp-spec.txt The greeting message format is: · line 96
Quoted 4 lines
docs/interop/qmp-spec.txt +4
-0+ +QMP clients can get a list of supported QMP capabilities of the QMP +server in the greeting message mentioned above. By default, all the +capabilities are off. To enable a specific or multiple of QMP
s/specific or multiple of/any/
docs/interop/qmp-spec.txt The greeting message format is: · line 97
Quoted 1 line
docs/interop/qmp-spec.txt +1
-0+capabilities, QMP client needs to send "qmp_capabilities" command with
s/QMP/the QMP
s/send/send the/
s/with/with an/
docs/interop/qmp-spec.txt The greeting message format is: · line 98
Quoted 1 line
docs/interop/qmp-spec.txt +1
-0+extra parameter for the capabilities.
s/the/the requested/
Quoted 16 lines
docs/interop/qmp-spec.txt +2
-12.3 Issuing Commands -------------------- The format for command execution is: -{ "execute": json-string, "arguments": json-object, "id": json-value } +{ "execute": json-string, "arguments": json-object, "id": json-value, + "control": json-dict } Where,@@ -102,10 +113,16 @@ The format for command execution is: required. Each command documents what contents will be considered valid when handling the json-argument - The "id" member is a transaction identification associated with the
docs/interop/qmp-spec.txt The format for command execution is: · line 124
Quoted 10 lines
docs/interop/qmp-spec.txt +6
-1- command execution, it is optional and will be part of the response if + command execution. It is required if OOB is enabled, and optional + if not. The same "id" field will be part of the response if provided. The "id" member can be any json-value, although most clients merely use a json-number incremented for each successive command +- The "control" member is optional, and currently only used for + "out-of-band" execution ("oob" as shortcut). The handling or + response of an "oob" command can overtake prior in-band commands. + To enable "oob" feature, just provide a control field with: {
To enable "oob" handling of a particular command,
Quoted 17 lines
docs/interop/qmp-spec.txt +6
-0+ "control": { "run-oob": true } } 2.4 Commands Responses ----------------------@@ -113,6 +130,11 @@ The format for command execution is: There are two possible responses which the Server will issue as the result of a command execution: success or error. +As long as the commands were issued with a proper "id" field, then the +same "id" field will be attached in the corresponding response message +so that requests and responses can match. Clients should drop all the +responses that are with unknown "id" field. + 2.4.1 success -------------
Signature · 3 lines
Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
Re: [Qemu-devel] [PATCH v7 01/23] docs: update QMP documents for OOB commands
Quoted 54 lines · Eric Blake
Quoted 14 lines · Peter Xu
Update both the developer and spec for the new QMP OOB (Out-Of-Band) command.
Signed-off-by Peter Xu <[email protected]>--- docs/devel/qapi-code-gen.txt | 68 ++++++++++++++++++++++++++++++++++++++++---- docs/interop/qmp-spec.txt | 30 ++++++++++++++++--- 2 files changed, 89 insertions(+), 9 deletions(-)docs/devel/qapi-code-gen.txt +0
-0docs/devel/qapi-code-gen.txt following example objects: · line 560
Quoted 8 lines
docs/devel/qapi-code-gen.txt +4
-1@@ -554,9 +554,12 @@ following example objects: === Commands === +--- General Command Layout --- + Usage: { 'command': STRING, '*data': COMPLEX-TYPE-NAME-OR-DICT, '*returns': TYPE-NAME, '*boxed': true, - '*gen': false, '*success-response': false } + '*gen': false, '*success-response': false, + '*allow-oob': false }Shouldn't this be '*allow-oob': true, as the only time you add the field is if you turn oob on (as it already defaults to off)?
docs/devel/qapi-code-gen.txt possible, the command expression should include the optional key · line 642
Quoted 7 lines
docs/devel/qapi-code-gen.txt +2
-0Commands are defined by using a dictionary containing several members, where three members are most common. The 'command' member is a@@ -636,6 +639,59 @@ possible, the command expression should include the optional key 'success-response' with boolean value false. So far, only QGA makes use of this member. +A command can be declared to support Out-Of-Band (OOB) execution. By +default, commands do not support OOB. To declare a command to supports/to support/that supports/
docs/devel/qapi-code-gen.txt possible, the command expression should include the optional key · line 655
Quoted 12 lines
docs/devel/qapi-code-gen.txt +12
-0+it, we need an extra 'allow-oob' field. For example: + + { 'command': 'migrate_recover', + 'data': { 'uri': 'str' }, 'allow-oob': true } + +To execute a command in Out-Of-Band way, we need to specify the +"control" field in the request, with "run-oob" set to true. Example: + + => { "execute": "command-support-oob", + "arguments": { ... }, + "control": { "run-oob": true } } + <= { "return": { } }This talks more about the QMP user protocol, while the rest of the document is about QAPI constructs. But we do have an example of 'my-first-command' in the document, so I guess this is okay.
Quoted 11 lines
docs/devel/qapi-code-gen.txt possible, the command expression should include the optional key · line 663
Quoted 8 lines
docs/devel/qapi-code-gen.txt +8
-0+ +Without it, even the commands that support out-of-band execution will +still be run In-Band. + +Please read the "Out-Of-Band Command Execution" section below for more +information on how OOB execution works. + +--- About Out-Of-Band (OOB) Command Execution ---Do we really need the paragraph mentioning a forward reference, when the very next thing is the item that was referenced?
Yeah, I can remove that paragraph.
For all the rest of comments (above or below, which I omitted), all of them make sense, and I'll fix accordingly. Thanks for reviewing.
Signature · 1 line
Peter Xu
PATCH v7 2/23 qobject: introduce qstring_get_try_str()
In reply to Peter XuThe only difference from qstring_get_str() is that it allows the qstring to be NULL. If so, NULL is returned.
--- include/qapi/qmp/qstring.h | 1 + qobject/qstring.c | 10 ++++++++++ 2 files changed, 11 insertions(+)
include/qapi/qmp/qstring.h +1 -0
@@ -27,6 +27,7 @@ QString *qstring_from_str(const char *str);QString *qstring_from_substr (const char *str,int start,int end);size_t qstring_get_length (const QString *qstring);const char *qstring_get_str (const QString *qstring); +const char *qstring_get_try_str (const QString *qstring);void qstring_append_int (QString *qstring,int64_t value);void qstring_append (QString *qstring,const char *str);void qstring_append_chr (QString *qstring,int c);
qobject/qstring.c +10 -0
@@ -128,6 +128,16 @@ const char *qstring_get_str(const QString *qstring)return qstring->string; } +/** +* qstring_get_try_str(): Return a pointer to the stored string +* +* NOTE: will return NULL if qstring is not provided. +*/ +const char *qstring_get_try_str (const QString *qstring) +{ +return qstring ?qstring_get_str (qstring) :NULL ; +} +/** * qstring_is_equal(): Test whether the two QStrings are equal */
Signature · 1 line
2.14.3
PATCH v7 3/23 qobject: introduce qobject_get_try_str()
In reply to Peter XuA quick way to fetch string from qobject when it's a QString.
--- include/qapi/qmp/qstring.h | 1 + qobject/qstring.c | 11 +++++++++++ 2 files changed, 12 insertions(+)
include/qapi/qmp/qstring.h +1 -0
@@ -28,6 +28,7 @@ QString *qstring_from_substr(const char *str, int start, int end);size_t qstring_get_length (const QString *qstring);const char *qstring_get_str (const QString *qstring);const char *qstring_get_try_str (const QString *qstring); +const char *qobject_get_try_str (const QObject *qstring);void qstring_append_int (QString *qstring,int64_t value);void qstring_append (QString *qstring,const char *str);void qstring_append_chr (QString *qstring,int c);
qobject/qstring.c +11 -0
@@ -138,6 +138,17 @@ const char *qstring_get_try_str(const QString *qstring)return qstring ?qstring_get_str (qstring) :NULL ; } +/** +* qobject_get_try_str(): Return a pointer to the corresponding string +* +* NOTE: the string will only be returned if the object is valid, and +* its type is QString, otherwise NULL is returned. +*/ +const char *qobject_get_try_str (const QObject *qstring) +{ +return qstring_get_try_str (qobject_to_qstring (qstring)); +} +/** * qstring_is_equal(): Test whether the two QStrings are equal */
Signature · 1 line
2.14.3
PATCH v7 4/23 qobject: let object_property_get_str() use new API
In reply to Peter XuWe can simplify object_property_get_str() using the new qobject_get_try_str().
--- qom/object.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
qom/object.c +3 -6
@@ -1116,18 +1116,15 @@ char *object_property_get_str(Object *obj, const char *name, Error **errp) {QObject *ret =object_property_get_qobject (obj, name, errp); -QString *qstring;char *retval;if (!ret) {return NULL ; } - qstring =qobject_to_qstring (ret); -if (!qstring) { + + retval =g_strdup (qobject_get_try_str (ret)); +if (!retval) {error_setg (errp,QERR_INVALID_PARAMETER_TYPE , name,"string" ); - retval =NULL ; - }else { - retval =g_strdup (qstring_get_str (qstring)); }qobject_decref (ret);
Signature · 1 line
2.14.3
PATCH v7 5/23 monitor: move skip_flush into monitor_data_init
In reply to Peter XuIt's part of the data init. Collect it.
--- monitor.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
monitor.c +4 -4
@@ -567,13 +567,14 @@ static void monitor_qapi_event_init(void)static void handle_hmp_command (Monitor *mon,const char *cmdline); -static void monitor_data_init (Monitor *mon) +static void monitor_data_init (Monitor *mon,bool skip_flush) {memset (mon,0 ,sizeof (Monitor));qemu_mutex_init (&mon->out_lock); mon->outbuf =qstring_new ();/* Use *mon_cmds by default. */ mon->cmd_table = mon_cmds; + mon->skip_flush = skip_flush; }static void monitor_data_destroy (Monitor *mon)
@@ -594,8 +595,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,char *output =NULL ;Monitor *old_mon, hmp; -monitor_data_init (&hmp); - hmp.skip_flush = true; +monitor_data_init (&hmp, true); old_mon = cur_mon; cur_mon = &hmp;
@@ -4088,7 +4088,7 @@ void monitor_init(Chardev *chr, int flags) } mon =g_malloc (sizeof (*mon)); -monitor_data_init (mon); +monitor_data_init (mon, false);qemu_chr_fe_init (&mon->chr, chr, &error_abort); mon->flags = flags;
Signature · 1 line
2.14.3
PATCH v7 6/23 monitor: move the cur_mon hack deeper for QMP
In reply to Peter XuIn monitor_qmp_read(), we have the hack to temporarily replace the cur_mon pointer. Now we move this hack deeper inside the QMP dispatcher routine since the Monitor pointer can be actually obtained using container_of() upon the parser object, just like most of the other JSON parser users do.
This does not make much sense as a single patch. However, this will be a big step for the next patch, when the QMP dispatcher routine will be split from the QMP parser.
--- monitor.c | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-)
monitor.c +10 -7
@@ -3811,7 +3811,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) {QObject *req, *rsp =NULL , *id =NULL ;QDict *qdict =NULL ; -Monitor *mon = cur_mon; +MonitorQMP *mon_qmp =container_of (parser, MonitorQMP, parser); +Monitor *old_mon, *mon =container_of (mon_qmp, Monitor, qmp); +Error *err =NULL ; req =json_parser_parse_err (tokens,NULL , &err);
@@ -3836,8 +3838,13 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)QDECREF (req_json); } + old_mon = cur_mon; + cur_mon = mon; + rsp =qmp_dispatch (cur_mon->qmp.commands, req); + cur_mon = old_mon; +if (mon->qmp.commands == &qmp_cap_negotiation_commands) { qdict =qdict_get_qdict (qobject_to_qdict (rsp),"error" ); if (qdict
@@ -3874,13 +3881,9 @@ err_out:static void monitor_qmp_read (void *opaque,const uint8_t *buf,int size) { -Monitor *old_mon = cur_mon; - - cur_mon = opaque; - -json_message_parser_feed (&cur_mon->qmp.parser, (const char *) buf, size); +Monitor *mon = opaque; - cur_mon = old_mon; +json_message_parser_feed (&mon->qmp.parser, (const char *) buf, size); }static void monitor_read (void *opaque,const uint8_t *buf,int size)
Signature · 1 line
2.14.3
PATCH v7 7/23 monitor: unify global init
In reply to Peter XuThere are many places where the monitor initializes its globals:
- monitor_init_qmp_commands() at the very beginning
- single function to init monitor_lock
- in the first entry of monitor_init() using "is_first_init"
Unify them a bit.
monitor_lock is not used before monitor_init() (as confirmed by code analysis and gdb watchpoints); so we are safe delaying what was a constructor-time initialization of the mutex into the later first call to monitor_init().
--- include/monitor/monitor.h | 2 +- monitor.c | 25 ++++++++++--------------- vl.c | 7 ++++++- 3 files changed, 17 insertions(+), 17 deletions(-)
include/monitor/monitor.h +1 -1
@@ -16,7 +16,7 @@ extern Monitor *cur_mon;bool monitor_cur_is_qmp (void ); -void monitor_init_qmp_commands (void ); +void monitor_init_globals (void );void monitor_init (Chardev *chr,int flags);void monitor_cleanup (void );
monitor.c +10 -15
@@ -1000,7 +1000,7 @@ static void qmp_unregister_commands_hack(void)#endif } -void monitor_init_qmp_commands (void ) +static void monitor_init_qmp_commands (void ) {/* * Two command lists:
@@ -4034,6 +4034,14 @@ static void sortcmdlist(void)qsort ((void *)info_cmds, array_num, elem_size, compare_mon_cmd); } +void monitor_init_globals (void ) +{ +monitor_init_qmp_commands (); +monitor_qapi_event_init (); +sortcmdlist (); +qemu_mutex_init (&monitor_lock); +} +/* These functions just adapt the readline interface in a typesafe way. We * could cast function pointers but that discards compiler checks. */
@@ -4074,23 +4082,10 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) } } -static void __attribute__((constructor))monitor_lock_init (void ) -{ -qemu_mutex_init (&monitor_lock); -} -void monitor_init (Chardev *chr,int flags) { -static int is_first_init =1 ; -Monitor *mon; - -if (is_first_init) { -monitor_qapi_event_init (); -sortcmdlist (); - is_first_init =0 ; - } +Monitor *mon =g_malloc (sizeof (*mon)); - mon =g_malloc (sizeof (*mon));monitor_data_init (mon, false);qemu_chr_fe_init (&mon->chr, chr, &error_abort);
vl.c +6 -1
@@ -3098,7 +3098,6 @@ int main(int argc, char **argv, char **envp)qemu_init_exec_dir (argv[0 ]);module_call_init (MODULE_INIT_QOM ); -monitor_init_qmp_commands ();qemu_add_opts (&qemu_drive_opts);qemu_add_drive_opts (&qemu_legacy_drive_opts);
@@ -4562,6 +4561,12 @@ int main(int argc, char **argv, char **envp)default_drive (default_floppy, snapshot,IF_FLOPPY ,0 ,FD_OPTS );default_drive (default_sdcard, snapshot,IF_SD ,0 ,SD_OPTS ); +/* +* Note: qtest_enabled() (which is used in monitor_qapi_event_init()) +* depends on configure_accelerator() above. +*/ +monitor_init_globals (); +if (qemu_opts_foreach (qemu_find_opts ("mon" ), mon_init_func,NULL ,NULL )) {exit (1 );
Signature · 1 line
2.14.3
PATCH v7 8/23 monitor: let mon_list be tail queue
In reply to Peter XuIt was QLIST. I want to use this list to do monitor priority job later, which need tail insertion ability. So switching to a tail queue.
--- monitor.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
monitor.c +6 -6
@@ -204,7 +204,7 @@ struct Monitor {void *password_opaque;mon_cmd_t *cmd_table; QLIST_HEAD(,mon_fd_t) fds; -QLIST_ENTRY (Monitor) entry; +QTAILQ_ENTRY (Monitor) entry; };/* QMP checker flags */
@@ -213,7 +213,7 @@ struct Monitor {/* Protects mon_list, monitor_event_state. */ static QemuMutex monitor_lock; -static QLIST_HEAD(mon_list, Monitor) mon_list; +static QTAILQ_HEAD(mon_list, Monitor) mon_list; static QLIST_HEAD(mon_fdsets, MonFdset) mon_fdsets;static int mon_refcount;
@@ -414,7 +414,7 @@ static void monitor_qapi_event_emit(QAPIEvent event, QDict *qdict)Monitor *mon;trace_monitor_protocol_event_emit (event, qdict); - QLIST_FOREACH(mon, &mon_list, entry) { + QTAILQ_FOREACH(mon, &mon_list, entry) {if (monitor_is_qmp (mon) && mon->qmp.commands != &qmp_cap_negotiation_commands) {monitor_json_emitter (mon,QOBJECT (qdict));
@@ -4109,7 +4109,7 @@ void monitor_init(Chardev *chr, int flags) }qemu_mutex_lock (&monitor_lock); -QLIST_INSERT_HEAD (&mon_list, mon, entry); +QTAILQ_INSERT_HEAD (&mon_list, mon, entry);qemu_mutex_unlock (&monitor_lock); }
@@ -4118,8 +4118,8 @@ void monitor_cleanup(void)Monitor *mon, *next;qemu_mutex_lock (&monitor_lock); - QLIST_FOREACH_SAFE(mon, &mon_list, entry, next) { -QLIST_REMOVE (mon, entry); + QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) { +QTAILQ_REMOVE (&mon_list, mon, entry);monitor_data_destroy (mon);g_free (mon); }
Signature · 1 line
2.14.3
PATCH v7 9/23 monitor: allow using IO thread for parsing
In reply to Peter XuFor each Monitor, add one field "use_io_thr" to show whether it will be using the dedicated monitor IO thread to handle input/output. When set, monitor IO parsing work will be offloaded to the dedicated monitor IO thread, rather than the original main loop thread.
This only works for QMP. HMP will always be run on the main loop thread.
Currently we're still keeping use_io_thr off always. Will turn it on later at some point.
One thing to mention is that we cannot set use_io_thr for every QMP monitor. The problem is that MUXed typed chardevs may not work well with it now. When MUX is used, frontend of chardev can be the monitor plus something else. The only thing we know would be safe to be run outside main thread so far is the monitor frontend. All the rest of the frontends should still be run in main thread only.
--- monitor.c | 107 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++----- 1 file changed, 99 insertions(+), 8 deletions(-)
monitor.c +99 -8
@@ -35,6 +35,7 @@#include "net/net.h" #include "net/slirp.h" #include "chardev/char-fe.h" +#include "chardev/char-io.h" #include "ui/qemu-spice.h" #include "sysemu/numa.h" #include "monitor/monitor.h"
@@ -75,6 +76,7 @@#include "qmp-introspect.h" #include "sysemu/qtest.h" #include "sysemu/cpus.h" +#include "sysemu/iothread.h" #include "qemu/cutils.h" #include "qapi/qmp/dispatch.h"
@@ -189,6 +191,7 @@ struct Monitor {int flags;int suspend_cnt;bool skip_flush; +bool use_io_thr;QemuMutex out_lock;QString *outbuf;
@@ -207,6 +210,11 @@ struct Monitor {QTAILQ_ENTRY (Monitor) entry; }; +/* Let's add monitor global variables to this struct. */ +static struct { +IOThread *mon_iothread; +} mon_global; +/* QMP checker flags */ #define QMP_ACCEPT_UNKNOWNS 1
@@ -567,7 +575,8 @@ static void monitor_qapi_event_init(void)static void handle_hmp_command (Monitor *mon,const char *cmdline); -static void monitor_data_init (Monitor *mon,bool skip_flush) +static void monitor_data_init (Monitor *mon,bool skip_flush, +bool use_io_thr) {memset (mon,0 ,sizeof (Monitor));qemu_mutex_init (&mon->out_lock);
@@ -575,6 +584,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush)/* Use *mon_cmds by default. */ mon->cmd_table = mon_cmds; mon->skip_flush = skip_flush; + mon->use_io_thr = use_io_thr; }static void monitor_data_destroy (Monitor *mon)
@@ -595,7 +605,7 @@ char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,char *output =NULL ;Monitor *old_mon, hmp; -monitor_data_init (&hmp, true); +monitor_data_init (&hmp, true, false); old_mon = cur_mon; cur_mon = &hmp;
@@ -4034,12 +4044,29 @@ static void sortcmdlist(void)qsort ((void *)info_cmds, array_num, elem_size, compare_mon_cmd); } +static GMainContext *monitor_io_context_get (void ) +{ +return iothread_get_g_main_context (mon_global.mon_iothread); +} + +static AioContext *monitor_aio_context_get (void ) +{ +return iothread_get_aio_context (mon_global.mon_iothread); +} + +static void monitor_iothread_init (void ) +{ + mon_global.mon_iothread =iothread_create ("mon_iothread" , + &error_abort); +} +void monitor_init_globals (void ) {monitor_init_qmp_commands ();monitor_qapi_event_init ();sortcmdlist ();qemu_mutex_init (&monitor_lock); +monitor_iothread_init (); }/* These functions just adapt the readline interface in a typesafe way. We
@@ -4082,11 +4109,41 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) } } +static void monitor_list_append (Monitor *mon) +{ +qemu_mutex_lock (&monitor_lock); +QTAILQ_INSERT_HEAD (&mon_list, mon, entry); +qemu_mutex_unlock (&monitor_lock); +} + +static void monitor_qmp_setup_handlers (void *data) +{ +Monitor *mon = data; +GMainContext *context; + +if (mon->use_io_thr) { +/* +* When use_io_thr is set, we use the global shared dedicated +* IO thread for this monitor to handle input/output. +*/ + context =monitor_io_context_get (); +/* We should have inited globals before reaching here. */ +assert (context); + }else { +/* The default main loop, which is the main thread */ + context =NULL ; + } + +qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, + monitor_qmp_event,NULL , mon, context, true); +monitor_list_append (mon); +} +void monitor_init (Chardev *chr,int flags) {Monitor *mon =g_malloc (sizeof (*mon)); -monitor_data_init (mon, false); +monitor_data_init (mon, false, false);qemu_chr_fe_init (&mon->chr, chr, &error_abort); mon->flags = flags;
@@ -4099,24 +4156,55 @@ void monitor_init(Chardev *chr, int flags) }if (monitor_is_qmp (mon)) { -qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, - monitor_qmp_event,NULL , mon,NULL , true);qemu_chr_fe_set_echo (&mon->chr, true);json_message_parser_init (&mon->qmp.parser, handle_qmp_command); +if (mon->use_io_thr) { +/* +* It's possible that we already have an IOWatchPoll +* registered for the Chardev during chardev_init_func(). +* When that happened, the gcontext was still the default +* main context, always. We need to make sure we +* unregister that first and from now on we run the +* chardev on the new gcontext. +*/ +remove_fd_in_watch (chr); +/* +* We can't call qemu_chr_fe_set_handlers() directly here +* since during the procedure the chardev will be active +* and running in monitor iothread, while we'll still do +* something before returning from it, which is a possible +* race too. To avoid that, we just create a BH to setup +* the handlers. +*/ +aio_bh_schedule_oneshot (monitor_aio_context_get (), + monitor_qmp_setup_handlers, mon); +/* We'll add this to mon_list in the BH when setup done */ +return ; + }else { +qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, + monitor_qmp_read, monitor_qmp_event, +NULL , mon,NULL , true); + } }else {qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_read, monitor_event,NULL , mon,NULL , true); } -qemu_mutex_lock (&monitor_lock); -QTAILQ_INSERT_HEAD (&mon_list, mon, entry); -qemu_mutex_unlock (&monitor_lock); +monitor_list_append (mon); }void monitor_cleanup (void ) {Monitor *mon, *next; +/* +* We need to explicitly stop the iothread (but not destroy it), +* cleanup the monitor resources, then destroy the iothread since +* we need to unregister from chardev below in +* monitor_data_destroy(), and chardev is not thread-safe yet +*/ +iothread_stop (mon_global.mon_iothread); +qemu_mutex_lock (&monitor_lock); QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {QTAILQ_REMOVE (&mon_list, mon, entry);
@@ -4124,6 +4212,9 @@ void monitor_cleanup(void)g_free (mon); }qemu_mutex_unlock (&monitor_lock); + +iothread_destroy (mon_global.mon_iothread); + mon_global.mon_iothread =NULL ; }QemuOptsList qemu_mon_opts = {
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 09/23] monitor: allow using IO thread for parsing
monitor.c static void sortcmdlist(void) · line 4055
Quoted 13 lines · Peter Xu
monitor.c +9
-0@@ -4034,12 +4044,29 @@ static void sortcmdlist(void)qsort ((void *)info_cmds, array_num, elem_size, compare_mon_cmd); } +static GMainContext *monitor_io_context_get (void ) +{ +return iothread_get_g_main_context (mon_global.mon_iothread); +} + +static AioContext *monitor_aio_context_get (void ) +{ +return iothread_get_aio_context (mon_global.mon_iothread); +}
Please follow the X_get_Y() naming convention instead of X_Y_get(). For example, see qemu_get_aio_context() and iothread_get_aio_context().
monitor.c void error_vprintf_unless_qmp(const char *fmt, va_list ap) · line 4119
Quoted 12 lines
monitor.c +8
-0@@ -4082,11 +4109,41 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) } } +static void monitor_list_append (Monitor *mon) +{ +qemu_mutex_lock (&monitor_lock); +QTAILQ_INSERT_HEAD (&mon_list, mon, entry); +qemu_mutex_unlock (&monitor_lock); +} + +static void monitor_qmp_setup_handlers (void *data)
BH functions are usually declared like this:
static void X_bh(void *opaque)
This way it's immediately clear that this function is invoked as a BH.
I suggest renaming the function to monitor_qmp_setup_handlers_bh(). Using 'opaque' instead of 'data' is common, too.
monitor.c void monitor_init(Chardev *chr, int flags) · line 4164
Quoted 12 lines
monitor.c +4
-2@@ -4099,24 +4156,55 @@ void monitor_init(Chardev *chr, int flags) }if (monitor_is_qmp (mon)) { -qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, - monitor_qmp_event,NULL , mon,NULL , true);qemu_chr_fe_set_echo (&mon->chr, true);json_message_parser_init (&mon->qmp.parser, handle_qmp_command); +if (mon->use_io_thr) { +/* +* It's possible that we already have an IOWatchPoll +* registered for the Chardev during chardev_init_func().
When does this happen?
This seems like a hack that breaks when certain -chardev options are used. For example, what happens if the chardev is a TCP connection with reconnect=5. In that case the socket will be connecting asynchronously and we cannot just remove the fd watch.
How does this interact with TCP listen chardevs? It looks like the listener socket uses the main loop (see tcp_chr_disconnect()).
I'm worried that the chardev layer isn't thread-safe and you haven't added anything to protect it or at least refuse to run in unsafe conditions.
Re: [Qemu-devel] [PATCH v7 09/23] monitor: allow using IO thread for parsing
Quoted 5 lines · Stefan Hajnoczi
Quoted 4 lines · Peter Xu
monitor.c +0
-0@@ -4034,12 +4044,29 @@ static void sortcmdlist(void)qsort ((void *)info_cmds, array_num, elem_size, compare_mon_cmd); }
Quoted 12 lines
monitor.c static void sortcmdlist(void) · line 4055
Quoted 9 lines
monitor.c +9
-0+static GMainContext *monitor_io_context_get (void ) +{ +return iothread_get_g_main_context (mon_global.mon_iothread); +} + +static AioContext *monitor_aio_context_get (void ) +{ +return iothread_get_aio_context (mon_global.mon_iothread); +}Please follow the
X_get_Y()naming convention instead ofX_Y_get(). For example, seeqemu_get_aio_context()andiothread_get_aio_context().
Sure.
Quoted 16 lines
monitor.c void error_vprintf_unless_qmp(const char *fmt, va_list ap) · line 4119
Quoted 13 lines
monitor.c +8
-0@@ -4082,11 +4109,41 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) } } +static void monitor_list_append (Monitor *mon) +{ +qemu_mutex_lock (&monitor_lock); +QTAILQ_INSERT_HEAD (&mon_list, mon, entry); +qemu_mutex_unlock (&monitor_lock); +} + +static void monitor_qmp_setup_handlers (void *data)BH functions are usually declared like this:
Quoted 6 lines
static void
X_bh(void *opaque)This way it's immediately clear that this function is invoked as a BH.
I suggest renaming the function to
monitor_qmp_setup_handlers_bh(). Using 'opaque' instead of 'data' is common, too.
Sure.
Quoted 21 lines
monitor.c void monitor_init(Chardev *chr, int flags) · line 4164
Quoted 13 lines
monitor.c +4
-2@@ -4099,24 +4156,55 @@ void monitor_init(Chardev *chr, int flags) }if (monitor_is_qmp (mon)) { -qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, - monitor_qmp_event,NULL , mon,NULL , true);qemu_chr_fe_set_echo (&mon->chr, true);json_message_parser_init (&mon->qmp.parser, handle_qmp_command); +if (mon->use_io_thr) { +/* +* It's possible that we already have an IOWatchPoll +* registered for the Chardev during chardev_init_func(). When does this happen?
This seems like a hack that breaks when certain -chardev options are used. For example, what happens if the chardev is a TCP connection with reconnect=5. In that case the socket will be connecting asynchronously and we cannot just remove the fd watch.
Quoted 6 lines
How does this interact with TCP listen chardevs? It looks like the listener socket uses the main loop (see
tcp_chr_disconnect()).I'm worried that the chardev layer isn't thread-safe and you haven't added anything to protect it or at least refuse to run in unsafe conditions.
Indeed, I did some more reading and noticed that the TCP typed chardev is really special.
Firstly there can be the QIO thread that handles sync connecting when "reconnect" is setup (I don't really understand why we only need the threads when reconnect != 0, but anyway, I'll just assume we need the threads). It's done in qmp_chardev_open_socket().
Secondly, TCP can support TLS or TELNET (tcp_chr_new_client() handles the main logic of it), so there can be actually more than one GSource created for a single TCP chardev. Meanwhile, the chr_update_read_handler() calls never handles the re-setup of those special GSources (TLS/TELNET), only the common GSource of TCP stream read/write.
And the whole TCP channel is based on QIO stuff, which means I need to add non-default context support to QIO stuff too... That's mostly about qio_channel_add_watch(). I may need to pass in context information, and switch to GSource for that function instead of the old tags, just like what I did to chardev in general.
I'll think about these. I may possibly need some pre-requisite and separated patches to fix existing problems before going on with OOB again.
This is the worst thing I'd like to see - "surprises". :(
Thanks,
Signature · 1 line
Peter Xu
Re: [Qemu-devel] [PATCH v7 09/23] monitor: allow using IO thread for parsing
Quoted 50 lines · Peter Xu
Quoted 21 lines · Stefan Hajnoczi
monitor.c void monitor_init(Chardev *chr, int flags) · line 4164
Quoted 12 lines · Peter Xu
monitor.c +4
-2@@ -4099,24 +4156,55 @@ void monitor_init(Chardev *chr, int flags) }if (monitor_is_qmp (mon)) { -qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, - monitor_qmp_event,NULL , mon,NULL , true);qemu_chr_fe_set_echo (&mon->chr, true);json_message_parser_init (&mon->qmp.parser, handle_qmp_command); +if (mon->use_io_thr) { +/* +* It's possible that we already have an IOWatchPoll +* registered for the Chardev during chardev_init_func(). When does this happen?
This seems like a hack that breaks when certain -chardev options are used. For example, what happens if the chardev is a TCP connection with reconnect=5. In that case the socket will be connecting asynchronously and we cannot just remove the fd watch.
Quoted 6 lines
How does this interact with TCP listen chardevs? It looks like the listener socket uses the main loop (see
tcp_chr_disconnect()).I'm worried that the chardev layer isn't thread-safe and you haven't added anything to protect it or at least refuse to run in unsafe conditions.
Indeed, I did some more reading and noticed that the TCP typed chardev is really special.
Firstly there can be the QIO thread that handles sync connecting when "reconnect" is setup (I don't really understand why we only need the threads when reconnect != 0, but anyway, I'll just assume we need the threads). It's done in
qmp_chardev_open_socket().Secondly, TCP can support TLS or TELNET (
tcp_chr_new_client()handles the main logic of it), so there can be actually more than one GSource created for a single TCP chardev. Meanwhile, thechr_update_read_handler()calls never handles the re-setup of those special GSources (TLS/TELNET), only the common GSource of TCP stream read/write.And the whole TCP channel is based on QIO stuff, which means I need to add non-default context support to QIO stuff too... That's mostly about
qio_channel_add_watch(). I may need to pass in context information, and switch to GSource for that function instead of the old tags, just like what I did to chardev in general.
Quoted 5 lines
I'll think about these. I may possibly need some pre-requisite and separated patches to fix existing problems before going on with OOB again.
This is the worst thing I'd like to see - "surprises". :(
Yes, this feature is more involved than anyone thought at the beginning.
Stefan
Re: [Qemu-devel] [PATCH v7 09/23] monitor: allow using IO thread for parsing
In reply to Peter XuQuoted 89 lines · Peter Xu
Quoted 5 lines · Stefan Hajnoczi
Quoted 4 lines · Peter Xu
monitor.c +0
-0@@ -4034,12 +4044,29 @@ static void sortcmdlist(void)qsort ((void *)info_cmds, array_num, elem_size, compare_mon_cmd); }Quoted 12 lines
monitor.c static void sortcmdlist(void) · line 4055
Quoted 9 lines
monitor.c +9
-0+static GMainContext *monitor_io_context_get (void ) +{ +return iothread_get_g_main_context (mon_global.mon_iothread); +} + +static AioContext *monitor_aio_context_get (void ) +{ +return iothread_get_aio_context (mon_global.mon_iothread); +}Please follow the
X_get_Y()naming convention instead ofX_Y_get(). For example, seeqemu_get_aio_context()andiothread_get_aio_context().Sure.
Quoted 16 lines
monitor.c void error_vprintf_unless_qmp(const char *fmt, va_list ap) · line 4119
Quoted 13 lines
monitor.c +8
-0@@ -4082,11 +4109,41 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) } } +static void monitor_list_append (Monitor *mon) +{ +qemu_mutex_lock (&monitor_lock); +QTAILQ_INSERT_HEAD (&mon_list, mon, entry); +qemu_mutex_unlock (&monitor_lock); +} + +static void monitor_qmp_setup_handlers (void *data)BH functions are usually declared like this:
Quoted 6 lines
static void
X_bh(void *opaque)This way it's immediately clear that this function is invoked as a BH.
I suggest renaming the function to
monitor_qmp_setup_handlers_bh(). Using 'opaque' instead of 'data' is common, too.Sure.
Quoted 21 lines
monitor.c void monitor_init(Chardev *chr, int flags) · line 4164
Quoted 13 lines
monitor.c +4
-2@@ -4099,24 +4156,55 @@ void monitor_init(Chardev *chr, int flags) }if (monitor_is_qmp (mon)) { -qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, - monitor_qmp_event,NULL , mon,NULL , true);qemu_chr_fe_set_echo (&mon->chr, true);json_message_parser_init (&mon->qmp.parser, handle_qmp_command); +if (mon->use_io_thr) { +/* +* It's possible that we already have an IOWatchPoll +* registered for the Chardev during chardev_init_func(). When does this happen?
This seems like a hack that breaks when certain -chardev options are used. For example, what happens if the chardev is a TCP connection with reconnect=5. In that case the socket will be connecting asynchronously and we cannot just remove the fd watch.
Quoted 6 lines
How does this interact with TCP listen chardevs? It looks like the listener socket uses the main loop (see
tcp_chr_disconnect()).I'm worried that the chardev layer isn't thread-safe and you haven't added anything to protect it or at least refuse to run in unsafe conditions.
Indeed, I did some more reading and noticed that the TCP typed chardev is really special.
Firstly there can be the QIO thread that handles sync connecting when "reconnect" is setup (I don't really understand why we only need the threads when reconnect != 0, but anyway, I'll just assume we need the threads). It's done in
qmp_chardev_open_socket().Secondly, TCP can support TLS or TELNET (
tcp_chr_new_client()handles the main logic of it), so there can be actually more than one GSource created for a single TCP chardev. Meanwhile, thechr_update_read_handler()calls never handles the re-setup of those special GSources (TLS/TELNET), only the common GSource of TCP stream read/write.
Quoted 5 lines
And the whole TCP channel is based on QIO stuff, which means I need to add non-default context support to QIO stuff too... That's mostly about
qio_channel_add_watch(). I may need to pass in context information, and switch to GSource for that function instead of the old tags, just like what I did to chardev in general.
Rather than changing qio_channel_add_watch() which affects all callers, just add a qio_channel_add_watch_full() variant which includes GMainContext as an extra arg
Regards,
Daniel
Signature · 3 lines
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
Re: [Qemu-devel] [PATCH v7 09/23] monitor: allow using IO thread for parsing
Quoted 96 lines · Daniel P. Berrangé
Quoted 89 lines · Peter Xu
Quoted 5 lines · Stefan Hajnoczi
Quoted 4 lines · Peter Xu
monitor.c +0
-0@@ -4034,12 +4044,29 @@ static void sortcmdlist(void)qsort ((void *)info_cmds, array_num, elem_size, compare_mon_cmd); }Quoted 12 lines
monitor.c static void sortcmdlist(void) · line 4055
Quoted 9 lines
monitor.c +9
-0+static GMainContext *monitor_io_context_get (void ) +{ +return iothread_get_g_main_context (mon_global.mon_iothread); +} + +static AioContext *monitor_aio_context_get (void ) +{ +return iothread_get_aio_context (mon_global.mon_iothread); +}Please follow the
X_get_Y()naming convention instead ofX_Y_get(). For example, seeqemu_get_aio_context()andiothread_get_aio_context().Sure.
Quoted 16 lines
monitor.c void error_vprintf_unless_qmp(const char *fmt, va_list ap) · line 4119
Quoted 13 lines
monitor.c +8
-0@@ -4082,11 +4109,41 @@ void error_vprintf_unless_qmp(const char *fmt, va_list ap) } } +static void monitor_list_append (Monitor *mon) +{ +qemu_mutex_lock (&monitor_lock); +QTAILQ_INSERT_HEAD (&mon_list, mon, entry); +qemu_mutex_unlock (&monitor_lock); +} + +static void monitor_qmp_setup_handlers (void *data)BH functions are usually declared like this:
Quoted 6 lines
static void
X_bh(void *opaque)This way it's immediately clear that this function is invoked as a BH.
I suggest renaming the function to
monitor_qmp_setup_handlers_bh(). Using 'opaque' instead of 'data' is common, too.Sure.
Quoted 21 lines
monitor.c void monitor_init(Chardev *chr, int flags) · line 4164
Quoted 13 lines
monitor.c +4
-2@@ -4099,24 +4156,55 @@ void monitor_init(Chardev *chr, int flags) }if (monitor_is_qmp (mon)) { -qemu_chr_fe_set_handlers (&mon->chr, monitor_can_read, monitor_qmp_read, - monitor_qmp_event,NULL , mon,NULL , true);qemu_chr_fe_set_echo (&mon->chr, true);json_message_parser_init (&mon->qmp.parser, handle_qmp_command); +if (mon->use_io_thr) { +/* +* It's possible that we already have an IOWatchPoll +* registered for the Chardev during chardev_init_func(). When does this happen?
This seems like a hack that breaks when certain -chardev options are used. For example, what happens if the chardev is a TCP connection with reconnect=5. In that case the socket will be connecting asynchronously and we cannot just remove the fd watch.
Quoted 6 lines
How does this interact with TCP listen chardevs? It looks like the listener socket uses the main loop (see
tcp_chr_disconnect()).I'm worried that the chardev layer isn't thread-safe and you haven't added anything to protect it or at least refuse to run in unsafe conditions.
Indeed, I did some more reading and noticed that the TCP typed chardev is really special.
Firstly there can be the QIO thread that handles sync connecting when "reconnect" is setup (I don't really understand why we only need the threads when reconnect != 0, but anyway, I'll just assume we need the threads). It's done in
qmp_chardev_open_socket().Secondly, TCP can support TLS or TELNET (
tcp_chr_new_client()handles the main logic of it), so there can be actually more than one GSource created for a single TCP chardev. Meanwhile, thechr_update_read_handler()calls never handles the re-setup of those special GSources (TLS/TELNET), only the common GSource of TCP stream read/write.Quoted 5 lines
And the whole TCP channel is based on QIO stuff, which means I need to add non-default context support to QIO stuff too... That's mostly about
qio_channel_add_watch(). I may need to pass in context information, and switch to GSource for that function instead of the old tags, just like what I did to chardev in general.
Quoted 3 lines
Rather than changing
qio_channel_add_watch()which affects all callers, just add aqio_channel_add_watch_full()variant which includes GMainContext as an extra arg
Yeh, will do. Thanks,
Signature · 1 line
Peter Xu
PATCH v7 10/23 qmp: introduce QMPCapability
In reply to Peter XuThere were no QMP capabilities defined. Define the first "oob" as capability to allow out-of-band messages.
After this patch, we will allow QMP clients to enable QMP capabilities when sending the first "qmp_capabilities" command. Originally we are starting QMP session with no arguments like:
{ "execute": "qmp_capabilities" }
Now we can enable some QMP capabilities using (take OOB as example, which is the only one capability that we support):
{ "execute": "qmp_capabilities",
"argument": { "enable": [ "oob" ] } }When the "argument" key is not provided, no capability is enabled.
For capability "oob", the monitor needs to be run on dedicated IO thread, otherwise the command will fail. For example, trying to enable OOB on a MUXed typed QMP monitor will fail.
One thing to mention is that, QMP capabilities are per-monitor, and also when the connection is closed due to some reason, the capabilities will be reset.
Also, touch up qmp-test.c to test the new bits.
--- monitor.c | 76 ++++++++++++++++++++++++++++++++++++++++++++++++++++---- qapi-schema.json | 32 +++++++++++++++++++++--- tests/qmp-test.c | 10 +++++++- 3 files changed, 109 insertions(+), 9 deletions(-)
monitor.c +71 -5
@@ -167,6 +167,7 @@ typedef struct {* mode. */ QmpCommandList *commands; +bool qmp_caps[QMP_CAPABILITY__MAX ]; } MonitorQMP;/*
@@ -1036,8 +1037,42 @@ static void monitor_init_qmp_commands(void) qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS); } -void qmp_qmp_capabilities (Error **errp) +static void qmp_caps_check (Monitor *mon,QMPCapabilityList *list, +Error **errp) +{ +for (; list; list = list->next) { +assert (list->value <QMP_CAPABILITY__MAX ); +switch (list->value) { +case QMP_CAPABILITY_OOB : +if (!mon->use_io_thr) { +/* +* Out-Of-Band only works with monitors that are +* running on dedicated IOThread. +*/ +error_setg (errp,"This monitor does not support " +"Out-Of-Band (OOB)" ); +return ; + } +break ; +default : +break ; + } + } +} + +/* This function should only be called after capabilities are checked. */ +static void qmp_caps_apply (Monitor *mon,QMPCapabilityList *list) { +for (; list; list = list->next) { + mon->qmp.qmp_caps[list->value] = true; + } +} + +void qmp_qmp_capabilities (bool has_enable,QMPCapabilityList *enable, +Error **errp) +{ +Error *local_err =NULL ; +if (cur_mon->qmp.commands == &qmp_commands) {error_set (errp,ERROR_CLASS_COMMAND_NOT_FOUND , "Capabilities negotiation is already complete, command "
@@ -1045,6 +1080,21 @@ void qmp_qmp_capabilities(Error **errp)return ; } +/* Enable QMP capabilities provided by the client if applicable. */ +if (has_enable) { +qmp_caps_check (cur_mon, enable, &local_err); +if (local_err) { +/* +* Failed check on any of the capabilities will fail the +* entire command (and thus not apply any of the other +* capabilities that were also requested). +*/ +error_propagate (errp, local_err); +return ; + } +qmp_caps_apply (cur_mon, enable); + } + cur_mon->qmp.commands = &qmp_commands; }
@@ -3942,14 +3992,29 @@ void monitor_resume(Monitor *mon)readline_show_prompt (mon->rs); } -static QObject *get_qmp_greeting (void ) +static QObject *get_qmp_greeting (Monitor *mon) { +QList *cap_list =qlist_new ();QObject *ver =NULL ; +QMPCapability cap;qmp_marshal_query_version (NULL , &ver,NULL ); -return qobject_from_jsonf ("{'QMP': {'version': %p, 'capabilities': []}}" , - ver); +for (cap =0 ; cap <QMP_CAPABILITY__MAX ; cap++) { +if (!mon->use_io_thr && cap ==QMP_CAPABILITY_OOB ) { +/* Monitors that are not using IOThread won't support OOB */ +continue ; + } +qlist_append (cap_list,qstring_from_str (QMPCapability_str (cap))); + } + +return qobject_from_jsonf ("{'QMP': {'version': %p, 'capabilities': %p}}" , + ver, cap_list); +} + +static void monitor_qmp_caps_reset (Monitor *mon) +{ +memset (mon->qmp.qmp_caps,0 ,sizeof (mon->qmp.qmp_caps)); }static void monitor_qmp_event (void *opaque,int event)
@@ -3960,7 +4025,8 @@ static void monitor_qmp_event(void *opaque, int event)switch (event) {case CHR_EVENT_OPENED : mon->qmp.commands = &qmp_cap_negotiation_commands; - data =get_qmp_greeting (); +monitor_qmp_caps_reset (mon); + data =get_qmp_greeting (mon);monitor_json_emitter (mon, data);qobject_decref (data); mon_refcount++;
qapi-schema.json +29 -3
@@ -102,21 +102,47 @@ # # Enable QMP capabilities. # -# Arguments: None. +# Arguments: +# +# @enable: List of QMPCapabilities to enable, which is optional. +# Client must not enable any capability that is not +# mentioned in QMP greeting message. If the field is not +# provided, it means no QMP capabilities will be enabled. +# (since 2.12) # # Example: # -# -> { "execute": "qmp_capabilities" } +# -> { "execute": "qmp_capabilities", +# "arguments": { "enable": [ "oob" ] } } # <- { "return": {} } # # Notes: This command is valid exactly when first connecting: it must be # issued before any other command will be accepted, and will fail once the # monitor is accepting other commands. (see qemu docs/interop/qmp-spec.txt) # +# QMP client needs to explicitly enable QMP capabilities, otherwise +# all the QMP capabilities will be turned off by default. +# # Since: 0.13 # ## -{ 'command': 'qmp_capabilities' } +{ 'command': 'qmp_capabilities', + 'data': { '*enable': [ 'QMPCapability' ] } } + +## +# @QMPCapability: +# +# Enumeration of capabilities to be advertised during initial client +# connection, used for agreeing on particular QMP extension behaviors. +# +# @oob: QMP ability to support Out-Of-Band requests. +# (Please refer to qmp-spec.txt for more information on OOB) +# +# Since: 2.12 +# +## +{ 'enum': 'QMPCapability', + 'data': [ 'oob' ] } ## # @VersionTriple:
tests/qmp-test.c +9 -1
@@ -17,6 +17,7 @@#include "qapi/qobject-input-visitor.h" #include "qapi/util.h" #include "qapi/visitor.h" +#include "qapi/qmp/qstring.h" const char common_args[] ="-nodefaults -machine none" ;
@@ -75,6 +76,8 @@ static void test_qmp_protocol(void) {QDict *resp, *q, *ret;QList *capabilities; +const QListEntry *entry; +QString *qstr; global_qtest =qtest_init_without_qmp_handshake (common_args);
@@ -84,7 +87,12 @@ static void test_qmp_protocol(void)g_assert (q);test_version (qdict_get (q,"version" )); capabilities =qdict_get_qlist (q,"capabilities" ); -g_assert (capabilities &&qlist_empty (capabilities)); +g_assert (capabilities); + entry =qlist_first (capabilities); +g_assert (entry); + qstr =qobject_to_qstring (entry->value); +g_assert (qstr); + g_assert_cmpstr(qstring_get_str(qstr), ==, "oob");QDECREF (resp);/* Test valid command before handshake */
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 10/23] qmp: introduce QMPCapability
Quoted 5 lines · Peter Xu
qapi-schema.json +0
-0
qapi-schema.json line 109
Quoted 9 lines
qapi-schema.json +5
-1@@ -102,21 +102,47 @@ # # Enable QMP capabilities. # -# Arguments: None. +# Arguments: +# +# @enable: List of QMPCapabilities to enable, which is optional. +# Client must not enable any capability that is not +# mentioned in QMP greeting message.
qapi-schema.json uses full sentences so I've added missing articles to the text. I also changed s/QMPCapabilities/QMPCapability values/ to avoid confusion about the type name:
An optional list of QMPCapability values to enable. The client must not enable any capability that is not mentioned in the QMP greeting message.
qapi-schema.json line 123
Quoted 13 lines
qapi-schema.json +3
-1# # Example: # -# -> { "execute": "qmp_capabilities" } +# -> { "execute": "qmp_capabilities", +# "arguments": { "enable": [ "oob" ] } } # <- { "return": {} } # # Notes: This command is valid exactly when first connecting: it must be # issued before any other command will be accepted, and will fail once the # monitor is accepting other commands. (see qemu docs/interop/qmp-spec.txt) # +# QMP client needs to explicitly enable QMP capabilities, otherwise
s/QMP client/The QMP client/
Re: [Qemu-devel] [PATCH v7 10/23] qmp: introduce QMPCapability
Quoted 24 lines · Stefan Hajnoczi
Quoted 5 lines · Peter Xu
qapi-schema.json +0
-0qapi-schema.json line 109
Quoted 9 lines
qapi-schema.json +5
-1@@ -102,21 +102,47 @@ # # Enable QMP capabilities. # -# Arguments: None. +# Arguments: +# +# @enable: List of QMPCapabilities to enable, which is optional. +# Client must not enable any capability that is not +# mentioned in QMP greeting message.qapi-schema.json uses full sentences so I've added missing articles to the text. I also changed s/QMPCapabilities/QMPCapability values/ to avoid confusion about the type name:
An optional list of QMPCapability values to enable. The client must not enable any capability that is not mentioned in the QMP greeting message.
Quoted 15 lines
qapi-schema.json line 123
Quoted 13 lines
qapi-schema.json +3
-1# # Example: # -# -> { "execute": "qmp_capabilities" } +# -> { "execute": "qmp_capabilities", +# "arguments": { "enable": [ "oob" ] } } # <- { "return": {} } # # Notes: This command is valid exactly when first connecting: it must be # issued before any other command will be accepted, and will fail once the # monitor is accepting other commands. (see qemu docs/interop/qmp-spec.txt) # +# QMP client needs to explicitly enable QMP capabilities, otherwises/QMP client/The QMP client/
Thanks; Fixing them all.
Signature · 1 line
Peter Xu
PATCH v7 11/23 monitor: introduce monitor_qmp_respond()
In reply to Peter XuA tiny refactoring, preparing to split the QMP dispatcher away.
--- monitor.c | 50 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 33 insertions(+), 17 deletions(-)
monitor.c +33 -17
@@ -3867,6 +3867,38 @@ static int monitor_can_read(void *opaque)return (mon->suspend_cnt ==0 ) ?1 :0 ; } +/* +* 1. This function takes ownership of rsp, err, and id. +* 2. rsp, err, and id may be NULL. +* 3. If err != NULL then rsp must be NULL. +*/ +static void monitor_qmp_respond (Monitor *mon,QObject *rsp, +Error *err,QObject *id) +{ +QDict *qdict =NULL ; + +if (err) { +assert (!rsp); + qdict =qdict_new (); +qdict_put_obj (qdict,"error" ,qmp_build_error_object (err)); +error_free (err); + rsp =QOBJECT (qdict); + } + +if (rsp) { +if (id) { +/* This is for the qdict below. */ +qobject_incref (id); +qdict_put_obj (qobject_to_qdict (rsp),"id" , id); + } + +monitor_json_emitter (mon, rsp); + } + +qobject_decref (id); +qobject_decref (rsp); +} +static void handle_qmp_command (JSONMessageParser *parser,GQueue *tokens) {QObject *req, *rsp =NULL , *id =NULL ;
@@ -3918,24 +3950,8 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) } err_out: -if (err) { - qdict =qdict_new (); -qdict_put_obj (qdict,"error" ,qmp_build_error_object (err)); -error_free (err); - rsp =QOBJECT (qdict); - } +monitor_qmp_respond (mon, rsp, err, id); -if (rsp) { -if (id) { -qdict_put_obj (qobject_to_qdict (rsp),"id" , id); - id =NULL ; - } - -monitor_json_emitter (mon, rsp); - } - -qobject_decref (id); -qobject_decref (rsp);qobject_decref (req); }
Signature · 1 line
2.14.3
PATCH v7 12/23 monitor: let suspend_cnt be thread safe
In reply to Peter XuMonitor code now can be run in more than one thread. Let it be thread safe when accessing suspend_cnt counter.
--- monitor.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
monitor.c +8 -7
@@ -190,7 +190,7 @@ struct Monitor {CharBackend chr;int reset_seen;int flags; -int suspend_cnt; +int suspend_cnt;/* Needs to be accessed atomically */ bool skip_flush;bool use_io_thr;
@@ -3864,7 +3864,7 @@ static int monitor_can_read(void *opaque) {Monitor *mon = opaque; -return (mon->suspend_cnt ==0 ) ?1 :0 ; +return !atomic_mb_read (&mon->suspend_cnt); }/*
@@ -3996,7 +3996,7 @@ int monitor_suspend(Monitor *mon) {if (!mon->rs)return -ENOTTY ; - mon->suspend_cnt++; +atomic_inc (&mon->suspend_cnt);return 0 ; }
@@ -4004,8 +4004,9 @@ void monitor_resume(Monitor *mon) {if (!mon->rs)return ; -if (--mon->suspend_cnt ==0 ) +if (atomic_dec_fetch (&mon->suspend_cnt) ==0 ) {readline_show_prompt (mon->rs); + } }static QObject *get_qmp_greeting (Monitor *mon)
@@ -4070,19 +4071,19 @@ static void monitor_event(void *opaque, int event)monitor_resume (mon);monitor_flush (mon); }else { - mon->suspend_cnt =0 ; +atomic_mb_set (&mon->suspend_cnt,0 ); }break ;case CHR_EVENT_MUX_OUT :if (mon->reset_seen) { -if (mon->suspend_cnt ==0 ) { +if (atomic_mb_read (&mon->suspend_cnt) ==0 ) {monitor_printf (mon,"\n" ); }monitor_flush (mon);monitor_suspend (mon); }else { - mon->suspend_cnt++; +atomic_inc (&mon->suspend_cnt); }qemu_mutex_lock (&mon->out_lock); mon->mux_out =1 ;
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 12/23] monitor: let suspend_cnt be thread safe
monitor.c int monitor_suspend(Monitor *mon) · line 3998
Quoted 4 lines · Peter Xu
monitor.c +0
-0@@ -3996,7 +3996,7 @@ int monitor_suspend(Monitor *mon) {if (!mon->rs)return -ENOTTY ;
Does this mean that QMP monitors like -qmp unix:/tmp/foo,server,nowait will not use suspend_cnt?
I thought the point was to use suspend_cnt for QMP monitors too.
Re: [Qemu-devel] [PATCH v7 12/23] monitor: let suspend_cnt be thread safe
In reply to Peter XuNevermind, I see the next patch adds QMP support.
Stefan
Re: [Qemu-devel] [PATCH v7 12/23] monitor: let suspend_cnt be thread safe
In reply to Peter XuQuoted 8 lines · Peter Xu
Monitor code now can be run in more than one thread. Let it be thread safe when accessing
suspend_cntcounter.Reviewed-by Eric Blake <[email protected]>Signed-off-by Peter Xu <[email protected]>--- monitor.c | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-)
PATCH v7 13/23 monitor: let suspend/resume work even with QMPs
In reply to Peter XuThis patches allows QMP monitors to be suspended/resumed.
One thing to mention is that for QMPs that are using IOThreads, we need an explicit kick for the IOThread in case it is sleeping.
Meanwhile, we need to take special care on non-interactive HMPs. Currently only gdbserver is using that. For these monitors, we still don't allow suspend/resume operations.
Since at it, add traces for the operations.
--- monitor.c | 45 ++++++++++++++++++++++++++++++++++++++++++--- trace-events | 1 + 2 files changed, 43 insertions(+), 3 deletions(-)
monitor.c +42 -3
@@ -246,6 +246,21 @@ static inline bool monitor_is_qmp(const Monitor *mon)return (mon->flags &MONITOR_USE_CONTROL ); } +/** +* Whether @mon is using readline? Note: not all HMP monitors can are +* using readline, e.g., gdbserver has non-interactive HMP monitor, so +* readline is not used there. +*/ +static inline bool monitor_uses_readline (const Monitor *mon) +{ +return mon->flags &MONITOR_USE_READLINE ; +} + +static inline bool monitor_is_hmp_non_interactive (const Monitor *mon) +{ +return !monitor_is_qmp (mon) && !monitor_uses_readline (mon); +} +/** * Is the current monitor, if any, a QMP monitor? */
@@ -3994,19 +4009,43 @@ static void monitor_command_cb(void *opaque, const char *cmdline,int monitor_suspend (Monitor *mon) { -if (!mon->rs) +if (monitor_is_hmp_non_interactive (mon)) {return -ENOTTY ; + } + +if (monitor_is_qmp (mon)) { +/* +* Kick iothread to make sure this takes effect. It'll be +* evaluated again in prepare() of the watch object. +*/ +aio_notify (iothread_get_aio_context (mon_global.mon_iothread)); + }atomic_inc (&mon->suspend_cnt); +trace_monitor_suspend (mon,1 );return 0 ; }void monitor_resume (Monitor *mon) { -if (!mon->rs) +if (monitor_is_hmp_non_interactive (mon)) {return ; + } +if (atomic_dec_fetch (&mon->suspend_cnt) ==0 ) { -readline_show_prompt (mon->rs); +if (monitor_is_qmp (mon)) { +/* +* For QMP monitors that are running in IOThread, let's +* kick the thread in case it's sleeping. +*/ +if (mon->use_io_thr) { +aio_notify (iothread_get_aio_context (mon_global.mon_iothread)); + } + }else { +assert (mon->rs); +readline_show_prompt (mon->rs); + } } +trace_monitor_suspend (mon,-1 ); }static QObject *get_qmp_greeting (Monitor *mon)
trace-events +1 -0
@@ -47,6 +47,7 @@ monitor_protocol_event_emit(uint32_t event, void *data) "event=%d data=%p" monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event=%d data=%p rate=%" PRId64 handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s" handle_qmp_command(void *mon, const char *req) "mon %p req: %s" +monitor_suspend(void *ptr, int cnt) "mon %p: %d" # dma-helpers.c dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 13/23] monitor: let suspend/resume work even with QMPs
monitor.c static inline bool monitor_is_qmp(const Monitor *mon) · line 251
Quoted 11 lines · Peter Xu
monitor.c +3
-0@@ -246,6 +246,21 @@ static inline bool monitor_is_qmp(const Monitor *mon)return (mon->flags &MONITOR_USE_CONTROL ); } +/** +* Whether @mon is using readline? Note: not all HMP monitors can are +* using readline, e.g., gdbserver has non-interactive HMP monitor, so
s/can are using/use/
s/has non-interactive HMP monitor/has a non-interactive HMP monitor/
Quoted 5 lines
monitor.c +0
-1@@ -3994,19 +4009,43 @@ static void monitor_command_cb(void *opaque, const char *cmdline,int monitor_suspend (Monitor *mon) { -if (!mon->rs)
monitor.c static void monitor_command_cb(void *opaque, const char *cmdline, · line 4021
Quoted 10 lines
monitor.c +9
-0+if (monitor_is_hmp_non_interactive (mon)) {return -ENOTTY ; + } + +if (monitor_is_qmp (mon)) { +/* +* Kick iothread to make sure this takes effect. It'll be +* evaluated again in prepare() of the watch object. +*/ +aio_notify (iothread_get_aio_context (mon_global.mon_iothread));
This must be done after incrementing suspend_cnt to avoid the race condition between the iothread and our thread.
Re: [Qemu-devel] [PATCH v7 13/23] monitor: let suspend/resume work even with QMPs
Quoted 19 lines · Stefan Hajnoczi
monitor.c static inline bool monitor_is_qmp(const Monitor *mon) · line 251
Quoted 11 lines · Peter Xu
monitor.c +3
-0@@ -246,6 +246,21 @@ static inline bool monitor_is_qmp(const Monitor *mon)return (mon->flags &MONITOR_USE_CONTROL ); } +/** +* Whether @mon is using readline? Note: not all HMP monitors can are +* using readline, e.g., gdbserver has non-interactive HMP monitor, so s/can are using/use/
s/has non-interactive HMP monitor/has a non-interactive HMP monitor/
Quoted 2 lines
monitor.c +0
-0@@ -3994,19 +4009,43 @@ static void monitor_command_cb(void *opaque, const char *cmdline,
Quoted 16 lines
monitor.c static void monitor_command_cb(void *opaque, const char *cmdline, · line 4021
Quoted 13 lines
monitor.c +9
-1int monitor_suspend (Monitor *mon) { -if (!mon->rs) +if (monitor_is_hmp_non_interactive (mon)) {return -ENOTTY ; + } + +if (monitor_is_qmp (mon)) { +/* +* Kick iothread to make sure this takes effect. It'll be +* evaluated again in prepare() of the watch object. +*/ +aio_notify (iothread_get_aio_context (mon_global.mon_iothread));This must be done after incrementing
suspend_cntto avoid the race condition between the iothread and our thread.
Fixed both places. Thanks,
Signature · 1 line
Peter Xu
PATCH v7 14/23 monitor: separate QMP parser and dispatcher
In reply to Peter XuOriginally QMP goes through these steps:
JSON Parser --> QMP Dispatcher --> Respond
/|\ (2) (3) |
(1) | \|/ (4)
+--------- main thread --------+This patch does this:
JSON Parser QMP Dispatcher --> Respond
/|\ | /|\ (4) |
| | (2) | (3) | (5)
(1) | +-----> | \|/
+--------- main thread <-------+So the parsing job and the dispatching job is isolated now. It gives us a chance in following up patches to totally move the parser outside.
The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is used for all the monitors.
--- monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 178 insertions(+), 23 deletions(-)
monitor.c +178 -23
@@ -168,6 +168,13 @@ typedef struct {*/ QmpCommandList *commands;bool qmp_caps[QMP_CAPABILITY__MAX ]; +/* +* Protects qmp request/response queue. Please take monitor_lock +* first when used together. +*/ +QemuMutex qmp_queue_lock; +/* Input queue that holds all the parsed QMP requests */ +GQueue *qmp_requests; } MonitorQMP;/*
@@ -214,6 +221,8 @@ struct Monitor {/* Let's add monitor global variables to this struct. */ static struct {IOThread *mon_iothread; +/* Bottom half to dispatch the requests received from IO thread */ +QEMUBH *qmp_dispatcher_bh; } mon_global;/* QMP checker flags */
@@ -596,11 +605,13 @@ static void monitor_data_init(Monitor *mon, bool skip_flush, {memset (mon,0 ,sizeof (Monitor));qemu_mutex_init (&mon->out_lock); +qemu_mutex_init (&mon->qmp.qmp_queue_lock); mon->outbuf =qstring_new ();/* Use *mon_cmds by default. */ mon->cmd_table = mon_cmds; mon->skip_flush = skip_flush; mon->use_io_thr = use_io_thr; + mon->qmp.qmp_requests =g_queue_new (); }static void monitor_data_destroy (Monitor *mon)
@@ -613,6 +624,8 @@ static void monitor_data_destroy(Monitor *mon)readline_free (mon->rs);QDECREF (mon->outbuf);qemu_mutex_destroy (&mon->out_lock); +qemu_mutex_destroy (&mon->qmp.qmp_queue_lock); +g_queue_free (mon->qmp.qmp_requests); } char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
@@ -1052,6 +1065,16 @@ static void monitor_init_qmp_commands(void) qmp_marshal_qmp_capabilities, QCO_NO_OPTIONS); } +static bool qmp_cap_enabled (Monitor *mon,QMPCapability cap) +{ +return mon->qmp.qmp_caps[cap]; +} + +static bool qmp_oob_enabled (Monitor *mon) +{ +return qmp_cap_enabled (mon,QMP_CAPABILITY_OOB ); +} +static void qmp_caps_check (Monitor *mon,QMPCapabilityList *list,Error **errp) {
@@ -3914,30 +3937,39 @@ static void monitor_qmp_respond(Monitor *mon, QObject *rsp,qobject_decref (rsp); } -static void handle_qmp_command (JSONMessageParser *parser,GQueue *tokens) +struct QMPRequest { +/* Owner of the request */ +Monitor *mon; +/* "id" field of the request */ +QObject *id; +/* Request object to be handled */ +QObject *req; +/* +* Whether we need to resume the monitor afterward. This flag is +* used to emulate the old QMP server behavior that the current +* command must be completed before execution of the next one. +*/ +bool need_resume; +}; +typedef struct QMPRequest QMPRequest ; + +/* +* Dispatch one single QMP request. The function will free the req_obj +* and objects inside it before return. +*/ +static void monitor_qmp_dispatch_one (QMPRequest *req_obj) { -QObject *req, *rsp =NULL , *id =NULL ; +Monitor *mon, *old_mon; +QObject *req, *rsp =NULL , *id;QDict *qdict =NULL ; -MonitorQMP *mon_qmp =container_of (parser, MonitorQMP, parser); -Monitor *old_mon, *mon =container_of (mon_qmp, Monitor, qmp); - -Error *err =NULL ; +bool need_resume; - req =json_parser_parse_err (tokens,NULL , &err); -if (!req && !err) { -/* json_parser_parse_err() sucks: can fail without setting @err */ -error_setg (&err,QERR_JSON_PARSING ); - } -if (err) { - goto err_out; - } + req = req_obj->req; + mon = req_obj->mon; + id = req_obj->id; + need_resume = req_obj->need_resume; - qdict =qobject_to_qdict (req); -if (qdict) { - id =qdict_get (qdict,"id" ); -qobject_incref (id); -qdict_del (qdict,"id" ); - }/* else will fail qmp_dispatch() */ +g_free (req_obj);if (trace_event_get_state_backends (TRACE_HANDLE_QMP_COMMAND )) {QString *req_json =qobject_to_json (req);
@@ -3948,7 +3980,7 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) old_mon = cur_mon; cur_mon = mon; - rsp =qmp_dispatch (cur_mon->qmp.commands, req); + rsp =qmp_dispatch (mon->qmp.commands, req); cur_mon = old_mon;
@@ -3964,12 +3996,122 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) } } -err_out: -monitor_qmp_respond (mon, rsp, err, id); +/* Respond if necessary */ +monitor_qmp_respond (mon, rsp,NULL , id); + +/* This pairs with the monitor_suspend() in handle_qmp_command(). */ +if (need_resume) { +monitor_resume (mon); + }qobject_decref (req); } +/* +* Pop one QMP request from monitor queues, return NULL if not found. +* We are using round-robin fashion to pop the request, to avoid +* processing command only on a very busy monitor. To achieve that, +* when we processed one request on specific monitor, we put that +* monitor to the end of mon_list queue. +*/ +static QMPRequest *monitor_qmp_requests_pop_one (void ) +{ +QMPRequest *req_obj =NULL ; +Monitor *mon; + +qemu_mutex_lock (&monitor_lock); + + QTAILQ_FOREACH(mon, &mon_list, entry) { +qemu_mutex_lock (&mon->qmp.qmp_queue_lock); + req_obj =g_queue_pop_head (mon->qmp.qmp_requests); +qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +if (req_obj) { +break ; + } + } + +if (req_obj) { +/* +* We found one request on the monitor. Degrade this monitor's +* priority to lowest by re-inserting it to end of queue. +*/ +QTAILQ_REMOVE (&mon_list, mon, entry); +QTAILQ_INSERT_TAIL (&mon_list, mon, entry); + } + +qemu_mutex_unlock (&monitor_lock); + +return req_obj; +} + +static void monitor_qmp_bh_dispatcher (void *data) +{ +QMPRequest *req_obj =monitor_qmp_requests_pop_one (); + +if (req_obj) { +monitor_qmp_dispatch_one (req_obj); +/* Reschedule instead of looping so the main loop stays responsive */ +qemu_bh_schedule (mon_global.qmp_dispatcher_bh); + } +} + +static void handle_qmp_command (JSONMessageParser *parser,GQueue *tokens) +{ +QObject *req, *id =NULL ; +QDict *qdict =NULL ; +MonitorQMP *mon_qmp =container_of (parser, MonitorQMP, parser); +Monitor *mon =container_of (mon_qmp, Monitor, qmp); +Error *err =NULL ; +QMPRequest *req_obj; + + req =json_parser_parse_err (tokens,NULL , &err); +if (!req && !err) { +/* json_parser_parse_err() sucks: can fail without setting @err */ +error_setg (&err,QERR_JSON_PARSING ); + } +if (err) { +monitor_qmp_respond (mon,NULL , err,NULL ); +qobject_decref (req); +return ; + } + + qdict =qobject_to_qdict (req); +if (qdict) { + id =qdict_get (qdict,"id" ); +qobject_incref (id); +qdict_del (qdict,"id" ); + }/* else will fail qmp_dispatch() */ + + req_obj =g_new0 (QMPRequest,1 ); + req_obj->mon = mon; + req_obj->id = id; + req_obj->req = req; + req_obj->need_resume = false; + +/* +* If OOB is not enabled on current monitor, we'll emulate the old +* behavior that we won't process current monitor any more until +* it is responded. This helps make sure that as long as OOB is +* not enabled, the server will never drop any command. +*/ +if (!qmp_oob_enabled (mon)) { +monitor_suspend (mon); + req_obj->need_resume = true; + } + +/* +* Put the request to the end of queue so that requests will be +* handled in time order. Ownership for req_obj, req, id, +* etc. will be delivered to the handler side. +*/ +qemu_mutex_lock (&mon->qmp.qmp_queue_lock); +g_queue_push_tail (mon->qmp.qmp_requests, req_obj); +qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); + +/* Kick the dispatcher routine */ +qemu_bh_schedule (mon_global.qmp_dispatcher_bh); +} +static void monitor_qmp_read (void *opaque,const uint8_t *buf,int size) {Monitor *mon = opaque;
@@ -4180,6 +4322,15 @@ static void monitor_iothread_init(void) { mon_global.mon_iothread =iothread_create ("mon_iothread" , &error_abort); + +/* +* This MUST be on main loop thread since we have commands that +* have assumption to be run on main loop thread. It would be +* nice that one day we can remove this assumption in the future. +*/ + mon_global.qmp_dispatcher_bh =aio_bh_new (qemu_get_aio_context (), + monitor_qmp_bh_dispatcher, +NULL ); }void monitor_init_globals (void )
@@ -4335,6 +4486,10 @@ void monitor_cleanup(void) }qemu_mutex_unlock (&monitor_lock); +/* QEMUBHs needs to be deleted before destroying the IOThread. */ +qemu_bh_delete (mon_global.qmp_dispatcher_bh); + mon_global.qmp_dispatcher_bh =NULL ; +iothread_destroy (mon_global.mon_iothread); mon_global.mon_iothread =NULL ; }
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 14/23] monitor: separate QMP parser and dispatcher
Quoted 18 lines · Peter Xu
Originally QMP goes through these steps:
JSON Parser --> QMP Dispatcher --> Respond
/|\ (2) (3) | (1) | \|/ (4) +--------- main thread --------+This patch does this:
JSON Parser QMP Dispatcher --> Respond
/|\ | /|\ (4) | | | (2) | (3) | (5) (1) | +-----> | \|/ +--------- main thread <-------+So the parsing job and the dispatching job is isolated now. It gives us a chance in following up patches to totally move the parser outside.
Quoted 7 lines
The isolation is done using one QEMUBH. Only one dispatcher QEMUBH is used for all the monitors.
Signed-off-by Peter Xu <[email protected]>--- monitor.c | 201 +++++++++++++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 178 insertions(+), 23 deletions(-)
PATCH v7 15/23 qmp: add new event "command-dropped"
In reply to Peter XuThis event will be emitted if one QMP command is dropped. Along, declare an enum for the reasons.
--- qapi-schema.json | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
qapi-schema.json +37 -0
@@ -3215,3 +3215,40 @@ # Since: 2.11 ## { 'command': 'watchdog-set-action', 'data' : {'action': 'WatchdogAction'} } + +## +# @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. +# +# @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' } }
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 15/23] qmp: add new event "command-dropped"
Quoted 8 lines · Peter Xu
This event will be emitted if one QMP command is dropped. Along, declare an enum for the reasons.
Reviewed-by Fam Zheng <[email protected]>Signed-off-by Peter Xu <[email protected]>--- qapi-schema.json | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+)
PATCH v7 16/23 monitor: send event when command queue full
In reply to Peter XuSet maximum QMP command queue length to 8. If queue full, instead of queue the command, we directly return a "command-dropped" event, telling client that specific command is dropped.
Note that this flow control mechanism is only valid if OOB is enabled. If it's not, the effective queue length will always be 1, which strictly follows original behavior of QMP command handling (which never drop messages).
--- monitor.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
monitor.c +17 -1
@@ -4055,6 +4055,8 @@ static void monitor_qmp_bh_dispatcher(void *data) } } +#define QMP_REQ_QUEUE_LEN_MAX (8) +static void handle_qmp_command (JSONMessageParser *parser,GQueue *tokens) {QObject *req, *id =NULL ;
@@ -4088,6 +4090,9 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens) req_obj->req = req; req_obj->need_resume = false; +/* Protect qmp_requests and fetching its length. */ +qemu_mutex_lock (&mon->qmp.qmp_queue_lock); +/* * If OOB is not enabled on current monitor, we'll emulate the old * behavior that we won't process current monitor any more until
@@ -4097,6 +4102,18 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)if (!qmp_oob_enabled (mon)) {monitor_suspend (mon); req_obj->need_resume = true; + }else { +/* Drop the request if queue is full. */ +if (mon->qmp.qmp_requests->length >=QMP_REQ_QUEUE_LEN_MAX ) { +qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +qapi_event_send_command_dropped (id, +COMMAND_DROP_REASON_QUEUE_FULL , +NULL ); +qobject_decref (id); +qobject_decref (req); +g_free (req_obj); +return ; + } }/*
@@ -4104,7 +4121,6 @@ static void handle_qmp_command(JSONMessageParser *parser, GQueue *tokens)* handled in time order. Ownership for req_obj, req, id, * etc. will be delivered to the handler side. */ -qemu_mutex_lock (&mon->qmp.qmp_queue_lock);g_queue_push_tail (mon->qmp.qmp_requests, req_obj);qemu_mutex_unlock (&mon->qmp.qmp_queue_lock);
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 16/23] monitor: send event when command queue full
Quoted 9 lines · Peter Xu
Set maximum QMP command queue length to 8. If queue full, instead of queue the command, we directly return a "command-dropped" event, telling client that specific command is dropped.
Note that this flow control mechanism is only valid if OOB is enabled. If it's not, the effective queue length will always be 1, which strictly follows original behavior of QMP command handling (which never drop messages).
Quoted 4 lines
Signed-off-by Peter Xu <[email protected]>--- monitor.c | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)
PATCH v7 17/23 qapi: introduce new cmd option "allow-oob"
In reply to Peter XuHere "oob" stands for "Out-Of-Band". When "allow-oob" is set, it means the command allows out-of-band execution.
The "oob" idea is proposed by Markus Armbruster in following thread:
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg02057.html
This new "allow-oob" boolean will be exposed by "query-qmp-schema" as well for command entries, so that QMP clients can know which command can be used as out-of-band calls. For example the command "migrate" originally looks like:
{"name": "migrate", "ret-type": "17", "meta-type": "command",
"arg-type": "86"}
And it'll be changed into:
{"name": "migrate", "ret-type": "17", "allow-oob": false,
"meta-type": "command", "arg-type": "86"}
This patch only provides the QMP interface level changes. It does not contains the real out-of-band execution implementation yet.
--- include/qapi/qmp/dispatch.h | 1 + qapi/introspect.json | 6 +++++- scripts/qapi-commands.py | 19 ++++++++++++++----- scripts/qapi-introspect.py | 10 ++++++++-- scripts/qapi.py | 15 ++++++++++----- scripts/qapi2texi.py | 2 +- tests/qapi-schema/test-qapi.py | 2 +- 7 files changed, 40 insertions(+), 15 deletions(-)
include/qapi/qmp/dispatch.h +1 -0
@@ -23,6 +23,7 @@ typedef enum QmpCommandOptions {QCO_NO_OPTIONS =0x0 ,QCO_NO_SUCCESS_RESP =0x1 , +QCO_ALLOW_OOB =0x2 , } QmpCommandOptions; typedef struct QmpCommand
qapi/introspect.json +5 -1
@@ -259,12 +259,16 @@ # # @ret-type: the name of the command's result type. # +# @allow-oob: whether the command allows out-of-band execution. +# (Since: 2.12) +# # TODO: @success-response (currently irrelevant, because it's QGA, not QMP) # # Since: 2.5 ## { 'struct': 'SchemaInfoCommand', - 'data': { 'arg-type': 'str', 'ret-type': 'str' } } + 'data': { 'arg-type': 'str', 'ret-type': 'str', + 'allow-oob': 'bool' } } ## # @SchemaInfoEvent:
scripts/qapi-commands.py +14 -5
@@ -192,10 +192,18 @@ out: return ret -def gen_register_command(name, success_response): - options = 'QCO_NO_OPTIONS' +def gen_register_command(name, success_response, allow_oob): + options = [] + if not success_response: - options = 'QCO_NO_SUCCESS_RESP' + options += ['QCO_NO_SUCCESS_RESP'] + if allow_oob: + options += ['QCO_ALLOW_OOB'] + + if not options: + options = ['QCO_NO_OPTIONS'] + + options = " | ".join(options) ret = mcgen(''' qmp_register_command(cmds, "%(name)s",
@@ -241,7 +249,7 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor): self._visited_ret_types = None def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, allow_oob): if not gen: return self.decl += gen_command_decl(name, arg_type, boxed, ret_type)
@@ -250,7 +258,8 @@ class QAPISchemaGenCommandVisitor(QAPISchemaVisitor): self.defn += gen_marshal_output(ret_type) self.decl += gen_marshal_decl(name) self.defn += gen_marshal(name, arg_type, boxed, ret_type) - self._regy += gen_register_command(name, success_response) + self._regy += gen_register_command(name, success_response, + allow_oob) (input_file, output_dir, do_c, do_h, prefix, opts) = parse_command_line()
scripts/qapi-introspect.py +8 -2
@@ -28,6 +28,11 @@ def to_json(obj, level=0): to_json(obj[key], level + 1)) for key in sorted(obj.keys())] ret = '{' + ', '.join(elts) + '}' + elif isinstance(obj, bool): + if obj: + ret = 'true' + else: + ret = 'false' else: assert False # not implemented if level == 1:
@@ -154,12 +159,13 @@ const char %(c_name)s[] = %(c_string)s; for m in variants.variants]}) def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, allow_oob): arg_type = arg_type or self._schema.the_empty_object_type ret_type = ret_type or self._schema.the_empty_object_type self._gen_json(name, 'command', {'arg-type': self._use_type(arg_type), - 'ret-type': self._use_type(ret_type)}) + 'ret-type': self._use_type(ret_type), + 'allow-oob': allow_oob}) def visit_event(self, name, info, arg_type, boxed): arg_type = arg_type or self._schema.the_empty_object_type
scripts/qapi.py +10 -5
@@ -911,7 +911,8 @@ def check_exprs(exprs): elif 'command' in expr: meta = 'command' check_keys(expr_elem, 'command', [], - ['data', 'returns', 'gen', 'success-response', 'boxed']) + ['data', 'returns', 'gen', 'success-response', + 'boxed', 'allow-oob']) elif 'event' in expr: meta = 'event' check_keys(expr_elem, 'event', [], ['data', 'boxed'])
@@ -1022,7 +1023,7 @@ class QAPISchemaVisitor(object): pass def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, allow_oob): pass def visit_event(self, name, info, arg_type, boxed):
@@ -1389,7 +1390,7 @@ class QAPISchemaAlternateType(QAPISchemaType): class QAPISchemaCommand(QAPISchemaEntity): def __init__(self, name, info, doc, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, allow_oob): QAPISchemaEntity.__init__(self, name, info, doc) assert not arg_type or isinstance(arg_type, str) assert not ret_type or isinstance(ret_type, str)
@@ -1400,6 +1401,7 @@ class QAPISchemaCommand(QAPISchemaEntity): self.gen = gen self.success_response = success_response self.boxed = boxed + self.allow_oob = allow_oob def check(self, schema): if self._arg_type_name:
@@ -1423,7 +1425,8 @@ class QAPISchemaCommand(QAPISchemaEntity): def visit(self, visitor): visitor.visit_command(self.name, self.info, self.arg_type, self.ret_type, - self.gen, self.success_response, self.boxed) + self.gen, self.success_response, + self.boxed, self.allow_oob) class QAPISchemaEvent(QAPISchemaEntity):
@@ -1631,6 +1634,7 @@ class QAPISchema(object): gen = expr.get('gen', True) success_response = expr.get('success-response', True) boxed = expr.get('boxed', False) + allow_oob = expr.get('allow-oob', False) if isinstance(data, OrderedDict): data = self._make_implicit_object_type( name, info, doc, 'arg', self._make_members(data, info))
@@ -1638,7 +1642,8 @@ class QAPISchema(object): assert len(rets) == 1 rets = self._make_array_type(rets[0], info) self._def_entity(QAPISchemaCommand(name, info, doc, data, rets, - gen, success_response, boxed)) + gen, success_response, + boxed, allow_oob)) def _def_event(self, expr, info, doc): name = expr['event']
scripts/qapi2texi.py +1 -1
@@ -226,7 +226,7 @@ class QAPISchemaGenDocVisitor(qapi.QAPISchemaVisitor): body=texi_entity(doc, 'Members')) def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, allow_oob): doc = self.cur_doc if boxed: body = texi_body(doc)
tests/qapi-schema/test-qapi.py +1 -1
@@ -36,7 +36,7 @@ class QAPISchemaTestVisitor(QAPISchemaVisitor): self._print_variants(variants) def visit_command(self, name, info, arg_type, ret_type, - gen, success_response, boxed): + gen, success_response, boxed, allow_oob): print 'command %s %s -> %s' % \ (name, arg_type and arg_type.name, ret_type and ret_type.name) print ' gen=%s success_response=%s boxed=%s' % \
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 17/23] qapi: introduce new cmd option "allow-oob"
Quoted 42 lines · On Wed, 24 Jan 2018 13:39:51 +0800 Peter Xu <[email protected]> wrote:
Here "oob" stands for "Out-Of-Band". When "allow-oob" is set, it means the command allows out-of-band execution.
The "oob" idea is proposed by Markus Armbruster in following thread:
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg02057.html
This new "allow-oob" boolean will be exposed by "query-qmp-schema" as well for command entries, so that QMP clients can know which command can be used as out-of-band calls. For example the command "migrate" originally looks like:
{"name": "migrate", "ret-type": "17", "meta-type": "command",
"arg-type": "86"}And it'll be changed into:
{"name": "migrate", "ret-type": "17", "allow-oob": false,
"meta-type": "command", "arg-type": "86"}This patch only provides the QMP interface level changes. It does not contains the real out-of-band execution implementation yet.
Suggested-by Markus Armbruster <[email protected]>Reviewed-by Stefan Hajnoczi <[email protected]>Reviewed-by Fam Zheng <[email protected]>Signed-off-by Peter Xu <[email protected]>--- include/qapi/qmp/dispatch.h | 1 + qapi/introspect.json | 6 +++++- scripts/qapi-commands.py | 19 ++++++++++++++----- scripts/qapi-introspect.py | 10 ++++++++-- scripts/qapi.py | 15 ++++++++++----- scripts/qapi2texi.py | 2 +- tests/qapi-schema/test-qapi.py | 2 +- 7 files changed, 40 insertions(+), 15 deletions(-)include/qapi/qmp/dispatch.h +0
-0
include/qapi/qmp/dispatch.h typedef enum QmpCommandOptions · line 26
Quoted 4 lines
include/qapi/qmp/dispatch.h +1
-0@@ -23,6 +23,7 @@ typedef enum QmpCommandOptions {QCO_NO_OPTIONS =0x0 ,QCO_NO_SUCCESS_RESP =0x1 , + QCO_ALLOW_OOB = 0x2,
Options are essentially bitmask,
so using shift for value would be more explicit way to express it.
QCO_ALLOW_OOB = (1U << 1)
Quoted 3 lines
include/qapi/qmp/dispatch.h +0
-0} QmpCommandOptions; typedef struct QmpCommand
[...]
Re: [Qemu-devel] [PATCH v7 17/23] qapi: introduce new cmd option "allow-oob"
Quoted 52 lines · Igor Mammedov
Quoted 42 lines · On Wed, 24 Jan 2018 13:39:51 +0800 Peter Xu <[email protected]> wrote:
Here "oob" stands for "Out-Of-Band". When "allow-oob" is set, it means the command allows out-of-band execution.
The "oob" idea is proposed by Markus Armbruster in following thread:
https://lists.gnu.org/archive/html/qemu-devel/2017-09/msg02057.html
This new "allow-oob" boolean will be exposed by "query-qmp-schema" as well for command entries, so that QMP clients can know which command can be used as out-of-band calls. For example the command "migrate" originally looks like:
{"name": "migrate", "ret-type": "17", "meta-type": "command",
"arg-type": "86"}And it'll be changed into:
{"name": "migrate", "ret-type": "17", "allow-oob": false,
"meta-type": "command", "arg-type": "86"}This patch only provides the QMP interface level changes. It does not contains the real out-of-band execution implementation yet.
Suggested-by Markus Armbruster <[email protected]>Reviewed-by Stefan Hajnoczi <[email protected]>Reviewed-by Fam Zheng <[email protected]>Signed-off-by Peter Xu <[email protected]>--- include/qapi/qmp/dispatch.h | 1 + qapi/introspect.json | 6 +++++- scripts/qapi-commands.py | 19 ++++++++++++++----- scripts/qapi-introspect.py | 10 ++++++++-- scripts/qapi.py | 15 ++++++++++----- scripts/qapi2texi.py | 2 +- tests/qapi-schema/test-qapi.py | 2 +- 7 files changed, 40 insertions(+), 15 deletions(-)include/qapi/qmp/dispatch.h +0
-0include/qapi/qmp/dispatch.h typedef enum QmpCommandOptions · line 26
Quoted 4 lines
include/qapi/qmp/dispatch.h +1
-0@@ -23,6 +23,7 @@ typedef enum QmpCommandOptions {QCO_NO_OPTIONS =0x0 ,QCO_NO_SUCCESS_RESP =0x1 , + QCO_ALLOW_OOB = 0x2,Options are essentially bitmask,
so using shift for value would be more explicit way to express it.
Quoted 1 line
QCO_ALLOW_OOB= (1U << 1)
Yes, shifting would be more clear. Thanks,
Signature · 1 line
Peter Xu
Re: [Qemu-devel] [PATCH v7 17/23] qapi: introduce new cmd option "allow-oob"
In reply to Igor MammedovOn Thu, Mar 08, 2018 at 11:04:13AM +0100, Igor Mammedov wrote:
[...]
Quoted 12 lines
include/qapi/qmp/dispatch.h typedef enum QmpCommandOptions · line 26
Quoted 9 lines
include/qapi/qmp/dispatch.h +1
-0@@ -23,6 +23,7 @@ typedef enum QmpCommandOptions {QCO_NO_OPTIONS =0x0 ,QCO_NO_SUCCESS_RESP =0x1 , + QCO_ALLOW_OOB = 0x2,Options are essentially bitmask,
so using shift for value would be more explicit way to express it.
Quoted 1 line
QCO_ALLOW_OOB= (1U << 1)
I'll squash the change into the patch:
include/qapi/qmp/dispatch.h +3 -3
@@ -20,9 +20,9 @@ typedef void (QmpCommandFunc)(QDict *, QObject **, Error **);typedef enum QmpCommandOptions { -QCO_NO_OPTIONS =0x0 , -QCO_NO_SUCCESS_RESP =0x1 , -QCO_ALLOW_OOB =0x2 , +QCO_NO_OPTIONS =0x0 , +QCO_NO_SUCCESS_RESP = (1U <<0 ), +QCO_ALLOW_OOB = (1U <<1 ), }QmpCommandOptions ; typedef struct QmpCommand
Since the change is trivial, I'll keep r-bs for Stefan and Fam. Anyone who thinks I shouldn't please shoot. Thanks,
Signature · 1 line
Peter Xu
PATCH v7 18/23 qmp: support out-of-band (oob) execution
In reply to Peter XuHaving "allow-oob" to true for a command does not mean that this command will always be run in out-of-band mode. The out-of-band quick path will only be executed if we specify the extra "run-oob" flag when sending the QMP request:
{ "execute": "command-that-allows-oob",
"arguments": { ... },
"control": { "run-oob": true } }The "control" key is introduced to store this extra flag. "control" field is used to store arguments that are shared by all the commands, rather than command specific arguments. Let "run-oob" be the first.
Note that in the patch I exported qmp_dispatch_check_obj() to be used to check the request earlier, and at the same time allowed "id" field to be there since actually we always allow that.
--- include/qapi/qmp/dispatch.h | 2 ++ monitor.c | 84 ++++++++++++++++++++++++++++++++++++++++----- qapi/qmp-dispatch.c | 32 ++++++++++++++++- trace-events | 2 ++ 4 files changed, 110 insertions(+), 10 deletions(-)
include/qapi/qmp/dispatch.h +2 -0
@@ -49,6 +49,8 @@ bool qmp_command_is_enabled(const QmpCommand *cmd);const char *qmp_command_name (const QmpCommand *cmd);bool qmp_has_success_response (const QmpCommand *cmd);QObject *qmp_build_error_object (Error *err); +QDict *qmp_dispatch_check_obj (const QObject *request,Error **errp); +bool qmp_is_oob (QDict *dict);typedef void (*qmp_cmd_callback_fn )(QmpCommand *cmd,void *opaque);
monitor.c +75 -9
116 lines not on this page · raw message
qapi/qmp-dispatch.c +31 -1
@@ -20,7 +20,7 @@#include "qapi-types.h" #include "qapi/qmp/qerror.h" -static QDict *qmp_dispatch_check_obj (const QObject *request,Error **errp) +QDict *qmp_dispatch_check_obj (const QObject *request,Error **errp) {const QDictEntry *ent;const char *arg_name;
@@ -52,6 +52,14 @@ static QDict *qmp_dispatch_check_obj(const QObject *request, Error **errp) "QMP input member 'arguments' must be an object");return NULL ; } + }else if (!strcmp (arg_name,"id" )) { +continue ; + }else if (!strcmp (arg_name,"control" )) { +if (qobject_type (arg_obj) !=QTYPE_QDICT ) { +error_setg (errp, +"QMP input member 'control' must be a dict" ); +return NULL ; + } }else {error_setg (errp,"QMP input member '%s' is unexpected" , arg_name);
@@ -122,6 +130,28 @@ QObject *qmp_build_error_object(Error *err) error_get_pretty(err)); } +/* +* Detect whether a request should be run out-of-band, by quickly +* peeking at whether we have: { "control": { "run-oob": True } }. By +* default commands are run in-band. +*/ +bool qmp_is_oob (QDict *dict) +{ +QBool *bool_obj; + + dict =qdict_get_qdict (dict,"control" ); +if (!dict) { +return false; + } + + bool_obj =qobject_to_qbool (qdict_get (dict,"run-oob" )); +if (!bool_obj) { +return false; + } + +return qbool_get_bool (bool_obj); +} +QObject *qmp_dispatch (QmpCommandList *cmds,QObject *request) {Error *err =NULL ;
trace-events +2 -0
@@ -48,6 +48,8 @@ monitor_protocol_event_queue(uint32_t event, void *qdict, uint64_t rate) "event= handle_hmp_command(void *mon, const char *cmdline) "mon %p cmdline: %s" handle_qmp_command(void *mon, const char *req) "mon %p req: %s" monitor_suspend(void *ptr, int cnt) "mon %p: %d" +monitor_qmp_cmd_in_band(const char *id) "%s" +monitor_qmp_cmd_out_of_band(const char *id) "%s" # dma-helpers.c dma_blk_io(void *dbs, void *bs, int64_t offset, bool to_dev) "dbs=%p bs=%p offset=%" PRId64 " to_dev=%d"
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 18/23] qmp: support out-of-band (oob) execution
Quoted 17 lines · Peter Xu
Having "allow-oob" to true for a command does not mean that this command will always be run in out-of-band mode. The out-of-band quick path will only be executed if we specify the extra "run-oob" flag when sending the QMP request:
{ "execute": "command-that-allows-oob", "arguments": { ... }, "control": { "run-oob": true } }The "control" key is introduced to store this extra flag. "control" field is used to store arguments that are shared by all the commands, rather than command specific arguments. Let "run-oob" be the first.
Note that in the patch I exported
qmp_dispatch_check_obj()to be used to check the request earlier, and at the same time allowed "id" field to be there since actually we always allow that.
Quoted 7 lines
Signed-off-by Peter Xu <[email protected]>--- include/qapi/qmp/dispatch.h | 2 ++ monitor.c | 84 ++++++++++++++++++++++++++++++++++++++++----- qapi/qmp-dispatch.c | 32 ++++++++++++++++- trace-events | 2 ++ 4 files changed, 110 insertions(+), 10 deletions(-)
PATCH v7 19/23 qmp: isolate responses into io thread
In reply to Peter XuFor those monitors who have enabled IO thread, we'll offload the responding procedure into IO thread. The main reason is that chardev is not thread safe, and we need to do all the read/write IOs in the same thread. For use_io_thr=true monitors, that thread is the IO thread.
We do this isolation in similar pattern as what we have done to the request queue: we first create one response queue for each monitor, then instead of replying directly in the main thread, we queue the responses and kick the IO thread to do the rest of the job for us.
A funny thing after doing this is that, when the QMP clients send "quit" to QEMU, it's possible that we close the IOThread even earlier than replying to that "quit". So another thing we need to do before cleaning up the monitors is that we need to flush the response queue (we don't need to do that for command queue; after all we are quitting) to make sure replies for handled commands are always flushed back to clients.
--- monitor.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-)
monitor.c +95 -1
@@ -175,6 +175,8 @@ typedef struct {QemuMutex qmp_queue_lock;/* Input queue that holds all the parsed QMP requests */ GQueue *qmp_requests; +/* Output queue contains all the QMP responses in order */ +GQueue *qmp_responses; } MonitorQMP;/*
@@ -201,6 +203,7 @@ struct Monitor {bool skip_flush;bool use_io_thr; +/* We can't access guest memory when holding the lock */ QemuMutex out_lock;QString *outbuf;guint out_watch;
@@ -223,6 +226,8 @@ static struct {IOThread *mon_iothread;/* Bottom half to dispatch the requests received from IO thread */ QEMUBH *qmp_dispatcher_bh; +/* Bottom half to deliver the responses back to clients */ +QEMUBH *qmp_respond_bh; } mon_global;/* QMP checker flags */
@@ -412,7 +417,8 @@ int monitor_fprintf(FILE *stream, const char *fmt, ...)return 0 ; } -static void monitor_json_emitter (Monitor *mon,const QObject *data) +static void monitor_json_emitter_raw (Monitor *mon, +QObject *data) {QString *json;
@@ -426,6 +432,71 @@ static void monitor_json_emitter(Monitor *mon, const QObject *data)QDECREF (json); } +static void monitor_json_emitter (Monitor *mon,QObject *data) +{ +if (mon->use_io_thr) { +/* +* If using IO thread, we need to queue the item so that IO +* thread will do the rest for us. Take refcount so that +* caller won't free the data (which will be finally freed in +* responder thread). +*/ +qobject_incref (data); +qemu_mutex_lock (&mon->qmp.qmp_queue_lock); +g_queue_push_tail (mon->qmp.qmp_responses, (void *)data); +qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +qemu_bh_schedule (mon_global.qmp_respond_bh); + }else { +/* +* If not using monitor IO thread, then we are in main thread. +* Do the emission right away. +*/ +monitor_json_emitter_raw (mon, data); + } +} + +struct QMPResponse { +Monitor *mon; +QObject *data; +}; +typedef struct QMPResponse QMPResponse ; + +/* +* Return one QMPResponse. The response is only valid if +* response.data is not NULL. +*/ +static QMPResponse monitor_qmp_response_pop_one (void ) +{ +Monitor *mon; +QObject *data =NULL ; + +qemu_mutex_lock (&monitor_lock); + QTAILQ_FOREACH(mon, &mon_list, entry) { +qemu_mutex_lock (&mon->qmp.qmp_queue_lock); + data =g_queue_pop_head (mon->qmp.qmp_responses); +qemu_mutex_unlock (&mon->qmp.qmp_queue_lock); +if (data) { +break ; + } + } +qemu_mutex_unlock (&monitor_lock); +return (QMPResponse ) { .mon = mon, .data = data }; +} + +static void monitor_qmp_bh_responder (void *opaque) +{ +QMPResponse response; + +while (true) { + response =monitor_qmp_response_pop_one (); +if (!response.data) { +break ; + } +monitor_json_emitter_raw (response.mon, response.data); +qobject_decref (response.data); + } +} +static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX ] = {/* Limit guest-triggerable events to 1 per second */ [QAPI_EVENT_RTC_CHANGE ] = {1000 *SCALE_MS },
@@ -612,6 +683,7 @@ static void monitor_data_init(Monitor *mon, bool skip_flush, mon->skip_flush = skip_flush; mon->use_io_thr = use_io_thr; mon->qmp.qmp_requests =g_queue_new (); + mon->qmp.qmp_responses =g_queue_new (); }static void monitor_data_destroy (Monitor *mon)
@@ -626,6 +698,7 @@ static void monitor_data_destroy(Monitor *mon)qemu_mutex_destroy (&mon->out_lock);qemu_mutex_destroy (&mon->qmp.qmp_queue_lock);g_queue_free (mon->qmp.qmp_requests); +g_queue_free (mon->qmp.qmp_responses); } char *qmp_human_monitor_command(const char *command_line, bool has_cpu_index,
@@ -4413,6 +4486,15 @@ static void monitor_iothread_init(void) mon_global.qmp_dispatcher_bh =aio_bh_new (qemu_get_aio_context (), monitor_qmp_bh_dispatcher,NULL ); + +/* +* Unlike the dispatcher BH, this must be run on the monitor IO +* thread, so that monitors that are using IO thread will make +* sure read/write operations are all done on the IO thread. +*/ + mon_global.qmp_respond_bh =aio_bh_new (monitor_aio_context_get (), + monitor_qmp_bh_responder, +NULL ); }void monitor_init_globals (void )
@@ -4560,9 +4642,19 @@ void monitor_cleanup(void)*/ iothread_stop (mon_global.mon_iothread); +/* +* After we have IOThread to send responses, it's possible that +* when we stop the IOThread there are still replies queued in the +* responder queue. Flush all of them. Note that even after this +* flush it's still possible that out buffer is not flushed. +* It'll be done in below monitor_flush() as the last resort. +*/ +monitor_qmp_bh_responder (NULL ); +qemu_mutex_lock (&monitor_lock); QTAILQ_FOREACH_SAFE(mon, &mon_list, entry, next) {QTAILQ_REMOVE (&mon_list, mon, entry); +monitor_flush (mon);monitor_data_destroy (mon);g_free (mon); }
@@ -4571,6 +4663,8 @@ void monitor_cleanup(void)/* QEMUBHs needs to be deleted before destroying the IOThread. */ qemu_bh_delete (mon_global.qmp_dispatcher_bh); mon_global.qmp_dispatcher_bh =NULL ; +qemu_bh_delete (mon_global.qmp_respond_bh); + mon_global.qmp_respond_bh =NULL ;iothread_destroy (mon_global.mon_iothread); mon_global.mon_iothread =NULL ;
Signature · 1 line
2.14.3
Re: [Qemu-devel] [PATCH v7 19/23] qmp: isolate responses into io thread
Quoted 17 lines · Peter Xu
For those monitors who have enabled IO thread, we'll offload the responding procedure into IO thread. The main reason is that chardev is not thread safe, and we need to do all the read/write IOs in the same thread. For use_io_thr=true monitors, that thread is the IO thread.
We do this isolation in similar pattern as what we have done to the request queue: we first create one response queue for each monitor, then instead of replying directly in the main thread, we queue the responses and kick the IO thread to do the rest of the job for us.
A funny thing after doing this is that, when the QMP clients send "quit" to QEMU, it's possible that we close the IOThread even earlier than replying to that "quit". So another thing we need to do before cleaning up the monitors is that we need to flush the response queue (we don't need to do that for command queue; after all we are quitting) to make sure replies for handled commands are always flushed back to clients.
Quoted 5 lines
Reviewed-by Fam Zheng <[email protected]>Signed-off-by Peter Xu <[email protected]>--- monitor.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-)
PATCH v7 20/23 monitor: enable IO thread for (qmp & !mux) typed
In reply to Peter XuStart to use dedicate IO thread for QMP monitors that are not using MUXed chardev.
--- monitor.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
monitor.c +4 -1
@@ -36,6 +36,7 @@#include "net/slirp.h" #include "chardev/char-fe.h" #include "chardev/char-io.h" +#include "chardev/char-mux.h" #include "ui/qemu-spice.h" #include "sysemu/numa.h" #include "monitor/monitor.h"
@@ -4579,8 +4580,10 @@ static void monitor_qmp_setup_handlers(void *data)void monitor_init (Chardev *chr,int flags) {Monitor *mon =g_malloc (sizeof (*mon)); +/* Enable IOThread for QMPs that are not using MUX chardev backends. */ +bool use_io_thr = (!CHARDEV_IS_MUX (chr)) && (flags &MONITOR_USE_CONTROL ); -monitor_data_init (mon, false, false); +monitor_data_init (mon, false, use_io_thr);qemu_chr_fe_init (&mon->chr, chr, &error_abort); mon->flags = flags;
Signature · 1 line
2.14.3
PATCH v7 21/23 qmp: add command "x-oob-test"
In reply to Peter XuThis command is only used to test OOB functionality. It should not be used for any other purposes.
--- qapi-schema.json | 18 ++++++++++++++++++ qmp.c | 16 ++++++++++++++++ 2 files changed, 34 insertions(+)
qapi-schema.json +18 -0
@@ -3252,3 +3252,21 @@ ## { 'event': 'COMMAND_DROPPED' , 'data': { 'id': 'any', 'reason': 'CommandDropReason' } } + +## +# @x-oob-test: +# +# Test OOB functionality. When send this command with lock=true, +# it'll try to hang the dispatcher. When send it with lock=false, +# it'll try to notify the locked thread to continue. Note: it should +# only be used by QMP test program rather than anything else. +# +# Since: 2.12 +# +# Example: +# +# { "execute": "x-oob-test", +# "arguments": { "lock": true } } +## +{ 'command': 'x-oob-test', 'data' : { 'lock': 'bool' }, + 'allow-oob': true }
qmp.c +16 -0
@@ -717,3 +717,19 @@ MemoryInfo *qmp_query_memory_size_summary(Error **errp)return mem_info; } + +static QemuSemaphore x_oob_test_sem; + +static void __attribute__((constructor))x_oob_test_init (void ) +{ +qemu_sem_init (&x_oob_test_sem,0 ); +} + +void qmp_x_oob_test (bool lock,Error **errp) +{ +if (lock) { +qemu_sem_wait (&x_oob_test_sem); + }else { +qemu_sem_post (&x_oob_test_sem); + } +}
Signature · 1 line
2.14.3
PATCH v7 22/23 tests: qmp-test: verify command batching
In reply to Peter XuOOB introduced DROP event for flow control. This should not affect old QMP clients. Add a command batching check to make sure of it.
--- tests/qmp-test.c | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+)
tests/qmp-test.c +22 -0
@@ -78,6 +78,7 @@ static void test_qmp_protocol(void)QList *capabilities;const QListEntry *entry;QString *qstr; +int i; global_qtest =qtest_init_without_qmp_handshake (common_args);
@@ -135,6 +136,27 @@ static void test_qmp_protocol(void) g_assert_cmpint(qdict_get_int(resp, "id"), ==, 2);QDECREF (resp); +/* +* Test command batching. In current test OOB is not enabled, we +* should be able to run as many commands in batch as we like. +* Using 16 (>8, which is OOB queue length) to make sure OOB won't +* break existing clients. Note: this test does not control the +* scheduling of QEMU's QMP command processing threads so it may +* not really trigger batching inside QEMU. This is just a +* best-effort test. +*/ +for (i =0 ; i <16 ; i++) { +qmp_async ("{ 'execute': 'query-version' }" ); + } +/* Verify the replies to make sure no command is dropped. */ +for (i =0 ; i <16 ; i++) { + resp =qmp_receive (); +/* It should never be dropped. Each of them should be a reply. */ +g_assert (qdict_haskey (resp,"return" )); +g_assert (!qdict_haskey (resp,"event" )); +QDECREF (resp); + } +qtest_end (); }
Signature · 1 line
2.14.3
PATCH v7 23/23 tests: qmp-test: add oob test
In reply to Peter XuTest the new OOB capability. Here we used the new "x-oob-test" command. Firstly, we send a lock=true and oob=false command to hang the main thread. Then send another lock=false and oob=true command (which will be run inside parser this time) to free that hanged command.
--- tests/qmp-test.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 65 insertions(+)
tests/qmp-test.c +65 -0
@@ -160,6 +160,70 @@ static void test_qmp_protocol(void)qtest_end (); } +/* Tests for Out-Of-Band support. */ +static void test_qmp_oob (void ) +{ +QDict *resp; +int acks =0 ; +const char *cmd_id; + + global_qtest =qtest_init_without_qmp_handshake (common_args); + +/* Ignore the greeting message. */ + resp =qmp_receive (); +g_assert (qdict_get_qdict (resp,"QMP" )); +QDECREF (resp); + +/* Try a fake capability, it should fail. */ + resp =qmp ("{ 'execute': 'qmp_capabilities', " +" 'arguments': { 'enable': [ 'cap-does-not-exist' ] } }" ); +g_assert (qdict_haskey (resp,"error" )); +QDECREF (resp); + +/* Now, enable OOB in current QMP session, it should success. */ + resp =qmp ("{ 'execute': 'qmp_capabilities', " +" 'arguments': { 'enable': [ 'oob' ] } }" ); +g_assert (qdict_haskey (resp,"return" )); +QDECREF (resp); + +/* +* Try any command that does not support OOB but with OOB flag. We +* should get failure. +*/ + resp =qmp ("{ 'execute': 'query-cpus'," +" 'control': { 'run-oob': true } }" ); +g_assert (qdict_haskey (resp,"error" )); +QDECREF (resp); + +/* +* Firstly send the "x-oob-test" command with lock=true and +* oob=false, it should hang the dispatcher and main thread; +* later, we send another lock=false with oob=true to continue +* that thread processing. Finally we should receive replies from +* both commands. +*/ +qmp_async ("{ 'execute': 'x-oob-test'," +" 'arguments': { 'lock': true }, " +" 'id': 'lock-cmd'}" ); +qmp_async ("{ 'execute': 'x-oob-test', " +" 'arguments': { 'lock': false }, " +" 'control': { 'run-oob': true }, " +" 'id': 'unlock-cmd' }" ); + +/* Ignore all events. Wait for 2 acks */ +while (acks <2 ) { + resp =qmp_receive (); + cmd_id =qdict_get_str (resp,"id" ); +if (!g_strcmp0 (cmd_id,"lock-cmd" ) || + !g_strcmp0 (cmd_id,"unlock-cmd" )) { + acks++; + } +QDECREF (resp); + } + +qtest_end (); +} +static int query_error_class (const char *cmd) { static struct {
@@ -339,6 +403,7 @@ int main(int argc, char *argv[])g_test_init (&argc, &argv,NULL );qtest_add_func ("qmp/protocol" , test_qmp_protocol); +qtest_add_func ("qmp/oob" , test_qmp_oob);qmp_schema_init (&schema);add_query_tests (&schema);
Signature · 1 line
2.14.3
Quoted 23 lines · Peter Xu
This version should have addressed all comments in previous one, also fixed another race condition after I addressed all the comments (a new race condition introduced by addressing the comments...). For some more details of the race condition, please see the last entry of change log, and please refer to patch 9 for the code change.
I removed RFC tag from this version. Please review. Thanks.
v7:
- add some r-bs, and remove some.
- remove the chardev fix since already queued by Paolo
- use local var inqemu_chr_fe_add_watch[Stefan]
- move doc patch to front, mention it in some patches [Eric]
- Quite a few of English fixes [Eric]
- fix unlock missing inhandle_qmp_command[Stefan]
- squash some patches according to the review comments
- don't break gdbserver usage on HMP non-interactive mode by fixing up the suspend/resume logic [Fam, Stefan]
- move theqemu_chr_fe_set_handlers()call inmonitor_init()into a bottom half to avoid race between the call itself and iothread. [Stefan]
- spent quite a lot of time debugging another assertion failure inio_watch_poll_finalize()after above change is made (ouch! I really
Quoted 8 lines
hoped we always have the latest glib): when QEMU inits chardevs in
chardev_init_func()it's possible that QEMU registers the chardev handlers there, even before CharBackend is connected to that chardev inmonitor_init(). Then, when we reachmonitor_init()we must make sure we unregister that old one first, or there can have one orphan GSource still in default gcontext (note that this can really happen when we start to use QEMUBH to setup chardev frontends, which is above change).
Online repository updated for v7:
https://github.com/xzpeter/qemu/tree/monitor-oob
Thanks,
Signature · 1 line
Peter Xu
Quoted 33 lines · * Peter Xu ([email protected]) wrote:
Quoted 23 lines · Peter Xu
This version should have addressed all comments in previous one, also fixed another race condition after I addressed all the comments (a new race condition introduced by addressing the comments...). For some more details of the race condition, please see the last entry of change log, and please refer to patch 9 for the code change.
I removed RFC tag from this version. Please review. Thanks.
v7:
- add some r-bs, and remove some.
- remove the chardev fix since already queued by Paolo
- use local var inqemu_chr_fe_add_watch[Stefan]
- move doc patch to front, mention it in some patches [Eric]
- Quite a few of English fixes [Eric]
- fix unlock missing inhandle_qmp_command[Stefan]
- squash some patches according to the review comments
- don't break gdbserver usage on HMP non-interactive mode by fixing up the suspend/resume logic [Fam, Stefan]
- move theqemu_chr_fe_set_handlers()call inmonitor_init()into a bottom half to avoid race between the call itself and iothread. [Stefan]
- spent quite a lot of time debugging another assertion failure inio_watch_poll_finalize()after above change is made (ouch! I reallyQuoted 8 lines
hoped we always have the latest glib): when QEMU inits chardevs in
chardev_init_func()it's possible that QEMU registers the chardev handlers there, even before CharBackend is connected to that chardev inmonitor_init(). Then, when we reachmonitor_init()we must make sure we unregister that old one first, or there can have one orphan GSource still in default gcontext (note that this can really happen when we start to use QEMUBH to setup chardev frontends, which is above change).
Quoted 3 lines
Online repository updated for v7:
This version seems to have fixed the problem I had where after an error I couldn't reconnect (the one I mentioned on the 12th).
Dave
Quoted 4 lines
Thanks,
Signature · 1 line
Peter Xu
--
Dr. David Alan Gilbert / [email protected] / Manchester, UK
Quoted 7 lines · Peter Xu
This version should have addressed all comments in previous one, also fixed another race condition after I addressed all the comments (a new race condition introduced by addressing the comments...). For some more details of the race condition, please see the last entry of change log, and please refer to patch 9 for the code change.
I removed RFC tag from this version. Please review. Thanks.
It's still a month there until the soft freeze of 2.12, but I would still like to have a gentle ping for this series in case there will be further surprises. Hopefully, this series can catch the train for 2.12.
I'll respin the postcopy recovery series very soon. Thanks,
Signature · 1 line
Peter Xu
Quoted 7 lines · Peter Xu
This version should have addressed all comments in previous one, also fixed another race condition after I addressed all the comments (a new race condition introduced by addressing the comments...). For some more details of the race condition, please see the last entry of change log, and please refer to patch 9 for the code change.
I removed RFC tag from this version. Please review. Thanks.
I have posted comments on a few patches.
Stefan