Hi Reinette, On 10/15/24 22:14, Reinette Chatre wrote: > Hi Babu, > > On 10/9/24 10:39 AM, Babu Moger wrote: >> Hardware provides a set of counters when mbm_assign_mode is supported. >> These counters are assigned to the MBM monitoring events of a MON group >> that needs to be tracked. The kernel must manage and track the available >> counters. >> >> Introduce mbm_cntr_free_map bitmap to track available counters and set >> of routines to allocate and free the counters. Move dom_data_init() after >> mbm_cntr_assign detection. > > Regarding "Move dom_data_init() after mbm_cntr_assign detection." - this is > clear from the patch, please use changelog to explain *why*. Will change it to. dom_data_init() requires mbm_cntr_assign state to initialize mbm_cntr_free_map bitmap. Move dom_data_init() after mbm_cntr_assign detection. > >> >> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >> --- > > >> --- >> arch/x86/kernel/cpu/resctrl/internal.h | 2 ++ >> arch/x86/kernel/cpu/resctrl/monitor.c | 43 +++++++++++++++++++++++--- >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 19 ++++++++++++ >> include/linux/resctrl.h | 2 ++ >> 4 files changed, 62 insertions(+), 4 deletions(-) >> >> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h >> index 92eae4672312..99f9103a35ba 100644 >> --- a/arch/x86/kernel/cpu/resctrl/internal.h >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >> @@ -654,6 +654,8 @@ void __check_limbo(struct rdt_mon_domain *d, bool force_free); >> void rdt_domain_reconfigure_cdp(struct rdt_resource *r); >> void __init resctrl_file_fflags_init(const char *config, >> unsigned long fflags); >> +int mbm_cntr_alloc(struct rdt_resource *r); >> +void mbm_cntr_free(struct rdt_resource *r, u32 cntr_id); >> void rdt_staged_configs_clear(void); >> bool closid_allocated(unsigned int closid); >> int resctrl_find_cleanest_closid(void); >> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c >> index 66b06574f660..5c2a28565747 100644 >> --- a/arch/x86/kernel/cpu/resctrl/monitor.c >> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c >> @@ -983,6 +983,27 @@ void mbm_setup_overflow_handler(struct rdt_mon_domain *dom, unsigned long delay_ >> schedule_delayed_work_on(cpu, &dom->mbm_over, delay); >> } >> >> +/* >> + * Counter bitmap for tracking the available counters. >> + * 'mbm_cntr_assign' mode provides set of hardware counters for assigning >> + * RMID, event pair. Each RMID and event pair takes one hardware counter. >> + */ > > "counters for assigning RMID, event pair" sounds strange and it seems like the same > thing is mentioned twice. > How about: > Bitmap tracking the available hardware counters when operating in > "mbm_cntr_assign" mode. A hardware counter can be assigned to a > RMID, event pair. Sure. > >> +static __init unsigned long *mbm_cntrs_init(struct rdt_resource *r) >> +{ >> + r->mon.mbm_cntr_free_map = bitmap_zalloc(r->mon.num_mbm_cntrs, >> + GFP_KERNEL); >> + if (r->mon.mbm_cntr_free_map) >> + bitmap_fill(r->mon.mbm_cntr_free_map, r->mon.num_mbm_cntrs); >> + >> + return r->mon.mbm_cntr_free_map; >> +} >> + >> +static __exit void mbm_cntrs_exit(struct rdt_resource *r) >> +{ >> + bitmap_free(r->mon.mbm_cntr_free_map); >> + r->mon.mbm_cntr_free_map = NULL; >> +} >> + >> static __init int dom_data_init(struct rdt_resource *r) >> { >> u32 idx_limit = resctrl_arch_system_num_rmid_idx(); >> @@ -1020,6 +1041,17 @@ static __init int dom_data_init(struct rdt_resource *r) >> goto out_unlock; >> } >> >> + if (r->mon.mbm_cntr_assignable && !mbm_cntrs_init(r)) { >> + if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) { >> + kfree(closid_num_dirty_rmid); >> + closid_num_dirty_rmid = NULL; >> + } >> + kfree(rmid_ptrs); >> + rmid_ptrs = NULL; >> + err = -ENOMEM; >> + goto out_unlock; >> + } >> + >> for (i = 0; i < idx_limit; i++) { >> entry = &rmid_ptrs[i]; >> INIT_LIST_HEAD(&entry->list); >> @@ -1056,6 +1088,9 @@ static void __exit dom_data_exit(struct rdt_resource *r) >> kfree(rmid_ptrs); >> rmid_ptrs = NULL; >> >> + if (r->mon.mbm_cntr_assignable) >> + mbm_cntrs_exit(r); >> + >> mutex_unlock(&rdtgroup_mutex); >> } >> >> @@ -1210,10 +1245,6 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r) >> */ >> resctrl_rmid_realloc_threshold = resctrl_arch_round_mon_val(threshold); >> >> - ret = dom_data_init(r); >> - if (ret) >> - return ret; >> - >> if (rdt_cpu_has(X86_FEATURE_BMEC)) { >> u32 eax, ebx, ecx, edx; >> >> @@ -1240,6 +1271,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r) >> } >> } >> >> + ret = dom_data_init(r); >> + if (ret) >> + return ret; >> + >> l3_mon_evt_init(r); >> >> r->mon_capable = true; >> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index c48b5450e6c2..8ffebd203c31 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -185,6 +185,25 @@ bool closid_allocated(unsigned int closid) >> return !test_bit(closid, &closid_free_map); >> } >> >> +int mbm_cntr_alloc(struct rdt_resource *r) >> +{ >> + int cntr_id; >> + >> + cntr_id = find_first_bit(r->mon.mbm_cntr_free_map, >> + r->mon.num_mbm_cntrs); >> + if (cntr_id >= r->mon.num_mbm_cntrs) >> + return -ENOSPC; >> + >> + __clear_bit(cntr_id, r->mon.mbm_cntr_free_map); >> + >> + return cntr_id; >> +} >> + >> +void mbm_cntr_free(struct rdt_resource *r, u32 cntr_id) >> +{ >> + __set_bit(cntr_id, r->mon.mbm_cntr_free_map); >> +} >> + >> /** >> * rdtgroup_mode_by_closid - Return mode of resource group with closid >> * @closid: closid if the resource group >> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h >> index f11d6fdfd977..5a4d6adec974 100644 >> --- a/include/linux/resctrl.h >> +++ b/include/linux/resctrl.h >> @@ -187,12 +187,14 @@ enum resctrl_scope { >> * @num_rmid: Number of RMIDs available >> * @num_mbm_cntrs: Number of assignable monitoring counters >> * @mbm_cntr_assignable:Is system capable of supporting monitor assignment? >> + * @mbm_cntr_free_map: bitmap of free MBM counters >> * @evt_list: List of monitoring events >> */ > > Please follow custom of existing doc and have description start with capital letter. Sure. > >> struct resctrl_mon { >> int num_rmid; >> int num_mbm_cntrs; >> bool mbm_cntr_assignable; >> + unsigned long *mbm_cntr_free_map; >> struct list_head evt_list; >> }; >> > > Reinette > -- Thanks Babu Moger