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