Messages in this thread Patch in this message | | | Date | Wed, 4 Oct 2023 13:01:08 -0500 | From | "Moger, Babu" <> | Subject | Re: [PATCH v6 04/24] x86/resctrl: Move rmid allocation out of mkdir_rdt_prepare() |
| |
Hi James,
On 9/14/23 12:21, 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. > > 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. > > Reviewed-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com> > Tested-By: Peter Newman <peternewman@google.com> > Signed-off-by: James Morse <james.morse@arm.com> > --- > Changes since v2: > * Moved kernfs_activate() later to preserve atomicity of files being visible > > Changes since v5: > * Renamed out_id_free as out_closid_free. > --- > 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 7a7369a323b5..d25cb8c9a20e 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -3189,6 +3189,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->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) > @@ -3254,12 +3260,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);
You should not remove "kernfs_activate(kn); from here (only the last line).
kernfs_create_dir is called in this function.
/* kernfs creates the directory for rdtgrp */ kn = kernfs_create_dir(parent_kn, name, mode, rdtgrp);
There should be matching kernfs_activate.
> - > /* > * The caller unlocks the parent_kn upon success. > */ > @@ -3278,7 +3278,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->mon.rmid); > rdtgroup_remove(rgrp); > } > > @@ -3300,12 +3299,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);
I dont see the need for this. There is kernfs_activate inside mkdir_rdt_prepare_rmid_alloc (mkdir_rdt_prepare_rmid_alloc ->mkdir_mondata_all) for all the files created. Also mkdir_rdt_prepare already has kernfs_activate for the files it created.
> + > /* > * 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; > } > @@ -3336,9 +3344,16 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn, > ret = 0; > > rdtgrp->closid = closid; > + > + ret = mkdir_rdt_prepare_rmid_alloc(rdtgrp); > + if (ret) > + goto out_closid_free; > + > + kernfs_activate(rdtgrp->kn); > +
Same as above.
> ret = rdtgroup_init_alloc(rdtgrp); > if (ret < 0) > - goto out_id_free; > + goto out_rmid_free; > > list_add(&rdtgrp->rdtgroup_list, &rdt_all_groups); > > @@ -3358,7 +3373,9 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn, > > out_del_list: > list_del(&rdtgrp->rdtgroup_list); > -out_id_free: > +out_rmid_free: > + mkdir_rdt_prepare_rmid_free(rdtgrp); > +out_closid_free: > closid_free(closid); > out_common_fail: > mkdir_rdt_prepare_clean(rdtgrp);
-- Thanks Babu Moger
| |