[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH V2 2/2] libxl: fix dom0 maximum memory setting



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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]