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

Re: [libvirt] [RFC PATCH v1 6/7] deploy the newly introduced virCgroupItem.



On Wed, Jan 16, 2013 at 10:10:50AM +0000, Daniel P. Berrange wrote:
> On Wed, Jan 16, 2013 at 10:53:08AM +0800, Hu Tao wrote:
> > diff --git a/daemon/libvirtd.c b/daemon/libvirtd.c
> > index 7cb99b1..92e3292 100644
> > --- a/daemon/libvirtd.c
> > +++ b/daemon/libvirtd.c
> > @@ -1442,6 +1442,9 @@ int main(int argc, char **argv) {
> >          VIR_FORCE_CLOSE(statuswrite);
> >      }
> >  
> > +    if (virCgroupInit() < 0)
> > +        goto cleanup;
> > +
> 
> I don't like this addition. Our aim has been to *remove* the need
> to global initializers like this, not add new ones. AFAICT the
> reason you needed to add this is because you removed code from
> the individual drivers which would initialize the cgroups they
> required.

I think I can eliminate this init/uninit thing.

> 
> > diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
> > index 2ac338c..23ff2c9 100644
> > --- a/src/conf/domain_conf.h
> > +++ b/src/conf/domain_conf.h
> > @@ -48,6 +48,7 @@
> >  # include "device_conf.h"
> >  # include "virbitmap.h"
> >  # include "virstoragefile.h"
> > +# include "vircgroup.h"
> >  
> >  /* forward declarations of all device types, required by
> >   * virDomainDeviceDef
> > @@ -1843,6 +1844,10 @@ struct _virDomainDef {
> >  
> >      /* Application-specific custom metadata */
> >      xmlNodePtr metadata;
> > +
> > +    virCgroupItemPtr cgroup[VIR_CGROUP_CONTROLLER_LAST];
> > +    virCgroupItemPtr (*vcpuCgroups)[VIR_CGROUP_CONTROLLER_LAST];
> > +    virCgroupItemPtr emulatorCgroup[VIR_CGROUP_CONTROLLER_LAST];
> >  };
> 
> Two things here. First, this is driver state and so should *not* be
> in the virDomainDef struct - it should be in the driver specific
> private data structs.

Agreed.

> 
> Second, the new cgroups API you've got here is really bad. It was
> an explicit design decision in the original API to *not* expose
> the concept of individual cgroup controllers to the driver APIs.
> The only time the drivers should can about individual controllers
> is when they first create the cgroup and decide which controllers
> they want to use. From then onwards the virCgroupPtr APIs should
> just 'do the right thing'.

The explanation is helpful. Fortunately I think the new virCgroup
can leave the original API unchanged(let me try in v2).

What are important in the new virCgroup are:

  - the lazy creation of cgroup directories, despite of what level
    they are.
  - cgroup directories are removed if no one is using the corresponding
    virCgroup.

Do you think it's worth doing it? If yes, can you review patch 5 about
the new implementation? (forget about the API change)

> 
> >      }
> >  
> > +    cgroup = vm->def->cgroup[VIR_CGROUP_CONTROLLER_DEVICES];
> > +
> >      if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_DEVICES)) {
> >          qemuCgroupData data = { vm, cgroup };
> >          rc = virCgroupDenyAllDevices(cgroup);
> > @@ -300,6 +301,8 @@ int qemuSetupCgroup(virQEMUDriverPtr driver,
> >          }
> >      }
> >  
> > +    cgroup = vm->def->cgroup[VIR_CGROUP_CONTROLLER_BLKIO];
> > +
> >      if (vm->def->blkio.weight != 0) {
> >          if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_BLKIO)) {
> >              rc = virCgroupSetBlkioWeight(cgroup, vm->def->blkio.weight);
> > @@ -339,6 +342,8 @@ int qemuSetupCgroup(virQEMUDriverPtr driver,
> >          }
> >      }
> >  
> > +    cgroup = vm->def->cgroup[VIR_CGROUP_CONTROLLER_MEMORY];
> > +
> >      if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_MEMORY)) {
> >          unsigned long long hard_limit = vm->def->mem.hard_limit;
> >  
> > @@ -392,6 +397,8 @@ int qemuSetupCgroup(virQEMUDriverPtr driver,
> >          VIR_WARN("Could not autoset a RSS limit for domain %s", vm->def->name);
> >      }
> >  
> > +    cgroup = vm->def->cgroup[VIR_CGROUP_CONTROLLER_CPU];
> > +
> >      if (vm->def->cputune.shares != 0) {
> >          if (qemuCgroupControllerActive(driver, VIR_CGROUP_CONTROLLER_CPU)) {
> >              rc = virCgroupSetCpuShares(cgroup, vm->def->cputune.shares);
> > @@ -407,6 +414,8 @@ int qemuSetupCgroup(virQEMUDriverPtr driver,
> >          }
> >      }
> >  
> > +    cgroup = vm->def->cgroup[VIR_CGROUP_CONTROLLER_CPUSET];
> > +
> >      if ((vm->def->numatune.memory.nodemask ||
> >           (vm->def->numatune.memory.placement_mode ==
> >            VIR_DOMAIN_NUMATUNE_MEM_PLACEMENT_MODE_AUTO)) &&
> 
> These 4 additions are a perfect example of what this new API design is
> awful. The drivers now have to remember which controller they need to
> use for which tunable.

If the original API is kept, these will be the form of:

cgroup = vm->cgroup;

...

doSomethingWithCgroup(cgroup)

...

Is this acceptable?

> 
> I'm not going to review any more because this change is fatally flawed
> from a design POV.
> 
> Regards,
> Daniel
> -- 
> |: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
> |: http://libvirt.org              -o-             http://virt-manager.org :|
> |: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
> |: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|


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