Re: [libvirt] [PATCH 09/16] Move cgroup setup code out of lxc_controller.c

On Wed, Jul 18, 2012 at 05:32:30PM +0100, Daniel P. Berrange wrote: > From: "Daniel P. Berrange" <berrange(a)redhat.com&gt; > > 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(a)redhat.com&gt; > --- > 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 ?

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