Hi Babu, Subject: "Assign" -> "assign" On 9/4/24 3:21 PM, Babu Moger wrote: > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 ++++++++++++++++++++++++++ > 1 file changed, 61 insertions(+) > > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 21b9ca4ce493..bf94e4e05540 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -2866,6 +2866,52 @@ static void schemata_list_destroy(void) > } > } > > +/* > + * Called when a new group is created. If `mbm_cntr_assign` mode is enabled, > + * counters are automatically assigned. Each group requires two counters: > + * one for the total event and one for the local event. Due to the limited > + * number of counters, assignments may fail in some cases. However, it is > + * not necessary to fail the group creation. Users have the option to > + * modify the assignments after the group has been created. > + */ > +static int rdtgroup_assign_grp(struct rdtgroup *rdtgrp) > +{ > + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; > + int ret = 0; > + > + if (!resctrl_arch_mbm_cntr_assign_enabled(r)) > + return 0; > + > + if (is_mbm_total_enabled()) > + ret = rdtgroup_assign_cntr(r, rdtgrp, NULL, QOS_L3_MBM_TOTAL_EVENT_ID); > + > + if (!ret && is_mbm_local_enabled()) > + ret = rdtgroup_assign_cntr(r, rdtgrp, NULL, QOS_L3_MBM_LOCAL_EVENT_ID); > + > + return ret; > +} > + > +/* > + * Called when a group is deleted. Counters are unassigned if it was in > + * assigned state. > + */ > +static int rdtgroup_unassign_grp(struct rdtgroup *rdtgrp) > +{ > + struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; > + int ret = 0; > + > + if (!resctrl_arch_mbm_cntr_assign_enabled(r)) > + return 0; > + > + if (is_mbm_total_enabled()) > + ret = rdtgroup_unassign_cntr(r, rdtgrp, NULL, QOS_L3_MBM_TOTAL_EVENT_ID); > + > + if (!ret && is_mbm_local_enabled()) > + ret = rdtgroup_unassign_cntr(r, rdtgrp, NULL, QOS_L3_MBM_LOCAL_EVENT_ID); > + > + return ret; > +} > + > static int rdt_get_tree(struct fs_context *fc) > { > struct rdt_fs_context *ctx = rdt_fc2context(fc); > @@ -2925,6 +2971,8 @@ static int rdt_get_tree(struct fs_context *fc) > if (ret < 0) > goto out_mongrp; > rdtgroup_default.mon.mon_data_kn = kn_mondata; > + > + rdtgroup_assign_grp(&rdtgroup_default); > } > > ret = rdt_pseudo_lock_init(); > @@ -2955,6 +3003,7 @@ static int rdt_get_tree(struct fs_context *fc) > out_psl: > rdt_pseudo_lock_release(); > out_mondata: > + rdtgroup_unassign_grp(&rdtgroup_default); > if (resctrl_arch_mon_capable()) > kernfs_remove(kn_mondata); > out_mongrp: > @@ -3214,6 +3263,8 @@ static void rdt_kill_sb(struct super_block *sb) > resctrl_arch_disable_alloc(); > if (resctrl_arch_mon_capable()) > resctrl_arch_disable_mon(); > + > + rdtgroup_unassign_grp(&rdtgroup_default); > resctrl_mounted = false; > kernfs_kill_sb(sb); > mutex_unlock(&rdtgroup_mutex); > @@ -3805,6 +3856,8 @@ static int rdtgroup_mkdir_mon(struct kernfs_node *parent_kn, > goto out_unlock; > } > > + rdtgroup_assign_grp(rdtgrp); > + > kernfs_activate(rdtgrp->kn); > > /* > @@ -3849,6 +3902,8 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn, > if (ret) > goto out_closid_free; > > + rdtgroup_assign_grp(rdtgrp); > + > kernfs_activate(rdtgrp->kn); > > ret = rdtgroup_init_alloc(rdtgrp); > @@ -3874,6 +3929,7 @@ static int rdtgroup_mkdir_ctrl_mon(struct kernfs_node *parent_kn, > out_del_list: > list_del(&rdtgrp->rdtgroup_list); > out_rmid_free: > + rdtgroup_unassign_grp(rdtgrp); > mkdir_rdt_prepare_rmid_free(rdtgrp); > out_closid_free: > closid_free(closid); > @@ -3944,6 +4000,9 @@ static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask) > update_closid_rmid(tmpmask, NULL); > > rdtgrp->flags = RDT_DELETED; > + > + rdtgroup_unassign_grp(rdtgrp); > + > free_rmid(rdtgrp->closid, rdtgrp->mon.rmid); > > /* > @@ -3990,6 +4049,8 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask) > cpumask_or(tmpmask, tmpmask, &rdtgrp->cpu_mask); > update_closid_rmid(tmpmask, NULL); > > + rdtgroup_unassign_grp(rdtgrp); > + > free_rmid(rdtgrp->closid, rdtgrp->mon.rmid); > closid_free(rdtgrp->closid); > Apart from earlier comment about rdtgroup_assign_grp()/rdtgroup_unassign_grp() naming, please also take care about how these functions are integrated since it seems to be inconsistent wrt whether it is called on mon capable resource. Also, I can see how the counter is removed when CTRL_MON group and MON group are explicitly removed but it is not clear to me how when a user removes a CTRL_MON group how the counters assigned to its child MON groups are unassigned. Reinette