Re: [PATCH v11 16/23] x86/resctrl: Add the functionality to unassigm MBM events

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

Hi Babu,

subject: unassigm -> unassign

On 1/22/25 12:20 PM, Babu Moger wrote:
> The mbm_cntr_assign mode provides a limited number of hardware counters

(now back to "limited number of hardware counters")

> that can be assigned to an RMID, event pair to monitor bandwidth while
> assigned. If all counters are in use, the kernel will show an error
> message: "Out of MBM assignable counters" when a new assignment is
> requested. To make space for a new assignment, users must unassign an

To me "kernel will show an error" implies the kernel ring buffer. Please make
the message accurate and mention that it will be in 
last_cmd_status while also considering to use -ENOSPC to help user space.

> already assigned counter and retry the assignment again..

".." -> "."

> 
> Add the functionality to unassign and free the counters in the domain.
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>

...

> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 127c4000a81a..b6d188d0f9b7 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1518,3 +1518,42 @@ int resctrl_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
>  
>  	return ret;
>  }
> +
> +/*
> + * Unassign and free the counter if assigned else return success.
> + */
> +static int resctrl_free_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> +				    struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
> +{
> +	int cntr_id, ret = 0;
> +
> +	cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
> +	if (cntr_id != -ENOENT) {

This can be simplified and indent level reduced with:

	cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
	if (cntr_id < 0)
		return ret;

> +		ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
> +					  rdtgrp->closid, cntr_id, false);
> +		if (!ret)
> +			mbm_cntr_free(d, cntr_id);
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Unassign a hardware counter associated with @evtid from the domain and
> + * the group. Unassign the counters from all the domains if @d is NULL else
> + * unassign from @d.
> + */
> +int resctrl_unassign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
> +				struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
> +{
> +	int ret = 0;
> +
> +	if (!d) {
> +		list_for_each_entry(d, &r->mon_domains, hdr.list)
> +			ret = resctrl_free_config_cntr(r, d, rdtgrp, evtid);

Same issue as previous patch wrt error handling.

> +	} else {
> +		ret = resctrl_free_config_cntr(r, d, rdtgrp, evtid);
> +	}
> +
> +	return ret;
> +}

Reinette