lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Date: Thu, 6 Jul 2017 14:42:16 -0700 (PDT) From: Shivappa Vikas <vikas.shivappa@...el.com> To: Thomas Gleixner <tglx@...utronix.de> cc: Vikas Shivappa <vikas.shivappa@...ux.intel.com>, x86@...nel.org, linux-kernel@...r.kernel.org, hpa@...or.com, peterz@...radead.org, "Shankar, Ravi V" <ravi.v.shankar@...el.com>, vikas.shivappa@...el.com, tony.luck@...el.com, "Yu, Fenghua" <fenghua.yu@...el.com> Subject: 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 >
Powered by blists - more mailing lists