Re: [PATCH 2/2] qemu: Migrate memory on numatune change

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

On Thu, Mar 19, 2015 at 07:30:26PM -0400, John Ferlan wrote:
> On 03/11/2015 08:39 AM, Martin Kletzander wrote:
> > We've never set the cpuset.memory_migrate value to anything, keeping it
> > on default.  However, we allow changing cpuset.mems on live domain.
> > That setting, however, don't have any consequence on a domain unless
> > it's going to allocate new memory.
> >
> > I managed to make 'virsh numatune' move all the memory to any node I
> > wanted even without disabling libnuma's numa_set_membind(), so this
> > should be safe to use with it as well.
> >
> > Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1198497
> >
> > Signed-off-by: Martin Kletzander <mkletzan@xxxxxxxxxx>
> > ---
> >  src/qemu/qemu_cgroup.c | 36 +++++++++++++++++++++++++++++++++++-
> >  1 file changed, 35 insertions(+), 1 deletion(-)
> >
> > diff --git a/src/qemu/qemu_cgroup.c b/src/qemu/qemu_cgroup.c
> > index 2b5a182..976a8c4 100644
> > --- a/src/qemu/qemu_cgroup.c
> > +++ b/src/qemu/qemu_cgroup.c
> > @@ -1,7 +1,7 @@
> >  /*
> >   * qemu_cgroup.c: QEMU cgroup management
> >   *
> > - * Copyright (C) 2006-2014 Red Hat, Inc.
> > + * Copyright (C) 2006-2015 Red Hat, Inc.
> >   * Copyright (C) 2006 Daniel P. Berrange
> >   *
> >   * This library is free software; you can redistribute it and/or
> > @@ -652,6 +652,9 @@ qemuSetupCpusetCgroup(virDomainObjPtr vm,
> >      if (!virCgroupHasController(priv->cgroup, VIR_CGROUP_CONTROLLER_CPUSET))
> >          return 0;
> >
> > +    if (virCgroupSetCpusetMemoryMigrate(priv->cgroup, true) < 0)
> > +        return -1;
> > +
>
> Is this OK to set even if NUMA isn't enabled?  I didn't follow the
> callers all the way back and this isn't something I know a lot about, so
> I'm curious. I do see from the man page it's available since Linux
> 2.6.16, so one would hope/think that we're OK in assuming we can access
> it...

This has nothing to do with guest's NUMA, but with host's.  If the
host is not a NUMA machine, then this should have no effect.

> Is it possible that someone wouldn't want to migrate the memory?  That
> is should this be a configurable attribute?

Well, by default there is nothing to migrate.  Migration can happen if
someone changes the setting of cpuset.mems (that controls on what NUMA
nodes should threads' memory reside).  Without memory_migrate that
has effect only on new allocations.  So if you have a guest that
already allocated all of its memory and management application wants
to move this guest to some other NUMA node nothing happens.  On the
other hand, if you set memory_migrate and don't set cgroup.mems,
nothing happens (that's why I'm reading and setting the same values
for all sub-cgroups).

> >      if (vm->def->cpumask ||
> >          (vm->def->placement_mode == VIR_DOMAIN_CPU_PLACEMENT_MODE_AUTO)) {
> >
> > @@ -792,9 +795,12 @@ static void
> >  qemuRestoreCgroupState(virDomainObjPtr vm)
> >  {
> >      char *mem_mask = NULL;
> > +    char *nodeset = NULL;
> >      int empty = -1;
> >      qemuDomainObjPrivatePtr priv = vm->privateData;
> > +    size_t i = 0;
> >      virBitmapPtr all_nodes;
> > +    virCgroupPtr cgroup_temp = NULL;
> >
> >      if (!(all_nodes = virNumaGetHostNodeset()))
> >          goto error;
> > @@ -809,9 +815,37 @@ qemuRestoreCgroupState(virDomainObjPtr vm)
>
> So this is the path for currently running guests to be adjusted on
> livirtd restart after install, right?
>
> >      if (virCgroupSetCpusetMems(priv->cgroup, mem_mask) < 0)
> >          goto error;
> >
> > +    for (i = 0; i < priv->nvcpupids; i++) {
> > +        if (virCgroupNewVcpu(priv->cgroup, i, false, &cgroup_temp) < 0 ||
> > +            virCgroupSetCpusetMemoryMigrate(cgroup_temp, true) < 0 ||
> > +            virCgroupGetCpusetMems(cgroup_temp, &nodeset) < 0 ||
> > +            virCgroupSetCpusetMems(cgroup_temp, nodeset) < 0)
> > +            goto cleanup;
> > +
> > +        virCgroupFree(&cgroup_temp);
> > +    }
> > +
> > +    for (i = 0; i < priv->niothreadpids; i++) {
> > +        if (virCgroupNewIOThread(priv->cgroup, i, false, &cgroup_temp) < 0 ||
>
> cgroup iothread id's are 1..n, so I believe this should be 'i + 1'

Yes, once again I felt for the contraption in this.  That shall not
happen henceforth!

> simple enough to test by adding <iothreads>2</iothreads> to your domain
> definition and making sure this works...

I remember trying with <iothreads>1</iothreads>, there must have been
no allocation as I only checked that the memory moved -- my bad.

> Beyond that - seems reasonable to me
>
> John
> > +            virCgroupSetCpusetMemoryMigrate(cgroup_temp, true) < 0 ||
> > +            virCgroupGetCpusetMems(cgroup_temp, &nodeset) < 0 ||
> > +            virCgroupSetCpusetMems(cgroup_temp, nodeset) < 0)
> > +            goto cleanup;
> > +
> > +        virCgroupFree(&cgroup_temp);
> > +    }
> > +
> > +    if (virCgroupNewEmulator(priv->cgroup, false, &cgroup_temp) < 0 ||
> > +        virCgroupSetCpusetMemoryMigrate(cgroup_temp, true) < 0 ||
> > +        virCgroupGetCpusetMems(cgroup_temp, &nodeset) < 0 ||
> > +        virCgroupSetCpusetMems(cgroup_temp, nodeset) < 0)
> > +        goto cleanup;
> > +
> >   cleanup:
> >      VIR_FREE(mem_mask);
> > +    VIR_FREE(nodeset);
> >      virBitmapFree(all_nodes);
> > +    virCgroupFree(&cgroup_temp);
> >      return;
> >
> >   error:

Attachment: pgpH2nianOGaV.pgp
Description: PGP signature

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