noon
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(a)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(a)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(a)oracle.com>
Thanks. I've pushed these patches now.
Regards,
Jim