On Fri, Dec 16, 2011 at 03:45:36PM -0700, Eric Blake wrote:

On 12/15/2011 03:50 AM, Hu Tao wrote:

...

Set up the types for the numa functions and insert them into the virDriver structure definition. --- include/libvirt/libvirt.h.in | 23 +++++++++++++++++++++++ python/generator.py | 2 ++ src/driver.h | 15 +++++++++++++++ src/libvirt_public.syms | 6 ++++++ 4 files changed, 46 insertions(+), 0 deletions(-)

...

+/* Manage numa parameters */ + +/** + * VIR_DOMAIN_NUMA_NODESET: + * + * numa nodeset + */ +#define VIR_DOMAIN_NUMA_NODESET "numa_nodeset" + +/** + * VIR_DOMAIN_NUMA_MODE: + * + * numa mode + */ +#define VIR_DOMAIN_NUMA_MODE "numa_mode"

That's awfully sparse on details, such as what type is expected. Furthermore, after reading 6/8, I noticed that you are using numa_mode to hold a non-public enum value, which makes it a non-starter. I'm squashing this in, to make the enum public and add some documentation.

Sorry didn't notice the enums are not public.

diff --git i/include/libvirt/libvirt.h.in w/include/libvirt/libvirt.h.in index 55f27ae..da370fb 100644 --- i/include/libvirt/libvirt.h.in +++ w/include/libvirt/libvirt.h.in @@ -1347,16 +1347,32 @@ typedef enum { /* Manage numa parameters */

/** + * virDomainNumatuneMemMode: + * Representation of the various modes in the <numatune> element of + * a domain. + */ +enum virDomainNumatuneMemMode {

Error on generating docs here. Changing to typedef enum { ... } virDomainNumatuneMemMode; makes it works. The rest seems ok to me.

+ VIR_DOMAIN_NUMATUNE_MEM_STRICT = 0, + VIR_DOMAIN_NUMATUNE_MEM_PREFERRED = 1, + VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE = 2, + + /* This constant is subject to change */ + VIR_DOMAIN_NUMATUNE_MEM_LAST +}; + +/** * VIR_DOMAIN_NUMA_NODESET: * - * numa nodeset + * Macro for typed parameter name that lists the numa nodeset of a + * domain, as a string. */ #define VIR_DOMAIN_NUMA_NODESET "numa_nodeset"

/** * VIR_DOMAIN_NUMA_MODE: * - * numa mode + * Macro for typed parameter name that lists the numa mode of a domain, + * as an int containing a virDomainNumatuneMemMode value. */ #define VIR_DOMAIN_NUMA_MODE "numa_mode"

diff --git i/src/conf/domain_conf.h w/src/conf/domain_conf.h index 3229a6f..189b8f6 100644 --- i/src/conf/domain_conf.h +++ w/src/conf/domain_conf.h @@ -1355,14 +1355,6 @@ virDomainVcpuPinDefPtr virDomainVcpuPinFindByVcpu(virDomainVcpuPinDefPtr *def, int nvcpupin, int vcpu);

-enum virDomainNumatuneMemMode { - VIR_DOMAIN_NUMATUNE_MEM_STRICT, - VIR_DOMAIN_NUMATUNE_MEM_PREFERRED, - VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE, - - VIR_DOMAIN_NUMATUNE_MEM_LAST -}; - typedef struct _virDomainNumatuneDef virDomainNumatuneDef; typedef virDomainNumatuneDef *virDomainNumatuneDefPtr; struct _virDomainNumatuneDef {

-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

-- Thanks, Hu Tao

On 12/15/2011 03:50 AM, Hu Tao wrote:

--- src/libvirt.c | 113 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 113 insertions(+), 0 deletions(-)

I've reviewed 1-4, and the first three look fine; however, I'm going to squash 3 and 4 into a single patch (it's easier to see the documentation in libvirt.c alongside the entry points added in libvirt.h in the same commit). Also,

+/** + * virDomainGetNumaParameters: + * @domain: pointer to domain object + * @params: pointer to numa parameter object + * (return value, allocated by the caller) + * @nparams: pointer to number of numa parameters + * @flags: one of virDomainModificationImpact + * + * This function may require privileged access to the hypervisor. This function + * expects the caller to allocate the @params.

You didn't document how to set @params and @nparams. So I squashed in: diff --git i/src/libvirt.c w/src/libvirt.c index e61d91e..574be16 100644 --- i/src/libvirt.c +++ w/src/libvirt.c @@ -3772,7 +3772,7 @@ error: * virDomainSetNumaParameters: * @domain: pointer to domain object * @params: pointer to numa parameter objects - * @nparams: number of numa parameter (this value can be the same or + * @nparams: number of numa parameters (this value can be the same or * less than the number of parameters supported) * @flags: bitwise-OR of virDomainModificationImpact * @@ -3781,9 +3781,10 @@ error: * * Returns -1 in case of error, 0 in case of success. */ -int virDomainSetNumaParameters(virDomainPtr domain, - virTypedParameterPtr params, - int nparams, unsigned int flags) +int +virDomainSetNumaParameters(virDomainPtr domain, + virTypedParameterPtr params, + int nparams, unsigned int flags) { virConnectPtr conn; @@ -3812,7 +3813,8 @@ int virDomainSetNumaParameters(virDomainPtr domain, if (conn->driver->domainSetNumaParameters) { int ret; - ret = conn->driver->domainSetNumaParameters (domain, params, nparams, flags); + ret = conn->driver->domainSetNumaParameters(domain, params, nparams, + flags); if (ret < 0) goto error; return ret; @@ -3831,7 +3833,20 @@ error: * @params: pointer to numa parameter object * (return value, allocated by the caller) * @nparams: pointer to number of numa parameters - * @flags: one of virDomainModificationImpact + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags + * + * Get all numa parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. + * + * As a special case, calling with @params as NULL and @nparams as 0 on + * input will cause @nparams on output to contain the number of parameters + * supported by the hypervisor. The caller should then allocate @params + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API + * again. + * + * See virDomainGetMemoryParameters() for an equivalent usage example. * * This function may require privileged access to the hypervisor. This function * expects the caller to allocate the @params. @@ -3869,7 +3884,8 @@ virDomainGetNumaParameters(virDomainPtr domain, if (conn->driver->domainGetNumaParameters) { int ret; - ret = conn->driver->domainGetNumaParameters (domain, params, nparams, flags); + ret = conn->driver->domainGetNumaParameters(domain, params, nparams, + flags); if (ret < 0) goto error; return ret; -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Fri, Dec 16, 2011 at 09:32:19AM -0700, Eric Blake wrote:

On 12/15/2011 03:50 AM, Hu Tao wrote:

...

--- daemon/remote.c | 64 ++++++++++++++++++++++++++++++++++++++++++ src/remote/remote_driver.c | 50 ++++++++++++++++++++++++++++++++ src/remote/remote_protocol.x | 24 +++++++++++++++- src/remote_protocol-structs | 16 ++++++++++ 4 files changed, 153 insertions(+), 1 deletions(-)

Your changes to remote_protocol-structs were incomplete (on Fedora systems, 'yum install dwarves' to get it right). ACK with this squashed in:

Thanks. I have dwarves installed but no effect(in a Fedora 14 system). Do I have to run any commands included in dwarves, or make will call them automatically?

diff --git i/src/remote_protocol-structs w/src/remote_protocol-structs index ce01b5a..30e9244 100644 --- i/src/remote_protocol-structs +++ w/src/remote_protocol-structs @@ -168,22 +168,6 @@ struct remote_node_get_memory_stats_ret { } params; int nparams; }; -struct remote_domain_set_numa_parameters_args { - remote_nonnull_domain dom; - remote_typed_param params<REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX>; - unsigned int flags; -}; - -struct remote_domain_get_numa_parameters_args { - remote_nonnull_domain dom; - int nparams; - unsigned int flags; -}; - -struct remote_domain_get_numa_parameters_ret { - remote_typed_param params<REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX>; - int nparams; -}; struct remote_node_get_cells_free_memory_args { int startCell; int maxcells; @@ -286,6 +270,22 @@ struct remote_domain_block_resize_args { uint64_t size; u_int flags; }; +struct remote_domain_set_numa_parameters_args { + remote_nonnull_domain dom; + remote_typed_param params<REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX>; + unsigned int flags; +}; + +struct remote_domain_get_numa_parameters_args { + remote_nonnull_domain dom; + int nparams; + unsigned int flags; +}; + +struct remote_domain_get_numa_parameters_ret { + remote_typed_param params<REMOTE_DOMAIN_MEMORY_PARAMETERS_MAX>; + int nparams; +}; struct remote_domain_block_stats_args { remote_nonnull_domain dom; remote_nonnull_string path; @@ -2060,4 +2060,6 @@ enum remote_procedure { REMOTE_PROC_DOMAIN_BLOCK_RESIZE = 251, REMOTE_PROC_DOMAIN_SET_BLOCK_IO_TUNE = 252, REMOTE_PROC_DOMAIN_GET_BLOCK_IO_TUNE = 253, + REMOTE_PROC_DOMAIN_SET_NUMA_PARAMETERS = 254, + REMOTE_PROC_DOMAIN_GET_NUMA_PARAMETERS = 255, };

-- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

-- Thanks, Hu Tao

On 12/15/2011 03:50 AM, Hu Tao wrote:

--- src/qemu/qemu_driver.c | 334 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 334 insertions(+), 0 deletions(-)

+ + isActive = virDomainObjIsActive(vm); + + if (flags == VIR_DOMAIN_AFFECT_CURRENT) { + if (isActive) + flags = VIR_DOMAIN_AFFECT_LIVE; + else + flags = VIR_DOMAIN_AFFECT_CONFIG; + }

This can be simplified by the code in commit ae523427.

+ + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (vm->def->numatune.memory.mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("change of nodeset for running domain is supported only when numa mode is strict")); + ret = -1;

I think OPERATION_INVALID fits better here than INVALID_ARG, since once the domain goes offline, the same argument will then start working. Also, the message was long.

+ continue; + } + rc = virCgroupSetCpusetMems(group, params[i].value.s); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to set memory hard_limit tunable"));

Too much copy and paste.

+ ret = -1; + continue; + } + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + char *oldnodemask = strdup(persistentDef->numatune.memory.nodemask);

strdup is wrong; the CPU mask can contain NUL bytes in the middle. Also, you have a memory leak. Using stack allocation and memcpy solves both issues.

+ if (!oldnodemask) { + virReportOOMError(); + ret = -1; + continue; + } + if (virDomainCpuSetParse(params[i].value.s, + 0,

Indentation.

+ } else if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) { + if (param->type != VIR_TYPED_PARAM_ULLONG) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for numa strict tunable, expected a 'ullong'"));

Why a ullong? That's awfully wide, when domain_conf only stores numatune.memory.mode as an int, and that in turn only maps to an enum valued between 0 and 2 inclusive (see my tweaks to 3/8). Not to mention you used value.i later on, instead of value.ul.

+ ret = -1; + continue; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + qemuReportError(VIR_ERR_INVALID_ARG, _("can't change numa mode for running domain"));

Missing "%s". Again, OPERATION_INVALID fits better here.

+ +static int qemuDomainGetNumaParameters(virDomainPtr dom, + virTypedParameterPtr params, + int *nparams, + unsigned int flags)

Style-wise, we aren't yet consistent in libvirt (let alone in the qemu driver), but we are starting to converge on a style where new code lists the return type on one line and function name on column 1 of the next.

+{ + struct qemud_driver *driver = dom->conn->privateData; + int i; + virCgroupPtr group = NULL; + virDomainObjPtr vm = NULL; + virDomainDefPtr persistentDef = NULL; + char *nodeset = NULL; + int ret = -1; + int rc; + bool isActive;

Again, you don't need this.

+ + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG | + VIR_TYPED_PARAM_STRING_OKAY, -1); + + qemuDriverLock(driver); + + flags &= ~VIR_TYPED_PARAM_STRING_OKAY;

A comment helps here.

+ + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find cgroup for domain %s"), vm->def->name); + goto cleanup; + } + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (!vm->persistent) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("cannot change persistent config of a transient domain")); + goto cleanup; + } + if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) + goto cleanup; + } + + if ((*nparams) == 0) { + *nparams = QEMU_NB_NUMA_PARAM; + ret = 0; + goto cleanup; + }

Do this check earlier, to avoid time doing a wasted cgroup lookup.

+ + if ((*nparams) < QEMU_NB_NUMA_PARAM) { + qemuReportError(VIR_ERR_INVALID_ARG, + "%s", _("Invalid parameter count")); + goto cleanup; + }

Drop this check. It needlessly limits the user.

+ + if (flags & VIR_DOMAIN_AFFECT_CONFIG) {

Rather than making this the outer condition, and repeating the loop twice, I find it simpler to make it the inner condition only in the places where it matters within a single loop.

+ for (i = 0; i < *nparams; i++) {

Cap this loop to the max number of parameters we know about.

+ virMemoryParameterPtr param = ¶ms[i]; + param->value.ul = 0; + param->type = VIR_TYPED_PARAM_ULLONG;

Wrong. One param is string, and the other int (given my earlier tweaks).

+ + switch (i) { + case 0: /* fill numa nodeset here */ + if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field numa nodeset too long for destination")); + goto cleanup; + } + if (persistentDef->numatune.memory.nodemask) { + nodeset = virDomainCpuSetFormat(persistentDef->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN); + if (!nodeset) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("failed to format nodeset for NUMA memory tuning")); + goto cleanup; + } + param->value.s = nodeset; + nodeset = NULL; + } else { + param->value.s = strdup("");

Check for allocation failure.

+ } + param->type = VIR_TYPED_PARAM_STRING; + break; + + case 1: /* fill numa mode here */

I find it confusing listing nodeset before mode here, when the XML lists it in the reverse direction. For consistency, I also swapped the order in the setter routine (but there you still have the issue that the user can pass in parameters out of order, and result in changing the numaset prior to incorrectly requesting a mode change, and we don't roll back the numaset change in that case - oh well).

+ if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE) == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + "%s", _("Field numa mode too long for destination")); + goto cleanup; + } + param->value.i = persistentDef->numatune.memory.mode; + break; + + default: + break; + /* should not hit here */ + } + } + goto out; + } + + for (i = 0; i < QEMU_NB_NUMA_PARAM; i++) {

Wrong - you just did a buffer overflow if the user passed *nparams = 1 (well once you take in my earlier comment that requires a minimum *nparams).

+ virTypedParameterPtr param = ¶ms[i]; + param->value.ul = 0; + param->type = VIR_TYPED_PARAM_ULLONG; + + /* Coverity does not realize that if we get here, group is set. */ + sa_assert(group);

I'd rather omit this until we know for sure what Coverity complains about.

+ + switch (i) { + case 0: /* fill numa nodeset here */ + rc = virCgroupGetCpusetMems(group, &nodeset);

Hmm - here, we are reading cgroups in preference to trusting what is in domain_conf. But earlier, in the set routine, we didn't update vm->def (just cgroups). That means if we alter the nodeset at runtim, then run 'virsh dumpxml', the XML will be wrong, even though the virsh query of the numatune parameters will be correct. The fix is either to make setting _also_ update vm->def in addition to cgroups (but if we do that, then why query cgroups - just read off vm->def); or to make sure that virsh dumpxml _always_ reads cgroups rather than trusting vm->def. I'm not sure which approach is better, so for now, I added a FIXME and am hoping we get it resolved soon (I think you're not the first person to fall prey to this design bug; my recollection is that the per-device blkiotune parameters added in 0.9.8 have the same bug, so cleaning all instances up in a separate patch later makes sense anyways).;

+ if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to get numa nodeset")); + goto cleanup; + }

+out: + *nparams = QEMU_NB_NUMA_PARAM; + ret = 0;

If the user passed *nparams as 1, then this causes the user to buffer overrun.

+ +cleanup: + if (group) + virCgroupFree(&group);

Useless if before free. Wow - I had to really touch this up. src/qemu/qemu_driver.c | 282 ++++++++++++++++++------------------------------ 1 files changed, 105 insertions(+), 177 deletions(-) diff --git i/src/qemu/qemu_driver.c w/src/qemu/qemu_driver.c index 24f3047..12235ef 100644 --- i/src/qemu/qemu_driver.c +++ w/src/qemu/qemu_driver.c @@ -6606,10 +6606,11 @@ cleanup: return ret; } -static int qemuDomainSetNumaParameters(virDomainPtr dom, - virTypedParameterPtr params, - int nparams, - unsigned int flags) +static int +qemuDomainSetNumaParameters(virDomainPtr dom, + virTypedParameterPtr params, + int nparams, + unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; int i; @@ -6617,7 +6618,6 @@ static int qemuDomainSetNumaParameters(virDomainPtr dom, virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; int ret = -1; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); @@ -6632,22 +6632,11 @@ static int qemuDomainSetNumaParameters(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); - - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; - } + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) + goto cleanup; if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!isActive) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUSET)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cgroup cpuset controller is not mounted")); @@ -6656,84 +6645,80 @@ static int qemuDomainSetNumaParameters(virDomainPtr dom, if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find cgroup for domain %s"), vm->def->name); - goto cleanup; - } - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); + _("cannot find cgroup for domain %s"), + vm->def->name); goto cleanup; } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; } ret = 0; for (i = 0; i < nparams; i++) { virTypedParameterPtr param = ¶ms[i]; - if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) { + if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) { + if (param->type != VIR_TYPED_PARAM_INT) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for numa strict tunable, " + "expected an 'int'")); + ret = -1; + continue; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("can't change numa mode for running domain")); + ret = -1; + goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + persistentDef->numatune.memory.mode = params[i].value.i; + } + } else if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) { int rc; if (param->type != VIR_TYPED_PARAM_STRING) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid type for numa nodeset tunable, expected a 'string'")); + _("invalid type for numa nodeset tunable, " + "expected a 'string'")); ret = -1; continue; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (vm->def->numatune.memory.mode != VIR_DOMAIN_NUMATUNE_MEM_STRICT) { - qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("change of nodeset for running domain is supported only when numa mode is strict")); + if (vm->def->numatune.memory.mode != + VIR_DOMAIN_NUMATUNE_MEM_STRICT) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("change of nodeset for running domain " + "requires strict numa mode")); ret = -1; continue; } rc = virCgroupSetCpusetMems(group, params[i].value.s); if (rc != 0) { virReportSystemError(-rc, "%s", - _("unable to set memory hard_limit tunable")); + _("unable to set numa tunable")); ret = -1; continue; } + /* XXX FIXME - should we also be updating vm->def + * here? If not, then we need to fix dumpxml to always + * read from cgroups rather than trusting vm->def. */ } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - char *oldnodemask = strdup(persistentDef->numatune.memory.nodemask); - if (!oldnodemask) { - virReportOOMError(); - ret = -1; - continue; - } + char oldnodemask[VIR_CPU_MAPLEN(VIR_DOMAIN_CPUMASK_LEN)]; + memcpy(oldnodemask, persistentDef->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN); if (virDomainCpuSetParse(params[i].value.s, - 0, - persistentDef->numatune.memory.nodemask, - VIR_DOMAIN_CPUMASK_LEN) < 0) { - VIR_FREE(persistentDef->numatune.memory.nodemask); - persistentDef->numatune.memory.nodemask = oldnodemask; + 0, + persistentDef->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN) < 0) { + memcpy(persistentDef->numatune.memory.nodemask, + oldnodemask, VIR_DOMAIN_CPUMASK_LEN); ret = -1; continue; } } - } else if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) { - if (param->type != VIR_TYPED_PARAM_ULLONG) { - qemuReportError(VIR_ERR_INVALID_ARG, "%s", - _("invalid type for numa strict tunable, expected a 'ullong'")); - ret = -1; - continue; - } - - if (flags & VIR_DOMAIN_AFFECT_LIVE) { - qemuReportError(VIR_ERR_INVALID_ARG, _("can't change numa mode for running domain")); - ret = -1; - goto cleanup; - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentDef->numatune.memory.mode = params[i].value.i; - } } else { qemuReportError(VIR_ERR_INVALID_ARG, _("Parameter `%s' not supported"), param->field); @@ -6754,10 +6739,11 @@ cleanup: return ret; } -static int qemuDomainGetNumaParameters(virDomainPtr dom, - virTypedParameterPtr params, - int *nparams, - unsigned int flags) +static int +qemuDomainGetNumaParameters(virDomainPtr dom, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) { struct qemud_driver *driver = dom->conn->privateData; int i; @@ -6767,7 +6753,6 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, char *nodeset = NULL; int ret = -1; int rc; - bool isActive; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG | @@ -6775,6 +6760,9 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, qemuDriverLock(driver); + /* We blindly return a string, and let libvirt.c and + * remote_driver.c do the filtering on behalf of older clients + * that can't parse it. */ flags &= ~VIR_TYPED_PARAM_STRING_OKAY; vm = virDomainFindByUUID(&driver->domains, dom->uuid); @@ -6785,22 +6773,17 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, goto cleanup; } - isActive = virDomainObjIsActive(vm); + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) + goto cleanup; - if (flags == VIR_DOMAIN_AFFECT_CURRENT) { - if (isActive) - flags = VIR_DOMAIN_AFFECT_LIVE; - else - flags = VIR_DOMAIN_AFFECT_CONFIG; + if ((*nparams) == 0) { + *nparams = QEMU_NB_NUMA_PARAM; + ret = 0; + goto cleanup; } if (flags & VIR_DOMAIN_AFFECT_LIVE) { - if (!isActive) { - qemuReportError(VIR_ERR_OPERATION_INVALID, - "%s", _("domain is not running")); - goto cleanup; - } - if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) { qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", _("cgroup memory controller is not mounted")); @@ -6809,112 +6792,58 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - _("cannot find cgroup for domain %s"), vm->def->name); - goto cleanup; - } - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - if (!vm->persistent) { - qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", - _("cannot change persistent config of a transient domain")); + _("cannot find cgroup for domain %s"), + vm->def->name); goto cleanup; } - if (!(persistentDef = virDomainObjGetPersistentDef(driver->caps, vm))) - goto cleanup; } - if ((*nparams) == 0) { - *nparams = QEMU_NB_NUMA_PARAM; - ret = 0; - goto cleanup; - } - - if ((*nparams) < QEMU_NB_NUMA_PARAM) { - qemuReportError(VIR_ERR_INVALID_ARG, - "%s", _("Invalid parameter count")); - goto cleanup; - } - - if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - for (i = 0; i < *nparams; i++) { - virMemoryParameterPtr param = ¶ms[i]; - param->value.ul = 0; - param->type = VIR_TYPED_PARAM_ULLONG; - - switch (i) { - case 0: /* fill numa nodeset here */ - if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET) == NULL) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field numa nodeset too long for destination")); - goto cleanup; - } - if (persistentDef->numatune.memory.nodemask) { - nodeset = virDomainCpuSetFormat(persistentDef->numatune.memory.nodemask, - VIR_DOMAIN_CPUMASK_LEN); - if (!nodeset) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("failed to format nodeset for NUMA memory tuning")); - goto cleanup; - } - param->value.s = nodeset; - nodeset = NULL; - } else { - param->value.s = strdup(""); - } - param->type = VIR_TYPED_PARAM_STRING; - break; - - case 1: /* fill numa mode here */ - if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE) == NULL) { - qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field numa mode too long for destination")); - goto cleanup; - } - param->value.i = persistentDef->numatune.memory.mode; - break; - - default: - break; - /* should not hit here */ - } - } - goto out; - } - - for (i = 0; i < QEMU_NB_NUMA_PARAM; i++) { - virTypedParameterPtr param = ¶ms[i]; - param->value.ul = 0; - param->type = VIR_TYPED_PARAM_ULLONG; - - /* Coverity does not realize that if we get here, group is set. */ - sa_assert(group); + for (i = 0; i < QEMU_NB_NUMA_PARAM && i < *nparams; i++) { + virMemoryParameterPtr param = ¶ms[i]; switch (i) { - case 0: /* fill numa nodeset here */ - rc = virCgroupGetCpusetMems(group, &nodeset); - if (rc != 0) { - virReportSystemError(-rc, "%s", - _("unable to get numa nodeset")); - goto cleanup; - } - if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET) == NULL) { + case 0: /* fill numa mode here */ + if (!virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field numa nodeset too long for destination")); - VIR_FREE(nodeset); + _("Field '%s' too long for destination"), + VIR_DOMAIN_NUMA_MODE); goto cleanup; } - param->value.s = nodeset; - param->type = VIR_TYPED_PARAM_STRING; + param->type = VIR_TYPED_PARAM_INT; + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + param->value.i = persistentDef->numatune.memory.mode; + else + param->value.i = vm->def->numatune.memory.mode; break; - case 1: /* file numa mode here */ - if (virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE) == NULL) { + case 1: /* fill numa nodeset here */ + if (!virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET)) { qemuReportError(VIR_ERR_INTERNAL_ERROR, - "%s", _("Field numa exclusive too long for destination")); + _("Field '%s' too long for destination"), + VIR_DOMAIN_NUMA_NODESET); + goto cleanup; + } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + char *mask = persistentDef->numatune.memory.nodemask; + if (mask) + nodeset = virDomainCpuSetFormat(mask, + VIR_DOMAIN_CPUMASK_LEN); + else + nodeset = strdup(""); + } else { + rc = virCgroupGetCpusetMems(group, &nodeset); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to get numa nodeset")); + goto cleanup; + } + } + if (!nodeset) { + virReportOOMError(); goto cleanup; } - param->value.ul = vm->def->numatune.memory.mode; + param->type = VIR_TYPED_PARAM_STRING; + param->value.s = nodeset; break; default: @@ -6923,13 +6852,12 @@ static int qemuDomainGetNumaParameters(virDomainPtr dom, } } -out: - *nparams = QEMU_NB_NUMA_PARAM; + if (*nparams > QEMU_NB_NUMA_PARAM) + *nparams = QEMU_NB_NUMA_PARAM; ret = 0; cleanup: - if (group) - virCgroupFree(&group); + virCgroupFree(&group); if (vm) virDomainObjUnlock(vm); qemuDriverUnlock(driver); -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/15/2011 03:50 AM, Hu Tao wrote:

add new command numatune to virsh to get/set numa parameters --- tools/virsh.c | 181 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 181 insertions(+), 0 deletions(-)

diff --git a/tools/virsh.c b/tools/virsh.c index f6571f7..2b509bf 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -61,6 +61,7 @@ #include "virkeycode.h" #include "virnetdevbandwidth.h" #include "util/bitmap.h" +#include "conf/domain_conf.h"

Hmm, this is the first case of including a file from src/conf/ from within tools/, just to translate between strings and numa modes. If the functionality needs to be common, should it be moved to live in util (cf. virKeycodeValueFromString)? Daniel (danpb or DV), is that a problem that we need to worry about?

+ for (i = 0; i < nparams; i++) { + switch (params[i].type) { + case VIR_TYPED_PARAM_INT: + vshPrint(ctl, "%-15s: %d\n", params[i].field, + params[i].value.i); + break; + case VIR_TYPED_PARAM_UINT: + vshPrint(ctl, "%-15s: %u\n", params[i].field, + params[i].value.ui); + break; + case VIR_TYPED_PARAM_LLONG: + vshPrint(ctl, "%-15s: %lld\n", params[i].field, + params[i].value.l); + break; + case VIR_TYPED_PARAM_ULLONG: + if (STREQ(params[i].field, VIR_DOMAIN_NUMA_MODE)) + vshPrint(ctl, "%-15s: %s\n", params[i].field, + virDomainNumatuneMemModeTypeToString(params[i].value.ul));

This code needs tweaking, since I fixed this to be an INT in value.i, not a ULLONG. Also, this whole case statement is redundant; we already have a helper method for formatting typed parameters. I'll submit a separate patch for starting the cleanup.

+ for (i = 0; i < nparams; i++) { + temp = ¶ms[i]; + temp->type = VIR_TYPED_PARAM_ULLONG;

Wrong type. This needs to be done per parameter.

+ + /* + * Some magic here, this is used to fill the params structure with + * the valid arguments passed, after filling the particular + * argument we purposely make them 0, so on the next pass it goes + * to the next valid argument and so on. + */ + if (nodeset) { + temp->value.s = vshStrdup(ctl, nodeset); + temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_NUMA_NODESET, + sizeof(temp->field))) + goto cleanup; + nodeset = NULL; + } else if (mode) {

We should prefer mode before nodeset, to match the XML, and since it gives saner cleanup behavior if mode is incompatible with a runtime change in nodeset.

+ strncpy(temp->field, VIR_DOMAIN_NUMA_MODE, + sizeof(temp->field));

Why strncpy here, but virStrcpy on the other field?

+ cleanup: + VIR_FREE(params);

Memory leak. params can contain strdup'd text.

@@ -15148,6 +15328,7 @@ static const vshCmdDef domManagementCmds[] = { info_managedsaveremove, 0}, {"maxvcpus", cmdMaxvcpus, opts_maxvcpus, info_maxvcpus, 0}, {"memtune", cmdMemtune, opts_memtune, info_memtune, 0}, + {"numatune", cmdNumatune, opts_numatune, info_numatune, 0}, {"migrate", cmdMigrate, opts_migrate, info_migrate, 0},

Sorting. Here's what I'm squashing so far, although I'd like an answer on whether we can keep the conf/domain_conf.h include before I can push anything: diff --git i/tools/virsh.c w/tools/virsh.c index ad570d3..e148f95 100644 --- i/tools/virsh.c +++ w/tools/virsh.c @@ -5143,10 +5143,10 @@ static const vshCmdInfo info_numatune[] = { static const vshCmdOptDef opts_numatune[] = { {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, - {"nodeset", VSH_OT_DATA, VSH_OFLAG_NONE, - N_("NUMA node to set")}, {"mode", VSH_OT_DATA, VSH_OFLAG_NONE, N_("NUMA mode, one of strict, preferred and interleave")}, + {"nodeset", VSH_OT_DATA, VSH_OFLAG_NONE, + N_("NUMA node selections to set")}, {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, @@ -5224,41 +5224,14 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) } for (i = 0; i < nparams; i++) { - switch (params[i].type) { - case VIR_TYPED_PARAM_INT: - vshPrint(ctl, "%-15s: %d\n", params[i].field, - params[i].value.i); - break; - case VIR_TYPED_PARAM_UINT: - vshPrint(ctl, "%-15s: %u\n", params[i].field, - params[i].value.ui); - break; - case VIR_TYPED_PARAM_LLONG: - vshPrint(ctl, "%-15s: %lld\n", params[i].field, - params[i].value.l); - break; - case VIR_TYPED_PARAM_ULLONG: - if (STREQ(params[i].field, VIR_DOMAIN_NUMA_MODE)) - vshPrint(ctl, "%-15s: %s\n", params[i].field, - virDomainNumatuneMemModeTypeToString(params[i].value.ul)); - else - vshPrint(ctl, "%-15s: %llu\n", params[i].field, - params[i].value.ul); - break; - case VIR_TYPED_PARAM_DOUBLE: - vshPrint(ctl, "%-15s: %f\n", params[i].field, - params[i].value.d); - break; - case VIR_TYPED_PARAM_BOOLEAN: - vshPrint(ctl, "%-15s: %d\n", params[i].field, - params[i].value.b); - break; - case VIR_TYPED_PARAM_STRING: - vshPrint(ctl, "%-15s: %s\n", params[i].field, - params[i].value.s); - break; - default: - vshPrint(ctl, "unimplemented numa parameter type\n"); + if (params[i].type == VIR_TYPED_PARAM_INT && + STREQ(params[i].field, VIR_DOMAIN_NUMA_MODE)) { + vshPrint(ctl, "%-15s: %s\n", params[i].field, + virDomainNumatuneMemModeTypeToString(params[i].value.i)); + } else { + char *str = vshGetTypedParamValue(ctl, ¶ms[i]); + vshPrint(ctl, "%-15s: %s\n", params[i].field, str); + VIR_FREE(str); } } @@ -5269,7 +5242,6 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) for (i = 0; i < nparams; i++) { temp = ¶ms[i]; - temp->type = VIR_TYPED_PARAM_ULLONG; /* * Some magic here, this is used to fill the params structure with @@ -5277,22 +5249,27 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) * argument we purposely make them 0, so on the next pass it goes * to the next valid argument and so on. */ - if (nodeset) { - temp->value.s = vshStrdup(ctl, nodeset); - temp->type = VIR_TYPED_PARAM_STRING; - if (!virStrcpy(temp->field, VIR_DOMAIN_NUMA_NODESET, - sizeof(temp->field))) - goto cleanup; - nodeset = NULL; - } else if (mode) { + if (mode) { + /* Accept string or integer, in case server + * understands newer integer than what strings we were + * compiled with */ if ((temp->value.i = virDomainNumatuneMemModeTypeFromString(mode)) < 0) { vshError(ctl, "%s %s", _("Invalid mode"), mode); goto cleanup; } - strncpy(temp->field, VIR_DOMAIN_NUMA_MODE, - sizeof(temp->field)); + if (!virStrcpy(temp->field, VIR_DOMAIN_NUMA_MODE, + sizeof(temp->field))) + goto cleanup; + temp->type = VIR_TYPED_PARAM_INT; mode = NULL; + } else if (nodeset) { + temp->value.s = vshStrdup(ctl, nodeset); + temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_NUMA_NODESET, + sizeof(temp->field))) + goto cleanup; + nodeset = NULL; } } if (virDomainSetNumaParameters(dom, params, nparams, flags) != 0) @@ -5302,6 +5279,7 @@ cmdNumatune(vshControl * ctl, const vshCmd * cmd) } cleanup: + virTypedParameterArrayClear(params, nparams); VIR_FREE(params); virDomainFree(dom); return ret; @@ -15412,7 +15390,6 @@ static const vshCmdDef domManagementCmds[] = { info_managedsaveremove, 0}, {"maxvcpus", cmdMaxvcpus, opts_maxvcpus, info_maxvcpus, 0}, {"memtune", cmdMemtune, opts_memtune, info_memtune, 0}, - {"numatune", cmdNumatune, opts_numatune, info_numatune, 0}, {"migrate", cmdMigrate, opts_migrate, info_migrate, 0}, {"migrate-setmaxdowntime", cmdMigrateSetMaxDowntime, opts_migrate_setmaxdowntime, info_migrate_setmaxdowntime, 0}, @@ -15420,6 +15397,7 @@ static const vshCmdDef domManagementCmds[] = { opts_migrate_setspeed, info_migrate_setspeed, 0}, {"migrate-getspeed", cmdMigrateGetMaxSpeed, opts_migrate_getspeed, info_migrate_getspeed, 0}, + {"numatune", cmdNumatune, opts_numatune, info_numatune, 0}, {"reboot", cmdReboot, opts_reboot, info_reboot, 0}, {"reset", cmdReset, opts_reset, info_reset, 0}, {"restore", cmdRestore, opts_restore, info_restore, 0}, -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On 12/15/2011 03:50 AM, Hu Tao wrote:

--- tools/virsh.pod | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)

This should be squashed in with 7/8.

diff --git a/tools/virsh.pod b/tools/virsh.pod index c468f13..91baf4d 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -832,6 +832,19 @@ migrated to another host.

Get the maximum migration bandwidth (in Mbps) for a domain.

+=item B<numatune> I<domain> +[I<--nodeset nodeset>] [I<--mode mode>]

I would swap these two.

+[[I<--config>] [I<--live>] | [I<--current>]] + +Set or get domain's numa parameters. + +I<nodeset> is a list of numa node.Its syntax is a comma separated list

Spacing.

+and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can also +be allowed. The '-' denotes the range and the '^' denotes exclusive. +I<mode> can be one of `strict', `interleave' and `preferred'. +For running domains(aka with I<--live>), the mode can't be changed, +and the nodeset can be changed only when the mode is `strict'.

Document --live and friends.

+ =item B<reboot> I<domain-id>

Reboot a domain. This acts just as if the domain had the B<reboot>

I'm using this instead: diff --git i/tools/virsh.pod w/tools/virsh.pod index 05c7f99..085f3aa 100644 --- i/tools/virsh.pod +++ w/tools/virsh.pod @@ -832,18 +832,23 @@ migrated to another host. Get the maximum migration bandwidth (in Mbps) for a domain. -=item B<numatune> I<domain> -[I<--nodeset nodeset>] [I<--mode mode>] +=item B<numatune> I<domain> [I<mode>] [I<nodeset>] [[I<--config>] [I<--live>] | [I<--current>]] -Set or get domain's numa parameters. +Set or get a domain's numa parameters, corresponding to the <numatune> +element of domain XML. -I<nodeset> is a list of numa node.Its syntax is a comma separated list -and a special markup using '-' and '^' (ex. '0-4', '0-3,^2') can also -be allowed. The '-' denotes the range and the '^' denotes exclusive. -I<mode> can be one of `strict', `interleave' and `preferred'. -For running domains(aka with I<--live>), the mode can't be changed, -and the nodeset can be changed only when the mode is `strict'. +I<mode> can be one of `strict', `interleave' and `preferred'. For a +running domain, the mode can't be changed, and the nodeset can be +changed only if the domain was started with a mode of `strict'. + +I<nodeset> is a list of numa nodes used by the host for running the domain. +Its syntax is a comma separated list, with '-' for ranges and '^' for +excluding a node. + +If I<--live> is specified, set scheduler information of a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. =item B<reboot> I<domain-id> -- Eric Blake eblake@redhat.com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

From: Eric Blake <eblake@redhat.com> https://www.redhat.com/archives/libvir-list/2011-December/msg00854.html --- daemon/remote.c | 22 ++++++++++------------ 1 files changed, 10 insertions(+), 12 deletions(-) diff --git a/daemon/remote.c b/daemon/remote.c index e1d208c..cb4e369 100644 --- a/daemon/remote.c +++ b/daemon/remote.c @@ -759,7 +759,7 @@ cleanup: if (val) { for (i = 0; i < nparams; i++) { VIR_FREE(val[i].field); - if (params[i].type == VIR_TYPED_PARAM_STRING) + if (val[i].value.type == VIR_TYPED_PARAM_STRING) VIR_FREE(val[i].value.remote_typed_param_value_u.s); } VIR_FREE(val); @@ -898,9 +898,10 @@ remoteDispatchDomainGetSchedulerParameters(virNetServerPtr server ATTRIBUTE_UNUS cleanup: if (rv < 0) virNetMessageSaveError(rerr); + virTypedParameterArrayClear(params, nparams); + VIR_FREE(params); if (dom) virDomainFree(dom); - VIR_FREE(params); return rv; no_memory: @@ -953,9 +954,10 @@ remoteDispatchDomainGetSchedulerParametersFlags(virNetServerPtr server ATTRIBUTE cleanup: if (rv < 0) virNetMessageSaveError(rerr); + virTypedParameterArrayClear(params, nparams); + VIR_FREE(params); if (dom) virDomainFree(dom); - VIR_FREE(params); return rv; no_memory: @@ -1140,17 +1142,12 @@ success: rv = 0; cleanup: - if (rv < 0) { + if (rv < 0) virNetMessageSaveError(rerr); - if (ret->params.params_val) { - for (i = 0; i < nparams; i++) - VIR_FREE(ret->params.params_val[i].field); - VIR_FREE(ret->params.params_val); - } - } + virTypedParameterArrayClear(params, nparams); + VIR_FREE(params); if (dom) virDomainFree(dom); - VIR_FREE(params); return rv; } @@ -1623,9 +1620,10 @@ success: cleanup: if (rv < 0) virNetMessageSaveError(rerr); + virTypedParameterArrayClear(params, nparams); + VIR_FREE(params); if (dom) virDomainFree(dom); - VIR_FREE(params); return rv; } -- 1.7.4.4

--- tools/virsh.c | 4 ++++ 1 files changed, 4 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 3654589..17919e9 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -17086,6 +17086,10 @@ vshGetTypedParamValue(vshControl *ctl, virTypedParameterPtr item) ret = virAsprintf(&str, "%s", item->value.b ? _("yes") : _("no")); break; + case VIR_TYPED_PARAM_STRING: + ret = virAsprintf(&str, "%s", item->value.s); + break; + default: vshError(ctl, _("unimplemented block statistics parameter type")); } -- 1.7.4.4

--- src/libvirt_private.syms | 2 ++ src/util/cgroup.c | 32 ++++++++++++++++++++++++++++++++ src/util/cgroup.h | 3 +++ 3 files changed, 37 insertions(+), 0 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index a649054..4a86bdc 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -78,6 +78,7 @@ virCgroupGetCpuShares; virCgroupGetCpuCfsPeriod; virCgroupGetCpuCfsQuota; virCgroupGetCpuacctUsage; +virCgroupGetCpusetMems; virCgroupGetFreezerState; virCgroupGetMemoryHardLimit; virCgroupGetMemorySoftLimit; @@ -94,6 +95,7 @@ virCgroupSetBlkioWeight; virCgroupSetCpuShares; virCgroupSetCpuCfsPeriod; virCgroupSetCpuCfsQuota; +virCgroupSetCpusetMems; virCgroupSetFreezerState; virCgroupSetMemory; virCgroupSetMemoryHardLimit; diff --git a/src/util/cgroup.c b/src/util/cgroup.c index b4d3d8b..25f2691 100644 --- a/src/util/cgroup.c +++ b/src/util/cgroup.c @@ -1205,6 +1205,38 @@ int virCgroupGetMemSwapHardLimit(virCgroupPtr group, unsigned long long *kb) } /** + * virCgroupSetCpusetMems: + * + * @group: The cgroup to set cpuset.mems for + * @mems: the numa nodes to set + * + * Returns: 0 on success + */ +int virCgroupSetCpusetMems(virCgroupPtr group, const char *mems) +{ + return virCgroupSetValueStr(group, + VIR_CGROUP_CONTROLLER_CPUSET, + "cpuset.mems", + mems); +} + +/** + * virCgroupGetCpusetMems: + * + * @group: The cgroup to get cpuset.mems for + * @mems: the numa nodes to get + * + * Returns: 0 on success + */ +int virCgroupGetCpusetMems(virCgroupPtr group, char **mems) +{ + return virCgroupGetValueStr(group, + VIR_CGROUP_CONTROLLER_CPUSET, + "cpuset.mems", + mems); +} + +/** * virCgroupDenyAllDevices: * * @group: The cgroup to deny all permissions, for all devices diff --git a/src/util/cgroup.h b/src/util/cgroup.h index 70dd392..8d75735 100644 --- a/src/util/cgroup.h +++ b/src/util/cgroup.h @@ -119,6 +119,9 @@ int virCgroupGetCpuacctUsage(virCgroupPtr group, unsigned long long *usage); int virCgroupSetFreezerState(virCgroupPtr group, const char *state); int virCgroupGetFreezerState(virCgroupPtr group, char **state); +int virCgroupSetCpusetMems(virCgroupPtr group, const char *mems); +int virCgroupGetCpusetMems(virCgroupPtr group, char **mems); + int virCgroupRemove(virCgroupPtr group); void virCgroupFree(virCgroupPtr *group); -- 1.7.4.4

Set up the types for the numa functions and insert them into the virDriver structure definition. --- include/libvirt/libvirt.h.in | 39 +++++++++++++ python/generator.py | 2 + src/conf/domain_conf.h | 8 --- src/driver.h | 15 +++++ src/libvirt.c | 129 ++++++++++++++++++++++++++++++++++++++++++ src/libvirt_public.syms | 6 ++ 6 files changed, 191 insertions(+), 8 deletions(-) diff --git a/include/libvirt/libvirt.h.in b/include/libvirt/libvirt.h.in index 2480add..7f26521 100644 --- a/include/libvirt/libvirt.h.in +++ b/include/libvirt/libvirt.h.in @@ -1344,6 +1344,45 @@ typedef enum { } virDomainMemoryModFlags; +/* Manage numa parameters */ + +/** + * virDomainNumatuneMemMode: + * Representation of the various modes in the <numatune> element of + * a domain. + */ +typedef enum { + VIR_DOMAIN_NUMATUNE_MEM_STRICT = 0, + VIR_DOMAIN_NUMATUNE_MEM_PREFERRED = 1, + VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE = 2, + + /* This constant is subject to change */ + VIR_DOMAIN_NUMATUNE_MEM_LAST +} virDomainNumatuneMemMode; + +/** + * VIR_DOMAIN_NUMA_NODESET: + * + * Macro for typed parameter name that lists the numa nodeset of a + * domain, as a string. + */ +#define VIR_DOMAIN_NUMA_NODESET "numa_nodeset" + +/** + * VIR_DOMAIN_NUMA_MODE: + * + * Macro for typed parameter name that lists the numa mode of a domain, + * as an int containing a virDomainNumatuneMemMode value. + */ +#define VIR_DOMAIN_NUMA_MODE "numa_mode" + +int virDomainSetNumaParameters(virDomainPtr domain, + virTypedParameterPtr params, + int nparams, unsigned int flags); +int virDomainGetNumaParameters(virDomainPtr domain, + virTypedParameterPtr params, + int *nparams, unsigned int flags); + /* * Dynamic control of domains */ diff --git a/python/generator.py b/python/generator.py index 1657f4f..181f70e 100755 --- a/python/generator.py +++ b/python/generator.py @@ -383,6 +383,8 @@ skip_impl = ( 'virDomainGetBlkioParameters', 'virDomainSetMemoryParameters', 'virDomainGetMemoryParameters', + 'virDomainSetNumaParameters', + 'virDomainGetNumaParameters', 'virDomainGetVcpus', 'virDomainPinVcpu', 'virDomainPinVcpuFlags', diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 3229a6f..189b8f6 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1355,14 +1355,6 @@ virDomainVcpuPinDefPtr virDomainVcpuPinFindByVcpu(virDomainVcpuPinDefPtr *def, int nvcpupin, int vcpu); -enum virDomainNumatuneMemMode { - VIR_DOMAIN_NUMATUNE_MEM_STRICT, - VIR_DOMAIN_NUMATUNE_MEM_PREFERRED, - VIR_DOMAIN_NUMATUNE_MEM_INTERLEAVE, - - VIR_DOMAIN_NUMATUNE_MEM_LAST -}; - typedef struct _virDomainNumatuneDef virDomainNumatuneDef; typedef virDomainNumatuneDef *virDomainNumatuneDefPtr; struct _virDomainNumatuneDef { diff --git a/src/driver.h b/src/driver.h index 941ff51..bbd6417 100644 --- a/src/driver.h +++ b/src/driver.h @@ -158,6 +158,19 @@ typedef int int *nparams, unsigned int flags); typedef int + (*virDrvDomainSetNumaParameters) + (virDomainPtr domain, + virTypedParameterPtr params, + int nparams, + unsigned int flags); +typedef int + (*virDrvDomainGetNumaParameters) + (virDomainPtr domain, + virTypedParameterPtr params, + int *nparams, + unsigned int flags); + +typedef int (*virDrvDomainSetBlkioParameters) (virDomainPtr domain, virTypedParameterPtr params, @@ -816,6 +829,8 @@ struct _virDriver { virDrvDomainSetMemoryFlags domainSetMemoryFlags; virDrvDomainSetMemoryParameters domainSetMemoryParameters; virDrvDomainGetMemoryParameters domainGetMemoryParameters; + virDrvDomainSetNumaParameters domainSetNumaParameters; + virDrvDomainGetNumaParameters domainGetNumaParameters; virDrvDomainSetBlkioParameters domainSetBlkioParameters; virDrvDomainGetBlkioParameters domainGetBlkioParameters; virDrvDomainGetInfo domainGetInfo; diff --git a/src/libvirt.c b/src/libvirt.c index 9977915..574be16 100644 --- a/src/libvirt.c +++ b/src/libvirt.c @@ -3769,6 +3769,135 @@ error: } /** + * virDomainSetNumaParameters: + * @domain: pointer to domain object + * @params: pointer to numa parameter objects + * @nparams: number of numa parameters (this value can be the same or + * less than the number of parameters supported) + * @flags: bitwise-OR of virDomainModificationImpact + * + * Change all or a subset of the numa tunables. + * This function may require privileged access to the hypervisor. + * + * Returns -1 in case of error, 0 in case of success. + */ +int +virDomainSetNumaParameters(virDomainPtr domain, + virTypedParameterPtr params, + int nparams, unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d, flags=%x", + params, nparams, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + if (domain->conn->flags & VIR_CONNECT_RO) { + virLibDomainError(VIR_ERR_OPERATION_DENIED, __FUNCTION__); + goto error; + } + if ((nparams <= 0) || (params == NULL)) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + if (virTypedParameterValidateSet(domain, params, nparams) < 0) + return -1; + + conn = domain->conn; + + if (conn->driver->domainSetNumaParameters) { + int ret; + ret = conn->driver->domainSetNumaParameters(domain, params, nparams, + flags); + if (ret < 0) + goto error; + return ret; + } + + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(domain->conn); + return -1; +} + +/** + * virDomainGetNumaParameters: + * @domain: pointer to domain object + * @params: pointer to numa parameter object + * (return value, allocated by the caller) + * @nparams: pointer to number of numa parameters + * @flags: bitwise-OR of virDomainModificationImpact and virTypedParameterFlags + * + * Get all numa parameters. On input, @nparams gives the size of the + * @params array; on output, @nparams gives how many slots were filled + * with parameter information, which might be less but will not exceed + * the input value. + * + * As a special case, calling with @params as NULL and @nparams as 0 on + * input will cause @nparams on output to contain the number of parameters + * supported by the hypervisor. The caller should then allocate @params + * array, i.e. (sizeof(@virTypedParameter) * @nparams) bytes and call the API + * again. + * + * See virDomainGetMemoryParameters() for an equivalent usage example. + * + * This function may require privileged access to the hypervisor. This function + * expects the caller to allocate the @params. + * + * Returns -1 in case of error, 0 in case of success. + */ + +int +virDomainGetNumaParameters(virDomainPtr domain, + virTypedParameterPtr params, + int *nparams, unsigned int flags) +{ + virConnectPtr conn; + + VIR_DOMAIN_DEBUG(domain, "params=%p, nparams=%d, flags=%x", + params, (nparams) ? *nparams : -1, flags); + + virResetLastError(); + + if (!VIR_IS_CONNECTED_DOMAIN(domain)) { + virLibDomainError(VIR_ERR_INVALID_DOMAIN, __FUNCTION__); + virDispatchError(NULL); + return -1; + } + if ((nparams == NULL) || (*nparams < 0) || + (params == NULL && *nparams != 0)) { + virLibDomainError(VIR_ERR_INVALID_ARG, __FUNCTION__); + goto error; + } + if (VIR_DRV_SUPPORTS_FEATURE(domain->conn->driver, domain->conn, + VIR_DRV_FEATURE_TYPED_PARAM_STRING)) + flags |= VIR_TYPED_PARAM_STRING_OKAY; + + conn = domain->conn; + + if (conn->driver->domainGetNumaParameters) { + int ret; + ret = conn->driver->domainGetNumaParameters(domain, params, nparams, + flags); + if (ret < 0) + goto error; + return ret; + } + virLibConnError(VIR_ERR_NO_SUPPORT, __FUNCTION__); + +error: + virDispatchError(domain->conn); + return -1; +} + +/** * virDomainSetBlkioParameters: * @domain: pointer to domain object * @params: pointer to blkio parameter objects diff --git a/src/libvirt_public.syms b/src/libvirt_public.syms index 164039a..3709f08 100644 --- a/src/libvirt_public.syms +++ b/src/libvirt_public.syms @@ -508,4 +508,10 @@ LIBVIRT_0.9.8 { virNodeSuspendForDuration; } LIBVIRT_0.9.7; +LIBVIRT_0.9.9 { + global: + virDomainGetNumaParameters; + virDomainSetNumaParameters; +} LIBVIRT_0.9.8; + # .... define new API here using predicted next version number .... -- 1.7.4.4

--- src/qemu/qemu_driver.c | 273 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 273 insertions(+), 0 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 660d771..bb03c5c 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -97,6 +97,8 @@ #define QEMU_NB_BLOCK_IO_TUNE_PARAM 6 +#define QEMU_NB_NUMA_PARAM 2 + #if HAVE_LINUX_KVM_H # include <linux/kvm.h> #endif @@ -6602,6 +6604,275 @@ cleanup: } static int +qemuDomainSetNumaParameters(virDomainPtr dom, + virTypedParameterPtr params, + int nparams, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + int i; + virDomainDefPtr persistentDef = NULL; + virCgroupPtr group = NULL; + virDomainObjPtr vm = NULL; + int ret = -1; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG, -1); + + qemuDriverLock(driver); + + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + + if (vm == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), dom->uuid); + goto cleanup; + } + + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) + goto cleanup; + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPUSET)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cgroup cpuset controller is not mounted")); + goto cleanup; + } + + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find cgroup for domain %s"), + vm->def->name); + goto cleanup; + } + } + + ret = 0; + for (i = 0; i < nparams; i++) { + virTypedParameterPtr param = ¶ms[i]; + + if (STREQ(param->field, VIR_DOMAIN_NUMA_MODE)) { + if (param->type != VIR_TYPED_PARAM_INT) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for numa strict tunable, " + "expected an 'int'")); + ret = -1; + continue; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("can't change numa mode for running domain")); + ret = -1; + goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + persistentDef->numatune.memory.mode = params[i].value.i; + } + } else if (STREQ(param->field, VIR_DOMAIN_NUMA_NODESET)) { + int rc; + char oldnodemask[VIR_DOMAIN_CPUMASK_LEN]; + if (param->type != VIR_TYPED_PARAM_STRING) { + qemuReportError(VIR_ERR_INVALID_ARG, "%s", + _("invalid type for numa nodeset tunable, " + "expected a 'string'")); + ret = -1; + continue; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (vm->def->numatune.memory.mode != + VIR_DOMAIN_NUMATUNE_MEM_STRICT) { + qemuReportError(VIR_ERR_OPERATION_INVALID, "%s", + _("change of nodeset for running domain " + "requires strict numa mode")); + ret = -1; + continue; + } + rc = virCgroupSetCpusetMems(group, params[i].value.s); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to set numa tunable")); + ret = -1; + continue; + } + + /* update vm->def here so that dumpxml can read the new + * values from vm->def. */ + memcpy(oldnodemask, vm->def->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN); + if (virDomainCpuSetParse(params[i].value.s, + 0, + vm->def->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN) < 0) { + memcpy(vm->def->numatune.memory.nodemask, + oldnodemask, VIR_DOMAIN_CPUMASK_LEN); + ret = -1; + continue; + } + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + memcpy(oldnodemask, persistentDef->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN); + if (virDomainCpuSetParse(params[i].value.s, + 0, + persistentDef->numatune.memory.nodemask, + VIR_DOMAIN_CPUMASK_LEN) < 0) { + memcpy(persistentDef->numatune.memory.nodemask, + oldnodemask, VIR_DOMAIN_CPUMASK_LEN); + ret = -1; + continue; + } + } + } else { + qemuReportError(VIR_ERR_INVALID_ARG, + _("Parameter `%s' not supported"), param->field); + ret = -1; + } + } + + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + if (virDomainSaveConfig(driver->configDir, persistentDef) < 0) + ret = -1; + } + +cleanup: + virCgroupFree(&group); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int +qemuDomainGetNumaParameters(virDomainPtr dom, + virTypedParameterPtr params, + int *nparams, + unsigned int flags) +{ + struct qemud_driver *driver = dom->conn->privateData; + int i; + virCgroupPtr group = NULL; + virDomainObjPtr vm = NULL; + virDomainDefPtr persistentDef = NULL; + char *nodeset = NULL; + int ret = -1; + int rc; + + virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | + VIR_DOMAIN_AFFECT_CONFIG | + VIR_TYPED_PARAM_STRING_OKAY, -1); + + qemuDriverLock(driver); + + /* We blindly return a string, and let libvirt.c and + * remote_driver.c do the filtering on behalf of older clients + * that can't parse it. */ + flags &= ~VIR_TYPED_PARAM_STRING_OKAY; + + vm = virDomainFindByUUID(&driver->domains, dom->uuid); + + if (vm == NULL) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("No such domain %s"), dom->uuid); + goto cleanup; + } + + if (virDomainLiveConfigHelperMethod(driver->caps, vm, &flags, + &persistentDef) < 0) + goto cleanup; + + if ((*nparams) == 0) { + *nparams = QEMU_NB_NUMA_PARAM; + ret = 0; + goto cleanup; + } + + if (flags & VIR_DOMAIN_AFFECT_LIVE) { + if (!qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) { + qemuReportError(VIR_ERR_OPERATION_INVALID, + "%s", _("cgroup memory controller is not mounted")); + goto cleanup; + } + + if (virCgroupForDomain(driver->cgroup, vm->def->name, &group, 0) != 0) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("cannot find cgroup for domain %s"), + vm->def->name); + goto cleanup; + } + } + + for (i = 0; i < QEMU_NB_NUMA_PARAM && i < *nparams; i++) { + virMemoryParameterPtr param = ¶ms[i]; + + switch (i) { + case 0: /* fill numa mode here */ + if (!virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_MODE)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field '%s' too long for destination"), + VIR_DOMAIN_NUMA_MODE); + goto cleanup; + } + param->type = VIR_TYPED_PARAM_INT; + if (flags & VIR_DOMAIN_AFFECT_CONFIG) + param->value.i = persistentDef->numatune.memory.mode; + else + param->value.i = vm->def->numatune.memory.mode; + break; + + case 1: /* fill numa nodeset here */ + if (!virStrcpyStatic(param->field, VIR_DOMAIN_NUMA_NODESET)) { + qemuReportError(VIR_ERR_INTERNAL_ERROR, + _("Field '%s' too long for destination"), + VIR_DOMAIN_NUMA_NODESET); + goto cleanup; + } + if (flags & VIR_DOMAIN_AFFECT_CONFIG) { + char *mask = persistentDef->numatune.memory.nodemask; + if (mask) + nodeset = virDomainCpuSetFormat(mask, + VIR_DOMAIN_CPUMASK_LEN); + else + nodeset = strdup(""); + } else { + rc = virCgroupGetCpusetMems(group, &nodeset); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to get numa nodeset")); + goto cleanup; + } + } + if (!nodeset) { + virReportOOMError(); + goto cleanup; + } + param->type = VIR_TYPED_PARAM_STRING; + param->value.s = nodeset; + break; + + default: + break; + /* should not hit here */ + } + } + + if (*nparams > QEMU_NB_NUMA_PARAM) + *nparams = QEMU_NB_NUMA_PARAM; + ret = 0; + +cleanup: + virCgroupFree(&group); + if (vm) + virDomainObjUnlock(vm); + qemuDriverUnlock(driver); + return ret; +} + +static int qemuSetVcpusBWLive(virDomainObjPtr vm, virCgroupPtr cgroup, unsigned long long period, long long quota) { @@ -11362,6 +11633,8 @@ static virDriver qemuDriver = { .nodeSuspendForDuration = nodeSuspendForDuration, /* 0.9.8 */ .domainSetBlockIoTune = qemuDomainSetBlockIoTune, /* 0.9.8 */ .domainGetBlockIoTune = qemuDomainGetBlockIoTune, /* 0.9.8 */ + .domainSetNumaParameters = qemuDomainSetNumaParameters, /* 0.9.9 */ + .domainGetNumaParameters = qemuDomainGetNumaParameters, /* 0.9.9 */ }; -- 1.7.4.4

add new command numatune to virsh to get/set numa parameters --- tools/virsh.c | 159 +++++++++++++++++++++++++++++++++++++++++++++++++++++++ tools/virsh.pod | 19 +++++++ 2 files changed, 178 insertions(+), 0 deletions(-) diff --git a/tools/virsh.c b/tools/virsh.c index 17919e9..45b7658 100644 --- a/tools/virsh.c +++ b/tools/virsh.c @@ -61,6 +61,7 @@ #include "virkeycode.h" #include "virnetdevbandwidth.h" #include "util/bitmap.h" +#include "conf/domain_conf.h" static char *progname; @@ -5199,6 +5200,163 @@ cmdMemtune(vshControl * ctl, const vshCmd * cmd) } /* + * "numatune" command + */ +static const vshCmdInfo info_numatune[] = { + {"help", N_("Get or set numa parameters")}, + {"desc", N_("Get or set the current numa parameters for a guest" \ + " domain.\n" \ + " To get the numa parameters use following command: \n\n" \ + " virsh # numatune <domain>")}, + {NULL, NULL} + +}; + +static const vshCmdOptDef opts_numatune[] = { + {"domain", VSH_OT_DATA, VSH_OFLAG_REQ, N_("domain name, id or uuid")}, + {"mode", VSH_OT_DATA, VSH_OFLAG_NONE, + N_("NUMA mode, one of strict, preferred and interleave")}, + {"nodeset", VSH_OT_DATA, VSH_OFLAG_NONE, + N_("NUMA node selections to set")}, + {"config", VSH_OT_BOOL, 0, N_("affect next boot")}, + {"live", VSH_OT_BOOL, 0, N_("affect running domain")}, + {"current", VSH_OT_BOOL, 0, N_("affect current domain")}, + {NULL, 0, 0, NULL} +}; + +static bool +cmdNumatune(vshControl * ctl, const vshCmd * cmd) +{ + virDomainPtr dom; + int nparams = 0; + unsigned int i = 0; + virTypedParameterPtr params = NULL, temp = NULL; + const char *nodeset = NULL; + bool ret = false; + unsigned int flags = 0; + int current = vshCommandOptBool(cmd, "current"); + int config = vshCommandOptBool(cmd, "config"); + int live = vshCommandOptBool(cmd, "live"); + const char *mode = NULL; + + if (current) { + if (live || config) { + vshError(ctl, "%s", _("--current must be specified exclusively")); + return false; + } + flags = VIR_DOMAIN_AFFECT_CURRENT; + } else { + if (config) + flags |= VIR_DOMAIN_AFFECT_CONFIG; + if (live) + flags |= VIR_DOMAIN_AFFECT_LIVE; + } + + if (!vshConnectionUsability(ctl, ctl->conn)) + return false; + + if (!(dom = vshCommandOptDomain(ctl, cmd, NULL))) + return false; + + if (vshCommandOptString(cmd, "nodeset", &nodeset) < 0) { + vshError(ctl, "%s", _("Unable to parse nodeset.")); + virDomainFree(dom); + return false; + } + if (nodeset) + nparams++; + if (vshCommandOptString(cmd, "mode", &mode) < 0) { + vshError(ctl, "%s", _("Unable to parse mode.")); + virDomainFree(dom); + return false; + } + if (mode) + nparams++; + + if (nparams == 0) { + /* get the number of numa parameters */ + if (virDomainGetNumaParameters(dom, NULL, &nparams, flags) != 0) { + vshError(ctl, "%s", + _("Unable to get number of memory parameters")); + goto cleanup; + } + + if (nparams == 0) { + /* nothing to output */ + ret = true; + goto cleanup; + } + + /* now go get all the numa parameters */ + params = vshCalloc(ctl, nparams, sizeof(*params)); + if (virDomainGetNumaParameters(dom, params, &nparams, flags) != 0) { + vshError(ctl, "%s", _("Unable to get numa parameters")); + goto cleanup; + } + + for (i = 0; i < nparams; i++) { + if (params[i].type == VIR_TYPED_PARAM_INT && + STREQ(params[i].field, VIR_DOMAIN_NUMA_MODE)) { + vshPrint(ctl, "%-15s: %s\n", params[i].field, + virDomainNumatuneMemModeTypeToString(params[i].value.i)); + } else { + char *str = vshGetTypedParamValue(ctl, ¶ms[i]); + vshPrint(ctl, "%-15s: %s\n", params[i].field, str); + VIR_FREE(str); + } + } + + ret = true; + } else { + /* set the numa parameters */ + params = vshCalloc(ctl, nparams, sizeof(*params)); + + for (i = 0; i < nparams; i++) { + temp = ¶ms[i]; + + /* + * Some magic here, this is used to fill the params structure with + * the valid arguments passed, after filling the particular + * argument we purposely make them 0, so on the next pass it goes + * to the next valid argument and so on. + */ + if (mode) { + /* Accept string or integer, in case server + * understands newer integer than what strings we were + * compiled with */ + if ((temp->value.i = + virDomainNumatuneMemModeTypeFromString(mode)) < 0) { + vshError(ctl, "%s %s", _("Invalid mode"), mode); + goto cleanup; + } + if (!virStrcpy(temp->field, VIR_DOMAIN_NUMA_MODE, + sizeof(temp->field))) + goto cleanup; + temp->type = VIR_TYPED_PARAM_INT; + mode = NULL; + } else if (nodeset) { + temp->value.s = vshStrdup(ctl, nodeset); + temp->type = VIR_TYPED_PARAM_STRING; + if (!virStrcpy(temp->field, VIR_DOMAIN_NUMA_NODESET, + sizeof(temp->field))) + goto cleanup; + nodeset = NULL; + } + } + if (virDomainSetNumaParameters(dom, params, nparams, flags) != 0) + vshError(ctl, "%s", _("Unable to change numa parameters")); + else + ret = true; + } + + cleanup: + virTypedParameterArrayClear(params, nparams); + VIR_FREE(params); + virDomainFree(dom); + return ret; +} + +/* * "nodeinfo" command */ static const vshCmdInfo info_nodeinfo[] = { @@ -15335,6 +15493,7 @@ static const vshCmdDef domManagementCmds[] = { opts_migrate_setspeed, info_migrate_setspeed, 0}, {"migrate-getspeed", cmdMigrateGetMaxSpeed, opts_migrate_getspeed, info_migrate_getspeed, 0}, + {"numatune", cmdNumatune, opts_numatune, info_numatune, 0}, {"reboot", cmdReboot, opts_reboot, info_reboot, 0}, {"reset", cmdReset, opts_reset, info_reset, 0}, {"restore", cmdRestore, opts_restore, info_restore, 0}, diff --git a/tools/virsh.pod b/tools/virsh.pod index dbe5165..2367673 100644 --- a/tools/virsh.pod +++ b/tools/virsh.pod @@ -832,6 +832,25 @@ migrated to another host. Get the maximum migration bandwidth (in Mbps) for a domain. +=item B<numatune> I<domain> [I<mode>] [I<nodeset>] +[[I<--config>] [I<--live>] | [I<--current>]] + +Set or get a domain's numa parameters, corresponding to the <numatune> +element of domain XML. Without flags, the current settings are +displayed. + +I<mode> can be one of `strict', `interleave' and `preferred'. For a +running domain, the mode can't be changed, and the nodeset can be +changed only if the domain was started with a mode of `strict'. + +I<nodeset> is a list of numa nodes used by the host for running the domain. +Its syntax is a comma separated list, with '-' for ranges and '^' for +excluding a node. + +If I<--live> is specified, set scheduler information of a running guest. +If I<--config> is specified, affect the next boot of a persistent guest. +If I<--current> is specified, affect the current guest state. + =item B<reboot> I<domain-id> Reboot a domain. This acts just as if the domain had the B<reboot> -- 1.7.4.4

the removal of unused variable i also removes codes like this: if (!xdr_int (xdrs, &objp->remote_typed_param_value_u.i)) which should not be removed. Sorry I'm not familiar with perl and can't find a perfect way to deal with this. --- src/rpc/genprotocol.pl | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/src/rpc/genprotocol.pl b/src/rpc/genprotocol.pl index 7af1b3b..cec93d8 100755 --- a/src/rpc/genprotocol.pl +++ b/src/rpc/genprotocol.pl @@ -72,7 +72,7 @@ while (<RPCGEN>) { # Remove decl of i, if i isn't used in the function. @uses = grep /\bi\b/, @function; - @function = grep !/\bi\b/, @function if @uses == 1; + #@function = grep !/\bi\b/, @function if @uses == 1; # (char **)&objp->... gives: # warning: dereferencing type-punned pointer will break -- 1.7.4.4