Messages in this thread | | | Date | Thu, 15 Jun 2023 15:05:03 -0700 | Subject | Re: [PATCH v4 04/24] x86/resctrl: Move rmid allocation out of mkdir_rdt_prepare() | From | Reinette Chatre <> |
| |
Hi James,
On 5/25/2023 11:01 AM, James Morse wrote: > RMID are allocated for each monitor or control group directory, because > each of these needs its own RMID. For control groups, > rdtgroup_mkdir_ctrl_mon() later goes on to allocate the CLOSID. > > MPAM's equivalent of RMID is not an independent number, so can't be > allocated until the CLOSID is known. An RMID allocation for one CLOSID > may fail, whereas another may succeed depending on how many monitor > groups a control group has. > > The RMID allocation needs to move to be after the CLOSID has been > allocated.
This third paragraph seems redundant.
> > Move the RMID allocation out of mkdir_rdt_prepare() to occur in its caller, > after the mkdir_rdt_prepare() call. This allows the RMID allocator to > know the CLOSID. > > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Signed-off-by: James Morse <james.morse@arm.com> > --- > Changes since v2: > * Moved kernfs_activate() later to preserve atomicity of files being visible > --- > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 35 +++++++++++++++++++------- > 1 file changed, 26 insertions(+), 9 deletions(-) > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 05774b185eec..8346a8f2ff9f 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -3176,6 +3176,12 @@ static int mkdir_rdt_prepare_rmid_alloc(struct rdtgroup *rdtgrp) > return 0; > } > > +static void mkdir_rdt_prepare_rmid_free(struct rdtgroup *rgrp) > +{ > + if (rdt_mon_capable) > + free_rmid(rgrp->closid, rgrp->mon.rmid); > +} > + > static int mkdir_rdt_prepare(struct kernfs_node *parent_kn, > const char *name, umode_t mode, > enum rdt_group_type rtype, struct rdtgroup **r) > @@ -3241,12 +3247,6 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn, > goto out_destroy; > } > > - ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp); > - if (ret) > - goto out_destroy; > - > - kernfs_activate(kn); > - > /* > * The caller unlocks the parent_kn upon success. > */ > @@ -3265,7 +3265,6 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn, > static void mkdir_rdt_prepare_clean(struct rdtgroup *rgrp) > { > kernfs_remove(rgrp->kn); > - free_rmid(rgrp->closid, rgrp->mon.rmid); > rdtgroup_remove(rgrp); > } > > @@ -3287,12 +3286,21 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn, > prgrp = rdtgrp->mon.parent; > rdtgrp->closid = prgrp->closid; > > + ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp); > + if (ret) { > + mkdir_rdt_prepare_clean(rdtgrp); > + goto out_unlock; > + } > + > + kernfs_activate(rdtgrp->kn); > + > /* > * Add the rdtgrp to the list of rdtgrps the parent > * ctrl_mon group has to track. > */ > list_add_tail(&rdtgrp->mon.crdtgrp_list, &prgrp->mon.crdtgrp_list); > > +out_unlock: > rdtgroup_kn_unlock(parent_kn); > return ret; > } > @@ -3323,10 +3331,17 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn, > ret = 0; > > rdtgrp->closid = closid; > - ret = rdtgroup_init_alloc(rdtgrp); > - if (ret < 0) > + > + ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp); > + if (ret) > goto out_id_free; > > + kernfs_activate(rdtgrp->kn); > +
I can see why you added this kernfs_activate() here and it certainly matches the resctrl pattern of its use. It does seem as though resctrl often calls kernfs_activate() as it adds files instead of a single call when all files are ready. This is not an issue with this patch but I do think this is an area that deserves a closer look before any more additions.
Reinette
| |