On Thu, Jul 19, 2012 at 04:37:14PM +0800, Daniel Veillard wrote:
On Wed, Jul 18, 2012 at 05:32:30PM +0100, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
Move the cgroup setup code out of the lxc_controller.c file and into lxc_cgroup.{c,h}. This reduces the size of the lxc_controller.c file and paves the way to invoke cgroup setup from lxc_driver.c instead of lxc_controller.c in the future
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- po/POTFILES.in | 1 + src/Makefile.am | 2 + src/lxc/lxc_cgroup.c | 272 ++++++++++++++++++++++++++++++++++++++++++++++ src/lxc/lxc_cgroup.h | 29 +++++ src/lxc/lxc_controller.c | 232 +-------------------------------------- 5 files changed, 306 insertions(+), 230 deletions(-) create mode 100644 src/lxc/lxc_cgroup.c create mode 100644 src/lxc/lxc_cgroup.h
+int virLXCCgroupSetup(virDomainDefPtr def) +{ + virCgroupPtr driver = NULL; + virCgroupPtr cgroup = NULL; + int rc = -1; + + rc = virCgroupForDriver("lxc", &driver, 1, 0); + if (rc != 0) { + /* Skip all if no driver cgroup is configured */ + if (rc == -ENXIO || rc == -ENOENT) + return 0; + + virReportSystemError(-rc, "%s", + _("Unable to get cgroup for driver")); + return rc; + } + + rc = virCgroupForDomain(driver, def->name, &cgroup, 1); + if (rc != 0) { + virReportSystemError(-rc, + _("Unable to create cgroup for domain %s"), + def->name); + goto cleanup; + } + + if (virLXCCgroupSetupCpuTune(def, cgroup) < 0) + goto cleanup; + + if (virLXCCgroupSetupBlkioTune(def, cgroup) < 0) + goto cleanup; + + if (virLXCCgroupSetupMemTune(def, cgroup) < 0) + goto cleanup; + + if (virLXCCgroupSetupDeviceACL(def, cgroup) < 0) + goto cleanup; + + rc = virCgroupAddTask(cgroup, getpid()); + if (rc != 0) { + virReportSystemError(-rc, + _("Unable to add task %d to cgroup for domain %s"), + getpid(), def->name); + } + +cleanup: + virCgroupFree(&cgroup); + virCgroupFree(&driver); + + return rc; +}
diff --git a/src/lxc/lxc_controller.c b/src/lxc/lxc_controller.c index 7a1ce14..4777d51 100644 --- a/src/lxc/lxc_controller.c +++ b/src/lxc/lxc_controller.c @@ -58,6 +58,7 @@
#include "lxc_conf.h" #include "lxc_container.h" +#include "lxc_cgroup.h" #include "virnetdev.h" #include "virnetdevveth.h" #include "memory.h" @@ -72,13 +73,6 @@
#define VIR_FROM_THIS VIR_FROM_LXC
-struct cgroup_device_policy { - char type; - int major; - int minor; -}; - - typedef struct _virLXCControllerConsole virLXCControllerConsole; typedef virLXCControllerConsole *virLXCControllerConsolePtr; struct _virLXCControllerConsole { @@ -127,8 +121,6 @@ struct _virLXCController {
/* Server socket */ virNetServerPtr server; - - virCgroupPtr cgroup; };
static void virLXCControllerFree(virLXCControllerPtr ctrl); @@ -201,8 +193,6 @@ static void virLXCControllerStopInit(virLXCControllerPtr ctrl) virLXCControllerCloseLoopDevices(ctrl, true); virPidAbort(ctrl->initpid); ctrl->initpid = 0; - - virCgroupFree(&ctrl->cgroup); }
I'm a bit surprized there. The cgroup is moved out of that structure so that's fine, but i would have expected to see somewhere in the patch another chunk of code outside of the 2 new modules calling the free of the new structure, but it's nowhere to be found in src/lxc/lxc_controller.c diff. Something escapes me, where do we free those now ?
If you look at the virLXCControllerSetupResourceLimits() method below, you'll see we call virCgroupForDomain in the old code to create 'ctrl->cgroup', and free it in virLXCControllerStopInit(). In the new virLXCCgroupSetup() method above, we create & free the cgroup object within the scope of that method. So we don't need final cleanup when stopping the controler anymore.
static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl) { - virCgroupPtr driver; - int rc = -1;
if (virLXCControllerSetupCpuAffinity(ctrl) < 0) return -1; @@ -722,48 +535,7 @@ static int virLXCControllerSetupResourceLimits(virLXCControllerPtr ctrl) if (virLXCControllerSetupNUMAPolicy(ctrl) < 0) return -1;
- rc = virCgroupForDriver("lxc", &driver, 1, 0); - if (rc != 0) { - /* Skip all if no driver cgroup is configured */ - if (rc == -ENXIO || rc == -ENOENT) - return 0; - - virReportSystemError(-rc, "%s", - _("Unable to get cgroup for driver")); - return rc; - } - - rc = virCgroupForDomain(driver, ctrl->def->name, &ctrl->cgroup, 1); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to create cgroup for domain %s"), - ctrl->def->name); - goto cleanup; - } - - if (virLXCControllerSetupCgroupsCpuTune(ctrl) < 0) - goto cleanup; - - if (virLXCControllerSetupCgroupsBlkioTune(ctrl) < 0) - goto cleanup; - - if (virLXCControllerSetupCgroupsMemTune(ctrl) < 0) - goto cleanup; - - if (virLXCControllerSetupCgroupsDeviceACL(ctrl) < 0) - goto cleanup; - - rc = virCgroupAddTask(ctrl->cgroup, getpid()); - if (rc != 0) { - virReportSystemError(-rc, - _("Unable to add task %d to cgroup for domain %s"), - getpid(), ctrl->def->name); - } - -cleanup: - virCgroupFree(&driver); - - return rc; + return virLXCCgroupSetup(ctrl->def); }
So we do call the Setup() but never the free ... I'm missing something :-) ACK pending on clarification !
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 :|