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

["Moger, Babu" <bmoger@amd.com>]

Hi Reinette,

On 7/16/2024 3:42 PM, Reinette Chatre wrote:
> 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.

Yes. Make sense.
> 
> 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?

Yes. Thanks for the clarifications.

-- 
- Babu Moger