Re: [PATCH v5 10/20] x86/resctrl: Introduce mbm_total_cfg and mbm_local_cfg

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

Hi Babu,

On 7/16/24 12:21 PM, Moger, Babu wrote:
> On 7/12/24 17:08, Reinette Chatre wrote:
>> On 7/3/24 2:48 PM, Babu Moger wrote:

>>> @@ -662,6 +666,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);
>>> +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom);
>>> +unsigned int mon_event_config_index_get(u32 evtid);
>>>    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 7a93a6d2b2de..b96b0a8bd7d3 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>>> @@ -1256,6 +1256,28 @@ int __init rdt_get_mon_l3_config(struct
>>> rdt_resource *r)
>>>        return 0;
>>>    }
>>>    +void resctrl_arch_mbm_evt_config(struct rdt_hw_mon_domain *hw_dom)
>>
>> A function is expected to have a verb in its name and the verb here seems
>> to be
>> "config", which does not seem appropriate and creates confusion with
>> resctrl_arch_event_config_set(). How about resctrl_arch_mbm_evt_config_init()
>> with proper initializer of the config values to also cover case when
>> events are
>> not configurable (INVALID_CONFIG_VALUE introduced in next patch?) ?
> 
> Sorry. I am not clear on this comment. Can you please elaborate?

This comment has two parts.

First, there is the naming of the function.
The name of the function should reflect what the function does and I
believe that resctrl_arch_mbm_evt_config() is close enough to
resctrl_arch_event_config_set() to cause confusion while also lacking
an expected verb in the function name (since "config" should not be
considered a verb here) . I proposed resctrl_arch_mbm_evt_config_init()
as a new function name that has the "init" verb to indicate that this
function "init"ializes the MBM config values.

Second, there is the work done by the function.
In this implementation the function initializes
rdt_hw_mon_domain->mbm_total_cfg and rdt_hw_mon_domain->mbm_local_cfg
when the events are configurable. I proposed that as an initializer
the function can be expected to initialize rdt_hw_mon_domain->mbm_total_cfg
and rdt_hw_mon_domain->mbm_local_cfg whether the events are configurable
or not. In the latter case they can be initialized with INVALID_CONFIG_VALUE?

Reinette