linux-kernel - Re: [PATCH v4 09/19] x86/resctrl: Initialize ABMC counters bitmap

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: <ace7d045-1717-c8fd-8723-59c2cd8feecf@amd.com>
Date: Fri, 21 Jun 2024 11:01:32 -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 v4 09/19] x86/resctrl: Initialize ABMC counters bitmap

Hi Reinette,

On 6/20/2024 5:20 PM, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/19/24 10:03 AM, Moger, Babu wrote:
>> On 6/13/24 20:42, Reinette Chatre wrote:
>>> On 5/24/24 5:23 AM, Babu Moger wrote:
> 
> ...
> 
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index 3071bbb7a15e..400ae405e10e 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -186,6 +186,23 @@ bool closid_allocated(unsigned int closid)
>>>>        return !test_bit(closid, &closid_free_map);
>>>>    }
>>>>    +/*
>>>> + * ABMC Counter bitmap and length for tracking available counters.
>>>> + * ABMC feature provides set of hardware counters for enabling events.
>>>> + * Each event takes one hardware counter. Kernel needs to keep track
>>>> + * of number of available counters.
>>>> + */
>>>> +static unsigned long num_cntrs_free_map;
>>>
>>> Why does variable have "num" in its name? That seems strange. How
>>> about just "mon_cntrs_free_map
>>
>> It came from patch 4/19.
>>
>> struct resctrl_mon {
>>          int                     num_rmid;
>> +       int                     num_cntrs;
>>          struct list_head        evt_list;
>>   };
>>
>> num_cntrs_free_map is a bitmap representing num_cntrs. Kept the matching
>> name for better understanding. Renaming it will loose that connection.
> 
> I disagree. The "num" in "num_cntrs" indicates that this variable stores
> the _number_ of a particular entity. In this case "cntrs" or ... counters.
> This is just like how resctrl uses "num_closid" to indicate how many closid
> are available and then have a separate "closid_free_map" to actually track
> now closids are used ... it is _not_ "num_closid_free_map". Similarly,
> "num_rmid" indicates how many RMID are available and the "rmid_free_lru"
> tracks how RMID are used ... it is _not_ "num_rmid_free_lru".
> 
>> If I rename then I will have to rename both.
> 
> No, you do not.
> 
>>
>> How about mbm_cntrs and mbm_cntrs_free_map?
> 
> "mbm_cntrs" does sound good. It is more specific than "cntrs". I would 
> suggest that
> use "num_mbm_cntrs" to match with "num_rmid" and "num_closid" and then
> you can introduce "mbm_cntrs_free_map".

Sure.

> 
>>>> +static u32 num_cntrs_free_map_len;
>>>
>>> Same comment about "num" ... also, any special reason why u32 is needed?
>>
>> Only reason is, it is supposed to be unsigned.  I can change it "unsigned
>> int".
>>
>>>
>>>> +
>>>> +static void num_cntrs_init(void)
>>>
>>> mon_cntrs_init() ?
>>
>> mbm_cntrs_init?
> 
> Sounds good.
ok

> 
>>
>>>
>>>> +{
>>>> +    struct rdt_resource *r =
>>>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>>> +
>>>> +    bitmap_fill(&num_cntrs_free_map, r->mon.num_cntrs);
>>>> +    num_cntrs_free_map_len = r->mon.num_cntrs;
>>>> +}
>>>> +
>>>>    /**
>>>>     * rdtgroup_mode_by_closid - Return mode of resource group with 
>>>> closid
>>>>     * @closid: closid if the resource group
>>>> @@ -2459,6 +2476,12 @@ static int resctrl_abmc_set_all(enum
>>>> resctrl_res_level l, bool enable)
>>>
>>> resctrl_abmc_set_all() was initially created as a complement of
>>> resctrl_abmc_set_one() ... but with more initialization added to
>>> resctrl_abmc_set_all() this relationship becomes vague.
>>
>> Yes. Understood. Let me know if want me to change anything here.
> 
> How about renaming resctrl_abmc_set_all() to _resctrl_abmc_enable()?

Sure.
-- 
- Babu Moger