Re: [PATCH v10 17/24] x86/resctrl: Add the interface to unassign a counter

[Reinette Chatre]

Hi Babu,

On 12/12/24 12:15 PM, Babu Moger wrote:
> The mbm_cntr_assign mode provides a 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
> already assigned counter.
> 
> Introduce an interface that allows for the unassignment of counter IDs
> from the domain.

Subject and changelog claims this introduces an interface, there is no new
resctrl interface introduced here. Can this be more specific?

> 
> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> ---
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  2 +
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 52 ++++++++++++++++++++++++++
>  2 files changed, 54 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 70d2577fc377..f858098dbe4b 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -706,6 +706,8 @@ int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>  			     u32 cntr_id, bool assign);
>  int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>  			       struct rdt_mon_domain *d, enum resctrl_event_id evtid);
> +int rdtgroup_unassign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
> +				 struct rdt_mon_domain *d, enum resctrl_event_id evtid);

(please use consistent parameter ordering)

>  struct mbm_state *get_mbm_state(struct rdt_mon_domain *d, u32 closid,
>  				u32 rmid, enum resctrl_event_id evtid);
>  #endif /* _ASM_X86_RESCTRL_INTERNAL_H */
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 1c8694a68cf4..a71a8389b649 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1990,6 +1990,20 @@ static void mbm_cntr_free(struct rdt_resource *r, struct rdt_mon_domain *d,
>  	}
>  }
>  
> +static int mbm_cntr_get(struct rdt_resource *r, struct rdt_mon_domain *d,
> +			struct rdtgroup *rdtgrp, enum resctrl_event_id evtid)
> +{
> +	int cntr_id;
> +
> +	for (cntr_id = 0; cntr_id < r->mon.num_mbm_cntrs; cntr_id++) {
> +		if (d->cntr_cfg[cntr_id].rdtgrp == rdtgrp &&
> +		    d->cntr_cfg[cntr_id].evtid == evtid)
> +			return cntr_id;
> +	}
> +
> +	return -EINVAL;

This could be -ENOENT?

> +}

mbm_cntr_get() seems to be essentially a duplicate of mbm_cntr_assigned() that returns
actual counter ID instrad of true/false. Could only one be used?

> +
>  /*
>   * Assign a hardware counter to event @evtid of group @rdtgrp.
>   * Counter will be assigned to all the domains if rdt_mon_domain is NULL
> @@ -2037,6 +2051,44 @@ int rdtgroup_assign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
>  	return ret;
>  }
>  
> +/*
> + * Unassign a hardware counter associated with @evtid from the domain and
> + * the group. Unassign the counters from all the domains if rdt_mon_domain
> + * is NULL else unassign from the specific domain.

(same comment as previous patch about consistency in referring to function
parameters)

> + */
> +int rdtgroup_unassign_cntr_event(struct rdt_resource *r, struct rdtgroup *rdtgrp,
> +				 struct rdt_mon_domain *d, enum resctrl_event_id evtid)
> +{
> +	int cntr_id, ret = 0;
> +
> +	if (!d) {
> +		list_for_each_entry(d, &r->mon_domains, hdr.list) {
> +			if (!mbm_cntr_assigned(r, d, rdtgrp, evtid))
> +				continue;
> +
> +			cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
> +

It seems unnecessary to loop over array twice here. mbm_cntr_assigned() seems
unnecessary. Return value of mbm_cntr_get() can be used to determine if it
is assigned or not?

> +			ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
> +						  rdtgrp->closid, cntr_id, false);
> +			if (!ret)
> +				mbm_cntr_free(r, d, rdtgrp, evtid);

... and by providing cntr_id to mbm_cntr_free() another unnecessary loop can be avoided.

> +		}
> +	} else {
> +		if (!mbm_cntr_assigned(r, d, rdtgrp, evtid))
> +			goto out_done_unassign;
> +
> +		cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
> +
> +		ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid,
> +					  rdtgrp->closid, cntr_id, false);
> +		if (!ret)
> +			mbm_cntr_free(r, d, rdtgrp, evtid);
> +	}
> +
> +out_done_unassign:
> +	return ret;
> +}
> +
>  /* rdtgroup information files for one cache resource. */
>  static struct rftype res_common_files[] = {
>  	{

Reinette