Hi Reinette, On 7/12/24 17:10, Reinette Chatre wrote: > Hi Babu, > > On 7/3/24 2:48 PM, Babu Moger wrote: >> Assign/unassign counters on resctrl group creation/deletion. If the >> counters are exhausted, report the warnings and continue. It is not >> required to fail group creation for assignment failures. Users have >> the option to modify the assignments later. >> >> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >> --- >> v5: Removed the code to enable/disable ABMC during the mount. >> That will be another patch. >> Added arch callers to get the arch specific data. >> Renamed fuctions to match the other abmc function. >> Added code comments for assignment failures. >> >> v4: Few name changes based on the upstream discussion. >> Commit message update. >> >> v3: This is a new patch. Patch addresses the upstream comment to enable >> ABMC feature by default if the feature is available. >> --- >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 78 ++++++++++++++++++++++++++ >> 1 file changed, 78 insertions(+) >> >> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index ffde30b36c1a..475a0c7b2a25 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -2910,6 +2910,46 @@ static void schemata_list_destroy(void) >> } >> } >> +/* >> + * Called when new group is created. Assign the counters if ABMC is >> + * already enabled. Two counters are required per group, one for total >> + * event and one for local event. With limited number of counters, >> + * the assignments can fail in some cases. But, it is not required to >> + * fail the group creation. Users have the option to modify the >> + * assignments after the group creation. >> + */ >> +static int rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp) >> +{ >> + int ret = 0; >> + >> + if (!resctrl_arch_get_abmc_enabled()) >> + return 0; >> + >> + if (is_mbm_total_enabled()) >> + ret = rdtgroup_assign_cntr(rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID); >> + >> + if (!ret && is_mbm_local_enabled()) >> + ret = rdtgroup_assign_cntr(rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID); >> + >> + return ret; >> +} >> + >> +static int rdtgroup_unassign_cntrs(struct rdtgroup *rdtgrp) >> +{ >> + int ret = 0; >> + >> + if (!resctrl_arch_get_abmc_enabled()) >> + return 0; >> + >> + if (is_mbm_total_enabled()) >> + ret = rdtgroup_unassign_cntr(rdtgrp, QOS_L3_MBM_TOTAL_EVENT_ID); >> + >> + if (!ret && is_mbm_local_enabled()) >> + ret = rdtgroup_unassign_cntr(rdtgrp, QOS_L3_MBM_LOCAL_EVENT_ID); >> + >> + return ret; >> +} >> + >> static int rdt_get_tree(struct fs_context *fc) >> { >> struct rdt_resource *r = >> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl; >> @@ -2972,6 +3012,16 @@ static int rdt_get_tree(struct fs_context *fc) >> if (ret < 0) >> goto out_mongrp; >> rdtgroup_default.mon.mon_data_kn = kn_mondata; >> + >> + /* >> + * Assign the counters if ABMC is already enabled. >> + * With limited number of counters, the assignments can >> + * fail in some cases. But, it is not required to fail >> + * the group creation. Users have the option to modify >> + * the assignments after the group creation. >> + */ > > The function has detailed comments - it seems unnecessary to me that the > same comments are duplicated at each call site. Sure. Will remove duplicates. > >> + if (rdtgroup_assign_cntrs(&rdtgroup_default) < 0) >> + rdt_last_cmd_puts("Monitor assignment failed\n"); > > rdtgroup_assign_cntrs() already prints message, why print another? Error > handling can then be dropped. Sure. > >> } >> ret = rdt_pseudo_lock_init(); >> @@ -3246,6 +3296,8 @@ static void rdt_kill_sb(struct super_block *sb) >> cpus_read_lock(); >> mutex_lock(&rdtgroup_mutex); >> + rdtgroup_unassign_cntrs(&rdtgroup_default); >> + > > This seems appropriate to be in the "Put everything back to default values" > section. Sure. Will move it down. > >> rdt_disable_ctx(); >> /*Put everything back to default values. */ >> @@ -3850,6 +3902,16 @@ static int rdtgroup_mkdir_mon(struct kernfs_node >> *parent_kn, >> goto out_unlock; >> } >> + /* >> + * Assign the counters if ABMC is already enabled. >> + * With the limited number of counters, there can be cases >> + * only on assignment succeed. It is not required to fail >> + * here in that case. Users have the option to modify the >> + * assignments later. >> + */ >> + if (rdtgroup_assign_cntrs(rdtgrp) < 0) >> + rdt_last_cmd_puts("Monitor assignment failed\n"); >> + >> kernfs_activate(rdtgrp->kn); >> /* >> @@ -3894,6 +3956,17 @@ static int rdtgroup_mkdir_ctrl_mon(struct >> kernfs_node *parent_kn, >> if (ret) >> goto out_closid_free; >> + /* >> + * Assign the counters if ABMC is already enabled. >> + * With the limited number of counters, there can be cases >> + * only on assignment succeed. It is not required to fail >> + * here in that case. Users have the option to assign the >> + * counter later. >> + */ >> + >> + if (rdtgroup_assign_cntrs(rdtgrp) < 0) >> + rdt_last_cmd_puts("Monitor assignment failed\n"); >> + >> kernfs_activate(rdtgrp->kn); >> ret = rdtgroup_init_alloc(rdtgrp); >> @@ -3989,6 +4062,9 @@ static int rdtgroup_rmdir_mon(struct rdtgroup >> *rdtgrp, cpumask_var_t tmpmask) >> update_closid_rmid(tmpmask, NULL); >> rdtgrp->flags = RDT_DELETED; >> + >> + rdtgroup_unassign_cntrs(rdtgrp); >> + >> free_rmid(rdtgrp->closid, rdtgrp->mon.rmid); >> /* >> @@ -4035,6 +4111,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_cntrs(rdtgrp); >> + >> free_rmid(rdtgrp->closid, rdtgrp->mon.rmid); >> closid_free(rdtgrp->closid); >> > > Reinette > -- Thanks Babu Moger