Re: [Qemu-devel] [PATCH V4 08/10] NUMA: add qmp command set-mpol to set memory policy for NUMA node

[Luiz Capitulino]

On Mon, 8 Jul 2013 14:25:14 -0400
Luiz Capitulino <address@hidden> wrote:

> On Thu, 4 Jul 2013 17:53:15 +0800
> Wanlong Gao <address@hidden> 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 <address@hidden>
> > ---
> >  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
>