Re: [libvirt] [PATCH v2] domain: fix parsing of memory tunables on 32-bit machines
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 ebl...@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 signature.asc Description: Digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH v2] domain: fix parsing of memory tunables on 32-bit machines
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 signature.asc Description: OpenPGP digital signature -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
[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 ebl...@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 0x * 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
