On Fri, Oct 05, 2018 at 12:14:55PM +0200, Michal Privoznik wrote:
> On 10/02/2018 10:44 AM, Pavel Hrdina wrote:
> > This enables to use both cgroup v1 and v2 at the same time together
> > with libvirt. It is supported by kernel and there is valid use-case,
> > not all controllers are implemented in cgroup v2 so there might be
> > configurations where administrator would enable these missing
> > controllers in cgroup v1.
> >
> > Signed-off-by: Pavel Hrdina <phrdina@xxxxxxxxxx>
> > ---
> > src/util/vircgroup.c | 351 ++++++++++++++++++++++++++----------
> > src/util/vircgroupbackend.c | 20 ++
> > src/util/vircgroupbackend.h | 16 +-
> > src/util/vircgrouppriv.h | 2 +-
> > 4 files changed, 291 insertions(+), 98 deletions(-)
> >
> > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> > index dc249bfe33..4aec5f1bcf 100644
> > --- a/src/util/vircgroup.c
> > +++ b/src/util/vircgroup.c
> > @@ -228,6 +228,7 @@ virCgroupDetectMounts(virCgroupPtr group)
> > struct mntent entry;
> > char buf[CGROUP_MAX_VAL];
> > int ret = -1;
> > + size_t i;
> >
> > mounts = fopen("/proc/mounts", "r");
> > if (mounts == NULL) {
> > @@ -236,11 +237,14 @@ virCgroupDetectMounts(virCgroupPtr group)
> > }
> >
> > while (getmntent_r(mounts, &entry, buf, sizeof(buf)) != NULL) {
> > - if (group->backend->detectMounts(group,
> > - entry.mnt_type,
> > - entry.mnt_opts,
> > - entry.mnt_dir) < 0) {
> > - goto cleanup;
> > + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
> > + if (group->backends[i] &&
> > + group->backends[i]->detectMounts(group,
> > + entry.mnt_type,
> > + entry.mnt_opts,
> > + entry.mnt_dir) < 0) {
> > + goto cleanup;
> > + }
> > }
> > }
> >
> > @@ -303,6 +307,7 @@ virCgroupDetectPlacement(virCgroupPtr group,
> > }
> >
> > while (fgets(line, sizeof(line), mapping) != NULL) {
> > + size_t i;
> > char *controllers = strchr(line, ':');
> > char *selfpath = controllers ? strchr(controllers + 1, ':') : NULL;
> > char *nl = selfpath ? strchr(selfpath, '\n') : NULL;
> > @@ -317,9 +322,12 @@ virCgroupDetectPlacement(virCgroupPtr group,
> > controllers++;
> > selfpath++;
> >
> > - if (group->backend->detectPlacement(group, path, controllers,
> > - selfpath) < 0) {
> > - goto cleanup;
> > + for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
> > + if (group->backends[i] &&
> > + group->backends[i]->detectPlacement(group, path, controllers,
> > + selfpath) < 0) {
> > + goto cleanup;
> > + }
>
> So a loop like this appears all over the patch:
>
> for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
> if (group->backends[i] &&
> group->backends[i]->cb();
> }
>
> I wonder if we should have wrappers around these chunks of code. But
> that could be saved as a follow up patch.
Right, we can create a wrapper for this use-case. The reason why
I did not create one is that not all the loops are the same.
> > }
> > }
> >
> > @@ -338,8 +346,9 @@ virCgroupDetect(virCgroupPtr group,
> > const char *path,
> > virCgroupPtr parent)
> > {
> > - int rc;
> > size_t i;
> > + bool backendAvailable = false;
> > + int controllersAvailable = 0;
> > virCgroupBackendPtr *backends = virCgroupBackendGetAll();
> >
> > VIR_DEBUG("group=%p controllers=%d path=%s parent=%p",
> > @@ -350,31 +359,40 @@ virCgroupDetect(virCgroupPtr group,
> >
> > for (i = 0; i < VIR_CGROUP_BACKEND_TYPE_LAST; i++) {
> > if (backends[i] && backends[i]->available()) {
> > - group->backend = backends[i];
> > - break;
> > + group->backends[i] = backends[i];
> > + backendAvailable = true;
>
> No need to remove the 'break'.
We have to remove the 'break' here, otherwise we would not detect
all backends for hybrid mode. We assign the backends that are
available to the cgroup so we need to go through all the backends.
Pavel
Attachment:
signature.asc
Description: PGP signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
