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

[Daniel P. Berrange]

On Thu, Jan 17, 2013 at 12:02:52PM +0800, Hu Tao wrote:
> 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)

Ok, that sounds more reasonable.


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