Re: [PATCH v4 15/19] x86/resctrl: Add the interface to unassign ABMC counter

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

Hi Reinette,

On 6/13/24 20:49, Reinette Chatre wrote:
> Hi Babu,
> 
> On 5/24/24 5:23 AM, Babu Moger wrote:
>> Hardware provides a limited number of ABMC counters. Once all the
>> counters are exhausted, counters need to be freed for new assignments.
>>
>> Provide the interface to unassign the counter.
> 
> Please write a proper changelog. This needs information that explains
> what this patch does and why.

Sure. Will elaborate on this.
> 
>>
>> The feature details are documented in the APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>>      Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable
>> Bandwidth
>>      Monitoring (ABMC).
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> ---
>> v4: Added domain specific unassign feature.
>>      Few name changes.
>>
>> v3: Removed the static from the prototype of rdtgroup_unassign_abmc.
>>      The function is not called directly from user anymore. These
>>      changes are related to global assignment interface.
>>
>> v2: No changes.
>> ---
>>   arch/x86/kernel/cpu/resctrl/internal.h |  2 ++
>>   arch/x86/kernel/cpu/resctrl/rdtgroup.c | 42 ++++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
>> b/arch/x86/kernel/cpu/resctrl/internal.h
>> index a88c8fc5e4df..e16244895350 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -660,6 +660,8 @@ void arch_domain_mbm_evt_config(struct rdt_hw_domain
>> *hw_dom);
>>   int resctrl_arch_assign(struct rdt_domain *d, u32 evtid, u32 rmid,
>>               u32 ctr_id, u32 closid, bool enable);
>>   int resctrl_grp_assign(struct rdtgroup *rdtgrp, u32 evtid);
>> +int resctrl_grp_unassign(struct rdtgroup *rdtgrp, u32 evtid);
>> +void num_cntrs_free(u32 ctr_id);
>>   void rdt_staged_configs_clear(void);
>>   bool closid_allocated(unsigned int closid);
>>   int resctrl_find_cleanest_closid(void);
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 48df76499a04..5ea1e58c7201 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -216,6 +216,11 @@ static int assign_cntrs_alloc(void)
>>       return ctr_id;
>>   }
>>   +void num_cntrs_free(u32 ctr_id)
> 
> The name does not reflect what it does. It neither frees the "num_cntrs"
> information
> nor does it free "num_cntrs" of counters. How about "mon_cntr_free()"?

Sure. If we change the name of num_cntrs to mbm_cntrs, then we can change
to mbm_cntr_free().
Here is the comments on name change.
https://lore.kernel.org/lkml/62fe683f-3a4c-4280-8770-d2aaff478d33@amd.com/


> 
> 
>> +{
>> +    __set_bit(ctr_id, &num_cntrs_free_map);
>> +}
>> +
>>   /**
>>    * rdtgroup_mode_by_closid - Return mode of resource group with closid
>>    * @closid: closid if the resource group
>> @@ -1931,6 +1936,43 @@ int resctrl_grp_assign(struct rdtgroup *rdtgrp,
>> u32 evtid)
>>       return 0;
>>   }
>>   +int resctrl_grp_unassign(struct rdtgroup *rdtgrp, u32 evtid)
> 
> Same comment wrt namespace. Also this function needs a description.

Sure.

> 
>> +{
>> +    struct rdt_resource *r =
>> &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>> +    struct rdt_domain *d;
>> +    u32 mon_state;
>> +    int index;
>> +
>> +    index = mon_event_config_index_get(evtid);
>> +    if (index == INVALID_CONFIG_INDEX) {
>> +        pr_warn_once("Invalid event id %d\n", evtid);
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (evtid == QOS_L3_MBM_TOTAL_EVENT_ID) {
>> +        mon_state = ASSIGN_TOTAL;
>> +    } else if (evtid == QOS_L3_MBM_LOCAL_EVENT_ID) {
>> +        mon_state = ASSIGN_LOCAL;
>> +    } else {
>> +        rdt_last_cmd_puts("Invalid event id\n");
>> +        return -EINVAL;
>> +    }
>> +
>> +    if (rdtgrp->mon.mon_state & mon_state) {
>> +        list_for_each_entry(d, &r->domains, list)
>> +            resctrl_arch_assign(d, evtid, rdtgrp->mon.rmid,
>> +                        rdtgrp->mon.ctr_id[index],
>> +                        rdtgrp->closid, 0);
>> +
>> +        /* Update the counter bitmap */
>> +        num_cntrs_free(rdtgrp->mon.ctr_id[index]);
>> +    }
>> +
>> +    rdtgrp->mon.mon_state &= ~mon_state;
>> +
>> +    return 0;
>> +}
>> +
>>   /* rdtgroup information files for one cache resource. */
>>   static struct rftype res_common_files[] = {
>>       {
> 
> 
> Reinette
> 

-- 
Thanks
Babu Moger