[libvirt] [PATCH V2 0/2] libxl: a few domain maximum memory fixes
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. V2: Check return value of libxl_get_physinfo and libxlDriverGetDom0MaxmemConf in patch2. Jim Fehlig (2): libxl: fix reporting of maximum memory libxl: fix dom0 maximum memory setting src/libxl/libxl_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_conf.h | 4 +++ src/libxl/libxl_driver.c | 10 ++++--- 3 files changed, 87 insertions(+), 4 deletions(-) -- 2.9.2
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> --- 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 38ad91e..040b986 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); } @@ -5174,7 +5173,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
On 02/08/2017 10:24 PM, Jim Fehlig wrote:
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>
Acked-by: Joao Martins <joao.m.martins@oracle.com>
--- 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 38ad91e..040b986 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); } @@ -5174,7 +5173,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);
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> --- V2: Check return value of libxl_get_physinfo and libxlDriverGetDom0MaxmemConf. src/libxl/libxl_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_conf.h | 4 +++ src/libxl/libxl_driver.c | 5 +++- 3 files changed, 85 insertions(+), 1 deletion(-) diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 28b05e1..7e30c7d 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" @@ -1533,6 +1534,82 @@ 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. + */ +int +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, + unsigned long long *maxmem) +{ + char **cmd_tokens = NULL; + char **mem_tokens = NULL; + size_t i; + size_t j; + libxl_physinfo physinfo; + int ret = -1; + + 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, maxmem) < 0) + break; + if (*p) { + switch (*p) { + case 'm': + case 'M': + multiplier = 1024; + break; + case 'g': + case 'G': + multiplier = 1024 * 1024; + break; + } + } + *maxmem = *maxmem * multiplier; + ret = 0; + goto cleanup; + } + } + } + + physmem: + /* No 'max' specified in dom0_mem, so dom0 can use all physical memory */ + libxl_physinfo_init(&physinfo); + if (libxl_get_physinfo(cfg->ctx, &physinfo)) { + VIR_WARN("libxl_get_physinfo failed"); + goto cleanup; + } + *maxmem = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024; + libxl_physinfo_dispose(&physinfo); + ret = 0; + + 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..4942171 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -174,6 +174,10 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg, const char *filename); int +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, + unsigned long long *maxmem); + +int libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev); int libxlMakeNic(virDomainDefPtr def, diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 040b986..1558d5b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -576,6 +576,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver) virDomainObjPtr vm = NULL; virDomainDefPtr oldDef = NULL; libxl_dominfo d_info; + unsigned long long maxmem; int ret = -1; libxl_dominfo_init(&d_info); @@ -615,7 +616,9 @@ 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); + if (libxlDriverGetDom0MaxmemConf(cfg, &maxmem) < 0) + maxmem = d_info.current_memkb; + virDomainDefSetMemoryTotal(vm->def, maxmem); ret = 0; -- 2.9.2
On 02/08/2017 10:24 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>
Argh, sorry. Failed to mention one (non-blocking) observation in the patch about the parsing of "dom0_mem". Regardless, Acked-by: Joao Martins <joao.m.martins@oracle.com>
---
V2: Check return value of libxl_get_physinfo and libxlDriverGetDom0MaxmemConf.
src/libxl/libxl_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_conf.h | 4 +++ src/libxl/libxl_driver.c | 5 +++- 3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 28b05e1..7e30c7d 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" @@ -1533,6 +1534,82 @@ 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. + */ +int +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, + unsigned long long *maxmem) +{ + char **cmd_tokens = NULL; + char **mem_tokens = NULL; + size_t i; + size_t j; + libxl_physinfo physinfo; + int ret = -1; + + 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; Hmm, perhaps worth a comment here (or in the command at the top of the function) because if the parameter doesn't have a suffix it defaults to kilobytes (as per Xen command line docs).
+ + while (c_isdigit(*p)) + p++; + if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, maxmem) < 0) + break; + if (*p) { + switch (*p) { + case 'm': + case 'M': + multiplier = 1024; + break; + case 'g': + case 'G': + multiplier = 1024 * 1024; + break; + } + } + *maxmem = *maxmem * multiplier; + ret = 0; + goto cleanup; This seems to match xen's parsing of the parameter that is for "max=4GG" or "max=4GM" the latter suffix will be ignored (thus considering 4G only). I was looking at the Xen's code of dom0_mem parsing and it's documentation - and noticed that we don't support Terabytes in the parsing above. Though this is only since Xen 4.5.
+ } + } + } + + physmem: + /* No 'max' specified in dom0_mem, so dom0 can use all physical memory */ + libxl_physinfo_init(&physinfo); + if (libxl_get_physinfo(cfg->ctx, &physinfo)) { + VIR_WARN("libxl_get_physinfo failed"); + goto cleanup; + } + *maxmem = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024; + libxl_physinfo_dispose(&physinfo); + ret = 0; + + 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..4942171 100644 --- a/src/libxl/libxl_conf.h +++ b/src/libxl/libxl_conf.h @@ -174,6 +174,10 @@ int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg, const char *filename);
int +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, + unsigned long long *maxmem); + +int libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev); int libxlMakeNic(virDomainDefPtr def, diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c index 040b986..1558d5b 100644 --- a/src/libxl/libxl_driver.c +++ b/src/libxl/libxl_driver.c @@ -576,6 +576,7 @@ libxlAddDom0(libxlDriverPrivatePtr driver) virDomainObjPtr vm = NULL; virDomainDefPtr oldDef = NULL; libxl_dominfo d_info; + unsigned long long maxmem; int ret = -1;
libxl_dominfo_init(&d_info); @@ -615,7 +616,9 @@ 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); + if (libxlDriverGetDom0MaxmemConf(cfg, &maxmem) < 0) + maxmem = d_info.current_memkb; + virDomainDefSetMemoryTotal(vm->def, maxmem);
ret = 0;
On 02/09/2017 12:39 PM, Joao Martins wrote:
On 02/08/2017 10:24 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>
Argh, sorry. Failed to mention one (non-blocking) observation in the patch about the parsing of "dom0_mem". Regardless,
Acked-by: Joao Martins <joao.m.martins@oracle.com>
---
V2: Check return value of libxl_get_physinfo and libxlDriverGetDom0MaxmemConf.
src/libxl/libxl_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_conf.h | 4 +++ src/libxl/libxl_driver.c | 5 +++- 3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 28b05e1..7e30c7d 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" @@ -1533,6 +1534,82 @@ 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. + */ +int +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, + unsigned long long *maxmem) +{ + char **cmd_tokens = NULL; + char **mem_tokens = NULL; + size_t i; + size_t j; + libxl_physinfo physinfo; + int ret = -1; + + 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; Hmm, perhaps worth a comment here (or in the command at the top of the function) because if the parameter doesn't have a suffix it defaults to kilobytes (as per Xen command line docs).
+ + while (c_isdigit(*p)) + p++; + if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, maxmem) < 0) + break; + if (*p) { + switch (*p) { + case 'm': + case 'M': + multiplier = 1024; + break; + case 'g': + case 'G': + multiplier = 1024 * 1024; + break; + } + } + *maxmem = *maxmem * multiplier; + ret = 0; + goto cleanup; This seems to match xen's parsing of the parameter that is for "max=4GG" or "max=4GM" the latter suffix will be ignored (thus considering 4G only). I was looking at the Xen's code of dom0_mem parsing and it's documentation - and noticed that we don't support Terabytes in the parsing above. Though this is only since Xen 4.5. To avoid ambiguity here I meant the snippet above doesn't Terabytes, but its correspondent support on Xen is only since 4.5.
Joao
Joao Martins wrote:
On 02/08/2017 10:24 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>
Argh, sorry. Failed to mention one (non-blocking) observation in the patch about the parsing of "dom0_mem". Regardless,
Acked-by: Joao Martins <joao.m.martins@oracle.com>
---
V2: Check return value of libxl_get_physinfo and libxlDriverGetDom0MaxmemConf.
src/libxl/libxl_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_conf.h | 4 +++ src/libxl/libxl_driver.c | 5 +++- 3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 28b05e1..7e30c7d 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" @@ -1533,6 +1534,82 @@ 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. + */ +int +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, + unsigned long long *maxmem) +{ + char **cmd_tokens = NULL; + char **mem_tokens = NULL; + size_t i; + size_t j; + libxl_physinfo physinfo; + int ret = -1; + + 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; Hmm, perhaps worth a comment here (or in the command at the top of the function) because if the parameter doesn't have a suffix it defaults to kilobytes (as per Xen command line docs).
I've added another sentence to the comment at the top of the function.
+ + while (c_isdigit(*p)) + p++; + if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, maxmem) < 0) + break; + if (*p) { + switch (*p) { + case 'm': + case 'M': + multiplier = 1024; + break; + case 'g': + case 'G': + multiplier = 1024 * 1024; + break; + } + } + *maxmem = *maxmem * multiplier; + ret = 0; + goto cleanup; This seems to match xen's parsing of the parameter that is for "max=4GG" or "max=4GM" the latter suffix will be ignored (thus considering 4G only). I was looking at the Xen's code of dom0_mem parsing and it's documentation - and noticed that we don't support Terabytes in the parsing above. Though this is only since Xen 4.5.
Good point. I've included support for the terabyte suffix. How about the below diff squashed in? Regards, Jim --- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1564,6 +1564,8 @@ 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 + * Supported unit suffixes are [bBkKmMgGtT]. If not specified the default + * unit is kilobytes. * * If not constrained by the user, dom0 can effectively use all host memory. * This function returns the configured maximum memory for dom0 in kilobytes, @@ -1609,6 +1611,10 @@ libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, case 'G': multiplier = 1024 * 1024; break; + case 't': + case 'T': + multiplier = 1024 * 1024 * 1024; + break; } } *maxmem = *maxmem * multiplier;
On 02/09/2017 04:58 PM, Jim Fehlig wrote:
Joao Martins wrote:
On 02/08/2017 10:24 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>
Argh, sorry. Failed to mention one (non-blocking) observation in the patch about the parsing of "dom0_mem". Regardless,
Acked-by: Joao Martins <joao.m.martins@oracle.com>
---
V2: Check return value of libxl_get_physinfo and libxlDriverGetDom0MaxmemConf.
src/libxl/libxl_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_conf.h | 4 +++ src/libxl/libxl_driver.c | 5 +++- 3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 28b05e1..7e30c7d 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" @@ -1533,6 +1534,82 @@ 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. + */ +int +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, + unsigned long long *maxmem) +{ + char **cmd_tokens = NULL; + char **mem_tokens = NULL; + size_t i; + size_t j; + libxl_physinfo physinfo; + int ret = -1; + + 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; Hmm, perhaps worth a comment here (or in the command at the top of the function) because if the parameter doesn't have a suffix it defaults to kilobytes (as per Xen command line docs).
I've added another sentence to the comment at the top of the function.
+ + while (c_isdigit(*p)) + p++; + if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, maxmem) < 0) + break; + if (*p) { + switch (*p) { + case 'm': + case 'M': + multiplier = 1024; + break; + case 'g': + case 'G': + multiplier = 1024 * 1024; + break; + } + } + *maxmem = *maxmem * multiplier; + ret = 0; + goto cleanup; This seems to match xen's parsing of the parameter that is for "max=4GG" or "max=4GM" the latter suffix will be ignored (thus considering 4G only). I was looking at the Xen's code of dom0_mem parsing and it's documentation - and noticed that we don't support Terabytes in the parsing above. Though this is only since Xen 4.5.
Good point. I've included support for the terabyte suffix. How about the below diff squashed in?
Looking good indeed, and with that diff I have no further comments: Acked-by: Joao Martins <joao.m.martins@oracle.com>
Regards, Jim
--- a/src/libxl/libxl_conf.c +++ b/src/libxl/libxl_conf.c @@ -1564,6 +1564,8 @@ 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 + * Supported unit suffixes are [bBkKmMgGtT]. If not specified the default + * unit is kilobytes. * * If not constrained by the user, dom0 can effectively use all host memory. * This function returns the configured maximum memory for dom0 in kilobytes, @@ -1609,6 +1611,10 @@ libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, case 'G': multiplier = 1024 * 1024; break; + case 't': + case 'T': + multiplier = 1024 * 1024 * 1024; + break; } } *maxmem = *maxmem * multiplier;
Joao Martins wrote:
On 02/09/2017 04:58 PM, Jim Fehlig wrote:
Joao Martins 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> Argh, sorry. Failed to mention one (non-blocking) observation in the patch about
On 02/08/2017 10:24 PM, Jim Fehlig wrote: the parsing of "dom0_mem". Regardless,
Acked-by: Joao Martins <joao.m.martins@oracle.com>
---
V2: Check return value of libxl_get_physinfo and libxlDriverGetDom0MaxmemConf.
src/libxl/libxl_conf.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++ src/libxl/libxl_conf.h | 4 +++ src/libxl/libxl_driver.c | 5 +++- 3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c index 28b05e1..7e30c7d 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" @@ -1533,6 +1534,82 @@ 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. + */ +int +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg, + unsigned long long *maxmem) +{ + char **cmd_tokens = NULL; + char **mem_tokens = NULL; + size_t i; + size_t j; + libxl_physinfo physinfo; + int ret = -1; + + 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; Hmm, perhaps worth a comment here (or in the command at the top of the function) because if the parameter doesn't have a suffix it defaults to kilobytes (as per Xen command line docs). I've added another sentence to the comment at the top of the function.
+ + while (c_isdigit(*p)) + p++; + if (virStrToLong_ull(mem_tokens[j] + 4, &p, 10, maxmem) < 0) + break; + if (*p) { + switch (*p) { + case 'm': + case 'M': + multiplier = 1024; + break; + case 'g': + case 'G': + multiplier = 1024 * 1024; + break; + } + } + *maxmem = *maxmem * multiplier; + ret = 0; + goto cleanup; This seems to match xen's parsing of the parameter that is for "max=4GG" or "max=4GM" the latter suffix will be ignored (thus considering 4G only). I was looking at the Xen's code of dom0_mem parsing and it's documentation - and noticed that we don't support Terabytes in the parsing above. Though this is only since Xen 4.5. Good point. I've included support for the terabyte suffix. How about the below diff squashed in?
Looking good indeed, and with that diff I have no further comments:
Acked-by: Joao Martins <joao.m.martins@oracle.com>
Thanks. I've pushed these patches now. Regards, Jim
participants (2)
-
Jim Fehlig -
Joao Martins