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

From: Shivappa Vikas
Date: Thu Jul 06 2017 - 17:40:58 EST

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

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).

>> + 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

* 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..