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

[Daniel Veillard]

On Thu, Jul 19, 2012 at 09:54:31AM +0100, Daniel P. Berrange wrote:
> 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 at 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 at 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.

  okidoc, ACK :-)

Daniel

-- 
Daniel Veillard      | libxml Gnome XML XSLT toolkit  http://xmlsoft.org/
daniel at veillard.com  | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library  http://libvirt.org/