Messages in this thread | | | Date | Thu, 13 Jul 2017 11:37:22 -0700 (PDT) | From | Shivappa Vikas <> | Subject | Re: [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
| |