Re: [PATCH 13/21] x86/intel_rdt/cqm: Add cpus file support

[Shivappa Vikas]

On Thu, 6 Jul 2017, Thomas Gleixner wrote:

> On Thu, 6 Jul 2017, Shivappa Vikas wrote:
> > On Sun, 2 Jul 2017, Thomas Gleixner wrote:
> > > > +	/* Check whether cpus belong to parent ctrl group */
> > > > +	cpumask_andnot(tmpmask, newmask, &pr->cpu_mask);
> > > > +	if (cpumask_weight(tmpmask)) {
> > > > +		ret = -EINVAL;
> > > > +		goto out;
> > > > +	}
> > > > +
> > > > +	/* Check whether cpus are dropped from this group */
> > > > +	cpumask_andnot(tmpmask, &rdtgrp->cpu_mask, newmask);
> > > > +	if (cpumask_weight(tmpmask)) {
> > > > +		/* Give any dropped cpus to parent rdtgroup */
> > > > +		cpumask_or(&pr->cpu_mask, &pr->cpu_mask, tmpmask);
> > >
> > > This does not make any sense. The check above verifies that all cpus in
> > > newmask belong to the parent->cpu_mask. If they don't then you return
> > > -EINVAL, but here you give them back to parent->cpu_mask. How is that
> > > supposed to work? You never get into this code path!
> >
> > The parent->cpu_mask always is the parent->cpus_valid_mask if i understand
> > right. With monitor group, the cpu is present is always present in "one"
> > ctrl_mon group and one mon_group. And the mon group can have only cpus in its
> > parent. May be it needs a comment? (its explaind in the documentation patch).
>
> Sigh, the code needs to be written in a way that it is halfways obvious
> what's going on.
>
> > # mkdir /sys/fs/resctrl/p1
> > # mkdir /sys/fs/resctrl/p1/mon_groups/m1
> > # echo 5-10 > /sys/fs/resctr/p1/cpus_list
> > Say p1 has RMID 2
> > cpus 5-10 have RMID 2
>
> So what you say, is that parent is always the resource control group
> itself.
>
> Can we please have a proper distinction in the code? I tripped over that
> ambigiousities several times.
>
> The normal meaning of parent->child relations is that both have the same
> type. While this is the case at the implementation detail level (both are
> type struct rdtgroup), from a conceptual level they are different:
>
>  parent is a resource group and child is a monitoring group
>
> That should be expressed in the code, at the very least by variable naming,
> so it becomes immediately clear that this operates on two different
> entities.
>
> The proper solution is to have different data types or at least embedd the
> monitoring bits in a seperate entity inside of struct rdtgroup.

Yes they are conceptually different. There were data which were 
specific to monitoring only but they share a lot of data. So I was still 
thinking whats best but kept a type which seperates them both. But the 
monitoring only data seems like only the 'parent' so we can embed the monitoring 
bits in a seperate struct (The parent is initialized for ctrl_mon group but 
never really used).

Thanks,
Vikas