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*. > > 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. > +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. > 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