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

Groups > linux.kernel > #1682730

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

Show all headers | View raw

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
>

Back to linux.kernel | Previous | Next — Next in thread | Find similar | Unroll thread

Thread

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

csiph-web