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@amd.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;
+static u32 num_cntrs_free_map_len;
+
+static void num_cntrs_init(void)
+{
+	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)
 	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.
+	 */
+	num_cntrs_init();
+
 	/*
 	 * Update QOS_CFG MSR on all the CPUs associated with the resource
 	 * Hardware counters will reset after switching the monotor mode.
-- 
2.34.1

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@amd.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

> +static u32 num_cntrs_free_map_len;

Same comment about "num" ... also, any special reason why u32 is needed?

> +
> +static void num_cntrs_init(void)

mon_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.

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


> +	 */
> +	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

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@amd.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

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

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