Messages in this thread | | | Date | Sun, 2 Jul 2017 14:29:27 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [PATCH 13/21] x86/intel_rdt/cqm: Add cpus file support |
| |
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.
> + /* 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!
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.
> + 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 ?
> + 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.
Thanks,
tglx
| |