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
