Re: [PATCH v12 14/26] x86/resctrl: Add the functionality to assign MBM events

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 4/11/25 16:04, Reinette Chatre wrote:
> 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.
> 

Yes. Multiple counter-ids can be assigned to 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"?

Sure.

> 
>>
>> 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@amd.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?

Yes. It can be done. Then I have to export the functions like
mbm_get_assign_config() into monitor.c. To avoid that I passed it from
here which I felt much more cleaner.

> 
>>  
>>  #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.

Yea. I missed it. Will take care of it next version.

> 
>> +
>> +	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()).

Sure. Will add details about these functions.

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

Yes. It can be done. Then I have to export the functions like
mbm_get_assign_config() into monitor.c. To avoid that I passed it from
here which I felt much more cleaner.


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

Sure. We can print r->name.

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

Sure. We can do that.

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

-- 
Thanks
Babu Moger