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

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

 



On Fri, Feb 20, 2015 at 10:15:00 -0500, John Ferlan wrote:
> 
> 
> 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?

I'll try using GetMemoryAvailable for the next version.

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

I didn't need it currently. I'll look into it separately if I find it
necessary.

> 
> > +
> > +
> >  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.

The memory modules will be compared individually so it doesn't matter
much.

> 
> 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?

They are. I'll change them to use the Current/Available accessor
although it won't make a difference for now as memory hotplug is
explicitly forbidden for drivers other than qemu.

> 
> John

Peter

Attachment: signature.asc
Description: Digital signature

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list

[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]