Re: [libvirt] [PATCH 4/7] conf: Replace access to def-mem.max_balloon with accessor functions

On 02/18/2015 09:16 AM, Peter Krempa wrote: > As there are two possible approaches to define a domain's memory size - > one used with legacy, non-NUMA VMs configured in the <memory> element > and per-node based approach on NUMA machines - the user needs to make > sure that both are specified correctly in the NUMA case. > > To avoid this burden on the user I'd like to replace the NUMA case with > automatic totaling of the memory size. To achieve this I need to replace > direct access to the virDomainMemtune's 'max_balloon' field with > two separate getters depending on the desired size. > > The two sizes are needed as: > 1) Startup memory size doesn't include memory modules in some > hypervisors. > 2) After startup these count as the usable memory size. > > Note that the comments for the functions are future aware and document > state that will be present after a few later patches. > --- > src/conf/domain_conf.c | 66 ++++++++++++++++++++++++++++++++++------ > src/conf/domain_conf.h | 4 +++ > src/hyperv/hyperv_driver.c | 2 +- > src/libvirt_private.syms | 3 ++ > src/libxl/libxl_conf.c | 2 +- > src/libxl/libxl_driver.c | 8 ++--- > src/lxc/lxc_cgroup.c | 2 +- > src/lxc/lxc_driver.c | 10 +++--- > src/lxc/lxc_fuse.c | 4 +-- > src/lxc/lxc_native.c | 4 +-- > src/openvz/openvz_driver.c | 2 +- > src/parallels/parallels_driver.c | 2 +- > src/parallels/parallels_sdk.c | 12 ++++---- > src/phyp/phyp_driver.c | 11 ++++--- > src/qemu/qemu_command.c | 18 +++++++---- > src/qemu/qemu_driver.c | 19 ++++++------ > src/qemu/qemu_hotplug.c | 8 +++-- > src/qemu/qemu_process.c | 2 +- > src/test/test_driver.c | 8 ++--- > src/uml/uml_driver.c | 8 ++--- > src/vbox/vbox_common.c | 4 +-- > src/vmware/vmware_driver.c | 2 +- > src/vmx/vmx.c | 12 ++++---- > src/xen/xm_internal.c | 14 ++++----- > src/xenapi/xenapi_driver.c | 2 +- > src/xenapi/xenapi_utils.c | 4 +-- > src/xenconfig/xen_common.c | 8 +++-- > src/xenconfig/xen_sxpr.c | 9 +++--- > 28 files changed, 160 insertions(+), 90 deletions(-) > > diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c > index 6ef499d..e95851a 100644 > --- a/src/conf/domain_conf.c > +++ b/src/conf/domain_conf.c > @@ -3172,24 +3172,26 @@ virDomainDefPostParseInternal(virDomainDefPtr def, > return -1; > } > > - if (def->mem.cur_balloon > def->mem.max_balloon) { > + if (def->mem.cur_balloon > virDomainDefGetMemoryCurrent(def)) { > /* Older libvirt could get into this situation due to > * rounding; if the discrepancy is less than 4MiB, we silently > * round down, otherwise we flag the issue. */ > if (VIR_DIV_UP(def->mem.cur_balloon, 4096) > > - VIR_DIV_UP(def->mem.max_balloon, 4096)) { > + VIR_DIV_UP(virDomainDefGetMemoryCurrent(def), 4096)) { > virReportError(VIR_ERR_XML_ERROR, > _("current memory '%lluk' exceeds " > "maximum '%lluk'"), > - def->mem.cur_balloon, def->mem.max_balloon); > + def->mem.cur_balloon, > + virDomainDefGetMemoryCurrent(def)); > return -1; > } else { > VIR_DEBUG("Truncating current %lluk to maximum %lluk", > - def->mem.cur_balloon, def->mem.max_balloon); > - def->mem.cur_balloon = def->mem.max_balloon; > + def->mem.cur_balloon, > + virDomainDefGetMemoryCurrent(def)); > + def->mem.cur_balloon = virDomainDefGetMemoryCurrent(def); > } > } else if (def->mem.cur_balloon == 0) { > - def->mem.cur_balloon = def->mem.max_balloon; > + def->mem.cur_balloon = virDomainDefGetMemoryCurrent(def); > } > > /* > @@ -6882,6 +6884,51 @@ virDomainParseMemory(const char *xpath, > } > > > +/** > + * virDomainDefGetMemoryInitial: > + * @def: domain definition > + * > + * Returns the size of the initial amount of guest memory. The initial amount > + * is the memory size is either the configured amount in the <memory> element > + * or the sum of memory sizes of NUMA nodes in case NUMA is enabled in @def. > + */ > +unsigned long long > +virDomainDefGetMemoryInitial(virDomainDefPtr def) > +{ > + return def->mem.max_balloon; > +} > + > + > +/** > + * virDomainDefSetMemoryInitial: > + * @def: domain definition > + * @size: size to set > + * > + * Sets the initial memory size in @def. > + */ > +void > +virDomainDefSetMemoryInitial(virDomainDefPtr def, > + unsigned long long size) > +{ > + def->mem.max_balloon = size; > +} Odd concept - changing Initial memory, but I don't have a better name in mind... > + > + > +/** > + * virDomainDefGetMemoryCurrent: > + * @def: domain definition > + * > + * Returns the current maximum memory size usable by the domain described by > + * @def. This size is a sum of size returned by virDomainDefGetMemoryInitial > + * and possible additional memory devices. > + */ > +unsigned long long > +virDomainDefGetMemoryCurrent(virDomainDefPtr def) > +{ > + return virDomainDefGetMemoryInitial(def); > +} Ok - so part of me expect this one to return mem.cur_balloon - perhaps a name change to GetMemoryUsable would help?

and of course begs the question why not bite the bullet and return mem.cur_balloon in a GetMemoryCurrent accessor...

> + > + > static int > virDomainControllerModelTypeFromString(const virDomainControllerDef *def, > const char *model) > @@ -15956,10 +16003,11 @@ virDomainDefCheckABIStability(virDomainDefPtr src, > goto error; > } > > - if (src->mem.max_balloon != dst->mem.max_balloon) { > + if (virDomainDefGetMemoryInitial(src) != virDomainDefGetMemoryInitial(dst)) { > virReportError(VIR_ERR_CONFIG_UNSUPPORTED, > _("Target domain max memory %lld does not match source %lld"), > - dst->mem.max_balloon, src->mem.max_balloon); > + virDomainDefGetMemoryInitial(dst), > + virDomainDefGetMemoryInitial(src)); > goto error; > } Since the "future" (I assume) is that MemoryCurrent will include the hot (un)plug memory - does it make sense to also compare src/dst MemoryCurrent values even though for now it's essentially the same value? Although I believe that gets done in your later series and it's hard to keep them in my memory.

Of the remaining uses of GetInitial and {Get|Set}Current the ones I was wondering about are: virLXCCgroupSetupMemTune libxlMakeDomBuildInfo phypBuildLpar Wouldn't they be the corollary to qemuBuildCommandLine w/r/t using Initial instead of Current in order to define the memory for the guest as it starts up?

John