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)