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.

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

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>
---
v12: Fixed typo in the subjest line.
     Replaced several counters with "num_mbm_cntrs" counters.
     Changed the check in resctrl_alloc_config_cntr() to reduce the indentation.
     Fixed the handling error on first failure.
     Added domain id and event id on failure.
     Fixed the return error override.
     Added new parameter event configuration (evt_cfg) to get the event configuration
     from user space.

v11: Patch changed again quite a bit.
     Moved the functions to monitor.c.
     Renamed rdtgroup_assign_cntr_event() to resctrl_assign_cntr_event().
     Refactored the resctrl_assign_cntr_event().
     Added functionality to exit on the first error during assignment.
     Simplified mbm_cntr_free().
     Removed the function mbm_cntr_assigned(). Will be using mbm_cntr_get() to
     figure out if the counter is assigned or not.
     Updated commit message and code comments.

v10: Patch changed completely.
     Counters are managed at the domain based on the discussion.
     https://lore.kernel.org/lkml/CALPaoCj+zWq1vkHVbXYP0znJbe6Ke3PXPWjtri5AFgD9cQDCUg@mail.gmail.com/
     Reset non-architectural MBM state.
     Commit message update.

v9: Introduced new function resctrl_config_cntr to assign the counter, update
    the bitmap and reset the architectural state.
    Taken care of error handling(freeing the counter) when assignment fails.
    Moved mbm_cntr_assigned_to_domain here as it used in this patch.
    Minor text changes.

v8: Renamed rdtgroup_assign_cntr() to rdtgroup_assign_cntr_event().
    Added the code to return the error if rdtgroup_assign_cntr_event fails.
    Moved definition of MBM_EVENT_ARRAY_INDEX to resctrl/internal.h.
    Updated typo in the comments.

v7: New patch. Moved all the FS code here.
    Merged rdtgroup_assign_cntr and rdtgroup_alloc_cntr.
    Adde new #define MBM_EVENT_ARRAY_INDEX.
---
 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);
 
 #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;
+
+	m = get_mbm_state(d, closid, rmid, evtid);
+	if (m)
+		memset(m, 0, sizeof(struct mbm_state));
+
+	return ret;
+}
+
+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)
+{
+	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);
+		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);
+		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;
+}
-- 
2.34.1

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

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

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

Hi Babu,

On 4/15/25 7:20 AM, Moger, Babu wrote:
> 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.

oh, are you referring to the assignments of different counters across multiple
domains?

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

From what I can tell, for example by looking at patch #22, callers of
resctrl_assign_cntr_event() now need to call mbm_get_assign_config()
every time before calling resctrl_assign_cntr_event(). Calling
mbm_get_assign_config() from within resctrl_assign_cntr_event() seems
simpler to me and that may result in mbm_get_assign_config() moving to 
monitor.c as an extra benefit.

...

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

Maybe even resctrl_assign_cntr_event() does not need to call mbm_get_assign_config()
but only resctrl_alloc_config_cntr() needs to call mbm_get_assign_config(). Doing so
may avoid more burden on callers while reducing parameters needed throughout.

Reinette

Hi Reinette,

On 4/15/25 11:53, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/15/25 7:20 AM, Moger, Babu wrote:
>> 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.
> 
> oh, are you referring to the assignments of different counters across multiple
> domains?

May be I am confusing you here. This is what I meant.

Here is one example.

In a same group,
  Configure cntr_id 0, to count reads only (This maps to total event).
  Configure cntr_id 1, to count write only (This maps to local event).
  Configure cntr_id 2, to count dirty victims.
  so on..
  so on..
  Configure cntr_id 31, to count remote read only.

We have 32 counter ids in a domain. Basically, we can configure all the
counters in a domain to just one group if you want to.

We cannot do that right now because our data structures cannot do that.
We can only configure 2 events(local and total) right now.

My understanding it is same with MPAM also.

> 
>>
>>>>
>>>> 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.
> 
>>From what I can tell, for example by looking at patch #22, callers of
> resctrl_assign_cntr_event() now need to call mbm_get_assign_config()
> every time before calling resctrl_assign_cntr_event(). Calling
> mbm_get_assign_config() from within resctrl_assign_cntr_event() seems
> simpler to me and that may result in mbm_get_assign_config() moving to 
> monitor.c as an extra benefit.

Sure.

> 
> ...
> 
>>>> +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.
> 
> Maybe even resctrl_assign_cntr_event() does not need to call mbm_get_assign_config()
> but only resctrl_alloc_config_cntr() needs to call mbm_get_assign_config(). Doing so
> may avoid more burden on callers while reducing parameters needed throughout.
> 

ok. Sure. Will do.

-- 
Thanks
Babu Moger

Hi Babu,

On 4/16/25 10:09 AM, Moger, Babu wrote:
> On 4/15/25 11:53, Reinette Chatre wrote:
>> On 4/15/25 7:20 AM, Moger, Babu wrote:
>>> On 4/11/25 16:04, Reinette Chatre wrote:
>>>> 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.
>>
>> oh, are you referring to the assignments of different counters across multiple
>> domains?
> 
> May be I am confusing you here. This is what I meant.
> 
> Here is one example.
> 
> In a same group,

"same group" means single RMID, eg. RMID_A

>   Configure cntr_id 0, to count reads only (This maps to total event).

This will be one event, event0, so one counter assigned to RMID_A, event0 pair.

>   Configure cntr_id 1, to count write only (This maps to local event).

... event1, one counter assigned to RMID_A, event1 pair, ...

>   Configure cntr_id 2, to count dirty victims.

... event2, one counter assigned to RMID_A, event2 pair, ...

>   so on..
>   so on..
>   Configure cntr_id 31, to count remote read only.

... and event31, one counter assigned to RMID_A, event31 pair.

The example reflects that a *single* counter can be assigned to an RMID, event pair.

Considering above, perhaps changelog can start with something like:
	mbm_cntr_assign mode offers "num_mbm_cntrs" number of counters that
        can be assigned to RMID, event pairs ....

> 
> We have 32 counter ids in a domain. Basically, we can configure all the
> counters in a domain to just one group if you want to.

Understood.


> 
> We cannot do that right now because our data structures cannot do that.
> We can only configure 2 events(local and total) right now.
> 
> My understanding it is same with MPAM also.

Reinette

Hi Reinette,

On 4/16/25 14:02, Reinette Chatre wrote:
> Hi Babu,
> 
> On 4/16/25 10:09 AM, Moger, Babu wrote:
>> On 4/15/25 11:53, Reinette Chatre wrote:
>>> On 4/15/25 7:20 AM, Moger, Babu wrote:
>>>> On 4/11/25 16:04, Reinette Chatre wrote:
>>>>> 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.
>>>
>>> oh, are you referring to the assignments of different counters across multiple
>>> domains?
>>
>> May be I am confusing you here. This is what I meant.
>>
>> Here is one example.
>>
>> In a same group,
> 
> "same group" means single RMID, eg. RMID_A

Yes.

> 
>>   Configure cntr_id 0, to count reads only (This maps to total event).
> 
> This will be one event, event0, so one counter assigned to RMID_A, event0 pair.
> 
>>   Configure cntr_id 1, to count write only (This maps to local event).
> 
> ... event1, one counter assigned to RMID_A, event1 pair, ...
> 
>>   Configure cntr_id 2, to count dirty victims.
> 
> ... event2, one counter assigned to RMID_A, event2 pair, ...
> 
>>   so on..
>>   so on..
>>   Configure cntr_id 31, to count remote read only.
> 
> ... and event31, one counter assigned to RMID_A, event31 pair.

Yes. That is correct.

> 
> The example reflects that a *single* counter can be assigned to an RMID, event pair.
> 
> Considering above, perhaps changelog can start with something like:
> 	mbm_cntr_assign mode offers "num_mbm_cntrs" number of counters that
>         can be assigned to RMID, event pairs ....
> 

Sounds good.

-- 
Thanks
Babu Moger

On Wed, Apr 16, 2025 at 12:09:52PM -0500, Moger, Babu wrote:
> Hi Reinette,
> 
> On 4/15/25 11:53, Reinette Chatre wrote:
> > Hi Babu,
> > 
> > On 4/15/25 7:20 AM, Moger, Babu wrote:
> >> 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.
> > 
> > oh, are you referring to the assignments of different counters across multiple
> > domains?
> 
> May be I am confusing you here. This is what I meant.
> 
> Here is one example.
> 
> In a same group,
>   Configure cntr_id 0, to count reads only (This maps to total event).
>   Configure cntr_id 1, to count write only (This maps to local event).
>   Configure cntr_id 2, to count dirty victims.
>   so on..
>   so on..
>   Configure cntr_id 31, to count remote read only.
> 
> We have 32 counter ids in a domain. Basically, we can configure all the
> counters in a domain to just one group if you want to.
> 
> We cannot do that right now because our data structures cannot do that.
> We can only configure 2 events(local and total) right now.

Not just data structures, but also user visible files in
mon_data/mon_L3*/*

You'd need to create a new file for each counter.

My patch for making it easier to add more counters:

https://lore.kernel.org/all/20250407234032.241215-3-tony.luck@intel.com/

may help ... though you have to pick the number of simultaneous counters
at compile time to size the arrays in the domain structures:

	struct mbm_state	*mbm_states[QOS_NUM_MBM_EVENTS];

and if you are dynamically adding/removing events using the
configuration files, need to alloc/free the memory that those
arrays of pointers reference ... as well as adding/removing files
from the appropriate mon_data/mon_L3* directory.

> My understanding it is same with MPAM also.
> 
> > 
> >>
> >>>>
> >>>> 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.
> > 
> >>From what I can tell, for example by looking at patch #22, callers of
> > resctrl_assign_cntr_event() now need to call mbm_get_assign_config()
> > every time before calling resctrl_assign_cntr_event(). Calling
> > mbm_get_assign_config() from within resctrl_assign_cntr_event() seems
> > simpler to me and that may result in mbm_get_assign_config() moving to 
> > monitor.c as an extra benefit.
> 
> Sure.
> 
> > 
> > ...
> > 
> >>>> +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.
> > 
> > Maybe even resctrl_assign_cntr_event() does not need to call mbm_get_assign_config()
> > but only resctrl_alloc_config_cntr() needs to call mbm_get_assign_config(). Doing so
> > may avoid more burden on callers while reducing parameters needed throughout.
> > 
> 
> ok. Sure. Will do.
> 
> -- 
> Thanks
> Babu Moger

-Tony

Hi Tony,

On 4/16/25 12:55, Luck, Tony wrote:
> On Wed, Apr 16, 2025 at 12:09:52PM -0500, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 4/15/25 11:53, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 4/15/25 7:20 AM, Moger, Babu wrote:
>>>> 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.
>>>
>>> oh, are you referring to the assignments of different counters across multiple
>>> domains?
>>
>> May be I am confusing you here. This is what I meant.
>>
>> Here is one example.
>>
>> In a same group,
>>   Configure cntr_id 0, to count reads only (This maps to total event).
>>   Configure cntr_id 1, to count write only (This maps to local event).
>>   Configure cntr_id 2, to count dirty victims.
>>   so on..
>>   so on..
>>   Configure cntr_id 31, to count remote read only.
>>
>> We have 32 counter ids in a domain. Basically, we can configure all the
>> counters in a domain to just one group if you want to.
>>
>> We cannot do that right now because our data structures cannot do that.
>> We can only configure 2 events(local and total) right now.
> 
> Not just data structures, but also user visible files in
> mon_data/mon_L3*/*
> 
> You'd need to create a new file for each counter.

Yes. That is correct.

> 
> My patch for making it easier to add more counters:
> 
> https://lore.kernel.org/all/20250407234032.241215-3-tony.luck@intel.com/
> 
> may help ... though you have to pick the number of simultaneous counters
> at compile time to size the arrays in the domain structures:
> 
> 	struct mbm_state	*mbm_states[QOS_NUM_MBM_EVENTS];
> 
> and if you are dynamically adding/removing events using the
> configuration files, need to alloc/free the memory that those
> arrays of pointers reference ... as well as adding/removing files
> from the appropriate mon_data/mon_L3* directory.

Not just that. Also there is that overflow handler to keep all these
counters in sane state. So, pretty quickly it gets complicated. It is
probably best to handle as a separate series.

-- 
Thanks
Babu Moger