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 <jfeh...@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.mart...@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.mart...@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; > -- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list