Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1682730 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 13/21] x86/intel_rdt/cqm: Add cpus file support Shivappa Vikas <vikas.shivappa@intel.com> - 2017-07-06 23:50 +0200
    Re: [PATCH 13/21] x86/intel_rdt/cqm: Add cpus file support Thomas Gleixner <tglx@linutronix.de> - 2017-07-07 08:50 +0200
      Re: [PATCH 13/21] x86/intel_rdt/cqm: Add cpus file support Shivappa Vikas <vikas.shivappa@intel.com> - 2017-07-13 20:40 +0200

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

On Sun, 2 Jul 2017, Thomas Gleixner wrote:

> On Mon, 26 Jun 2017, Vikas Shivappa wrote:
>> -static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
>> -				   char *buf, size_t nbytes, loff_t off)
>> +static ssize_t cpus_mon_write(struct kernfs_open_file *of,
>> +			      char *buf, size_t nbytes,
>> +			      struct rdtgroup *rdtgrp)
>
> Again. Please make the split of rdtgroup_cpus_write() as a seperate
> preparatory change first and just move the guts of the existing write
> function out into cpus_ctrl_write() and then add the mon_write stuff as an
> extra patch.
>
>>  {
>> +	struct rdtgroup *pr = rdtgrp->parent, *cr;
>
> *pr and *cr really suck.
>
>>  	cpumask_var_t tmpmask, newmask;
>> -	struct rdtgroup *rdtgrp, *r;
>> +	struct list_head *llist;
>>  	int ret;
>>
>> -	if (!buf)
>> -		return -EINVAL;
>> -
>>  	if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL))
>>  		return -ENOMEM;
>>  	if (!zalloc_cpumask_var(&newmask, GFP_KERNEL)) {
>> @@ -233,10 +235,89 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
>>  		return -ENOMEM;
>>  	}
>>
>> -	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>> -	if (!rdtgrp) {
>> -		ret = -ENOENT;
>> -		goto unlock;
>> +	if (is_cpu_list(of))
>> +		ret = cpulist_parse(buf, newmask);
>> +	else
>> +		ret = cpumask_parse(buf, newmask);
>
> The cpuask allocation and parsing of the user buffer can be done in the
> common code. No point in duplicating that.
>
>> +
>> +	if (ret)
>> +		goto out;
>> +
>> +	/* check that user didn't specify any offline cpus */
>> +	cpumask_andnot(tmpmask, newmask, cpu_online_mask);
>> +	if (cpumask_weight(tmpmask)) {
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>
> Common code.

Will fix all above

>
>> +	/* 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).

# 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

# echo 5-6 > /sys/fs/resctrl/p1/mon_groups/m1/cpus_list
cpus 5-6 have RMID 3
cpus 7-10 have RMID 2

# cat /sys/fs/resctrl/p1/cpus_list
5-10

This is because when we query the data for p1 it adds its own data (RMID 2) and 
all the data for its child mon groups (hence all cpus from 5-10).

But
  >> +		cpumask_or(&pr->cpu_mask, &pr->cpu_mask, tmpmask);
can be removed because it does nothing like you suggest as the parent already 
has these cpus. We just need the update_rmid_closid(tmpmask, pr)

>
> So you need a seperate mask in the parent rdtgroup to store the CPUs which
> are valid in any monitoring group which belongs to it. So the logic
> becomes:
>
> 	/*
> 	 * Check whether the CPU mask is a subset of the CPUs
> 	 * which belong to the parent group.
> 	 */
> 	cpumask_andnot(tmpmask, newmask, parent->cpus_valid_mask);
> 	if (cpumask_weight(tmpmask))
> 		return -EINVAL;
>
> When CAT is not available, then parent->cpus_valid_mask is a pointer to
> cpu_online_mask. When CAT is enabled, then parent->cpus_valid_mask is a
> pointer to the CAT group cpu mask.

When CAT is unavailable we cannot create any ctrl_mon groups.

>
>> +		update_closid_rmid(tmpmask, pr);
>> +	}
>> +
>> +	/*
>> +	 * If we added cpus, remove them from previous group that owned them
>> +	 * and update per-cpu rmid
>> +	 */
>> +	cpumask_andnot(tmpmask, newmask, &rdtgrp->cpu_mask);
>> +	if (cpumask_weight(tmpmask)) {
>> +		llist = &pr->crdtgrp_list;
>
> llist is a bad name. We have a facility llist, i.e. lockless list. head ?

Will fix.

>
>> +		list_for_each_entry(cr, llist, crdtgrp_list) {
>> +			if (cr == rdtgrp)
>> +				continue;
>> +			cpumask_andnot(&cr->cpu_mask, &cr->cpu_mask, tmpmask);
>> +		}
>> +		update_closid_rmid(tmpmask, rdtgrp);
>> +	}
>
>> +static void cpumask_rdtgrp_clear(struct rdtgroup *r, struct cpumask *m)
>> +{
>> +	struct rdtgroup *cr;
>> +
>> +	cpumask_andnot(&r->cpu_mask, &r->cpu_mask, m);
>> +	/* update the child mon group masks as well*/
>> +	list_for_each_entry(cr, &r->crdtgrp_list, crdtgrp_list)
>> +		cpumask_and(&cr->cpu_mask, &r->cpu_mask, &cr->cpu_mask);
>
> That's equally wrong. See above.

Because of same reason above, each cpu is present in "one" ctrl_mon group and 
may be present in "one" mon group - we need to clear both..

Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>

[toc] | [next] | [standalone]

#1682958

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.

struct mongroup {
	monitoring stuff;
};

struct rdtgroup {
	common stuff;
	struct mongroup mon;
};

So the code can operate on r->mon.foo or mon->foo which makes it entirely
clear what kind of operation this is.

Sigh, cramming everything into a single struct without distinction is the
same as operating on a pile of global variables, which is the most common
pattern used by people learning C. You certainly belong not to that group,
so dammit, get your act together and structure the code so it's obvious and
maintainable.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1686838

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web