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@xxxxxxx>
>> ---
>> 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
