[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]