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
>
> diff --git a/po/POTFILES.in b/po/POTFILES.in
> index 6ca40d9..2d5735a 100644
> --- a/po/POTFILES.in
> +++ b/po/POTFILES.in
> @@ -42,6 +42,7 @@ src/libvirt.c
> src/libvirt-qemu.c
> src/locking/lock_driver_sanlock.c
> src/locking/lock_manager.c
> +src/lxc/lxc_cgroup.c
> src/lxc/lxc_container.c
> src/lxc/lxc_conf.c
> src/lxc/lxc_controller.c
> diff --git a/src/Makefile.am b/src/Makefile.am
> index e18b0dc..9e16d06 100644
> --- a/src/Makefile.am
> +++ b/src/Makefile.am
> @@ -350,12 +350,14 @@ endif
> LXC_DRIVER_SOURCES = \
> lxc/lxc_conf.c lxc/lxc_conf.h \
> lxc/lxc_container.c lxc/lxc_container.h \
> + lxc/lxc_cgroup.c lxc/lxc_cgroup.h \
> lxc/lxc_domain.c lxc/lxc_domain.h \
> lxc/lxc_driver.c lxc/lxc_driver.h
>
> LXC_CONTROLLER_SOURCES = \
> lxc/lxc_conf.c lxc/lxc_conf.h \
> lxc/lxc_container.c lxc/lxc_container.h \
> + lxc/lxc_cgroup.c lxc/lxc_cgroup.h \
> lxc/lxc_controller.c
>
> SECURITY_DRIVER_APPARMOR_HELPER_SOURCES = \
> diff --git a/src/lxc/lxc_cgroup.c b/src/lxc/lxc_cgroup.c
> new file mode 100644
> index 0000000..ddd4901
> --- /dev/null
> +++ b/src/lxc/lxc_cgroup.c
> @@ -0,0 +1,272 @@
> +/*
> + * Copyright (C) 2010-2012 Red Hat, Inc.
> + * Copyright IBM Corp. 2008
> + *
> + * lxc_cgroup.c: LXC cgroup helpers
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#include <config.h>
> +
> +#include "lxc_cgroup.h"
> +#include "lxc_container.h"
> +#include "virterror_internal.h"
> +#include "logging.h"
> +#include "memory.h"
> +#include "cgroup.h"
> +
> +#define VIR_FROM_THIS VIR_FROM_LXC
> +
> +static int virLXCCgroupSetupCpuTune(virDomainDefPtr def,
> + virCgroupPtr cgroup)
> +{
> + int ret = -1;
> + if (def->cputune.shares != 0) {
> + int rc = virCgroupSetCpuShares(cgroup, def->cputune.shares);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to set io cpu shares for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> + }
> + if (def->cputune.quota != 0) {
> + int rc = virCgroupSetCpuCfsQuota(cgroup, def->cputune.quota);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to set io cpu quota for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> + }
> + if (def->cputune.period != 0) {
> + int rc = virCgroupSetCpuCfsPeriod(cgroup, def->cputune.period);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to set io cpu period for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> + }
> + ret = 0;
> +cleanup:
> + return ret;
> +}
> +
> +
> +static int virLXCCgroupSetupBlkioTune(virDomainDefPtr def,
> + virCgroupPtr cgroup)
> +{
> + int ret = -1;
> +
> + if (def->blkio.weight) {
> + int rc = virCgroupSetBlkioWeight(cgroup, def->blkio.weight);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to set Blkio weight for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> + }
> +
> + ret = 0;
> +cleanup:
> + return ret;
> +}
> +
> +
> +static int virLXCCgroupSetupMemTune(virDomainDefPtr def,
> + virCgroupPtr cgroup)
> +{
> + int ret = -1;
> + int rc;
> +
> + rc = virCgroupSetMemory(cgroup, def->mem.max_balloon);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to set memory limit for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> +
> + if (def->mem.hard_limit) {
> + rc = virCgroupSetMemoryHardLimit(cgroup, def->mem.hard_limit);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to set memory hard limit for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> + }
> +
> + if (def->mem.soft_limit) {
> + rc = virCgroupSetMemorySoftLimit(cgroup, def->mem.soft_limit);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to set memory soft limit for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> + }
> +
> + if (def->mem.swap_hard_limit) {
> + rc = virCgroupSetMemSwapHardLimit(cgroup, def->mem.swap_hard_limit);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to set swap hard limit for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> + }
> +
> + ret = 0;
> +cleanup:
> + return ret;
> +}
> +
> +
> +typedef struct _virLXCCgroupDevicePolicy virLXCCgroupDevicePolicy;
> +typedef virLXCCgroupDevicePolicy *virLXCCgroupDevicePolicyPtr;
> +
> +struct _virLXCCgroupDevicePolicy {
> + char type;
> + int major;
> + int minor;
> +};
> +
> +
> +
> +static int virLXCCgroupSetupDeviceACL(virDomainDefPtr def,
> + virCgroupPtr cgroup)
> +{
> + int ret = -1;
> + int rc;
> + size_t i;
> + static virLXCCgroupDevicePolicy devices[] = {
> + {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_NULL},
> + {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_ZERO},
> + {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_FULL},
> + {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_RANDOM},
> + {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_URANDOM},
> + {'c', LXC_DEV_MAJ_TTY, LXC_DEV_MIN_TTY},
> + {'c', LXC_DEV_MAJ_TTY, LXC_DEV_MIN_PTMX},
> + {0, 0, 0}};
> +
> + rc = virCgroupDenyAllDevices(cgroup);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to deny devices for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> +
> + for (i = 0; devices[i].type != 0; i++) {
> + virLXCCgroupDevicePolicyPtr dev = &devices[i];
> + rc = virCgroupAllowDevice(cgroup,
> + dev->type,
> + dev->major,
> + dev->minor,
> + VIR_CGROUP_DEVICE_RWM);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to allow device %c:%d:%d for domain %s"),
> + dev->type, dev->major, dev->minor, def->name);
> + goto cleanup;
> + }
> + }
> +
> + for (i = 0 ; i < def->nfss ; i++) {
> + if (def->fss[i]->type != VIR_DOMAIN_FS_TYPE_BLOCK)
> + continue;
> +
> + rc = virCgroupAllowDevicePath(cgroup,
> + def->fss[i]->src,
> + def->fss[i]->readonly ?
> + VIR_CGROUP_DEVICE_READ :
> + VIR_CGROUP_DEVICE_RW);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to allow device %s for domain %s"),
> + def->fss[i]->src, def->name);
> + goto cleanup;
> + }
> + }
> +
> + rc = virCgroupAllowDeviceMajor(cgroup, 'c', LXC_DEV_MAJ_PTY,
> + VIR_CGROUP_DEVICE_RWM);
> + if (rc != 0) {
> + virReportSystemError(-rc,
> + _("Unable to allow PTY devices for domain %s"),
> + def->name);
> + goto cleanup;
> + }
> +
> + ret = 0;
> +cleanup:
> + return ret;
> +}
> +
> +
> +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_cgroup.h b/src/lxc/lxc_cgroup.h
> new file mode 100644
> index 0000000..97bb12a
> --- /dev/null
> +++ b/src/lxc/lxc_cgroup.h
> @@ -0,0 +1,29 @@
> +/*
> + * Copyright (C) 2010-2012 Red Hat, Inc.
> + * Copyright IBM Corp. 2008
> + *
> + * lxc_cgroup.c: LXC cgroup helpers
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2.1 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + */
> +
> +#ifndef __VIR_LXC_CGROUP_H__
> +# define __VIR_LXC_CGROUP_H__
> +
> +# include "domain_conf.h"
> +
> +int virLXCCgroupSetup(virDomainDefPtr def);
> +
> +#endif /* __VIR_LXC_CGROUP_H__ */
> 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 ?
> @@ -527,181 +517,6 @@ static int virLXCControllerSetupCpuAffinity(virLXCControllerPtr ctrl)
> }
>
>
> -static int virLXCControllerSetupCgroupsCpuTune(virLXCControllerPtr ctrl)
> -{
> - int ret = -1;
> - if (ctrl->def->cputune.shares != 0) {
> - int rc = virCgroupSetCpuShares(ctrl->cgroup, ctrl->def->cputune.shares);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to set io cpu shares for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> - }
> - if (ctrl->def->cputune.quota != 0) {
> - int rc = virCgroupSetCpuCfsQuota(ctrl->cgroup, ctrl->def->cputune.quota);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to set io cpu quota for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> - }
> - if (ctrl->def->cputune.period != 0) {
> - int rc = virCgroupSetCpuCfsPeriod(ctrl->cgroup, ctrl->def->cputune.period);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to set io cpu period for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> - }
> - ret = 0;
> -cleanup:
> - return ret;
> -}
> -
> -
> -static int virLXCControllerSetupCgroupsBlkioTune(virLXCControllerPtr ctrl)
> -{
> - int ret = -1;
> -
> - if (ctrl->def->blkio.weight) {
> - int rc = virCgroupSetBlkioWeight(ctrl->cgroup, ctrl->def->blkio.weight);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to set Blkio weight for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> - }
> -
> - ret = 0;
> -cleanup:
> - return ret;
> -}
> -
> -
> -static int virLXCControllerSetupCgroupsMemTune(virLXCControllerPtr ctrl)
> -{
> - int ret = -1;
> - int rc;
> -
> - rc = virCgroupSetMemory(ctrl->cgroup, ctrl->def->mem.max_balloon);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to set memory limit for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> -
> - if (ctrl->def->mem.hard_limit) {
> - rc = virCgroupSetMemoryHardLimit(ctrl->cgroup, ctrl->def->mem.hard_limit);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to set memory hard limit for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> - }
> -
> - if (ctrl->def->mem.soft_limit) {
> - rc = virCgroupSetMemorySoftLimit(ctrl->cgroup, ctrl->def->mem.soft_limit);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to set memory soft limit for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> - }
> -
> - if (ctrl->def->mem.swap_hard_limit) {
> - rc = virCgroupSetMemSwapHardLimit(ctrl->cgroup, ctrl->def->mem.swap_hard_limit);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to set swap hard limit for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> - }
> -
> - ret = 0;
> -cleanup:
> - return ret;
> -}
> -
> -
> -static int virLXCControllerSetupCgroupsDeviceACL(virLXCControllerPtr ctrl)
> -{
> - int ret = -1;
> - int rc;
> - size_t i;
> - static const struct cgroup_device_policy devices[] = {
> - {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_NULL},
> - {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_ZERO},
> - {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_FULL},
> - {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_RANDOM},
> - {'c', LXC_DEV_MAJ_MEMORY, LXC_DEV_MIN_URANDOM},
> - {'c', LXC_DEV_MAJ_TTY, LXC_DEV_MIN_TTY},
> - {'c', LXC_DEV_MAJ_TTY, LXC_DEV_MIN_PTMX},
> - {0, 0, 0}};
> -
> - rc = virCgroupDenyAllDevices(ctrl->cgroup);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to deny devices for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> -
> - for (i = 0; devices[i].type != 0; i++) {
> - const struct cgroup_device_policy *dev = &devices[i];
> - rc = virCgroupAllowDevice(ctrl->cgroup,
> - dev->type,
> - dev->major,
> - dev->minor,
> - VIR_CGROUP_DEVICE_RWM);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to allow device %c:%d:%d for domain %s"),
> - dev->type, dev->major, dev->minor, ctrl->def->name);
> - goto cleanup;
> - }
> - }
> -
> - for (i = 0 ; i < ctrl->def->nfss ; i++) {
> - if (ctrl->def->fss[i]->type != VIR_DOMAIN_FS_TYPE_BLOCK)
> - continue;
> -
> - rc = virCgroupAllowDevicePath(ctrl->cgroup,
> - ctrl->def->fss[i]->src,
> - ctrl->def->fss[i]->readonly ?
> - VIR_CGROUP_DEVICE_READ :
> - VIR_CGROUP_DEVICE_RW);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to allow device %s for domain %s"),
> - ctrl->def->fss[i]->src, ctrl->def->name);
> - goto cleanup;
> - }
> - }
> -
> - rc = virCgroupAllowDeviceMajor(ctrl->cgroup, 'c', LXC_DEV_MAJ_PTY,
> - VIR_CGROUP_DEVICE_RWM);
> - if (rc != 0) {
> - virReportSystemError(-rc,
> - _("Unable to allow PTY devices for domain %s"),
> - ctrl->def->name);
> - goto cleanup;
> - }
> -
> - ret = 0;
> -cleanup:
> - return ret;
> -}
> -
> -
> /**
> * virLXCControllerSetupResourceLimits
> * @ctrl: the controller state
> @@ -713,8 +528,6 @@ cleanup:
> */
> 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 !
Daniel
--
Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/
daniel@xxxxxxxxxxxx | Rpmfind RPM search engine http://rpmfind.net/
http://veillard.com/ | virtualization library http://libvirt.org/
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list