Patchwork [V4,08/10] NUMA: add qmp command set-mpol to set memory policy for NUMA node

login
register
mail settings
Submitter Wanlong Gao
Date July 4, 2013, 9:53 a.m.
Message ID <1372931597-28115-9-git-send-email-gaowanlong@cn.fujitsu.com>
Download mbox | patch
Permalink /patch/256860/
State New
Headers show

Comments

Wanlong Gao - July 4, 2013, 9:53 a.m.
The QMP command let it be able to set node's memory policy
through the QMP protocol. The qmp-shell command is like:
    set-mpol nodeid=0 mem-policy=membind mem-hostnode=0-1

Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
---
 cpus.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 qapi-schema.json | 15 +++++++++++++++
 qmp-commands.hx  | 35 +++++++++++++++++++++++++++++++++++
 3 files changed, 104 insertions(+)
Luiz Capitulino - July 8, 2013, 6:25 p.m.
On Thu, 4 Jul 2013 17:53:15 +0800
Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:

> The QMP command let it be able to set node's memory policy
> through the QMP protocol. The qmp-shell command is like:
>     set-mpol nodeid=0 mem-policy=membind mem-hostnode=0-1
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---
>  cpus.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi-schema.json | 15 +++++++++++++++
>  qmp-commands.hx  | 35 +++++++++++++++++++++++++++++++++++
>  3 files changed, 104 insertions(+)
> 
> diff --git a/cpus.c b/cpus.c
> index 7240de7..ff42b9d 100644
> --- a/cpus.c
> +++ b/cpus.c
> @@ -1417,3 +1417,57 @@ void qmp_inject_nmi(Error **errp)
>      error_set(errp, QERR_UNSUPPORTED);
>  #endif
>  }
> +
> +void qmp_set_mpol(int64_t nodeid, bool has_mpol, const char *mpol,
> +                  bool has_hostnode, const char *hostnode, Error **errp)
> +{
> +    unsigned int flags;
> +    DECLARE_BITMAP(host_mem, MAX_CPUMASK_BITS);
> +
> +    if (nodeid >= nb_numa_nodes) {
> +        error_setg(errp, "Only has '%d' NUMA nodes", nb_numa_nodes);
> +        return;
> +    }
> +
> +    bitmap_copy(host_mem, numa_info[nodeid].host_mem, MAX_CPUMASK_BITS);
> +    flags = numa_info[nodeid].flags;
> +
> +    numa_info[nodeid].flags = NODE_HOST_NONE;
> +    bitmap_zero(numa_info[nodeid].host_mem, MAX_CPUMASK_BITS);
> +
> +    if (!has_mpol) {
> +        if (set_node_mpol(nodeid) == -1) {
> +            error_setg(errp, "Failed to set memory policy for node%lu", nodeid);
> +            goto error;
> +        }
> +        return;
> +    }
> +
> +    numa_node_parse_mpol(nodeid, mpol, errp);
> +    if (error_is_set(errp)) {
> +        goto error;
> +    }
> +
> +    if (!has_hostnode) {
> +        bitmap_fill(numa_info[nodeid].host_mem, MAX_CPUMASK_BITS);
> +    }
> +
> +    if (hostnode) {
> +        numa_node_parse_hostnode(nodeid, hostnode, errp);
> +        if (error_is_set(errp)) {
> +            goto error;
> +        }
> +    }
> +
> +    if (set_node_mpol(nodeid) == -1) {
> +        error_setg(errp, "Failed to set memory policy for node%lu", nodeid);
> +        goto error;
> +    }
> +
> +    return;
> +
> +error:
> +    bitmap_copy(numa_info[nodeid].host_mem, host_mem, MAX_CPUMASK_BITS);
> +    numa_info[nodeid].flags = flags;
> +    return;
> +}
> diff --git a/qapi-schema.json b/qapi-schema.json
> index 5c32528..0870da2 100644
> --- a/qapi-schema.json
> +++ b/qapi-schema.json
> @@ -3712,3 +3712,18 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +# @set-mpol:
> +#
> +# Set the host memory binding policy for guest NUMA node.
> +#
> +# @nodeid: The node ID of guest NUMA node to set memory policy to.
> +#
> +# @mem-policy: The memory policy string to set.

Shouldn't this be an enum? Also, optional members have a leading '#optional'
string and if a default value is used it should be documented.

> +#
> +# @mem-hostnode: The host node or node range for memory policy.

It doesn't seem appropriate to use a string here. Maybe we could
use a list with only to values (like [0,2] for 0-2) or maybe a
list of nodes if that makes sense (like [0,1,2]).

> +#
> +# Since: 1.6.0
> +##
> +{ 'command': 'set-mpol', 'data': {'nodeid': 'int', '*mem-policy': 'str',
> +                                  '*mem-hostnode': 'str'} }
> diff --git a/qmp-commands.hx b/qmp-commands.hx
> index 362f0e1..ccab51b 100644
> --- a/qmp-commands.hx
> +++ b/qmp-commands.hx
> @@ -3043,3 +3043,38 @@ Example:
>  <- { "return": {} }
>  
>  EQMP
> +
> +    {
> +        .name      = "set-mpol",
> +        .args_type = "nodeid:i,mem-policy:s?,mem-hostnode:s?",
> +        .help      = "Set the host memory binding policy for guest NUMA node",
> +        .mhandler.cmd_new = qmp_marshal_input_set_mpol,
> +    },
> +
> +SQMP
> +set-mpol
> +------
> +
> +Set the host memory binding policy for guest NUMA node
> +
> +Arguments:
> +
> +- "nodeid": The nodeid of guest NUMA node to set memory policy to.
> +            (json-int)
> +- "mem-policy": The memory policy string to set.
> +                (json-string, optional)
> +- "mem-hostnode": The host nodes contained to mpol.
> +                  (json-string, optional)
> +
> +Example:
> +
> +-> { "execute": "set-mpol", "arguments": { "nodeid": 0, "mem-policy": "membind",
> +                                           "mem-hostnode": "0-1" }}
> +<- { "return": {} }
> +
> +Notes:
> +    1. If "mem-policy" is not set, the memory policy of this "nodeid" will be set
> +       to "default".
> +    2. If "mem-hostnode" is not set, the node mask of this "mpol" will be set
> +       to "all".
> +EQMP
Luiz Capitulino - July 8, 2013, 6:34 p.m.
On Mon, 8 Jul 2013 14:25:14 -0400
Luiz Capitulino <lcapitulino@redhat.com> wrote:

> On Thu, 4 Jul 2013 17:53:15 +0800
> Wanlong Gao <gaowanlong@cn.fujitsu.com> wrote:
> 
> > The QMP command let it be able to set node's memory policy
> > through the QMP protocol. The qmp-shell command is like:
> >     set-mpol nodeid=0 mem-policy=membind mem-hostnode=0-1
> > 
> > Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> > ---
> >  cpus.c           | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  qapi-schema.json | 15 +++++++++++++++
> >  qmp-commands.hx  | 35 +++++++++++++++++++++++++++++++++++
> >  3 files changed, 104 insertions(+)
> > 
> > diff --git a/cpus.c b/cpus.c
> > index 7240de7..ff42b9d 100644
> > --- a/cpus.c
> > +++ b/cpus.c
> > @@ -1417,3 +1417,57 @@ void qmp_inject_nmi(Error **errp)
> >      error_set(errp, QERR_UNSUPPORTED);
> >  #endif
> >  }
> > +
> > +void qmp_set_mpol(int64_t nodeid, bool has_mpol, const char *mpol,
> > +                  bool has_hostnode, const char *hostnode, Error **errp)
> > +{
> > +    unsigned int flags;
> > +    DECLARE_BITMAP(host_mem, MAX_CPUMASK_BITS);
> > +
> > +    if (nodeid >= nb_numa_nodes) {
> > +        error_setg(errp, "Only has '%d' NUMA nodes", nb_numa_nodes);
> > +        return;
> > +    }
> > +
> > +    bitmap_copy(host_mem, numa_info[nodeid].host_mem, MAX_CPUMASK_BITS);
> > +    flags = numa_info[nodeid].flags;
> > +
> > +    numa_info[nodeid].flags = NODE_HOST_NONE;
> > +    bitmap_zero(numa_info[nodeid].host_mem, MAX_CPUMASK_BITS);
> > +
> > +    if (!has_mpol) {
> > +        if (set_node_mpol(nodeid) == -1) {
> > +            error_setg(errp, "Failed to set memory policy for node%lu", nodeid);
> > +            goto error;
> > +        }
> > +        return;
> > +    }
> > +
> > +    numa_node_parse_mpol(nodeid, mpol, errp);
> > +    if (error_is_set(errp)) {
> > +        goto error;
> > +    }
> > +
> > +    if (!has_hostnode) {
> > +        bitmap_fill(numa_info[nodeid].host_mem, MAX_CPUMASK_BITS);
> > +    }
> > +
> > +    if (hostnode) {
> > +        numa_node_parse_hostnode(nodeid, hostnode, errp);
> > +        if (error_is_set(errp)) {
> > +            goto error;
> > +        }
> > +    }
> > +
> > +    if (set_node_mpol(nodeid) == -1) {
> > +        error_setg(errp, "Failed to set memory policy for node%lu", nodeid);
> > +        goto error;
> > +    }
> > +
> > +    return;
> > +
> > +error:
> > +    bitmap_copy(numa_info[nodeid].host_mem, host_mem, MAX_CPUMASK_BITS);
> > +    numa_info[nodeid].flags = flags;
> > +    return;
> > +}
> > diff --git a/qapi-schema.json b/qapi-schema.json
> > index 5c32528..0870da2 100644
> > --- a/qapi-schema.json
> > +++ b/qapi-schema.json
> > @@ -3712,3 +3712,18 @@
> >              '*cpuid-input-ecx': 'int',
> >              'cpuid-register': 'X86CPURegister32',
> >              'features': 'int' } }
> > +
> > +# @set-mpol:
> > +#
> > +# Set the host memory binding policy for guest NUMA node.
> > +#
> > +# @nodeid: The node ID of guest NUMA node to set memory policy to.
> > +#
> > +# @mem-policy: The memory policy string to set.
> 
> Shouldn't this be an enum? Also, optional members have a leading '#optional'
> string and if a default value is used it should be documented.
> 
> > +#
> > +# @mem-hostnode: The host node or node range for memory policy.
> 
> It doesn't seem appropriate to use a string here. Maybe we could
> use a list with only to values (like [0,2] for 0-2) or maybe a
> list of nodes if that makes sense (like [0,1,2]).

I forgot to bike-shed on the naming. I'd suggest the following:

 - s/set-mpol/set-numa-policy
 - s/mem-policy/policy
 - s/mem-hostnode/host-nodes

> 
> > +#
> > +# Since: 1.6.0
> > +##
> > +{ 'command': 'set-mpol', 'data': {'nodeid': 'int', '*mem-policy': 'str',
> > +                                  '*mem-hostnode': 'str'} }
> > diff --git a/qmp-commands.hx b/qmp-commands.hx
> > index 362f0e1..ccab51b 100644
> > --- a/qmp-commands.hx
> > +++ b/qmp-commands.hx
> > @@ -3043,3 +3043,38 @@ Example:
> >  <- { "return": {} }
> >  
> >  EQMP
> > +
> > +    {
> > +        .name      = "set-mpol",
> > +        .args_type = "nodeid:i,mem-policy:s?,mem-hostnode:s?",
> > +        .help      = "Set the host memory binding policy for guest NUMA node",
> > +        .mhandler.cmd_new = qmp_marshal_input_set_mpol,
> > +    },
> > +
> > +SQMP
> > +set-mpol
> > +------
> > +
> > +Set the host memory binding policy for guest NUMA node
> > +
> > +Arguments:
> > +
> > +- "nodeid": The nodeid of guest NUMA node to set memory policy to.
> > +            (json-int)
> > +- "mem-policy": The memory policy string to set.
> > +                (json-string, optional)
> > +- "mem-hostnode": The host nodes contained to mpol.
> > +                  (json-string, optional)
> > +
> > +Example:
> > +
> > +-> { "execute": "set-mpol", "arguments": { "nodeid": 0, "mem-policy": "membind",
> > +                                           "mem-hostnode": "0-1" }}
> > +<- { "return": {} }
> > +
> > +Notes:
> > +    1. If "mem-policy" is not set, the memory policy of this "nodeid" will be set
> > +       to "default".
> > +    2. If "mem-hostnode" is not set, the node mask of this "mpol" will be set
> > +       to "all".
> > +EQMP
>
Andreas Färber - July 8, 2013, 6:50 p.m.
Am 08.07.2013 20:34, schrieb Luiz Capitulino:
> I forgot to bike-shed on the naming. I'd suggest the following:
> 
>  - s/set-mpol/set-numa-policy
>  - s/mem-policy/policy
>  - s/mem-hostnode/host-nodes

I had suggested s/set-mpol/set-memory-policy/g on the previous round
(but didn't get any kind of response).

http://patchwork.ozlabs.org/patch/253699/

I'm fine with anything readable and following our conventions though.

Andreas
Luiz Capitulino - July 8, 2013, 7:03 p.m.
On Mon, 08 Jul 2013 20:50:46 +0200
Andreas Färber <afaerber@suse.de> wrote:

> Am 08.07.2013 20:34, schrieb Luiz Capitulino:
> > I forgot to bike-shed on the naming. I'd suggest the following:
> > 
> >  - s/set-mpol/set-numa-policy
> >  - s/mem-policy/policy
> >  - s/mem-hostnode/host-nodes
> 
> I had suggested s/set-mpol/set-memory-policy/g on the previous round
> (but didn't get any kind of response).
> 
> http://patchwork.ozlabs.org/patch/253699/
> 
> I'm fine with anything readable and following our conventions though.

I'm fine with your suggestion too.
Eric Blake - July 8, 2013, 7:16 p.m.
On 07/04/2013 03:53 AM, Wanlong Gao wrote:
> The QMP command let it be able to set node's memory policy

s/let it be able/allows users/

> through the QMP protocol. The qmp-shell command is like:
>     set-mpol nodeid=0 mem-policy=membind mem-hostnode=0-1
> 
> Signed-off-by: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> ---

Just an interface review:

> +++ b/qapi-schema.json
> @@ -3712,3 +3712,18 @@
>              '*cpuid-input-ecx': 'int',
>              'cpuid-register': 'X86CPURegister32',
>              'features': 'int' } }
> +
> +# @set-mpol:

I agree with other requests in this thread to make the name closer to
English words (set-memory-policy).  Also, I hate write-only interfaces;
what is the corresponding query-* command that lets me learn the policy
that is currently in effect?  I'm expecting that this series either
modifies an existing command or adds a new query command as the
counterpart to this set command.

> +#
> +# Set the host memory binding policy for guest NUMA node.
> +#
> +# @nodeid: The node ID of guest NUMA node to set memory policy to.
> +#
> +# @mem-policy: The memory policy string to set.
> +#
> +# @mem-hostnode: The host node or node range for memory policy.
> +#
> +# Since: 1.6.0
> +##
> +{ 'command': 'set-mpol', 'data': {'nodeid': 'int', '*mem-policy': 'str',
> +                                  '*mem-hostnode': 'str'} }

Make mem-policy an enum, not an open-coded string.  Also, make
mem-hostnode an array of nodes - a general rule of thumb is that if the
receiver (here, qemu) has to do further parsing of the data (such as
scraping out integer vs. dash to decide if it is one node or a range),
then the JSON was too high-level.  Using ['int'] instead of 'str' will
let the data be available already parsed into an integer list by the
visitor code, so you aren't having to write your own ad hoc parser on
the receiving end.
Wanlong Gao - July 15, 2013, 11:18 a.m.
On 07/09/2013 02:25 AM, Luiz Capitulino wrote:
>> +
>> > +# @set-mpol:
>> > +#
>> > +# Set the host memory binding policy for guest NUMA node.
>> > +#
>> > +# @nodeid: The node ID of guest NUMA node to set memory policy to.
>> > +#
>> > +# @mem-policy: The memory policy string to set.
> Shouldn't this be an enum? Also, optional members have a leading '#optional'
> string and if a default value is used it should be documented.

Thank you, will fix in V5.

> 
>> > +#
>> > +# @mem-hostnode: The host node or node range for memory policy.
> It doesn't seem appropriate to use a string here. Maybe we could
> use a list with only to values (like [0,2] for 0-2) or maybe a
> list of nodes if that makes sense (like [0,1,2]).

I wonder if not using string, how to support "+" and "!" ?

Thanks,
Wanlong Gao

Patch

diff --git a/cpus.c b/cpus.c
index 7240de7..ff42b9d 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1417,3 +1417,57 @@  void qmp_inject_nmi(Error **errp)
     error_set(errp, QERR_UNSUPPORTED);
 #endif
 }
+
+void qmp_set_mpol(int64_t nodeid, bool has_mpol, const char *mpol,
+                  bool has_hostnode, const char *hostnode, Error **errp)
+{
+    unsigned int flags;
+    DECLARE_BITMAP(host_mem, MAX_CPUMASK_BITS);
+
+    if (nodeid >= nb_numa_nodes) {
+        error_setg(errp, "Only has '%d' NUMA nodes", nb_numa_nodes);
+        return;
+    }
+
+    bitmap_copy(host_mem, numa_info[nodeid].host_mem, MAX_CPUMASK_BITS);
+    flags = numa_info[nodeid].flags;
+
+    numa_info[nodeid].flags = NODE_HOST_NONE;
+    bitmap_zero(numa_info[nodeid].host_mem, MAX_CPUMASK_BITS);
+
+    if (!has_mpol) {
+        if (set_node_mpol(nodeid) == -1) {
+            error_setg(errp, "Failed to set memory policy for node%lu", nodeid);
+            goto error;
+        }
+        return;
+    }
+
+    numa_node_parse_mpol(nodeid, mpol, errp);
+    if (error_is_set(errp)) {
+        goto error;
+    }
+
+    if (!has_hostnode) {
+        bitmap_fill(numa_info[nodeid].host_mem, MAX_CPUMASK_BITS);
+    }
+
+    if (hostnode) {
+        numa_node_parse_hostnode(nodeid, hostnode, errp);
+        if (error_is_set(errp)) {
+            goto error;
+        }
+    }
+
+    if (set_node_mpol(nodeid) == -1) {
+        error_setg(errp, "Failed to set memory policy for node%lu", nodeid);
+        goto error;
+    }
+
+    return;
+
+error:
+    bitmap_copy(numa_info[nodeid].host_mem, host_mem, MAX_CPUMASK_BITS);
+    numa_info[nodeid].flags = flags;
+    return;
+}
diff --git a/qapi-schema.json b/qapi-schema.json
index 5c32528..0870da2 100644
--- a/qapi-schema.json
+++ b/qapi-schema.json
@@ -3712,3 +3712,18 @@ 
             '*cpuid-input-ecx': 'int',
             'cpuid-register': 'X86CPURegister32',
             'features': 'int' } }
+
+# @set-mpol:
+#
+# Set the host memory binding policy for guest NUMA node.
+#
+# @nodeid: The node ID of guest NUMA node to set memory policy to.
+#
+# @mem-policy: The memory policy string to set.
+#
+# @mem-hostnode: The host node or node range for memory policy.
+#
+# Since: 1.6.0
+##
+{ 'command': 'set-mpol', 'data': {'nodeid': 'int', '*mem-policy': 'str',
+                                  '*mem-hostnode': 'str'} }
diff --git a/qmp-commands.hx b/qmp-commands.hx
index 362f0e1..ccab51b 100644
--- a/qmp-commands.hx
+++ b/qmp-commands.hx
@@ -3043,3 +3043,38 @@  Example:
 <- { "return": {} }
 
 EQMP
+
+    {
+        .name      = "set-mpol",
+        .args_type = "nodeid:i,mem-policy:s?,mem-hostnode:s?",
+        .help      = "Set the host memory binding policy for guest NUMA node",
+        .mhandler.cmd_new = qmp_marshal_input_set_mpol,
+    },
+
+SQMP
+set-mpol
+------
+
+Set the host memory binding policy for guest NUMA node
+
+Arguments:
+
+- "nodeid": The nodeid of guest NUMA node to set memory policy to.
+            (json-int)
+- "mem-policy": The memory policy string to set.
+                (json-string, optional)
+- "mem-hostnode": The host nodes contained to mpol.
+                  (json-string, optional)
+
+Example:
+
+-> { "execute": "set-mpol", "arguments": { "nodeid": 0, "mem-policy": "membind",
+                                           "mem-hostnode": "0-1" }}
+<- { "return": {} }
+
+Notes:
+    1. If "mem-policy" is not set, the memory policy of this "nodeid" will be set
+       to "default".
+    2. If "mem-hostnode" is not set, the node mask of this "mpol" will be set
+       to "all".
+EQMP