Re: [PATCH v5 09/20] x86/resctrl: Initialize monitor counters bitmap

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 7/12/24 17:07, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/3/24 2:48 PM, 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 enabled.
>>
>> Introduce mbm_cntrs_free_map bitmap to track available and free counters.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>> v5:
>>    Updated the comments and commit log.
>>    Few renames
>>     num_cntrs_free_map -> mbm_cntrs_free_map
>>     num_cntrs_init -> mbm_cntrs_init
>>     Added initialization in rdt_get_tree because the default ABMC
>>     enablement happens during the init.
>>
>> 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 | 29 ++++++++++++++++++++++++--
>>   1 file changed, 27 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 4f47f52e01c2..b3d3fa048f15 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -185,6 +185,23 @@ bool closid_allocated(unsigned int closid)
>>       return !test_bit(closid, &closid_free_map);
>>   }
>>   +/*
>> + * Counter bitmap and its 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
> 
> What is meant with "Kernel" here? It looks to be the fs code but the
> implementation has both fs and arch code reaching into the counter
> management. This should not be the case, either the fs code or the
> arch code needs to manage the counters, not both.

Yes. This needs to be done at FS code.

> 
>> + * of number of available counters.
>> + */
>> +static unsigned long mbm_cntrs_free_map;
> 
> With the lengths involved this needs a proper DECLARE_BITMAP()

Sure.

> 
>> +static unsigned int mbm_cntrs_free_map_len;
>> +
>> +static void mbm_cntrs_init(void)
>> +{
>> +    struct rdt_resource *r =
>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> +
>> +    bitmap_fill(&mbm_cntrs_free_map, r->mon.num_mbm_cntrs);
>> +    mbm_cntrs_free_map_len = r->mon.num_mbm_cntrs;
>> +}
>> +
>>   /**
>>    * rdtgroup_mode_by_closid - Return mode of resource group with closid
>>    * @closid: closid if the resource group
>> @@ -2466,6 +2483,12 @@ static int _resctrl_abmc_enable(struct
>> rdt_resource *r, bool enable)
>>   {
>>       struct rdt_mon_domain *d;
>>   +    /*
>> +     * Clear all the previous assignments while switching the monitor
>> +     * mode.
>> +     */
>> +    mbm_cntrs_init();
>> +
> 
> If the counters are managed by fs code then the arch code should not be
> doing this. If needed the fs code should init the counters before calling
> the arch helpers.

Yes. We cannot make this call from here. I need to move this call to FS
layer before coming to this code. Thanks for good point.

> 
>>       /*
>>        * Hardware counters will reset after switching the monitor mode.
>>        * Reset the architectural state so that reading of hardware
>> @@ -2724,10 +2747,10 @@ static void schemata_list_destroy(void)
>>     static int rdt_get_tree(struct fs_context *fc)
>>   {
>> +    struct rdt_resource *r =
>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>       struct rdt_fs_context *ctx = rdt_fc2context(fc);
>>       unsigned long flags = RFTYPE_CTRL_BASE;
>>       struct rdt_mon_domain *dom;
>> -    struct rdt_resource *r;
>>       int ret;
>>         cpus_read_lock();
>> @@ -2756,6 +2779,9 @@ static int rdt_get_tree(struct fs_context *fc)
>>         closid_init();
>>   +    if (r->mon.abmc_capable)
>> +        mbm_cntrs_init();
>> +
>>       if (resctrl_arch_mon_capable())
>>           flags |= RFTYPE_MON;
>>   @@ -2800,7 +2826,6 @@ static int rdt_get_tree(struct fs_context *fc)
>>           resctrl_mounted = true;
>>         if (is_mbm_enabled()) {
>> -        r = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>           list_for_each_entry(dom, &r->mon_domains, hdr.list)
>>               mbm_setup_overflow_handler(dom, MBM_OVERFLOW_INTERVAL,
>>                              RESCTRL_PICK_ANY_CPU);
> 
> Reinette
> 

-- 
Thanks
Babu Moger