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@xxxxxxxxxx>
> >
> > 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@xxxxxxxxxx>
> > ---
> > 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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list