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: <62fe683f-3a4c-4280-8770-d2aaff478d33@amd.com>
Date: Wed, 19 Jun 2024 12:03:06 -0500
From: "Moger, Babu" <babu.moger@....com>
To: Reinette Chatre <reinette.chatre@...el.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/13/24 20:42, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/24/24 5:23 AM, Babu Moger wrote:
>> Hardware provides a set of counters when the ABMC feature is supported.
>> These counters are used for enabling the events in resctrl group when
>> the feature is supported.
>>
>> Introduce num_cntrs_free_map bitmap to track available and free counters.
>>
>> Signed-off-by: Babu Moger <babu.moger@....com>
>> ---
>> v4: Changed the name to num_cntrs where applicable.
>>      Used bitmap apis.
>>      Added more comments for the globals.
>>
>> v3: Changed the bitmap name to assign_cntrs_free_map. Removed abmc
>>      from the name.
>>
>> v2: Changed the bitmap name to assignable_counter_free_map from
>>      abmc_counter_free_map.
>> ---
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 23 +++++++++++++++++++++++
>>   1 file changed, 23 insertions(+)
>>
>> 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.
If I rename then I will have to rename both.

How about mbm_cntrs and mbm_cntrs_free_map?


> 
>> +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?

> 
>> +{
>> +    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.

> 
>>       struct rdt_resource *r = &rdt_resources_all[l].r_resctrl;
>>       struct rdt_domain *d;
>>   +    /*
>> +     * The bitmap num_cntrs_free_map needs to be reset when switching
>> +     * the monitor mode.
> 
> This comment puts in words what can be understood by just looking at the
> code.
> What would be useful would be to explain _why_ this needs to be done ...
> what happens to the previously assigned counters?

Sure. Will add more details.

> 
> 
>> +     */
>> +    num_cntrs_init();
>> +
>>       /*
>>        * Update QOS_CFG MSR on all the CPUs associated with the resource
>>        * Hardware counters will reset after switching the monotor mode.
> 
> Reinette
> 

-- 
Thanks
Babu Moger