[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <5df878a3-2b47-4027-984c-88e8ac34de05@intel.com>
Date: Thu, 20 Jun 2024 15:20:47 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: <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 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".
>>> +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.
>
>>
>>> +{
>>> + 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()?
Reinette
Powered by blists - more mailing lists