Message-ID: <59fbd325-04e8-459f-a724-ae0c4536b1a5@intel.com>
Date: Fri, 11 Apr 2025 14:04:47 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Babu Moger <babu.moger@....com>, <tony.luck@...el.com>,
	<peternewman@...gle.com>
CC: <corbet@....net>, <tglx@...utronix.de>, <mingo@...hat.com>,
	<bp@...en8.de>, <dave.hansen@...ux.intel.com>, <x86@...nel.org>,
	<hpa@...or.com>, <paulmck@...nel.org>, <akpm@...ux-foundation.org>,
	<thuth@...hat.com>, <rostedt@...dmis.org>, <ardb@...nel.org>,
	<gregkh@...uxfoundation.org>, <daniel.sneddon@...ux.intel.com>,
	<jpoimboe@...nel.org>, <alexandre.chartre@...cle.com>,
	<pawan.kumar.gupta@...ux.intel.com>, <thomas.lendacky@....com>,
	<perry.yuan@....com>, <seanjc@...gle.com>, <kai.huang@...el.com>,
	<xiaoyao.li@...el.com>, <kan.liang@...ux.intel.com>, <xin3.li@...el.com>,
	<ebiggers@...gle.com>, <xin@...or.com>, <sohil.mehta@...el.com>,
	<andrew.cooper3@...rix.com>, <mario.limonciello@....com>,
	<linux-doc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<maciej.wieczor-retman@...el.com>, <eranian@...gle.com>
Subject: Re: [PATCH v12 14/26] x86/resctrl: Add the functionality to assign
 MBM events

Hi Babu,

On 4/3/25 5:18 PM, Babu Moger wrote:
> The mbm_cntr_assign mode offers "num_mbm_cntrs" number of counters that
> can be assigned to an RMID, event pair and monitor the bandwidth as long
> as it is assigned.

Above makes it sound as though multiple counters can be assigned to
an RMID, event pair.

> 
> Add the functionality to allocate and assign the counters to RMID, event
> pair in the domain.

"assign *a* counter to an RMID, event pair"?

> 
> If all the counters are in use, the kernel will log the error message
> "Unable to allocate counter in domain" in /sys/fs/resctrl/info/
> last_cmd_status when a new assignment is requested. Exit on the first
> failure when assigning counters across all the domains.
> 
> Signed-off-by: Babu Moger <babu.moger@....com>
> ---

...

> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |   2 +
>  arch/x86/kernel/cpu/resctrl/monitor.c  | 124 +++++++++++++++++++++++++
>  2 files changed, 126 insertions(+)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index 0b73ec451d2c..1a8ac511241a 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -574,6 +574,8 @@ bool closid_allocated(unsigned int closid);
>  int resctrl_find_cleanest_closid(void);
>  void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
>  unsigned int mon_event_config_index_get(u32 evtid);
> +int resctrl_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
> +			      struct rdtgroup *rdtgrp, enum resctrl_event_id evtid, u32 evt_cfg);

This is internal to resctrl fs. Why is it needed to provide both the event id
and the event configuration? Event configuration can be determined from event ID?

>  
>  #ifdef CONFIG_RESCTRL_FS_PSEUDO_LOCK
>  int rdtgroup_locksetup_enter(struct rdtgroup *rdtgrp);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 77f8662dc50b..ff55a4fe044f 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -1469,3 +1469,127 @@ int resctrl_arch_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
>  
>  	return 0;
>  }
> +
> +/*
> + * Configure the counter for the event, RMID pair for the domain. Reset the
> + * non-architectural state to clear all the event counters.
> + */
> +static int resctrl_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> +			       enum resctrl_event_id evtid, u32 rmid, u32 closid,
> +			       u32 cntr_id, u32 evt_cfg, bool assign)
> +{
> +	struct mbm_state *m;
> +	int ret;
> +
> +	ret = resctrl_arch_config_cntr(r, d, evtid, rmid, closid, cntr_id, evt_cfg, assign);
> +	if (ret)
> +		return ret;

I understood previous discussion to conclude that resctrl_arch_config_cntr() cannot fail
and thus I expect it to return void and not need any error checking from caller.
By extension this will result in resctrl_config_cntr() returning void and should simplify
a few flows. For example, it will make it clear that re-configuring an existing counter
cannot result in that counter being freed.

> +
> +	m = get_mbm_state(d, closid, rmid, evtid);
> +	if (m)
> +		memset(m, 0, sizeof(struct mbm_state));
> +
> +	return ret;
> +}
> +

Could you please add comments to these mbm_cntr* functions to provide information
on how the cntr_cfg data structure is used? Please also include details on
callers since it seems to me as though these functions are called
from paths where assignable counters are not supported (mon_event_read()).

> +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 -ENOENT;
> +}
> +
> +static int mbm_cntr_alloc(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) {
> +			d->cntr_cfg[cntr_id].rdtgrp = rdtgrp;
> +			d->cntr_cfg[cntr_id].evtid = evtid;
> +			return cntr_id;
> +		}
> +	}
> +
> +	return -ENOSPC;
> +}
> +
> +static void mbm_cntr_free(struct rdt_mon_domain *d, int cntr_id)
> +{
> +	memset(&d->cntr_cfg[cntr_id], 0, sizeof(struct mbm_cntr_cfg));
> +}
> +
> +/*
> + * Allocate a fresh counter and configure the event if not assigned already.
> + */
> +static int resctrl_alloc_config_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
> +				     struct rdtgroup *rdtgrp, enum resctrl_event_id evtid,
> +				     u32 evt_cfg)

Same here, why are both evtid and evt_cfg provided as arguments? 

> +{
> +	int cntr_id, ret = 0;
> +
> +	/*
> +	 * No need to allocate or configure if the counter is already assigned
> +	 * and the event configuration is up to date.
> +	 */
> +	cntr_id = mbm_cntr_get(r, d, rdtgrp, evtid);
> +	if (cntr_id >= 0) {
> +		if (d->cntr_cfg[cntr_id].evt_cfg == evt_cfg)
> +			return 0;
> +
> +		goto cntr_configure;
> +	}
> +
> +	cntr_id = mbm_cntr_alloc(r, d, rdtgrp, evtid);
> +	if (cntr_id <  0) {
> +		rdt_last_cmd_printf("Unable to allocate counter in domain %d\n",
> +				    d->hdr.id);

Please print resource name also.

> +		return cntr_id;
> +	}
> +
> +cntr_configure:
> +	/* Update and configure the domain with the new event configuration value */
> +	d->cntr_cfg[cntr_id].evt_cfg = evt_cfg;
> +
> +	ret = resctrl_config_cntr(r, d, evtid, rdtgrp->mon.rmid, rdtgrp->closid,
> +				  cntr_id, evt_cfg, true);
> +	if (ret) {
> +		rdt_last_cmd_printf("Assignment of event %d failed on domain %d\n",
> +				    evtid, d->hdr.id);

How is user expected to interpret the event ID (especially when looking forward
where events can be dynamic)? This should rather be the event name.

> +		mbm_cntr_free(d, cntr_id);
> +	}
> +
> +	return ret;
> +}
> +
> +/*
> + * Assign a hardware counter to event @evtid of group @rdtgrp. Counter will be
> + * assigned to all the domains if @d is NULL else the counter will be assigned
> + * to @d.
> + */
> +int resctrl_assign_cntr_event(struct rdt_resource *r, struct rdt_mon_domain *d,
> +			      struct rdtgroup *rdtgrp, enum resctrl_event_id evtid,
> +			      u32 evt_cfg)
> +{
> +	int ret = 0;
> +
> +	if (!d) {
> +		list_for_each_entry(d, &r->mon_domains, hdr.list) {
> +			ret = resctrl_alloc_config_cntr(r, d, rdtgrp, evtid, evt_cfg);
> +			if (ret)
> +				return ret;
> +		}
> +	} else {
> +		ret = resctrl_alloc_config_cntr(r, d, rdtgrp, evtid, evt_cfg);
> +	}
> +
> +	return ret;
> +}

Reinette

Powered by blists - more mailing lists