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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list