| Message ID | 1372931597-28115-9-git-send-email-gaowanlong@cn.fujitsu.com |
|---|---|
| State | New |
| Headers | show |
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
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 >
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
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.
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.
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
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
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(+)