Joao Martins wrote:
>
> 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@xxxxxxxx>
>>> 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@xxxxxxxxxx>
>>>
>>>> ---
>>>>
>>>> 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@xxxxxxxxxx>
Thanks. I've pushed these patches now.
Regards,
Jim
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
