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

Reinette Chatre
2024-06-14T01:49:44Z

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