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

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



Joao Martins wrote:
> On 02/08/2017 04:06 PM, Jim Fehlig wrote:
>> Joao Martins wrote:
>>> On 02/02/2017 10:31 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>
>>>> ---
>>>>  src/libxl/libxl_conf.c   | 75 ++++++++++++++++++++++++++++++++++++++++++++++++
>>>>  src/libxl/libxl_conf.h   |  3 ++
>>>>  src/libxl/libxl_driver.c |  2 +-
>>>>  3 files changed, 79 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/libxl/libxl_conf.c b/src/libxl/libxl_conf.c
>>>> index b5186f2..bfe0e92 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"
>>>> @@ -1530,6 +1531,80 @@ 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.
>>>> + */
>>>> +unsigned long long
>>>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg)
>>>> +{
>>>> +    char **cmd_tokens = NULL;
>>>> +    char **mem_tokens = NULL;
>>>> +    size_t i;
>>>> +    size_t j;
>>>> +    unsigned long long ret;
>>>> +    libxl_physinfo physinfo;
>>>> +
>>>> +    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, &ret) < 0)
>>>> +                    break;
>>>> +                if (*p) {
>>>> +                    switch (*p) {
>>>> +                    case 'k':
>>>> +                    case 'K':
>>>> +                        multiplier = 1024;
>>>> +                        break;
>>>> +                    case 'm':
>>>> +                    case 'M':
>>>> +                        multiplier = 1024 * 1024;
>>>> +                        break;
>>>> +                    case 'g':
>>>> +                    case 'G':
>>>> +                        multiplier = 1024 * 1024 * 1024;
>>>> +                        break;
>>>> +                    }
>>>> +                }
>>>> +                ret = (ret * multiplier) / 1024;
>>>> +                goto cleanup;
>>>> +            }
>>>> +        }
>>>> +    }
>>>> +
>>>> + physmem:
>>>> +    /* No 'max' specified in dom0_mem, so dom0 can use all physical memory */
>>>> +    libxl_physinfo_init(&physinfo);
>>>> +    libxl_get_physinfo(cfg->ctx, &physinfo);
>>> Despite being an unlikely event, libxl_get_physinfo can fail here - I think you
>>> need to check the return value here.
>> Bummer. I was hoping this function could always return a sane value for dom0 max
>> memory. But I'm not really sure what that would be if libxl_get_physinfo failed.
>>
> Yeah, We probably have some serious concerns if this fails. And given that
> libxlAddDom0 happens early while bootstrapping the libxl module, it might be
> good to warn the user (following similar pattern throughout the rest of the code):
> 
> virReportError(VIR_ERR_INTERNAL_ERROR, "%s",
>                _("libxl_get_physinfo_info failed"));
> 
> or perhaps simply VIR_WARN?
> 
>>>> +    ret = (physinfo.total_pages * cfg->verInfo->pagesize) / 1024;
>>>> +    libxl_physinfo_dispose(&physinfo);
>>>> +
>>>> + 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..c4ddbfe 100644
>>>> --- a/src/libxl/libxl_conf.h
>>>> +++ b/src/libxl/libxl_conf.h
>>>> @@ -173,6 +173,9 @@ libxlDriverNodeGetInfo(libxlDriverPrivatePtr driver,
>>>>  int libxlDriverConfigLoadFile(libxlDriverConfigPtr cfg,
>>>>                                const char *filename);
>>>>  
>>>> +unsigned long long
>>>> +libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg);
>>>> +
>>>>  int
>>>>  libxlMakeDisk(virDomainDiskDefPtr l_dev, libxl_device_disk *x_dev);
>>>>  int
>>>> diff --git a/src/libxl/libxl_driver.c b/src/libxl/libxl_driver.c
>>>> index 921cc93..e54b3b7 100644
>>>> --- a/src/libxl/libxl_driver.c
>>>> +++ b/src/libxl/libxl_driver.c
>>>> @@ -615,7 +615,7 @@ 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);
>>>> +    virDomainDefSetMemoryTotal(vm->def, libxlDriverGetDom0MaxmemConf(cfg));
>>> And perhaps checking here too on whether "libxlDriverGetDom0MaxmemConf(cfg) < 0" ?
>> ATM, libxlDriverGetDom0MaxmemConf returns an unsigned long long. I could change
>> the signature to
>>
>> int
>> libxlDriverGetDom0MaxmemConf(libxlDriverConfigPtr cfg,
>>                              unsigned long long *maxmem)
>>
>> but again I'm not really sure what to do with maxmem in case of failure. Perhaps
>> just set it to current_memkb?
> Yeap, that's what I was thinking too.

Thanks for the comments. I've sent a V2 incorporating them

https://www.redhat.com/archives/libvir-list/2017-February/msg00349.html

Regards,
Jim


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