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.
> 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.
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'.
> }
>
> + 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.
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