lkml.org 
[lkml]   [2017]   [Jul]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 13/21] x86/intel_rdt/cqm: Add cpus file support


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

\
 
 \ /
  Last update: 2017-07-13 20:36    [W:0.089 / U:0.612 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site