[libvirt] [PATCH v2] qemu: Set swap_hard_limit before hard_limit
Setting hard_limit larger than previous swap_hard_limit must fail, it's not that good if one wants to change the swap_hard_limit and hard_limit together. E.g. % virsh memtune rhel6 hard_limit : 1000000 soft_limit : 1000000 swap_hard_limit: 1000000 % virsh memtune rhel6 --hard-limit 1000020 --soft-limit 1000020 \ --swap-hard-limit 1000020 --live This patch reoder the limits setting to set the swap_hard_limit first, hard_limit then, and soft_limit last if it's greater than current swap_hard_limit. And soft_limit first, hard_limit then, swap_hard_limit last, if not. --- src/qemu/qemu_driver.c | 59 +++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 51 insertions(+), 8 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 80cfa84..a26d3cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6651,11 +6651,18 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; + virTypedParameterPtr hard_limit = NULL; + virTypedParameterPtr soft_limit = NULL; + virTypedParameterPtr swap_hard_limit = NULL; + virTypedParameterPtr sorted_params[nparams]; + unsigned long long val = 0; + int ret = -1; int rc; virCheckFlags(VIR_DOMAIN_AFFECT_LIVE | VIR_DOMAIN_AFFECT_CONFIG, -1); + if (virTypedParameterArrayValidate(params, nparams, VIR_DOMAIN_MEMORY_HARD_LIMIT, VIR_TYPED_PARAM_ULLONG, @@ -6694,13 +6701,49 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, } } + for (i = 0; i < nparams; i++) { + if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_HARD_LIMIT)) + hard_limit = ¶ms[i]; + else if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_SOFT_LIMIT)) + soft_limit = ¶ms[i]; + else if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) + swap_hard_limit = ¶ms[i]; + } + + /* It will fail if hard limit greater than swap hard limit anyway */ + if (swap_hard_limit && + hard_limit && + (hard_limit->value.ul > swap_hard_limit->value.ul)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("hard limit must be lower than swap hard limit")); + goto cleanup; + } + + /* Get current swap hard limit */ + rc = virCgroupGetMemSwapHardLimit(group, &val); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to get swap hard limit")); + goto cleanup; + } + + if (swap_hard_limit->value.ul > val) { + sorted_params[0] = swap_hard_limit; + sorted_params[1] = hard_limit; + sorted_params[2] = soft_limit; + } else { + sorted_params[0] = soft_limit; + sorted_params[1] = hard_limit; + sorted_params[2] = swap_hard_limit; + } + ret = 0; for (i = 0; i < nparams; i++) { - virTypedParameterPtr param = ¶ms[i]; + virTypedParameterPtr param = sorted_params[i]; if (STREQ(param->field, VIR_DOMAIN_MEMORY_HARD_LIMIT)) { if (flags & VIR_DOMAIN_AFFECT_LIVE) { - rc = virCgroupSetMemoryHardLimit(group, params[i].value.ul); + rc = virCgroupSetMemoryHardLimit(group, sorted_params[i]->value.ul); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to set memory hard_limit tunable")); @@ -6709,11 +6752,11 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentDef->mem.hard_limit = params[i].value.ul; + persistentDef->mem.hard_limit = sorted_params[i]->value.ul; } } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SOFT_LIMIT)) { if (flags & VIR_DOMAIN_AFFECT_LIVE) { - rc = virCgroupSetMemorySoftLimit(group, params[i].value.ul); + rc = virCgroupSetMemorySoftLimit(group, sorted_params[i]->value.ul); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to set memory soft_limit tunable")); @@ -6722,11 +6765,11 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentDef->mem.soft_limit = params[i].value.ul; + persistentDef->mem.soft_limit = sorted_params[i]->value.ul; } } else if (STREQ(param->field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) { if (flags & VIR_DOMAIN_AFFECT_LIVE) { - rc = virCgroupSetMemSwapHardLimit(group, params[i].value.ul); + rc = virCgroupSetMemSwapHardLimit(group, sorted_params[i]->value.ul); if (rc != 0) { virReportSystemError(-rc, "%s", _("unable to set swap_hard_limit tunable")); @@ -6734,7 +6777,7 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, } } if (flags & VIR_DOMAIN_AFFECT_CONFIG) { - persistentDef->mem.swap_hard_limit = params[i].value.ul; + persistentDef->mem.swap_hard_limit = sorted_params[i]->value.ul; } } } @@ -7011,7 +7054,7 @@ qemuDomainSetNumaParameters(virDomainPtr dom, continue; } - /* Ensure the cpuset string is formated before passing to cgroup */ + /* Ensure the cpuset string is formatted before passing to cgroup */ if (!(nodeset_str = virDomainCpuSetFormat(nodeset, VIR_DOMAIN_CPUMASK_LEN))) { virReportError(VIR_ERR_INTERNAL_ERROR, "%s", -- 1.7.7.3
On 08/16/2012 12:37 PM, Osier Yang wrote:
Setting hard_limit larger than previous swap_hard_limit must fail, it's not that good if one wants to change the swap_hard_limit and hard_limit together. E.g.
% virsh memtune rhel6 hard_limit : 1000000 soft_limit : 1000000 swap_hard_limit: 1000000
% virsh memtune rhel6 --hard-limit 1000020 --soft-limit 1000020 \ --swap-hard-limit 1000020 --live
This patch reoder the limits setting to set the swap_hard_limit first, hard_limit then, and soft_limit last if it's greater than current swap_hard_limit. And soft_limit first, hard_limit then, swap_hard_limit last, if not. --- src/qemu/qemu_driver.c | 59 +++++++++++++++++++++++++++++++++++++++++------ 1 files changed, 51 insertions(+), 8 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 80cfa84..a26d3cd 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6651,11 +6651,18 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, virDomainDefPtr persistentDef = NULL; virCgroupPtr group = NULL; virDomainObjPtr vm = NULL; + virTypedParameterPtr hard_limit = NULL; + virTypedParameterPtr soft_limit = NULL; + virTypedParameterPtr swap_hard_limit = NULL; + virTypedParameterPtr sorted_params[nparams];
You allocate the correct number of params here, but ...
@@ -6694,13 +6701,49 @@ qemuDomainSetMemoryParameters(virDomainPtr dom, } }
+ for (i = 0; i < nparams; i++) { + if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_HARD_LIMIT)) + hard_limit = ¶ms[i]; + else if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_SOFT_LIMIT)) + soft_limit = ¶ms[i]; + else if (STREQ(params[i].field, VIR_DOMAIN_MEMORY_SWAP_HARD_LIMIT)) + swap_hard_limit = ¶ms[i]; + } + + /* It will fail if hard limit greater than swap hard limit anyway */ + if (swap_hard_limit && + hard_limit && + (hard_limit->value.ul > swap_hard_limit->value.ul)) { + virReportError(VIR_ERR_INVALID_ARG, "%s", + _("hard limit must be lower than swap hard limit")); + goto cleanup; + } + + /* Get current swap hard limit */ + rc = virCgroupGetMemSwapHardLimit(group, &val); + if (rc != 0) { + virReportSystemError(-rc, "%s", + _("unable to get swap hard limit")); + goto cleanup; + } + + if (swap_hard_limit->value.ul > val) { + sorted_params[0] = swap_hard_limit; + sorted_params[1] = hard_limit; + sorted_params[2] = soft_limit; + } else { + sorted_params[0] = soft_limit; + sorted_params[1] = hard_limit; + sorted_params[2] = swap_hard_limit; + }
... you always access all 3 of them and you go through this list. Unfortunately this means that in case you want to change only one value (e.g. hard limit), you will dereference NULL couple of lines later and it will crash libvirtd (tried). To test this, you can try running 'virsh memtune rhel6 --hard-limit 1000020' in your previous scenario. Martin.
participants (2)
-
Martin Kletzander -
Osier Yang