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

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(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 ? 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.