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

[Reinette Chatre <reinette.chatre@intel.com>]

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.

> 
> 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()"?


> +{
> +	__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.

> +{
> +	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