* [PATCH 0/2] libxl: a few domain maximum memory fixes
@ 2017-02-02 22:31 Jim Fehlig
2017-02-02 22:31 ` [PATCH 1/2] libxl: fix reporting of maximum memory Jim Fehlig
2017-02-02 22:31 ` [PATCH 2/2] libxl: fix dom0 maximum memory setting Jim Fehlig
0 siblings, 2 replies; 7+ messages in thread
From: Jim Fehlig @ 2017-02-02 22:31 UTC (permalink / raw)
To: libvir-list; +Cc: xen-devel
Patch 1 fixes reporting of domain maximum memory for running domains.
When creating a virDomainDef object to represent dom0, max memory was
not set correctly, which is fixed by patch2.
Jim Fehlig (2):
libxl: fix reporting of maximum memory
libxl: fix dom0 maximum memory setting
src/libxl/libxl_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
src/libxl/libxl_conf.h | 3 ++
src/libxl/libxl_driver.c | 7 ++---
3 files changed, 81 insertions(+), 4 deletions(-)
--
2.9.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/2] libxl: fix reporting of maximum memory
2017-02-02 22:31 [PATCH 0/2] libxl: a few domain maximum memory fixes Jim Fehlig
@ 2017-02-02 22:31 ` Jim Fehlig
2017-02-02 22:31 ` [PATCH 2/2] libxl: fix dom0 maximum memory setting Jim Fehlig
1 sibling, 0 replies; 7+ messages in thread
From: Jim Fehlig @ 2017-02-02 22:31 UTC (permalink / raw)
To: libvir-list; +Cc: Jim Fehlig, xen-devel
The libxl driver reports different values of maximum memory depending
on state of a domain. If inactive, maximum memory value is reported
correctly. When active, maximum memory is derived from max_pages value
returned by the XEN_SYSCTL_getdomaininfolist sysctl operation. But
max_pages can be changed by toolstacks and does not necessarily
represent the maximum memory a domain can use during its active
lifetime.
A better location for determining a domain's maximum memory is the
/local/domain/<id>/memory/static-max node in xenstore. This value
is set from the libxl_domain_build_info.max_memkb field when creating
the domain. Currently it cannot be changed nor can its value be
exceeded by a balloon operation. From libvirt's perspective, always
reporting maximum memory with virDomainDefGetMemoryTotal() will produce
the same results as reading the static-max node in xenstore.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
As an alternative, libvirt could call libxl_retrieve_domain_configuration
and use the max_memkb field embedded in the libxl_domain_config object.
src/libxl/libxl_driver.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 3a69720..921cc93 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -1640,10 +1640,10 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
if (virDomainGetInfoEnsureACL(dom->conn, vm->def) < 0)
goto cleanup;
+ info->maxMem = virDomainDefGetMemoryTotal(vm->def);
if (!virDomainObjIsActive(vm)) {
info->cpuTime = 0;
info->memory = vm->def->mem.cur_balloon;
- info->maxMem = virDomainDefGetMemoryTotal(vm->def);
} else {
libxl_dominfo_init(&d_info);
@@ -1655,7 +1655,6 @@ libxlDomainGetInfo(virDomainPtr dom, virDomainInfoPtr info)
}
info->cpuTime = d_info.cpu_time;
info->memory = d_info.current_memkb;
- info->maxMem = d_info.max_memkb;
libxl_dominfo_dispose(&d_info);
}
@@ -5172,7 +5171,7 @@ libxlDomainMemoryStats(virDomainPtr dom,
goto endjob;
}
mem = d_info.current_memkb;
- maxmem = d_info.max_memkb;
+ maxmem = virDomainDefGetMemoryTotal(vm->def);
LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_ACTUAL_BALLOON, mem);
LIBXL_SET_MEMSTAT(VIR_DOMAIN_MEMORY_STAT_AVAILABLE, maxmem);
--
2.9.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] libxl: fix dom0 maximum memory setting
2017-02-02 22:31 [PATCH 0/2] libxl: a few domain maximum memory fixes Jim Fehlig
2017-02-02 22:31 ` [PATCH 1/2] libxl: fix reporting of maximum memory Jim Fehlig
@ 2017-02-02 22:31 ` Jim Fehlig
2017-02-08 13:39 ` [libvirt] " Joao Martins
1 sibling, 1 reply; 7+ messages in thread
From: Jim Fehlig @ 2017-02-02 22:31 UTC (permalink / raw)
To: libvir-list; +Cc: Jim Fehlig, xen-devel
When the libxl driver is initialized, it creates a virDomainDef
object for dom0 and adds it to the list of domains. Total memory
for dom0 was being set from the max_memkb field of libxl_dominfo
struct retrieved from libxl, but this field can be set to
LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
explicitly set by the user.
This patch adds some simple parsing of the Xen commandline,
looking for a dom0_mem parameter that also specifies a 'max' value.
If not specified, dom0 maximum memory is effectively all physical
host memory.
Signed-off-by: Jim Fehlig <jfehlig@suse.com>
---
src/libxl/libxl_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
src/libxl/libxl_conf.h | 3 ++
src/libxl/libxl_driver.c | 2 +-
3 files changed, 79 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
index b5186f2..bfe0e92 100644
--- a/src/libxl/libxl_conf.c
+++ b/src/libxl/libxl_conf.c
@@ -34,6 +34,7 @@
#include "internal.h"
#include "virlog.h"
#include "virerror.h"
+#include "c-ctype.h"
#include "datatypes.h"
#include "virconf.h"
#include "virfile.h"
@@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
}
+/*
+ * dom0's maximum memory can be controled by the user with the 'dom0_mem' Xen
+ * command line parameter. E.g. to set dom0's initial memory to 4G and max
+ * memory to 8G: dom0_mem=4G,max:8G
+ *
+ * If not constrained by the user, dom0 can effectively use all host memory.
+ * This function returns the configured maximum memory for dom0 in kilobytes,
+ * either the user-specified value or total physical memory as a default.
+ */
+unsigned long long
+libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
+{
+ char **cmd_tokens = NULL;
+ char **mem_tokens = NULL;
+ size_t i;
+ size_t j;
+ unsigned long long ret;
+ libxl_physinfo physinfo;
+
+ if (cfg->verInfo->commandline == NULL ||
+ !(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
+ goto physmem;
+
+ for (i = 0; cmd_tokens[i] != NULL; i++) {
+ if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
+ continue;
+
+ if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
+ break;
+ for (j = 0; mem_tokens[j] != NULL; j++) {
+ if (STRPREFIX(mem_tokens[j], "max:")) {
+ char *p = mem_tokens[j] + 4;
+ unsigned long long multiplier = 1;
+
+ while (c_isdigit(*p))
+ p++;
+ if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, &ret) < 0)
+ break;
+ if (*p) {
+ switch (*p) {
+ case 'k':
+ case 'K':
+ multiplier = 1024;
+ break;
+ case 'm':
+ case 'M':
+ multiplier = 1024 * 1024;
+ break;
+ case 'g':
+ case 'G':
+ multiplier = 1024 * 1024 * 1024;
+ break;
+ }
+ }
+ ret = (ret * multiplier) / 1024;
+ goto cleanup;
+ }
+ }
+ }
+
+ physmem:
+ /* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
+ libxl_physinfo_init(&physinfo);
+ libxl_get_physinfo(cfg->ctx, &physinfo);
+ ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
+ libxl_physinfo_dispose(&physinfo);
+
+ cleanup:
+ virStringListFree(cmd_tokens);
+ virStringListFree(mem_tokens);
+ return ret;
+}
+
+
#ifdef LIBXL_HAVE_DEVICE_CHANNEL
static int
libxlPrepareChannel(virDomainChrDefPtr channel,
diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
index 69d7885..c4ddbfe 100644
--- a/src/libxl/libxl_conf.h
+++ b/src/libxl/libxl_conf.h
@@ -173,6 +173,9 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver,
int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
const char *filename);
+unsigned long long
+libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg);
+
int
libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
int
diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
index 921cc93..e54b3b7 100644
--- a/src/libxl/libxl_driver.c
+++ b/src/libxl/libxl_driver.c
@@ -615,7 +615,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
goto cleanup;
vm->def->mem.cur_balloon = d_info.current_memkb;
- virDomainDefSetMemoryTotal(vm->def, d_info.max_memkb);
+ virDomainDefSetMemoryTotal(vm->def, libxlDriverGetDom0MaxmemConf(cfg));
ret = 0;
--
2.9.2
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting
2017-02-02 22:31 ` [PATCH 2/2] libxl: fix dom0 maximum memory setting Jim Fehlig
@ 2017-02-08 13:39 ` Joao Martins
2017-02-08 16:06 ` Jim Fehlig
0 siblings, 1 reply; 7+ messages in thread
From: Joao Martins @ 2017-02-08 13:39 UTC (permalink / raw)
To: Jim Fehlig; +Cc: libvir-list, xen-devel
On 02/02/2017 10:31 PM, Jim Fehlig wrote:
> When the libxl driver is initialized, it creates a virDomainDef
> object for dom0 and adds it to the list of domains. Total memory
> for dom0 was being set from the max_memkb field of libxl_dominfo
> struct retrieved from libxl, but this field can be set to
> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
> explicitly set by the user.
>
> This patch adds some simple parsing of the Xen commandline,
> looking for a dom0_mem parameter that also specifies a 'max' value.
> If not specified, dom0 maximum memory is effectively all physical
> host memory.
>
> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
> ---
> src/libxl/libxl_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
> src/libxl/libxl_conf.h | 3 ++
> src/libxl/libxl_driver.c | 2 +-
> 3 files changed, 79 insertions(+), 1 deletion(-)
>
> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
> index b5186f2..bfe0e92 100644
> --- a/src/libxl/libxl_conf.c
> +++ b/src/libxl/libxl_conf.c
> @@ -34,6 +34,7 @@
> #include "internal.h"
> #include "virlog.h"
> #include "virerror.h"
> +#include "c-ctype.h"
> #include "datatypes.h"
> #include "virconf.h"
> #include "virfile.h"
> @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>
> }
>
> +/*
> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' Xen
> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
> + * memory to 8G: dom0_mem=4G,max:8G
> + *
> + * If not constrained by the user, dom0 can effectively use all host memory.
> + * This function returns the configured maximum memory for dom0 in kilobytes,
> + * either the user-specified value or total physical memory as a default.
> + */
> +unsigned long long
> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
> +{
> + char **cmd_tokens = NULL;
> + char **mem_tokens = NULL;
> + size_t i;
> + size_t j;
> + unsigned long long ret;
> + libxl_physinfo physinfo;
> +
> + if (cfg->verInfo->commandline == NULL ||
> + !(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
> + goto physmem;
> +
> + for (i = 0; cmd_tokens[i] != NULL; i++) {
> + if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
> + continue;
> +
> + if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
> + break;
> + for (j = 0; mem_tokens[j] != NULL; j++) {
> + if (STRPREFIX(mem_tokens[j], "max:")) {
> + char *p = mem_tokens[j] + 4;
> + unsigned long long multiplier = 1;
> +
> + while (c_isdigit(*p))
> + p++;
> + if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, &ret) < 0)
> + break;
> + if (*p) {
> + switch (*p) {
> + case 'k':
> + case 'K':
> + multiplier = 1024;
> + break;
> + case 'm':
> + case 'M':
> + multiplier = 1024 * 1024;
> + break;
> + case 'g':
> + case 'G':
> + multiplier = 1024 * 1024 * 1024;
> + break;
> + }
> + }
> + ret = (ret * multiplier) / 1024;
> + goto cleanup;
> + }
> + }
> + }
> +
> + physmem:
> + /* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
> + libxl_physinfo_init(&physinfo);
> + libxl_get_physinfo(cfg->ctx, &physinfo);
Despite being an unlikely event, libxl_get_physinfo can fail here - I think you
need to check the return value here.
> + ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
> + libxl_physinfo_dispose(&physinfo);
> +
> + cleanup:
> + virStringListFree(cmd_tokens);
> + virStringListFree(mem_tokens);
> + return ret;
> +}
> +
> +
> #ifdef LIBXL_HAVE_DEVICE_CHANNEL
> static int
> libxlPrepareChannel(virDomainChrDefPtr channel,
> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
> index 69d7885..c4ddbfe 100644
> --- a/src/libxl/libxl_conf.h
> +++ b/src/libxl/libxl_conf.h
> @@ -173,6 +173,9 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver,
> int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
> const char *filename);
>
> +unsigned long long
> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg);
> +
> int
> libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
> int
> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
> index 921cc93..e54b3b7 100644
> --- a/src/libxl/libxl_driver.c
> +++ b/src/libxl/libxl_driver.c
> @@ -615,7 +615,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
> if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
> goto cleanup;
> vm->def->mem.cur_balloon = d_info.current_memkb;
> - virDomainDefSetMemoryTotal(vm->def, d_info.max_memkb);
> + virDomainDefSetMemoryTotal(vm->def, libxlDriverGetDom0MaxmemConf(cfg));
And perhaps checking here too on whether "libxlDriverGetDom0MaxmemConf(cfg) < 0" ?
>
> ret = 0;
>
>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting
2017-02-08 13:39 ` [libvirt] " Joao Martins
@ 2017-02-08 16:06 ` Jim Fehlig
2017-02-08 16:59 ` Joao Martins
0 siblings, 1 reply; 7+ messages in thread
From: Jim Fehlig @ 2017-02-08 16:06 UTC (permalink / raw)
To: Joao Martins; +Cc: libvir-list, xen-devel
Joao Martins wrote:
> On 02/02/2017 10:31 PM, Jim Fehlig wrote:
>> When the libxl driver is initialized, it creates a virDomainDef
>> object for dom0 and adds it to the list of domains. Total memory
>> for dom0 was being set from the max_memkb field of libxl_dominfo
>> struct retrieved from libxl, but this field can be set to
>> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
>> explicitly set by the user.
>>
>> This patch adds some simple parsing of the Xen commandline,
>> looking for a dom0_mem parameter that also specifies a 'max' value.
>> If not specified, dom0 maximum memory is effectively all physical
>> host memory.
>>
>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>> ---
>> src/libxl/libxl_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
>> src/libxl/libxl_conf.h | 3 ++
>> src/libxl/libxl_driver.c | 2 +-
>> 3 files changed, 79 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>> index b5186f2..bfe0e92 100644
>> --- a/src/libxl/libxl_conf.c
>> +++ b/src/libxl/libxl_conf.c
>> @@ -34,6 +34,7 @@
>> #include "internal.h"
>> #include "virlog.h"
>> #include "virerror.h"
>> +#include "c-ctype.h"
>> #include "datatypes.h"
>> #include "virconf.h"
>> #include "virfile.h"
>> @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>>
>> }
>>
>> +/*
>> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' Xen
>> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
>> + * memory to 8G: dom0_mem=4G,max:8G
>> + *
>> + * If not constrained by the user, dom0 can effectively use all host memory.
>> + * This function returns the configured maximum memory for dom0 in kilobytes,
>> + * either the user-specified value or total physical memory as a default.
>> + */
>> +unsigned long long
>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
>> +{
>> + char **cmd_tokens = NULL;
>> + char **mem_tokens = NULL;
>> + size_t i;
>> + size_t j;
>> + unsigned long long ret;
>> + libxl_physinfo physinfo;
>> +
>> + if (cfg->verInfo->commandline == NULL ||
>> + !(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
>> + goto physmem;
>> +
>> + for (i = 0; cmd_tokens[i] != NULL; i++) {
>> + if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
>> + continue;
>> +
>> + if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
>> + break;
>> + for (j = 0; mem_tokens[j] != NULL; j++) {
>> + if (STRPREFIX(mem_tokens[j], "max:")) {
>> + char *p = mem_tokens[j] + 4;
>> + unsigned long long multiplier = 1;
>> +
>> + while (c_isdigit(*p))
>> + p++;
>> + if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, &ret) < 0)
>> + break;
>> + if (*p) {
>> + switch (*p) {
>> + case 'k':
>> + case 'K':
>> + multiplier = 1024;
>> + break;
>> + case 'm':
>> + case 'M':
>> + multiplier = 1024 * 1024;
>> + break;
>> + case 'g':
>> + case 'G':
>> + multiplier = 1024 * 1024 * 1024;
>> + break;
>> + }
>> + }
>> + ret = (ret * multiplier) / 1024;
>> + goto cleanup;
>> + }
>> + }
>> + }
>> +
>> + physmem:
>> + /* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
>> + libxl_physinfo_init(&physinfo);
>> + libxl_get_physinfo(cfg->ctx, &physinfo);
> Despite being an unlikely event, libxl_get_physinfo can fail here - I think you
> need to check the return value here.
Bummer. I was hoping this function could always return a sane value for dom0 max
memory. But I'm not really sure what that would be if libxl_get_physinfo failed.
>
>> + ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
>> + libxl_physinfo_dispose(&physinfo);
>> +
>> + cleanup:
>> + virStringListFree(cmd_tokens);
>> + virStringListFree(mem_tokens);
>> + return ret;
>> +}
>> +
>> +
>> #ifdef LIBXL_HAVE_DEVICE_CHANNEL
>> static int
>> libxlPrepareChannel(virDomainChrDefPtr channel,
>> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
>> index 69d7885..c4ddbfe 100644
>> --- a/src/libxl/libxl_conf.h
>> +++ b/src/libxl/libxl_conf.h
>> @@ -173,6 +173,9 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver,
>> int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>> const char *filename);
>>
>> +unsigned long long
>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg);
>> +
>> int
>> libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
>> int
>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>> index 921cc93..e54b3b7 100644
>> --- a/src/libxl/libxl_driver.c
>> +++ b/src/libxl/libxl_driver.c
>> @@ -615,7 +615,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
>> if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
>> goto cleanup;
>> vm->def->mem.cur_balloon = d_info.current_memkb;
>> - virDomainDefSetMemoryTotal(vm->def, d_info.max_memkb);
>> + virDomainDefSetMemoryTotal(vm->def, libxlDriverGetDom0MaxmemConf(cfg));
>
> And perhaps checking here too on whether "libxlDriverGetDom0MaxmemConf(cfg) < 0" ?
ATM, libxlDriverGetDom0MaxmemConf returns an unsigned long long. I could change
the signature to
int
libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg,
unsigned long long *maxmem)
but again I'm not really sure what to do with maxmem in case of failure. Perhaps
just set it to current_memkb?
Regards,
Jim
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting
2017-02-08 16:06 ` Jim Fehlig
@ 2017-02-08 16:59 ` Joao Martins
2017-02-08 22:23 ` Jim Fehlig
0 siblings, 1 reply; 7+ messages in thread
From: Joao Martins @ 2017-02-08 16:59 UTC (permalink / raw)
To: Jim Fehlig; +Cc: libvir-list, xen-devel
On 02/08/2017 04:06 PM, Jim Fehlig wrote:
> Joao Martins wrote:
>> On 02/02/2017 10:31 PM, Jim Fehlig wrote:
>>> When the libxl driver is initialized, it creates a virDomainDef
>>> object for dom0 and adds it to the list of domains. Total memory
>>> for dom0 was being set from the max_memkb field of libxl_dominfo
>>> struct retrieved from libxl, but this field can be set to
>>> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
>>> explicitly set by the user.
>>>
>>> This patch adds some simple parsing of the Xen commandline,
>>> looking for a dom0_mem parameter that also specifies a 'max' value.
>>> If not specified, dom0 maximum memory is effectively all physical
>>> host memory.
>>>
>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>>> ---
>>> src/libxl/libxl_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
>>> src/libxl/libxl_conf.h | 3 ++
>>> src/libxl/libxl_driver.c | 2 +-
>>> 3 files changed, 79 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>> index b5186f2..bfe0e92 100644
>>> --- a/src/libxl/libxl_conf.c
>>> +++ b/src/libxl/libxl_conf.c
>>> @@ -34,6 +34,7 @@
>>> #include "internal.h"
>>> #include "virlog.h"
>>> #include "virerror.h"
>>> +#include "c-ctype.h"
>>> #include "datatypes.h"
>>> #include "virconf.h"
>>> #include "virfile.h"
>>> @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>>>
>>> }
>>>
>>> +/*
>>> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' Xen
>>> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
>>> + * memory to 8G: dom0_mem=4G,max:8G
>>> + *
>>> + * If not constrained by the user, dom0 can effectively use all host memory.
>>> + * This function returns the configured maximum memory for dom0 in kilobytes,
>>> + * either the user-specified value or total physical memory as a default.
>>> + */
>>> +unsigned long long
>>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
>>> +{
>>> + char **cmd_tokens = NULL;
>>> + char **mem_tokens = NULL;
>>> + size_t i;
>>> + size_t j;
>>> + unsigned long long ret;
>>> + libxl_physinfo physinfo;
>>> +
>>> + if (cfg->verInfo->commandline == NULL ||
>>> + !(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
>>> + goto physmem;
>>> +
>>> + for (i = 0; cmd_tokens[i] != NULL; i++) {
>>> + if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
>>> + continue;
>>> +
>>> + if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
>>> + break;
>>> + for (j = 0; mem_tokens[j] != NULL; j++) {
>>> + if (STRPREFIX(mem_tokens[j], "max:")) {
>>> + char *p = mem_tokens[j] + 4;
>>> + unsigned long long multiplier = 1;
>>> +
>>> + while (c_isdigit(*p))
>>> + p++;
>>> + if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, &ret) < 0)
>>> + break;
>>> + if (*p) {
>>> + switch (*p) {
>>> + case 'k':
>>> + case 'K':
>>> + multiplier = 1024;
>>> + break;
>>> + case 'm':
>>> + case 'M':
>>> + multiplier = 1024 * 1024;
>>> + break;
>>> + case 'g':
>>> + case 'G':
>>> + multiplier = 1024 * 1024 * 1024;
>>> + break;
>>> + }
>>> + }
>>> + ret = (ret * multiplier) / 1024;
>>> + goto cleanup;
>>> + }
>>> + }
>>> + }
>>> +
>>> + physmem:
>>> + /* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
>>> + libxl_physinfo_init(&physinfo);
>>> + libxl_get_physinfo(cfg->ctx, &physinfo);
>> Despite being an unlikely event, libxl_get_physinfo can fail here - I think you
>> need to check the return value here.
>
> Bummer. I was hoping this function could always return a sane value for dom0 max
> memory. But I'm not really sure what that would be if libxl_get_physinfo failed.
>
Yeah, We probably have some serious concerns if this fails. And given that
libxlAddDom0 happens early while bootstrapping the libxl module, it might be
good to warn the user (following similar pattern throughout the rest of the code):
virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
_("libxl_get_physinfo_info failed"));
or perhaps simply VIR_WARN?
>>> + ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
>>> + libxl_physinfo_dispose(&physinfo);
>>> +
>>> + cleanup:
>>> + virStringListFree(cmd_tokens);
>>> + virStringListFree(mem_tokens);
>>> + return ret;
>>> +}
>>> +
>>> +
>>> #ifdef LIBXL_HAVE_DEVICE_CHANNEL
>>> static int
>>> libxlPrepareChannel(virDomainChrDefPtr channel,
>>> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
>>> index 69d7885..c4ddbfe 100644
>>> --- a/src/libxl/libxl_conf.h
>>> +++ b/src/libxl/libxl_conf.h
>>> @@ -173,6 +173,9 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver,
>>> int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>>> const char *filename);
>>>
>>> +unsigned long long
>>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg);
>>> +
>>> int
>>> libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
>>> int
>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>> index 921cc93..e54b3b7 100644
>>> --- a/src/libxl/libxl_driver.c
>>> +++ b/src/libxl/libxl_driver.c
>>> @@ -615,7 +615,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
>>> if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
>>> goto cleanup;
>>> vm->def->mem.cur_balloon = d_info.current_memkb;
>>> - virDomainDefSetMemoryTotal(vm->def, d_info.max_memkb);
>>> + virDomainDefSetMemoryTotal(vm->def, libxlDriverGetDom0MaxmemConf(cfg));
>>
>> And perhaps checking here too on whether "libxlDriverGetDom0MaxmemConf(cfg) < 0" ?
>
> ATM, libxlDriverGetDom0MaxmemConf returns an unsigned long long. I could change
> the signature to
>
> int
> libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg,
> unsigned long long *maxmem)
>
> but again I'm not really sure what to do with maxmem in case of failure. Perhaps
> just set it to current_memkb?
Yeap, that's what I was thinking too.
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [libvirt] [PATCH 2/2] libxl: fix dom0 maximum memory setting
2017-02-08 16:59 ` Joao Martins
@ 2017-02-08 22:23 ` Jim Fehlig
0 siblings, 0 replies; 7+ messages in thread
From: Jim Fehlig @ 2017-02-08 22:23 UTC (permalink / raw)
To: Joao Martins; +Cc: libvir-list, xen-devel
Joao Martins wrote:
> On 02/08/2017 04:06 PM, Jim Fehlig wrote:
>> Joao Martins wrote:
>>> On 02/02/2017 10:31 PM, Jim Fehlig wrote:
>>>> When the libxl driver is initialized, it creates a virDomainDef
>>>> object for dom0 and adds it to the list of domains. Total memory
>>>> for dom0 was being set from the max_memkb field of libxl_dominfo
>>>> struct retrieved from libxl, but this field can be set to
>>>> LIBXL_MEMKB_DEFAULT (~0ULL) if dom0 maximum memory has not been
>>>> explicitly set by the user.
>>>>
>>>> This patch adds some simple parsing of the Xen commandline,
>>>> looking for a dom0_mem parameter that also specifies a 'max' value.
>>>> If not specified, dom0 maximum memory is effectively all physical
>>>> host memory.
>>>>
>>>> Signed-off-by: Jim Fehlig <jfehlig@suse.com>
>>>> ---
>>>> src/libxl/libxl_conf.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>> src/libxl/libxl_conf.h | 3 ++
>>>> src/libxl/libxl_driver.c | 2 +-
>>>> 3 files changed, 79 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>>> index b5186f2..bfe0e92 100644
>>>> --- a/src/libxl/libxl_conf.c
>>>> +++ b/src/libxl/libxl_conf.c
>>>> @@ -34,6 +34,7 @@
>>>> #include "internal.h"
>>>> #include "virlog.h"
>>>> #include "virerror.h"
>>>> +#include "c-ctype.h"
>>>> #include "datatypes.h"
>>>> #include "virconf.h"
>>>> #include "virfile.h"
>>>> @@ -1530,6 +1531,80 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>>>>
>>>> }
>>>>
>>>> +/*
>>>> + * dom0's maximum memory can be controled by the user with the 'dom0_mem' Xen
>>>> + * command line parameter. E.g. to set dom0's initial memory to 4G and max
>>>> + * memory to 8G: dom0_mem=4G,max:8G
>>>> + *
>>>> + * If not constrained by the user, dom0 can effectively use all host memory.
>>>> + * This function returns the configured maximum memory for dom0 in kilobytes,
>>>> + * either the user-specified value or total physical memory as a default.
>>>> + */
>>>> +unsigned long long
>>>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
>>>> +{
>>>> + char **cmd_tokens = NULL;
>>>> + char **mem_tokens = NULL;
>>>> + size_t i;
>>>> + size_t j;
>>>> + unsigned long long ret;
>>>> + libxl_physinfo physinfo;
>>>> +
>>>> + if (cfg->verInfo->commandline == NULL ||
>>>> + !(cmd_tokens = virStringSplit(cfg->verInfo->commandline, " ", 0)))
>>>> + goto physmem;
>>>> +
>>>> + for (i = 0; cmd_tokens[i] != NULL; i++) {
>>>> + if (!STRPREFIX(cmd_tokens[i], "dom0_mem="))
>>>> + continue;
>>>> +
>>>> + if (!(mem_tokens = virStringSplit(cmd_tokens[i], ",", 0)))
>>>> + break;
>>>> + for (j = 0; mem_tokens[j] != NULL; j++) {
>>>> + if (STRPREFIX(mem_tokens[j], "max:")) {
>>>> + char *p = mem_tokens[j] + 4;
>>>> + unsigned long long multiplier = 1;
>>>> +
>>>> + while (c_isdigit(*p))
>>>> + p++;
>>>> + if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, &ret) < 0)
>>>> + break;
>>>> + if (*p) {
>>>> + switch (*p) {
>>>> + case 'k':
>>>> + case 'K':
>>>> + multiplier = 1024;
>>>> + break;
>>>> + case 'm':
>>>> + case 'M':
>>>> + multiplier = 1024 * 1024;
>>>> + break;
>>>> + case 'g':
>>>> + case 'G':
>>>> + multiplier = 1024 * 1024 * 1024;
>>>> + break;
>>>> + }
>>>> + }
>>>> + ret = (ret * multiplier) / 1024;
>>>> + goto cleanup;
>>>> + }
>>>> + }
>>>> + }
>>>> +
>>>> + physmem:
>>>> + /* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
>>>> + libxl_physinfo_init(&physinfo);
>>>> + libxl_get_physinfo(cfg->ctx, &physinfo);
>>> Despite being an unlikely event, libxl_get_physinfo can fail here - I think you
>>> need to check the return value here.
>> Bummer. I was hoping this function could always return a sane value for dom0 max
>> memory. But I'm not really sure what that would be if libxl_get_physinfo failed.
>>
> Yeah, We probably have some serious concerns if this fails. And given that
> libxlAddDom0 happens early while bootstrapping the libxl module, it might be
> good to warn the user (following similar pattern throughout the rest of the code):
>
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
> _("libxl_get_physinfo_info failed"));
>
> or perhaps simply VIR_WARN?
>
>>>> + ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
>>>> + libxl_physinfo_dispose(&physinfo);
>>>> +
>>>> + cleanup:
>>>> + virStringListFree(cmd_tokens);
>>>> + virStringListFree(mem_tokens);
>>>> + return ret;
>>>> +}
>>>> +
>>>> +
>>>> #ifdef LIBXL_HAVE_DEVICE_CHANNEL
>>>> static int
>>>> libxlPrepareChannel(virDomainChrDefPtr channel,
>>>> diff --git a/src/libxl/libxl_conf.h b/src/libxl/libxl_conf.h
>>>> index 69d7885..c4ddbfe 100644
>>>> --- a/src/libxl/libxl_conf.h
>>>> +++ b/src/libxl/libxl_conf.h
>>>> @@ -173,6 +173,9 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver,
>>>> int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>>>> const char *filename);
>>>>
>>>> +unsigned long long
>>>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg);
>>>> +
>>>> int
>>>> libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
>>>> int
>>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>>> index 921cc93..e54b3b7 100644
>>>> --- a/src/libxl/libxl_driver.c
>>>> +++ b/src/libxl/libxl_driver.c
>>>> @@ -615,7 +615,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver)
>>>> if (virDomainDefSetVcpus(vm->def, d_info.vcpu_online) < 0)
>>>> goto cleanup;
>>>> vm->def->mem.cur_balloon = d_info.current_memkb;
>>>> - virDomainDefSetMemoryTotal(vm->def, d_info.max_memkb);
>>>> + virDomainDefSetMemoryTotal(vm->def, libxlDriverGetDom0MaxmemConf(cfg));
>>> And perhaps checking here too on whether "libxlDriverGetDom0MaxmemConf(cfg) < 0" ?
>> ATM, libxlDriverGetDom0MaxmemConf returns an unsigned long long. I could change
>> the signature to
>>
>> int
>> libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg,
>> unsigned long long *maxmem)
>>
>> but again I'm not really sure what to do with maxmem in case of failure. Perhaps
>> just set it to current_memkb?
> Yeap, that's what I was thinking too.
Thanks for the comments. I've sent a V2 incorporating them
https://www.redhat.com/archives/libvir-list/2017-February/msg00349.html
Regards,
Jim
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-02-08 22:23 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-02 22:31 [PATCH 0/2] libxl: a few domain maximum memory fixes Jim Fehlig
2017-02-02 22:31 ` [PATCH 1/2] libxl: fix reporting of maximum memory Jim Fehlig
2017-02-02 22:31 ` [PATCH 2/2] libxl: fix dom0 maximum memory setting Jim Fehlig
2017-02-08 13:39 ` [libvirt] " Joao Martins
2017-02-08 16:06 ` Jim Fehlig
2017-02-08 16:59 ` Joao Martins
2017-02-08 22:23 ` Jim Fehlig
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.