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

[Shivappa Vikas <vikas.shivappa@intel.com>]

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