linux-kernel - Re: [PATCH v5 10/20] x86/resctrl: Introduce mbm_total_cfg and mbm_local

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <11f0276d-56d6-2977-49f8-1d50e7246382@amd.com>
Date: Tue, 16 Jul 2024 17:43:26 -0500
From: "Moger, Babu" <bmoger@....com>
To: Reinette Chatre <reinette.chatre@...el.com>, babu.moger@....com,
 corbet@....net, fenghua.yu@...el.com, tglx@...utronix.de, mingo@...hat.com,
 bp@...en8.de, dave.hansen@...ux.intel.com
Cc: x86@...nel.org, hpa@...or.com, paulmck@...nel.org, rdunlap@...radead.org,
 tj@...nel.org, peterz@...radead.org, yanjiewtw@...il.com,
 kim.phillips@....com, lukas.bulwahn@...il.com, seanjc@...gle.com,
 jmattson@...gle.com, leitao@...ian.org, jpoimboe@...nel.org,
 rick.p.edgecombe@...el.com, kirill.shutemov@...ux.intel.com,
 jithu.joseph@...el.com, kai.huang@...el.com, kan.liang@...ux.intel.com,
 daniel.sneddon@...ux.intel.com, pbonzini@...hat.com, sandipan.das@....com,
 ilpo.jarvinen@...ux.intel.com, peternewman@...gle.com,
 maciej.wieczor-retman@...el.com, linux-doc@...r.kernel.org,
 linux-kernel@...r.kernel.org, eranian@...gle.com, james.morse@....com
Subject: Re: [PATCH v5 10/20] x86/resctrl: Introduce mbm_total_cfg and
 mbm_local_cfg

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