Patchwork [11/12,v2] qmp: add cpu-set qmp command

login
register
mail settings
Submitter Igor Mammedov
Date March 25, 2013, 3:35 p.m.
Message ID <1364225711-32566-1-git-send-email-imammedo@redhat.com>
Download mbox | patch
Permalink /patch/230748/
State New
Headers show

Comments

Igor Mammedov - March 25, 2013, 3:35 p.m.
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
v2:
  * s/cpu_set/cpu-set/
  * qmp doc style fix
  * use bool type instead of opencodding online/offline string
     suggested-by: Eric Blake <eblake@redhat.com>
  changes are on WIP branch: https://github.com/imammedo/qemu/tree/cpu_set.WIP

---
 include/sysemu/sysemu.h |    2 ++
 qapi-schema.json        |   12 ++++++++++++
 qmp-commands.hx         |   24 ++++++++++++++++++++++++
 qmp.c                   |    9 +++++++++
 stubs/Makefile.objs     |    1 +
 stubs/do_cpu_hot_add.c  |    7 +++++++
 6 files changed, 55 insertions(+), 0 deletions(-)
 create mode 100644 stubs/do_cpu_hot_add.c
Luiz Capitulino - March 25, 2013, 8:09 p.m.
On Mon, 25 Mar 2013 16:35:11 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> v2:
>   * s/cpu_set/cpu-set/
>   * qmp doc style fix
>   * use bool type instead of opencodding online/offline string
>      suggested-by: Eric Blake <eblake@redhat.com>
>   changes are on WIP branch: https://github.com/imammedo/qemu/tree/cpu_set.WIP
> 
> ---
>  include/sysemu/sysemu.h |    2 ++
>  qapi-schema.json        |   12 ++++++++++++
>  qmp-commands.hx         |   24 ++++++++++++++++++++++++
>  qmp.c                   |    9 +++++++++
>  stubs/Makefile.objs     |    1 +
>  stubs/do_cpu_hot_add.c  |    7 +++++++
>  6 files changed, 55 insertions(+), 0 deletions(-)
>  create mode 100644 stubs/do_cpu_hot_add.c
> 
> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 4b8f721..8bcaf26 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -156,6 +156,8 @@ void drive_hot_add(Monitor *mon, const QDict *qdict);
>  void qemu_register_cpu_add_notifier(Notifier *notifier);
>  void qemu_system_cpu_hotplug_request(uint32_t id);
>  
> +void do_cpu_hot_add(const int64_t id, Error **errp);
> +
>  /* pcie aer error injection */
>  void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
>  int do_pcie_aer_inject_error(Monitor *mon,
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 088f4e1..aa5f3dc 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -1385,6 +1385,18 @@
>  { 'command': 'cpu', 'data': {'index': 'int'} }
>  
>  ##
> +# @cpu-set
> +#
> +# Sets specified cpu to online/offline mode
> +#
> +# @id: cpu id to be updated
> +#
> +# @online: true to put the cpu online, false to take it offline
> +#
> +##
> +{ 'command': 'cpu-set', 'data': {'id': 'int', 'online': 'bool'} }
> +
> +##
>  # @memsave:
>  #
>  # Save a portion of guest memory to a file.
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index b370060..2f9c256 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -385,6 +385,30 @@ Note: CPUs' indexes are obtained with the 'query-cpus' command.
>  EQMP
>  
>      {
> +        .name       = "cpu-set",
> +        .args_type  = "id:i,online:b",
> +        .mhandler.cmd_new = qmp_marshal_input_cpu_set,
> +    },
> +
> +SQMP
> +cpu-set
> +-------
> +
> +Sets virtual cpu to online/ofline mode
> +
> +Arguments:
> +
> +- "id": cpu id (json-int)
> +- "online": true to put the cpu online, false to take it offline (json-bool)
> +
> +Example:
> +
> +-> { "execute": "cpu-set", "arguments": { "id": 2, "online": true } }
> +<- { "return": {} }
> +
> +EQMP
> +
> +    {
>          .name       = "memsave",
>          .args_type  = "val:l,size:i,filename:s,cpu:i?",
>          .mhandler.cmd_new = qmp_marshal_input_memsave,
> diff --git a/qmp.c b/qmp.c
> index 55b056b..c211da5 100644
> --- a/qmp.c
> +++ b/qmp.c
> @@ -108,6 +108,15 @@ void qmp_cpu(int64_t index, Error **errp)
>      /* Just do nothing */
>  }
>  
> +void qmp_cpu_set(int64_t id, const bool online, Error **errp)
> +{
> +    if (online) {
> +        do_cpu_hot_add(id, errp);
> +    } else {
> +        error_setg(errp, "Unplug is not implemented");
> +    }
> +}

As a general rule, we don't allow command extensions to be done this
way because this is not queriable. A client would have to try online=off
to see if QEMU version X supports it, worse: the client would have to
parse the error message to be sure the failure actually corresponds
to unplug not implemented.

The alternative is to have cpu-set-online and later cpu-set-offline. Quite
verbose, but doesn't have issues.

Eric, what do you think?

> +
>  #ifndef CONFIG_VNC
>  /* If VNC support is enabled, the "true" query-vnc command is
>     defined in the VNC subsystem */
> diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
> index 6a492f5..4154a2b 100644
> --- a/stubs/Makefile.objs
> +++ b/stubs/Makefile.objs
> @@ -26,3 +26,4 @@ stub-obj-$(CONFIG_WIN32) += fd-register.o
>  stub-obj-y += resume_vcpu.o
>  stub-obj-y += get_icc_bus.o
>  stub-obj-y += qemu_system_cpu_hotplug_request.o
> +stub-obj-y += do_cpu_hot_add.o
> diff --git a/stubs/do_cpu_hot_add.c b/stubs/do_cpu_hot_add.c
> new file mode 100644
> index 0000000..1f6d7a6
> --- /dev/null
> +++ b/stubs/do_cpu_hot_add.c
> @@ -0,0 +1,7 @@
> +#include "qapi/error.h"
> +#include "sysemu/sysemu.h"
> +
> +void do_cpu_hot_add(const int64_t id, Error **errp)
> +{
> +    error_setg(errp, "Not implemented");
> +}
Eric Blake - March 25, 2013, 8:22 p.m.
On 03/25/2013 02:09 PM, Luiz Capitulino wrote:
> On Mon, 25 Mar 2013 16:35:11 +0100
> Igor Mammedov <imammedo@redhat.com> wrote:
> 

>> +void qmp_cpu_set(int64_t id, const bool online, Error **errp)
>> +{
>> +    if (online) {
>> +        do_cpu_hot_add(id, errp);
>> +    } else {
>> +        error_setg(errp, "Unplug is not implemented");
>> +    }
>> +}
> 
> As a general rule, we don't allow command extensions to be done this
> way because this is not queriable. A client would have to try online=off
> to see if QEMU version X supports it, worse: the client would have to
> parse the error message to be sure the failure actually corresponds
> to unplug not implemented.
> 
> The alternative is to have cpu-set-online and later cpu-set-offline. Quite
> verbose, but doesn't have issues.
> 
> Eric, what do you think?

Good point.  What is the likelihood of getting offline working before
1.5 is released?  If we are certain that offline cpu support won't make
this release, then having separate commands would indeed be easier to query.

On the other hand, why aren't we mirroring the QMP behavior to be
similar to what Lazlo already did for qga:
https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg01031.html

That is, by having a way to query details on the set of all possible
cpus (via a new command that returns an array with max cpus elements,
rather than just the single int of query-cpu-max in
https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg04441.html),
with information in that query including a second boolean stating
whether that cpu can be taken offline, would be sufficiently queryable.
 The initial implementation would then state that an offline cpu can be
taken online, but that an online cpu must remain in that state.

And while typing that, I also realize that I like Lazlo's approach for
another reason - guest-set-vcpus takes an array of actions, and performs
as many as possible in one transaction; whereas your current cpu-set
command has to be called multiple times to take multiple cpus online.
Igor Mammedov - March 26, 2013, 1:43 p.m.
On Mon, 25 Mar 2013 14:22:36 -0600
Eric Blake <eblake@redhat.com> wrote:

> On 03/25/2013 02:09 PM, Luiz Capitulino wrote:
> > On Mon, 25 Mar 2013 16:35:11 +0100
> > Igor Mammedov <imammedo@redhat.com> wrote:
> > 
> 
> >> +void qmp_cpu_set(int64_t id, const bool online, Error **errp)
> >> +{
> >> +    if (online) {
> >> +        do_cpu_hot_add(id, errp);
> >> +    } else {
> >> +        error_setg(errp, "Unplug is not implemented");
> >> +    }
> >> +}
> > 
> > As a general rule, we don't allow command extensions to be done this
> > way because this is not queriable. A client would have to try online=off
> > to see if QEMU version X supports it, worse: the client would have to
> > parse the error message to be sure the failure actually corresponds
> > to unplug not implemented.
> > 
> > The alternative is to have cpu-set-online and later cpu-set-offline. Quite
> > verbose, but doesn't have issues.
It looks like better as way to go. Later on we could keep them for
compatibility sake and map it to device_add/device_del commands when they are
ready.

> > 
> > Eric, what do you think?
> 
> Good point.  What is the likelihood of getting offline working before
> 1.5 is released?  If we are certain that offline cpu support won't make
> this release, then having separate commands would indeed be easier to query.
It's not likely to be ready for 1.5, since "offline" depends on code
introduced in this series and lacks VCPU hot-remove on KVM side. Some time
ago there were patches on list to introduce VCPU hot-remove in KVM, but it
didn't go well I think due to lack of VCPU hot-add and common infrastructure
it introduces.

> 
> On the other hand, why aren't we mirroring the QMP behavior to be
> similar to what Lazlo already did for qga:
> https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg01031.html
Looks like terms online/offline are confusing, it would be better if
"cpu-set id=xx online=xxx" is replaced with cpu_add/cpu_del commands
following pattern of drive_add/del, pci_add/del ...

> That is, by having a way to query details on the set of all possible
> cpus (via a new command that returns an array with max cpus elements,
> rather than just the single int of query-cpu-max in
> https://lists.gnu.org/archive/html/qemu-devel/2013-03/msg04441.html),
> with information in that query including a second boolean stating
> whether that cpu can be taken offline, would be sufficiently queryable.
>  The initial implementation would then state that an offline cpu can be
> taken online, but that an online cpu must remain in that state.
All of it wouldn't be necessary if we have only cpu_add without cpu_del
implemented.

As for querying present VCPUs, one could use qom to enumerate
/machine/unattached/icc-bus childs, looking for childs with type == *-cpu
It's possible to pre-allocate empty shortcut links like /machine/cpu[id] for
all VCPUs and populate corresponding links in x86_cpu_realizefn() when it
is going complete successfully. But query-ability of all possible CPUs could
be a bit out of scope of this series and requires changes to qdev, so I've
left it out for another time.

> And while typing that, I also realize that I like Lazlo's approach for
> another reason - guest-set-vcpus takes an array of actions, and performs
> as many as possible in one transaction; whereas your current cpu-set
> command has to be called multiple times to take multiple cpus online.
Yep, it has to be called for each CPU since it provides low level API at
QEMU level, allowing management to implement higher level ops like
transactions and all the logic associated with it (it is like device_add/del
commands towards which CPUs are moving slowly).

Considering all said above, would it be acceptable to replace cpu-set with
"cpu_add id=xx" command?
Luiz Capitulino - March 26, 2013, 2:02 p.m.
On Tue, 26 Mar 2013 14:43:01 +0100
Igor Mammedov <imammedo@redhat.com> wrote:

> On Mon, 25 Mar 2013 14:22:36 -0600
> Eric Blake <eblake@redhat.com> wrote:
> 
> > On 03/25/2013 02:09 PM, Luiz Capitulino wrote:
> > > On Mon, 25 Mar 2013 16:35:11 +0100
> > > Igor Mammedov <imammedo@redhat.com> wrote:
> > > 
> > 
> > >> +void qmp_cpu_set(int64_t id, const bool online, Error **errp)
> > >> +{
> > >> +    if (online) {
> > >> +        do_cpu_hot_add(id, errp);
> > >> +    } else {
> > >> +        error_setg(errp, "Unplug is not implemented");
> > >> +    }
> > >> +}
> > > 
> > > As a general rule, we don't allow command extensions to be done this
> > > way because this is not queriable. A client would have to try online=off
> > > to see if QEMU version X supports it, worse: the client would have to
> > > parse the error message to be sure the failure actually corresponds
> > > to unplug not implemented.
> > > 
> > > The alternative is to have cpu-set-online and later cpu-set-offline. Quite
> > > verbose, but doesn't have issues.
> It looks like better as way to go. Later on we could keep them for
> compatibility sake and map it to device_add/device_del commands when they are
> ready.

I find the batch API to be a bit overkill (I'd add it in the future if really
needed), but I won't oppose to it.
Eric Blake - March 26, 2013, 2:38 p.m.
On 03/26/2013 07:43 AM, Igor Mammedov wrote:

>>> The alternative is to have cpu-set-online and later cpu-set-offline. Quite
>>> verbose, but doesn't have issues.
> It looks like better as way to go. Later on we could keep them for
> compatibility sake and map it to device_add/device_del commands when they are
> ready.
> 

> Considering all said above, would it be acceptable to replace cpu-set with
> "cpu_add id=xx" command?

cpu_add id=xx from HMP, cpu-add from QMP is fine for getting things
started; it still leaves room for a future 'cpu-set' batching command if
batching turns out to be useful.

Patch

diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 4b8f721..8bcaf26 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -156,6 +156,8 @@  void drive_hot_add(Monitor *mon, const QDict *qdict);
 void qemu_register_cpu_add_notifier(Notifier *notifier);
 void qemu_system_cpu_hotplug_request(uint32_t id);
 
+void do_cpu_hot_add(const int64_t id, Error **errp);
+
 /* pcie aer error injection */
 void pcie_aer_inject_error_print(Monitor *mon, const QObject *data);
 int do_pcie_aer_inject_error(Monitor *mon,
diff --git a/qapi-schema.json b/qapi-schema.json
index 088f4e1..aa5f3dc 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -1385,6 +1385,18 @@ 
 { 'command': 'cpu', 'data': {'index': 'int'} }
 
 ##
+# @cpu-set
+#
+# Sets specified cpu to online/offline mode
+#
+# @id: cpu id to be updated
+#
+# @online: true to put the cpu online, false to take it offline
+#
+##
+{ 'command': 'cpu-set', 'data': {'id': 'int', 'online': 'bool'} }
+
+##
 # @memsave:
 #
 # Save a portion of guest memory to a file.
diff --git a/qmp-commands.hx b/qmp-commands.hx
index b370060..2f9c256 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -385,6 +385,30 @@  Note: CPUs' indexes are obtained with the 'query-cpus' command.
 EQMP
 
     {
+        .name       = "cpu-set",
+        .args_type  = "id:i,online:b",
+        .mhandler.cmd_new = qmp_marshal_input_cpu_set,
+    },
+
+SQMP
+cpu-set
+-------
+
+Sets virtual cpu to online/ofline mode
+
+Arguments:
+
+- "id": cpu id (json-int)
+- "online": true to put the cpu online, false to take it offline (json-bool)
+
+Example:
+
+-> { "execute": "cpu-set", "arguments": { "id": 2, "online": true } }
+<- { "return": {} }
+
+EQMP
+
+    {
         .name       = "memsave",
         .args_type  = "val:l,size:i,filename:s,cpu:i?",
         .mhandler.cmd_new = qmp_marshal_input_memsave,
diff --git a/qmp.c b/qmp.c
index 55b056b..c211da5 100644
--- a/qmp.c
+++ b/qmp.c
@@ -108,6 +108,15 @@  void qmp_cpu(int64_t index, Error **errp)
     /* Just do nothing */
 }
 
+void qmp_cpu_set(int64_t id, const bool online, Error **errp)
+{
+    if (online) {
+        do_cpu_hot_add(id, errp);
+    } else {
+        error_setg(errp, "Unplug is not implemented");
+    }
+}
+
 #ifndef CONFIG_VNC
 /* If VNC support is enabled, the "true" query-vnc command is
    defined in the VNC subsystem */
diff --git a/stubs/Makefile.objs b/stubs/Makefile.objs
index 6a492f5..4154a2b 100644
--- a/stubs/Makefile.objs
+++ b/stubs/Makefile.objs
@@ -26,3 +26,4 @@  stub-obj-$(CONFIG_WIN32) += fd-register.o
 stub-obj-y += resume_vcpu.o
 stub-obj-y += get_icc_bus.o
 stub-obj-y += qemu_system_cpu_hotplug_request.o
+stub-obj-y += do_cpu_hot_add.o
diff --git a/stubs/do_cpu_hot_add.c b/stubs/do_cpu_hot_add.c
new file mode 100644
index 0000000..1f6d7a6
--- /dev/null
+++ b/stubs/do_cpu_hot_add.c
@@ -0,0 +1,7 @@ 
+#include "qapi/error.h"
+#include "sysemu/sysemu.h"
+
+void do_cpu_hot_add(const int64_t id, Error **errp)
+{
+    error_setg(errp, "Not implemented");
+}