Messages in this thread | | | From | Peter Newman <> | Date | Thu, 2 Mar 2023 15:26:43 +0100 | Subject | Re: [PATCH v3 3/3] x86/resctrl: Implement rename op for mon groups |
| |
Hi Reinette,
On Sat, Feb 11, 2023 at 12:21 AM Reinette Chatre <reinette.chatre@intel.com> wrote:
> On 1/25/2023 2:13 AM, Peter Newman wrote:
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > > @@ -3238,6 +3238,80 @@ static int rdtgroup_rmdir(struct kernfs_node *kn) > > return ret; > > } > > > > +static void mongrp_move(struct rdtgroup *rdtgrp, struct rdtgroup *new_prdtgrp, > > + cpumask_var_t cpus) > > Could you please add some function comments on what is moved and how it is accomplished?
Sure, I think I should also make the name more descriptive. I think "move" is too high-level here.
> > + for_each_process_thread(p, t) { > > + if (is_closid_match(t, prdtgrp) && is_rmid_match(t, rdtgrp)) > > + rdt_move_one_task(t, new_prdtgrp->closid, t->rmid, > > + cpus); > > + } > > + read_unlock(&tasklist_lock); > > Can rdt_move_group_tasks() be used here?
As it is today, rdt_move_group_tasks() would move too many tasks. mongrp_move() needs both the CLOSID and RMID to match, while rdt_move_group_tasks() only needs 0-1 of the two to match.
I tried adding more parameters to rdt_move_group_tasks() to change the move condition, but I couldn't make the resulting code not look gross and after factoring the tricky stuff into rdt_move_one_task(), rdt_move_group_tasks() didn't look interesting enough to reuse.
> > > + > > + update_closid_rmid(cpus, NULL); > > +} > > I see the tasks in a monitor group handled. There is another way to create/manage > a monitor group. For example, by not writing tasks to the tasks file but instead > to write CPU ids to the CPU file. All tasks on a particular CPU will be monitored > by that group. One rule is that a MON group may only have CPUs that are owned by > the CTRL_MON group. > It is not clear to me how such a group is handled in this work.
Right, I overlooked this form of monitoring.
I don't think it makes sense to move such a monitoring group, because a CPU can only be assigned to one CTRL_MON group. Therefore a move of a MON group with an assigned CPU will always violate the parent CTRL_MON group rule after the move.
Based on this, I think rename of a MON group should fail when rdtgrp->cpu_mask is non-zero.
> > > > + > > +static int rdtgroup_rename(struct kernfs_node *kn, > > + struct kernfs_node *new_parent, const char *new_name) > > +{ > > + struct rdtgroup *new_prdtgrp; > > + struct rdtgroup *rdtgrp; > > + cpumask_var_t tmpmask; > > + int ret; > > + > > + if (!zalloc_cpumask_var(&tmpmask, GFP_KERNEL)) > > + return -ENOMEM; > > + > > + rdtgrp = kernfs_to_rdtgroup(kn); > > + new_prdtgrp = kernfs_to_rdtgroup(new_parent); > > + if (!rdtgrp || !new_prdtgrp) { > > + free_cpumask_var(tmpmask); > > + return -EPERM; > > + } > > + > > How robust is this against user space attempting to move files?
I'm not sure I understand the question. Can you be more specific?
> > > + /* Release both kernfs active_refs before obtaining rdtgroup mutex. */ > > + rdtgroup_kn_get(rdtgrp, kn); > > + rdtgroup_kn_get(new_prdtgrp, new_parent); > > + > > + mutex_lock(&rdtgroup_mutex); > > + > > + if ((rdtgrp->flags & RDT_DELETED) || (new_prdtgrp->flags & RDT_DELETED)) { > > + ret = -ESRCH; > > + goto out; > > + } > > + > > + /* Only a mon group can be moved to a new mon_groups directory. */ > > + if (rdtgrp->type != RDTMON_GROUP || > > + !is_mon_groups(new_parent, kn->name)) { > > + ret = -EPERM; > > + goto out; > > + } > > + > > Should in-place moves be allowed?
I don't think it's useful in the context of the intended use case. Also, it looks like kernfs_rename() would fail with EEXIST if I tried.
If it were important to someone, I could make it succeed before calling into kernfs_rename().
> > > + ret = kernfs_rename(kn, new_parent, new_name); > > + if (ret) > > + goto out; > > + > > + mongrp_move(rdtgrp, new_prdtgrp, tmpmask); > > + > > Can tmpmask allocation/free be done in mongrp_move()?
Yes, but it looked like most other functions in this file allocate the cpumask up front before validating parameters. If you have a preference for internalizing its allocation within mongrp_move(), I can try it.
Thank you for your review.
-Peter
| |