[libvirt] [PATCH 3/9] vircgroup: introduce controller mask for threads

[Henning Schild]

On Thu, 25 Feb 2016 17:53:07 -0500
John Ferlan <jferlan at redhat.com> wrote:

> On 02/23/2016 10:58 AM, Henning Schild wrote:
> > When using a cgroups hierarchy threads have child cgroups for
> > certain controllers. Introduce an enum for later reuse.
> > 
> > Signed-off-by: Henning Schild <henning.schild at siemens.com>
> > ---
> >  src/util/vircgroup.c | 12 +++---------
> >  src/util/vircgroup.h |  7 +++++++
> >  2 files changed, 10 insertions(+), 9 deletions(-)
> > 
> > diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c
> > index ad46dfc..11f33ab 100644
> > --- a/src/util/vircgroup.c
> > +++ b/src/util/vircgroup.c
> > @@ -277,9 +277,7 @@ virCgroupValidateMachineGroup(virCgroupPtr
> > group, goto cleanup;
> >  
> >          if (stripEmulatorSuffix &&
> > -            (i == VIR_CGROUP_CONTROLLER_CPU ||
> > -             i == VIR_CGROUP_CONTROLLER_CPUACCT ||
> > -             i == VIR_CGROUP_CONTROLLER_CPUSET)) {
> > +            (i & VIR_CGROUP_THREAD_CONTROLLER_MASK)) {  
> 
> Not sure this works as expected because 'i' is not a mask - it's just
> an int... On entry, VIR_CGROUP_THREAD_CONTROLLER_MASK is 7...
> 
> The loop goes from 0 to VIR_CGROUP_CONTROLLER_LAST (11).
> 
> If you logically go through the values of 'i':
> 
> 0 & 7
> 1 & 7
> 2 & 7
> 3 & 7
> 4 & 7
> ...
> etc
> 
> You'd find 0 & 8 fail, but 1 -> 7, 9, & 10 succeed
> 
> So what "would" work is :
> 
>     mask = (1 << i);
>     if (stripEmulatorSuffix &&
>         mask & VIR_CGROUP_THREAD_CONTROLLER_MASK))

Sure, Stupid mistake, will fix.

> John
> 
> >              if (STREQ(tmp, "/emulator"))
> >                  *tmp = '\0';
> >              tmp = strrchr(group->controllers[i].placement, '/');
> > @@ -1518,7 +1516,6 @@ virCgroupNewThread(virCgroupPtr domain,
> >  {
> >      int ret = -1;
> >      char *name = NULL;
> > -    int controllers;
> >  
> >      switch (nameval) {
> >      case VIR_CGROUP_THREAD_VCPU:
> > @@ -1539,11 +1536,8 @@ virCgroupNewThread(virCgroupPtr domain,
> >          goto cleanup;
> >      }
> >  
> > -    controllers = ((1 << VIR_CGROUP_CONTROLLER_CPU) |
> > -                   (1 << VIR_CGROUP_CONTROLLER_CPUACCT) |
> > -                   (1 << VIR_CGROUP_CONTROLLER_CPUSET));
> > -
> > -    if (virCgroupNew(-1, name, domain, controllers, group) < 0)
> > +    if (virCgroupNew(-1, name, domain,
> > VIR_CGROUP_THREAD_CONTROLLER_MASK,
> > +        group) < 0)
> >          goto cleanup;
> >  
> >      if (virCgroupMakeGroup(domain, *group, create,
> > VIR_CGROUP_NONE) < 0) { diff --git a/src/util/vircgroup.h
> > b/src/util/vircgroup.h index f244c24..f71aed5 100644
> > --- a/src/util/vircgroup.h
> > +++ b/src/util/vircgroup.h
> > @@ -52,6 +52,13 @@ VIR_ENUM_DECL(virCgroupController);
> >   * Make sure we will not overflow */
> >  verify(VIR_CGROUP_CONTROLLER_LAST < 8 * sizeof(int));
> >  
> > +enum {
> > +    VIR_CGROUP_THREAD_CONTROLLER_MASK =
> > +        ((1 << VIR_CGROUP_CONTROLLER_CPU) |
> > +         (1 << VIR_CGROUP_CONTROLLER_CPUACCT) |
> > +         (1 << VIR_CGROUP_CONTROLLER_CPUSET))
> > +};
> > +
> >  typedef enum {
> >      VIR_CGROUP_THREAD_VCPU = 0,
> >      VIR_CGROUP_THREAD_EMULATOR,
> >