Hi Babu, On 11/20/24 6:14 PM, Moger, Babu wrote: > On 11/18/2024 1:43 PM, Reinette Chatre wrote: >> On 10/29/24 4:21 PM, Babu Moger wrote: >>> +static void resctrl_arch_update_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, >>> + enum resctrl_event_id evtid, u32 rmid, >>> + u32 closid, u32 cntr_id, u32 val) >>> +{ >>> + union l3_qos_abmc_cfg abmc_cfg = { 0 }; >>> + >>> + abmc_cfg.split.cfg_en = 1; >>> + abmc_cfg.split.cntr_en = 1; >>> + abmc_cfg.split.cntr_id = cntr_id; >>> + abmc_cfg.split.bw_src = rmid; >>> + abmc_cfg.split.bw_type = val; >>> + >>> + wrmsrl(MSR_IA32_L3_QOS_ABMC_CFG, abmc_cfg.full); >> >> Is it needed to create an almost duplicate function? What if instead >> only resctrl_arch_config_cntr() exists and it uses parameter to decide >> whether to call resctrl_abmc_config_one_amd() directly or via >> smp_call_function_any()? I think that should help to make clear how >> the code flows. >> Also note that this is an almost identical arch callback with no >> error return. I expect that building on existing resctrl_arch_config_cntr() >> will make things easier to understand. > > It can be done. But it takes another parameter to the function. > It has 7 parameters already. This will be 8th. > Will change it if that is ok. Please correct me if I am wrong but I am not familiar with a restriction on number of parameters. It seems unnecessary to me to create two almost duplicate 7 parameter functions to avoid one 8 parameter function. >> Since MBM_EVENT_ARRAY_INDEX is a macro it can be called closer to where it is used, >> within rdtgroup_find_grp_by_cntr_id_index(), which prompts a reconsider of that function name. > > > How about ? > > static struct rdtgroup *rdtgroup_find_grp_by_cntr_id_event(int cntr_id, enum resctrl_event_id evtid) ... or for something shorter just get_rdtgroup_from_cntr_event(), but no hard requirement. Reinette