Re: [PATCH v9 24/26] x86/resctrl: Update assignments on event configuration changes

[Reinette Chatre <reinette.chatre@intel.com>]

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