[libvirt] [PATCH v2] domain: fix parsing of memory tunables on 32-bit machines
Commit 6c9a8a4 (Oct 2014) exposed a long-standing issue on 32-bit machines: code related to virDomainSetMemoryParameters has always been documented as using a 64-bit limit, but it was implemented by calling virDomainParseMemory which enforced an 'unsigned long' limit. Since VIR_DOMAIN_MEMORY_PARAM_UNLIMITED capped to a long is -1, but virDomainParseScaledValue no longer accepts negative values, an attempt to use 2^53-1 as a hard memory limit started failing the testsuite. However, the problem with capping things artificially low has existed for much longer - ever since commits 4888f0fb and 2e22f23 (Mar 2012) switched internal tracking from 'unsigned long' to 'unsigned long long' (prior to that time, the cap was a side-effect of the choice of types). We _have_ to cap the balloon memory values, (no thanks to baked in 'unsigned long' of API such as virDomainSetMaxMemory or virDomainGetInfo with no counterpart API that guarantees 64-bit access to those numbers) but memory parameters have never needed the artificial limit. At any rate, the solution is to make the parser function gain a parameter, and only do the reduced 32-bit cap for the values that are constrained due to API. * src/conf/domain_conf.h (_virDomainMemtune): Add comments. * src/conf/domain_conf.c (virDomainParseMemory): Add parameter. (virDomainDefParseXML): Adjust callers. Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 25 ++++++++++++++----------- src/conf/domain_conf.h | 14 ++++++++------ 2 files changed, 22 insertions(+), 17 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 39befb0..c594325 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6373,19 +6373,22 @@ virDomainParseScaledValue(const char *xpath, /* Parse a memory element located at XPATH within CTXT, and store the - * result into MEM. If REQUIRED, then the value must exist; - * otherwise, the value is optional. The value is in blocks of 1024. - * Return 0 on success, -1 on failure after issuing error. */ + * result into MEM, in blocks of 1024. If REQUIRED, then the value + * must exist; otherwise, the value is optional. The value must not + * exceed VIR_DOMAIN_MEMORY_PARAM_UNLIMITED once scaled; additionally, + * if CAPPED is true, the value must fit within an unsigned long (only + * matters on 32-bit platforms). Return 0 on success, -1 on failure + * after issuing error. */ static int virDomainParseMemory(const char *xpath, xmlXPathContextPtr ctxt, - unsigned long long *mem, bool required) + unsigned long long *mem, bool required, bool capped) { int ret = -1; unsigned long long bytes, max; /* On 32-bit machines, our bound is 0xffffffff * KiB. On 64-bit * machines, our bound is off_t (2^63). */ - if (sizeof(unsigned long) < sizeof(long long)) + if (capped && sizeof(unsigned long) < sizeof(long long)) max = 1024ull * ULONG_MAX; else max = LLONG_MAX; @@ -12215,11 +12218,11 @@ virDomainDefParseXML(xmlDocPtr xml, /* Extract domain memory */ if (virDomainParseMemory("./memory[1]", ctxt, - &def->mem.max_balloon, true) < 0) + &def->mem.max_balloon, true, true) < 0) goto error; if (virDomainParseMemory("./currentMemory[1]", ctxt, - &def->mem.cur_balloon, false) < 0) + &def->mem.cur_balloon, false, true) < 0) goto error; /* and info about it */ @@ -12339,19 +12342,19 @@ virDomainDefParseXML(xmlDocPtr xml, /* Extract other memory tunables */ if (virDomainParseMemory("./memtune/hard_limit[1]", ctxt, - &def->mem.hard_limit, false) < 0) + &def->mem.hard_limit, false, false) < 0) goto error; if (virDomainParseMemory("./memtune/soft_limit[1]", ctxt, - &def->mem.soft_limit, false) < 0) + &def->mem.soft_limit, false, false) < 0) goto error; if (virDomainParseMemory("./memtune/min_guarantee[1]", ctxt, - &def->mem.min_guarantee, false) < 0) + &def->mem.min_guarantee, false, false) < 0) goto error; if (virDomainParseMemory("./memtune/swap_hard_limit[1]", ctxt, - &def->mem.swap_hard_limit, false) < 0) + &def->mem.swap_hard_limit, false, false) < 0) goto error; n = virXPathULong("string(./vcpu[1])", ctxt, &count); diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h index 9908d88..fbb3b2f 100644 --- a/src/conf/domain_conf.h +++ b/src/conf/domain_conf.h @@ -1966,8 +1966,10 @@ typedef struct _virDomainMemtune virDomainMemtune; typedef virDomainMemtune *virDomainMemtunePtr; struct _virDomainMemtune { - unsigned long long max_balloon; /* in kibibytes */ - unsigned long long cur_balloon; /* in kibibytes */ + unsigned long long max_balloon; /* in kibibytes, capped at ulong thanks + to virDomainGetMaxMemory */ + unsigned long long cur_balloon; /* in kibibytes, capped at ulong thanks + to virDomainGetInfo */ virDomainHugePagePtr hugepages; size_t nhugepages; @@ -1975,10 +1977,10 @@ struct _virDomainMemtune { bool nosharepages; bool locked; int dump_core; /* enum virTristateSwitch */ - unsigned long long hard_limit; /* in kibibytes */ - unsigned long long soft_limit; /* in kibibytes */ - unsigned long long min_guarantee; /* in kibibytes */ - unsigned long long swap_hard_limit; /* in kibibytes */ + unsigned long long hard_limit; /* in kibibytes, limit at off_t bytes */ + unsigned long long soft_limit; /* in kibibytes, limit at off_t bytes */ + unsigned long long min_guarantee; /* in kibibytes, limit at off_t bytes */ + unsigned long long swap_hard_limit; /* in kibibytes, limit at off_t bytes */ }; typedef struct _virDomainPowerManagement virDomainPowerManagement; -- 1.9.3
On Thu, Oct 30, 2014 at 02:00:34PM -0600, Eric Blake wrote:
Commit 6c9a8a4 (Oct 2014) exposed a long-standing issue on 32-bit machines: code related to virDomainSetMemoryParameters has always been documented as using a 64-bit limit, but it was implemented by calling virDomainParseMemory which enforced an 'unsigned long' limit. Since VIR_DOMAIN_MEMORY_PARAM_UNLIMITED capped to a long is -1, but virDomainParseScaledValue no longer accepts negative values, an attempt to use 2^53-1 as a hard memory limit started failing the testsuite. However, the problem with capping things artificially low has existed for much longer - ever since commits 4888f0fb and 2e22f23 (Mar 2012) switched internal tracking from 'unsigned long' to 'unsigned long long' (prior to that time, the cap was a side-effect of the choice of types). We _have_ to cap the balloon memory values, (no thanks to baked in 'unsigned long' of API such as virDomainSetMaxMemory or virDomainGetInfo with no
Oh, this is why it needs to be capped. That makes sense. And great to have it in the comment next to those variables!
counterpart API that guarantees 64-bit access to those numbers) but memory parameters have never needed the artificial limit.
At any rate, the solution is to make the parser function gain a parameter, and only do the reduced 32-bit cap for the values that are constrained due to API.
* src/conf/domain_conf.h (_virDomainMemtune): Add comments. * src/conf/domain_conf.c (virDomainParseMemory): Add parameter. (virDomainDefParseXML): Adjust callers.
Signed-off-by: Eric Blake <eblake@redhat.com> --- src/conf/domain_conf.c | 25 ++++++++++++++----------- src/conf/domain_conf.h | 14 ++++++++------ 2 files changed, 22 insertions(+), 17 deletions(-)
diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 39befb0..c594325 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -6373,19 +6373,22 @@ virDomainParseScaledValue(const char *xpath,
/* Parse a memory element located at XPATH within CTXT, and store the - * result into MEM. If REQUIRED, then the value must exist; - * otherwise, the value is optional. The value is in blocks of 1024. - * Return 0 on success, -1 on failure after issuing error. */ + * result into MEM, in blocks of 1024. If REQUIRED, then the value + * must exist; otherwise, the value is optional. The value must not + * exceed VIR_DOMAIN_MEMORY_PARAM_UNLIMITED once scaled; additionally, + * if CAPPED is true, the value must fit within an unsigned long (only + * matters on 32-bit platforms). Return 0 on success, -1 on failure + * after issuing error. */
The last sentence (Return ...) could be in its own paragraph even though our documentation is not created for these functions. ACK with that and safe for freeze and also build-breaker for 32-bit machines. Martin
On 10/31/2014 12:20 AM, Martin Kletzander wrote:
On Thu, Oct 30, 2014 at 02:00:34PM -0600, Eric Blake wrote:
Commit 6c9a8a4 (Oct 2014) exposed a long-standing issue on 32-bit machines: code related to virDomainSetMemoryParameters has always been documented as using a 64-bit limit, but it was implemented by calling virDomainParseMemory which enforced an 'unsigned long' limit. Since VIR_DOMAIN_MEMORY_PARAM_UNLIMITED capped to a long is -1, but virDomainParseScaledValue no longer accepts negative values, an attempt to use 2^53-1 as a hard memory limit started failing the testsuite. However, the problem with capping things artificially low has existed for much longer - ever since commits 4888f0fb and 2e22f23 (Mar 2012) switched internal tracking from 'unsigned long' to 'unsigned long long' (prior to that time, the cap was a side-effect of the choice of types). We _have_ to cap the balloon memory values, (no thanks to baked in 'unsigned long' of API such as virDomainSetMaxMemory or virDomainGetInfo with no
Oh, this is why it needs to be capped. That makes sense. And great to have it in the comment next to those variables!
Comments can make a world of difference :)
+ * exceed VIR_DOMAIN_MEMORY_PARAM_UNLIMITED once scaled; additionally, + * if CAPPED is true, the value must fit within an unsigned long (only + * matters on 32-bit platforms). Return 0 on success, -1 on failure + * after issuing error. */
The last sentence (Return ...) could be in its own paragraph even though our documentation is not created for these functions.
ACK with that and safe for freeze and also build-breaker for 32-bit machines.
Tweaked and pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (2)
-
Eric Blake -
Martin Kletzander