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" <berra...@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 <berra...@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/
dan...@veillard.com | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
