x86/resctrl : Support AMD Assignable Bandwidth Monitoring Counters (ABMC)

Add the command line option to enable or disable the new resctrl feature ABMC (Assignable Bandwidth Monitoring Counters).

Hi Babu,

Quoted 2 lines · Babu Moger

Add the command line option to enable or disable the new resctrl feature ABMC (Assignable Bandwidth Monitoring Counters).

This does not reflect the fs and arch separation that this version highlights since ABMC is not a resctrl feature.

This can get confusing and I think this interface is indeed for the architecture where hardware features are enabled/disabled (highlighted by how the parameter is connected to the X86_FEATURE_ flag) ... so perhaps something like:

Add the command line option to enable or disable exposing the ABMC (Assignable Bandwidth Monitoring Counters) hardware feature to resctrl.

Patch looks good to me.

The cache allocation and memory bandwidth allocation feature properties are consolidated into struct resctrl_cache and struct resctrl_membw respectively.

In preparation for more monitoring properties that will clobber the existing resource struct more, re-organize the monitoring specific properties to also be in a separate structure.

Suggested-by Reinette Chatre <[email protected]>

Signed-off-by Babu Moger <[email protected]>

---

Hi Babu,

arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_mon_l3_config(struct rdt_resource *r) · line 1236

Quoted 13 lines · Babu Moger

arch/x86/kernel/cpu/resctrl/monitor.c +5 -0

index 795fe91a8feb..6a792f06f5ce 100644
@@ -1229,6 +1229,12 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 			mbm_local_event.configurable = true;
 			mbm_config_rftype_init("mbm_local_bytes_config");
 		}
+
+		if (rdt_cpu_has(X86_FEATURE_ABMC)) {
+			r->mon.mbm_cntr_assignable = true;
+			cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
+			r->mon.num_mbm_cntrs = (ebx & 0xFFFF) + 1;

This should use GENMASK()

Add the functionality to enable/disable AMD ABMC feature.

AMD ABMC feature is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG. When the state of ABMC is changed, the MSR needs to be updated on all the logical processors in the QOS Domain.

Hardware counters will reset when ABMC state is changed. Reset the architectural state maintained by resctrl so that reading of a hardware counter is not considered as an overflow in next update.

The ABMC feature details are documented in APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth Monitoring (ABMC).

Link https://bugzilla.kernel.org/show_bug.cgi?id=206537

Signed-off-by Babu Moger <[email protected]>

---

Hi Babu,

Quoted 6 lines · Babu Moger

Add the functionality to enable/disable AMD ABMC feature.

AMD ABMC feature is enabled by setting enabled bit(0) in MSR L3_QOS_EXT_CFG. When the state of ABMC is changed, the MSR needs to be updated on all the logical processors in the QOS Domain.

Quoted 3 lines

Hardware counters will reset when ABMC state is changed. Reset the architectural state maintained by resctrl so that reading of a hardware counter is not considered as an overflow in next update.

Above mentions that architectural state is also reset, but that does not seem to form part of this patch?

Quoted 10 lines

The ABMC feature details are documented in APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth Monitoring (ABMC).

Link https://bugzilla.kernel.org/show_bug.cgi?id=206537

Signed-off-by Babu Moger <[email protected]>

---

...

Quoted 9 lines

static inline struct rdt_hw_resource *resctrl_to_arch_res(struct rdt_resource *r)

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

index 7e76f8d839fc..0178555bf3f6 100644
@@ -2402,6 +2402,41 @@ int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable)
 	return 0;
 }
 

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable) · line 2406

Quoted 2 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +2 -0

+/*
+ * Update L3_QOS_EXT_CFG MSR on all the CPUs associated with the resource.

This comment is not accurate since the function below only sets MSR on current CPU.

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_set_cdp_enabled(enum resctrl_res_level l, bool enable) · line 2408

Quoted 3 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +2 -0

+ */
+static void resctrl_abmc_set_one_amd(void *arg)

Reinette

Introduce the interface file "mbm_assign_mode" to list monitor modes supported.

The "mbm_cntr_assign" mode provides the option to assign a hardware counter to an RMID and monitor the bandwidth as long as it is assigned.

On AMD systems "mbm_cntr_assign" is backed by the ABMC (Assignable Bandwidth Monitoring Counters) hardware feature. "mbm_cntr_assign" mode is enabled by default when supported.

The "default" mode is the existing monitoring mode that works without the explicit counter assignment, instead relying on dynamic counter assignment by hardware that may result in hardware not dedicating a counter resulting in monitoring data reads returning "Unavailable".

Provide an interface to display the monitor mode on the system. $cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode [mbm_cntr_assign]
default

Switching the mbm_assign_mode will reset all the MBM counters of all resctrl groups.

Signed-off-by Babu Moger <[email protected]>

---
v7: Updated the descriptions/commit log in resctrl.rst to generic text.
Thanks to James and Reinette.
Rename mbm_mode to mbm_assign_mode.
Introduced mutex lock in rdtgroup_mbm_mode_show().

v6: Added documentation for mbm_cntr_assign and legacy mode.
Moved mbm_mode fflags initialization to static initialization.

v5: Changed interface name to mbm_mode.
It will be always available even if ABMC feature is not supported. Added description in resctrl.rst about ABMC mode.
Fixed display abmc and legacy consistantly.

v4: Fixed the checks for legacy and abmc mode. Default it ABMC.

v3: New patch to display ABMC capability.

---
 Documentation/arch/x86/resctrl.rst     | 33 ++++++++++++++++++++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 ++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

Documentation/arch/x86/resctrl.rst +0 -0

index 30586728a4cd..a7b17ad8acb9 100644

Hi Babu,

Quoted 6 lines · Babu Moger

Introduce the interface file "mbm_assign_mode" to list monitor modes supported.

The "mbm_cntr_assign" mode provides the option to assign a hardware counter to an RMID and monitor the bandwidth as long as it is assigned.

Quoted 3 lines

On AMD systems "mbm_cntr_assign" is backed by the ABMC (Assignable Bandwidth Monitoring Counters) hardware feature. "mbm_cntr_assign" mode is enabled by default when supported.

As I understand this series changed this behavior to let the architecture dictate whether "mbm_cntr_assign" is enabled by default.

Quoted 46 lines

The "default" mode is the existing monitoring mode that works without the explicit counter assignment, instead relying on dynamic counter assignment by hardware that may result in hardware not dedicating a counter resulting in monitoring data reads returning "Unavailable".

Provide an interface to display the monitor mode on the system. $cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode [mbm_cntr_assign]
default

Switching the mbm_assign_mode will reset all the MBM counters of all resctrl groups.

Signed-off-by Babu Moger <[email protected]>

---
v7: Updated the descriptions/commit log in resctrl.rst to generic text.
Thanks to James and Reinette.
Rename mbm_mode to mbm_assign_mode.
Introduced mutex lock in rdtgroup_mbm_mode_show().

v6: Added documentation for mbm_cntr_assign and legacy mode.
Moved mbm_mode fflags initialization to static initialization.

v5: Changed interface name to mbm_mode.
It will be always available even if ABMC feature is not supported. Added description in resctrl.rst about ABMC mode.
Fixed display abmc and legacy consistantly.

v4: Fixed the checks for legacy and abmc mode. Default it ABMC.

v3: New patch to display ABMC capability.

---
 Documentation/arch/x86/resctrl.rst     | 33 ++++++++++++++++++++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 31 ++++++++++++++++++++++++
 2 files changed, 64 insertions(+)

Documentation/arch/x86/resctrl.rst +0 -0

index 30586728a4cd..a7b17ad8acb9 100644
@@ -257,6 +257,39 @@ with the following files:
 	    # cat /sys/fs/resctrl/info/L3_MON/mbm_local_bytes_config
 	    0=0x30;1=0x30;3=0x15;4=0x15
 

Documentation/arch/x86/resctrl.rst with the following files: · line 262

Quoted 3 lines

Documentation/arch/x86/resctrl.rst +3 -0

+"mbm_assign_mode":
+	Reports the list of monitoring modes supported. The enclosed brackets
+	indicate which feature is enabled.

"which feature is enabled" -> "which mode is enabled"?

Documentation/arch/x86/resctrl.rst with the following files: · line 272

Quoted 11 lines

Documentation/arch/x86/resctrl.rst +10 -0

+	::
+
+	  cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
+	  [mbm_cntr_assign]
+	  default
+
+	"mbm_cntr_assign":
+
+	In mbm_cntr_assign mode user-space is able to specify which control
+	or monitor groups in resctrl should have a hardware counter assigned

This documentation should ideally also be appropriate for when the "soft-ABMC" support lands. Considering that, should all the "hardware counter" instances perhaps be changed to just be "counter"?

Documentation/arch/x86/resctrl.rst with the following files: · line 275

Quoted 4 lines

Documentation/arch/x86/resctrl.rst +3 -0

+	using the 'mbm_control' file. The number of hardware counters available
+	is described in the 'num_mbm_cntrs' file. Changing to this mode will
+	cause all counters on a resource to reset.

Should resctrl commit to this? Resetting of the counters as implemented here does seem to be an architecture specific action so this text could be made more generic by stating "may cause all counters on a resource to reset".

Documentation/arch/x86/resctrl.rst with the following files: · line 277

Quoted 3 lines

Documentation/arch/x86/resctrl.rst +2 -0

+
+	The feature is needed on platforms which support more control and monitor

"The feature" -> "The mode"?

Documentation/arch/x86/resctrl.rst with the following files: · line 280

Quoted 4 lines

Documentation/arch/x86/resctrl.rst +3 -0

+	groups than hardware counters, meaning 'unassigned' control or monitor
+	groups will report 'Unavailable' or not count all the traffic in an
+	unpredictable way.

"or not count all the traffic in an unpredictable way" is a bit hard to parse ... how about "or count traffic in an unpredictable way"?

Documentation/arch/x86/resctrl.rst with the following files: · line 290

Quoted 12 lines

Documentation/arch/x86/resctrl.rst +10 -0

+
+	AMD Platforms with ABMC (Assignable Bandwidth Monitoring Counters) feature
+	enable this mode by default so that counters remain assigned even when the
+	corresponding RMID is not in use by any processor.
+
+	"default":
+
+	By default resctrl assumes each control and monitor group has a hardware counter.
+	Hardware without this property will still allow more control or monitor groups
+	than 'num_mbm_cntrs' to be created. Reading the mbm files may report 'Unavailable'

Please be specific what is meant with "the mbm files"

Documentation/arch/x86/resctrl.rst with the following files: · line 291

Quoted 2 lines

Documentation/arch/x86/resctrl.rst +1 -0

+	if there is no hardware resource assigned.

"no hardware resource" -> "no counter"?

arch/x86/kernel/cpu/resctrl/rdtgroup.c static int rdtgroup_rmid_show(struct kernfs_open_file *of, · line 847

Quoted 13 lines

Documentation/arch/x86/resctrl.rst +1 -0

+
 "max_threshold_occupancy":
 		Read/write file provides the largest value (in
 		bytes) at which a previously used LLC_occupancy

arch/x86/kernel/cpu/resctrl/rdtgroup.c +0 -0

index 0178555bf3f6..dbc8c5e63213 100644
@@ -845,6 +845,30 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
 	return ret;
 }
 

Reinette

The mbm_assign_cntr mode provides an option to the user to assign a hardware counter to an RMID, event pair and monitor the bandwidth as

long as the counter is assigned. Number of assignments depend on number of monitoring counters available.

Provide the interface to display the number of monitoring counters supported.

Signed-off-by Babu Moger <[email protected]>

---
v7: Minor commit log text changes.

v6: No changes.

v5: Changed the display name from num_cntrs to num_mbm_cntrs.
Updated the commit message.
Moved the patch after mbm_mode is introduced.

v4: Changed the counter name to num_cntrs. And few text changes.

v3: Changed the field name to mbm_assign_cntrs.

v2: Changed the field name to mbm_assignable_counters from abmc_counte

---
 Documentation/arch/x86/resctrl.rst     |  3 +++
 arch/x86/kernel/cpu/resctrl/monitor.c  |  1 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 ++++++++++++++++
 3 files changed, 20 insertions(+)

Documentation/arch/x86/resctrl.rst +0 -0

index a7b17ad8acb9..3e9302971faf 100644

Hi Babu,

Quoted 2 lines · Babu Moger

The mbm_assign_cntr mode provides an option to the user to assign a hardware counter to an RMID, event pair and monitor the bandwidth as

Could you please be consistent in this series in how you refer to an RMID, event pair ? For example later it becomes RMID-event pair.

Quoted 37 lines

long as the counter is assigned. Number of assignments depend on number of monitoring counters available.

Provide the interface to display the number of monitoring counters supported.

Signed-off-by Babu Moger <[email protected]>

---
v7: Minor commit log text changes.

v6: No changes.

v5: Changed the display name from num_cntrs to num_mbm_cntrs.
Updated the commit message.
Moved the patch after mbm_mode is introduced.

v4: Changed the counter name to num_cntrs. And few text changes.

v3: Changed the field name to mbm_assign_cntrs.

v2: Changed the field name to mbm_assignable_counters from abmc_counte

---
 Documentation/arch/x86/resctrl.rst     |  3 +++
 arch/x86/kernel/cpu/resctrl/monitor.c  |  1 +
 arch/x86/kernel/cpu/resctrl/rdtgroup.c | 16 ++++++++++++++++
 3 files changed, 20 insertions(+)

Documentation/arch/x86/resctrl.rst +0 -0

index a7b17ad8acb9..3e9302971faf 100644
@@ -290,6 +290,9 @@ with the following files:
 	than 'num_mbm_cntrs' to be created. Reading the mbm files may report 'Unavailable'
 	if there is no hardware resource assigned.
 

Documentation/arch/x86/resctrl.rst with the following files: · line 295

Quoted 3 lines

Documentation/arch/x86/resctrl.rst +3 -0

+"num_mbm_cntrs":
+	The number of monitoring counters available for assignment.
+

I think it will be helpful if the changelog and the above doc notes when this file can be expected to be visible since its visibility is not connected to visibility of

"mbm_assign_mode" that refers to it. There also seems to be a conflict here where "mbm_assign_mode" documentation contains section about "default" that refers to "num_mbm_cntrs", but "num_mbm_cntrs" may not be visible if "default" is the only mode.

Hardware provides a set of counters when mbm_cntr_assignable feature is supported. These counters are used for assigning the events in resctrl a group when the feature is enabled. The kernel must manage and track the

number of available counters.

Introduce mbm_cntr_free_map bitmap to track available counters and set of routines to allocate and free the counters.

Signed-off-by Babu Moger <[email protected]>

---

arch/x86/kernel/cpu/resctrl/monitor.c +17 -0

+{
+	if (r->mon.mbm_cntr_assignable) {
+		r->mon.mbm_cntr_free_map = bitmap_zalloc(r->mon.num_mbm_cntrs,
+							 GFP_KERNEL);
+		if (!r->mon.mbm_cntr_free_map)
+			return -ENOMEM;
+		bitmap_fill(r->mon.mbm_cntr_free_map, r->mon.num_mbm_cntrs);
+	}
+	return 0;
+}
+
+static void __exit mbm_cntrs_exit(struct rdt_resource *r)
+{
+	if (r->mon.mbm_cntr_assignable)
+		bitmap_free(r->mon.mbm_cntr_free_map);
+}
+
 int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 {
 	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;

arch/x86/kernel/cpu/resctrl/monitor.c +0 -0

 }
 
 void __init intel_rdt_mbm_apply_quirk(void)

arch/x86/kernel/cpu/resctrl/rdtgroup.c +19 -0

index ba737890d5c2..a51992984832 100644
@@ -185,6 +185,25 @@ bool closid_allocated(unsigned int closid)
 	return !test_bit(closid, &closid_free_map);
 }
 
+int mbm_cntr_alloc(struct rdt_resource *r)
+{
+	int cntr_id;
+
+	cntr_id = find_first_bit(r->mon.mbm_cntr_free_map,
+				 r->mon.num_mbm_cntrs);
+	if (cntr_id >= r->mon.num_mbm_cntrs)
+		return -ENOSPC;
+
+	__clear_bit(cntr_id, r->mon.mbm_cntr_free_map);
+
+	return cntr_id;
+}
+
+void mbm_cntr_free(struct rdt_resource *r, u32 cntr_id)
+{
+	__set_bit(cntr_id, r->mon.mbm_cntr_free_map);
+}
+
 /**
  * rdtgroup_mode_by_closid - Return mode of resource group with closid
  * @closid: closid if the resource group

include/linux/resctrl.h +0 -0

index f11d6fdfd977..aab22ff8e0c1 100644

Hi Babu,

Quoted 3 lines · Babu Moger

Hardware provides a set of counters when mbm_cntr_assignable feature is supported. These counters are used for assigning the events in resctrl a group when the feature is enabled. The kernel must manage and track the

The second sentence ("These counters ...") is difficult to parse.

Quoted 2 lines

number of available counters.

"The kernel must manage and track the number of available counters." -> "The kernel must manage and track the available counters." ?

Quoted 7 lines

Introduce mbm_cntr_free_map bitmap to track available counters and set of routines to allocate and free the counters.

Signed-off-by Babu Moger <[email protected]>

---

...

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/monitor.c +0 -0

index e3e71843401a..f98cc5b9bebc 100644
@@ -1175,6 +1175,30 @@ static __init int snc_get_config(void)
 	return ret;
 }
 

arch/x86/kernel/cpu/resctrl/monitor.c static __init int snc_get_config(void) · line 1181

Quoted 4 lines

arch/x86/kernel/cpu/resctrl/monitor.c +4 -0

+/*
+ * Counter bitmap for tracking the available counters.
+ * 'mbm_cntr_assign' mode provides set of hardware counters for assigning
+ * RMID, event pair. Each RMID and event pair takes one hardware counter.

(soft-ABMC may need to edit this comment)

arch/x86/kernel/cpu/resctrl/monitor.c static __init int snc_get_config(void) · line 1182

Quoted 2 lines

arch/x86/kernel/cpu/resctrl/monitor.c +1 -0

+ * Kernel needs to keep track of the number of available counters.

Last sentence seems to be duplicate of the first?

arch/x86/kernel/cpu/resctrl/monitor.c static __init int snc_get_config(void) · line 1184

Quoted 3 lines

arch/x86/kernel/cpu/resctrl/monitor.c +2 -0

+ */
+static int mbm_cntrs_init(struct rdt_resource *r)

Needs __init?

Quoted 25 lines

arch/x86/kernel/cpu/resctrl/monitor.c +17 -0

+{
+	if (r->mon.mbm_cntr_assignable) {
+		r->mon.mbm_cntr_free_map = bitmap_zalloc(r->mon.num_mbm_cntrs,
+							 GFP_KERNEL);
+		if (!r->mon.mbm_cntr_free_map)
+			return -ENOMEM;
+		bitmap_fill(r->mon.mbm_cntr_free_map, r->mon.num_mbm_cntrs);
+	}
+	return 0;
+}
+
+static void __exit mbm_cntrs_exit(struct rdt_resource *r)
+{
+	if (r->mon.mbm_cntr_assignable)
+		bitmap_free(r->mon.mbm_cntr_free_map);
+}
+
 int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 {
 	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;

@@ -1240,6 +1264,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 		}
 	}
 

arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_mon_l3_config(struct rdt_resource *r) · line 1269

Quoted 3 lines

arch/x86/kernel/cpu/resctrl/monitor.c +3 -0

+	ret = mbm_cntrs_init(r);
+	if (ret)
+		return ret;

Missing cleanup of earlier allocation on error path here. Even so, this does not seem to integrate with existing dom_data_init() wrt ordering and locking. Could this be more fitting when merged with dom_data_init() (after moving it)?

Quoted 9 lines

arch/x86/kernel/cpu/resctrl/monitor.c +1 -0

+
 	l3_mon_evt_init(r);
 
 	r->mon_capable = true;

@@ -1247,9 +1275,10 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 	return 0;
 }
 

arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_mon_l3_config(struct rdt_resource *r) · line 1281

Quoted 5 lines

arch/x86/kernel/cpu/resctrl/monitor.c +2 -1

-void __exit rdt_put_mon_l3_config(void)
+void __exit rdt_put_mon_l3_config(struct rdt_resource *r)
 {
 	dom_data_exit();
+	mbm_cntrs_exit(r);

There is a mismatch wrt locking used in dom_data_exit() and mbm_cntrs_exit() that is sure to cause confusion and difficulty in the MPAM transition.

Quoted 38 lines

arch/x86/kernel/cpu/resctrl/monitor.c +0 -0

 }
 
 void __init intel_rdt_mbm_apply_quirk(void)

arch/x86/kernel/cpu/resctrl/rdtgroup.c +19 -0

index ba737890d5c2..a51992984832 100644
@@ -185,6 +185,25 @@ bool closid_allocated(unsigned int closid)
 	return !test_bit(closid, &closid_free_map);
 }
 
+int mbm_cntr_alloc(struct rdt_resource *r)
+{
+	int cntr_id;
+
+	cntr_id = find_first_bit(r->mon.mbm_cntr_free_map,
+				 r->mon.num_mbm_cntrs);
+	if (cntr_id >= r->mon.num_mbm_cntrs)
+		return -ENOSPC;
+
+	__clear_bit(cntr_id, r->mon.mbm_cntr_free_map);
+
+	return cntr_id;
+}
+
+void mbm_cntr_free(struct rdt_resource *r, u32 cntr_id)
+{
+	__set_bit(cntr_id, r->mon.mbm_cntr_free_map);
+}
+
 /**
  * rdtgroup_mode_by_closid - Return mode of resource group with closid
  * @closid: closid if the resource group

include/linux/resctrl.h +0 -0

index f11d6fdfd977..aab22ff8e0c1 100644

include/linux/resctrl.h enum resctrl_scope { · line 190

Quoted 5 lines

include/linux/resctrl.h +1 -0

@@ -187,12 +187,14 @@ enum resctrl_scope {
  * @num_rmid:		Number of RMIDs available
  * @num_mbm_cntrs:	Number of assignable monitoring counters
  * @mbm_cntr_assignable:Is system capable of supporting monitor assignment?
+ * @mbm_cntr_free_map:	bitmap of number of assignable MBM counters

The "number of" is not clear ... it seems to indicate tracking a count? How about just "bitmap of free MBM counters"

include/linux/resctrl.h enum resctrl_scope { · line 200

Quoted 11 lines

include/linux/resctrl.h +1 -0

  * @evt_list:		List of monitoring events
  */
 struct resctrl_mon {
 	int			num_rmid;
 	int			num_mbm_cntrs;
 	bool			mbm_cntr_assignable;
+	unsigned long		*mbm_cntr_free_map;
 	struct list_head	evt_list;
 };
 

Reinette

Hi Reinette,

Quoted 7 lines · Reinette Chatre

Hi Babu,

Quoted 3 lines · Babu Moger

Hardware provides a set of counters when mbm_cntr_assignable feature is supported. These counters are used for assigning the events in resctrl a group when the feature is enabled. The kernel must manage and track the

Quoted 1 line

The second sentence ("These counters ...") is difficult to parse.

How about?

Counters are used for assigning the events in resctrl group.

Quoted 4 lines

Quoted 2 lines

+ */
+static int mbm_cntrs_init(struct rdt_resource *r)

Needs __init?

Did you mean to merge this with dom_data_init and don't have to have a separate function. Please clarify.

Hi Babu,

Quoted 12 lines · Babu

Hi Reinette,

Quoted 8 lines · Reinette Chatre

Hi Babu,

Quoted 3 lines · Babu Moger

Hardware provides a set of counters when mbm_cntr_assignable feature is supported. These counters are used for assigning the events in resctrl a group when the feature is enabled. The kernel must manage and track the

The second sentence ("These counters ...") is difficult to parse.

Quoted 3 lines

How about?

Counters are used for assigning the events in resctrl group.

Apologies but I am just not able to parse this. How about: "These counters are assigned to the MBM monitoring events of a MON group that needs to be tracked."

...

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

+ */
+static int mbm_cntrs_init(struct rdt_resource *r)

Needs __init?

Did you mean to merge this with dom_data_init and don't have to have a separate function. Please clarify.

Here I was referring to the actual __init storage class attribute. Since mbm_cntrs_init() is only called by __init code, it too should have the __init storage class attribute.
I do expect that mbm_cntrs_init() will be called by dom_data_init() and care should be taken when making this change since it seems that dom_data_init() itself needs the __init storage class attribute. Looks like this was missed by commit bd334c86b5d7 ("x86/resctrl: Add __init attribute to rdt_get_mon_l3_config()")

include/linux/resctrl.h +0 -0

index f11d6fdfd977..aab22ff8e0c1 100644
@@ -187,12 +187,14 @@ enum resctrl_scope {
  * @num_rmid:          Number of RMIDs available
  * @num_mbm_cntrs:     Number of assignable monitoring counters
  * @mbm_cntr_assignable:Is system capable of supporting monitor assignment?

Hi Babu,

Quoted 13 lines · Babu Moger

include/linux/resctrl.h +1 -0

index f11d6fdfd977..aab22ff8e0c1 100644
@@ -187,12 +187,14 @@ enum resctrl_scope {
  * @num_rmid:          Number of RMIDs available
  * @num_mbm_cntrs:     Number of assignable monitoring counters
  * @mbm_cntr_assignable:Is system capable of supporting monitor assignment?
+ * @mbm_cntr_free_map: bitmap of number of assignable MBM counters
  * @evt_list:          List of monitoring events
  */
 struct resctrl_mon {
        int                     num_rmid;

include/linux/resctrl.h enum resctrl_scope { · line 199

Quoted 5 lines

include/linux/resctrl.h +1 -0

        int                     num_mbm_cntrs;
        bool                    mbm_cntr_assignable;
+       unsigned long           *mbm_cntr_free_map;
        struct list_head        evt_list;
 };

This looks global still. Will only all-domain (*=) operations be supported initially?

If the BMEC (Bandwidth Monitoring Event Configuration) feature is supported, the bandwidth events can be configured to track specific events. The event configuration is domain specific. ABMC (Assignable Bandwidth Monitoring Counters) feature needs event configuration information to assign hardware counter to an RMID. Event configurations

are not stored in resctrl but instead always read from or written to hardware directly when prompted by user space.

Read the event configuration from the hardware during the domain initialization. Save the configuration value in rdt_hw_mon_domain,

so it can be used for counter assignment.

Signed-off-by Babu Moger <[email protected]>

---
v7: Fixed initializing INVALID_CONFIG_VALUE to mbm_local_cfg in case of error.

v6: Renamed resctrl_arch_mbm_evt_config -> resctrl_mbm_evt_config_init
Initialized value to INVALID_CONFIG_VALUE if it is not configurable. Minor commit message update.

v5: Exported mon_event_config_index_get.
Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.

v4: Read the configuration information from the hardware to initialize.
Added few commit messages.
Fixed the tab spaces.

v3: Minor changes related to rebase in mbm_config_write_domain.

v2: No changes.

---
 arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
 arch/x86/kernel/cpu/resctrl/internal.h |  9 +++++++++
 arch/x86/kernel/cpu/resctrl/monitor.c  | 26 ++++++++++++++++++++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |  4 +---
 4 files changed, 38 insertions(+), 3 deletions(-)

arch/x86/kernel/cpu/resctrl/core.c +0 -0

index 00ad00258df2..2a4be004a2df 100644

Hi Babu,

Quoted 5 lines · Babu Moger

If the BMEC (Bandwidth Monitoring Event Configuration) feature is supported, the bandwidth events can be configured to track specific events. The event configuration is domain specific. ABMC (Assignable Bandwidth Monitoring Counters) feature needs event configuration information to assign hardware counter to an RMID. Event configurations

"to assign hardware counter" -> "to assign a hardware counter"?

Quoted 6 lines

are not stored in resctrl but instead always read from or written to hardware directly when prompted by user space.

Read the event configuration from the hardware during the domain initialization. Save the configuration value in rdt_hw_mon_domain,

"rdt_hw_mon_domain" -> "struct rdt_hw_mon_domain"

Quoted 34 lines

so it can be used for counter assignment.

Signed-off-by Babu Moger <[email protected]>

---
v7: Fixed initializing INVALID_CONFIG_VALUE to mbm_local_cfg in case of error.

v6: Renamed resctrl_arch_mbm_evt_config -> resctrl_mbm_evt_config_init
Initialized value to INVALID_CONFIG_VALUE if it is not configurable. Minor commit message update.

v5: Exported mon_event_config_index_get.
Renamed arch_domain_mbm_evt_config to resctrl_arch_mbm_evt_config.

v4: Read the configuration information from the hardware to initialize.
Added few commit messages.
Fixed the tab spaces.

v3: Minor changes related to rebase in mbm_config_write_domain.

v2: No changes.

---
 arch/x86/kernel/cpu/resctrl/core.c     |  2 ++
 arch/x86/kernel/cpu/resctrl/internal.h |  9 +++++++++
 arch/x86/kernel/cpu/resctrl/monitor.c  | 26 ++++++++++++++++++++++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |  4 +---
 4 files changed, 38 insertions(+), 3 deletions(-)

arch/x86/kernel/cpu/resctrl/core.c +0 -0

index 00ad00258df2..2a4be004a2df 100644
@@ -632,6 +632,8 @@ static void domain_add_cpu_mon(int cpu, struct rdt_resource *r)
 

arch/x86/kernel/cpu/resctrl/core.c static void domain_add_cpu_mon(int cpu, struct rdt_resource *r) · line 635

Quoted 3 lines

arch/x86/kernel/cpu/resctrl/core.c +1 -0

 	arch_mon_domain_online(r, d);
 
+	resctrl_mbm_evt_config_init(hw_dom);

Now that the arch and fs separate becomes clear I wonder if it may help to understand this work if we start using clear namespaces to help this distinction. Surely the arch code is very inconsistent in this regard (thus this function fits in), but resctrl_ has to be the prefix for fs code.

The event configuration is domain specific and initialized during domain initialization. The values are stored in struct rdt_hw_mon_domain.

It is not required to read the configuration register every time user asks for it. Use the value stored in struct rdt_hw_mon_domain instead.

Introduce resctrl_arch_event_config_get() and resctrl_arch_event_config_set() to get/set architecture domain specific mbm_total_cfg/mbm_local_cfg values.

Signed-off-by Babu Moger <[email protected]>

---

Hi Babu,

Quoted 10 lines · Babu Moger

The event configuration is domain specific and initialized during domain initialization. The values are stored in struct rdt_hw_mon_domain.

It is not required to read the configuration register every time user asks for it. Use the value stored in struct rdt_hw_mon_domain instead.

Introduce resctrl_arch_event_config_get() and resctrl_arch_event_config_set() to get/set architecture domain specific mbm_total_cfg/mbm_local_cfg values.

Quoted 2 lines

Signed-off-by Babu Moger <[email protected]>

---

This change looks fine, but could the function names be more specific? For example,
resctrl_arch_mon_event_config_get()/resctrl_arch_mon_event_config_set()?

+/*
+ * ABMC counters can be configured by writing to L3_QOS_ABMC_CFG.
+ * @bw_type : Bandwidth configuration(supported by BMEC)
+ * tracked by the @cntr_id.
+ * @bw_src : Bandwidth source (RMID or CLOSID).
+ * @reserved1 : Reserved.
+ * @is_clos : @bw_src field is a CLOSID (not an RMID).

+ * @cntr_id : Counter identifier.
+ * @reserved : Reserved.
+ * @cntr_en : Tracking enable bit.
+ * @cfg_en : Configuration enable bit.
+ *
+ * Configuration and tracking:
+ * CfgEn=1,CtrEn=0 : Configure CtrID and but no tracking the events yet.
+ * CfgEn=1,CtrEn=1 : Configure CtrID and start tracking events.

Hi Babu,

Quoted 7 lines · Babu Moger

+/*
+ * ABMC counters can be configured by writing to L3_QOS_ABMC_CFG.
+ * @bw_type : Bandwidth configuration(supported by BMEC)
+ * tracked by the @cntr_id.
+ * @bw_src : Bandwidth source (RMID or CLOSID).
+ * @reserved1 : Reserved.
+ * @is_clos : @bw_src field is a CLOSID (not an RMID).

Quoted 8 lines

+ * @cntr_id : Counter identifier.
+ * @reserved : Reserved.
+ * @cntr_en : Tracking enable bit.
+ * @cfg_en : Configuration enable bit.
+ *
+ * Configuration and tracking:
+ * CfgEn=1,CtrEn=0 : Configure CtrID and but no tracking the events yet.
+ * CfgEn=1,CtrEn=1 : Configure CtrID and start tracking events.

Thanks for moving the text ... could it now be made to match the new (outside AMD arch document) destination? For example, "CfgEn" becomes "@cfg_en",

"CtrID" becomes "@cntr_id" etc. Also please fix language, for example what does "and but no tracking the events yet" mean? So far this work has focused on "counting" vs "not counting" events and it is not

clear how this "tracking" fits it ... this seems to be the hardware view that means "tracking the RMID to which @cntr_id is assigned"? Please help readers to understand how the implementation is supported by the hardware.

Hi Reinette,

Quoted 19 lines · Reinette Chatre

Hi Babu,

Quoted 7 lines · Babu Moger

+/*
+ * ABMC counters can be configured by writing to L3_QOS_ABMC_CFG.
+ * @bw_type : Bandwidth configuration(supported by BMEC)
+ * tracked by the @cntr_id.
+ * @bw_src : Bandwidth source (RMID or CLOSID).
+ * @reserved1 : Reserved.
+ * @is_clos : @bw_src field is a CLOSID (not an RMID).

Quoted 8 lines

+ * @cntr_id : Counter identifier.
+ * @reserved : Reserved.
+ * @cntr_en : Tracking enable bit.
+ * @cfg_en : Configuration enable bit.
+ *
+ * Configuration and tracking:
+ * CfgEn=1,CtrEn=0 : Configure CtrID and but no tracking the events yet.
+ * CfgEn=1,CtrEn=1 : Configure CtrID and start tracking events.

Quoted 2 lines

Thanks for moving the text ... could it now be made to match the new (outside AMD arch document) destination? For example, "CfgEn" becomes "@cfg_en",

Sure. Will do.

Quoted 3 lines

"CtrID" becomes "@cntr_id" etc. Also please fix language, for example what does "and but no tracking the events yet" mean? So far this work has focused on "counting" vs "not counting" events and it is not

I will change the text to "not counting". Hope this will clarify here.

Quoted 4 lines

clear how this "tracking" fits it ... this seems to be the hardware view that means "tracking the RMID to which @cntr_id is assigned"? Please help readers to understand how the implementation is supported by the hardware.

I have checked with hw team on this.
CfgEn: This corresponds counter assignment.

CtrEn: This is to start or stop counting.
We always set this to 1 to start counting.

Hi Babu,

Quoted 38 lines · Babu

Hi Reinette,

Quoted 19 lines · Reinette Chatre

Hi Babu,

Quoted 7 lines · Babu Moger

+/*
+ * ABMC counters can be configured by writing to L3_QOS_ABMC_CFG.
+ * @bw_type : Bandwidth configuration(supported by BMEC)
+ * tracked by the @cntr_id.
+ * @bw_src : Bandwidth source (RMID or CLOSID).
+ * @reserved1 : Reserved.
+ * @is_clos : @bw_src field is a CLOSID (not an RMID).

Quoted 8 lines

+ * @cntr_id : Counter identifier.
+ * @reserved : Reserved.
+ * @cntr_en : Tracking enable bit.
+ * @cfg_en : Configuration enable bit.
+ *
+ * Configuration and tracking:
+ * CfgEn=1,CtrEn=0 : Configure CtrID and but no tracking the events yet.
+ * CfgEn=1,CtrEn=1 : Configure CtrID and start tracking events.

Quoted 2 lines

Thanks for moving the text ... could it now be made to match the new (outside AMD arch document) destination? For example, "CfgEn" becomes "@cfg_en",

Sure. Will do.

Quoted 3 lines

"CtrID" becomes "@cntr_id" etc. Also please fix language, for example what does "and but no tracking the events yet" mean? So far this work has focused on "counting" vs "not counting" events and it is not

I will change the text to "not counting". Hope this will clarify here.

Quoted 4 lines

clear how this "tracking" fits it ... this seems to be the hardware view that means "tracking the RMID to which @cntr_id is assigned"? Please help readers to understand how the implementation is supported by the hardware.

Quoted 2 lines

I have checked with hw team on this.
CfgEn: This corresponds counter assignment.

To be specific this corresponds to *hardware* counter assignment? This is because software sets CfgEn to 1 whether it is assigned from kernel perspective or not.

Actually ... when I look at the AMD spec it becomes more clear to me. If I understand the spec correctly the CfgEn bit is used to coordinate changes between OS and HW. Seems like OS can leisurely write to any fields of L3_QOS_ABMC_CFG, but only when CfgEn bit is set will the actual hardware configuration be performed.

Quoted 2 lines

CtrEn: This is to start or stop counting.
We always set this to 1 to start counting.

Understood. Now that I read this portion of AMD spec it is more clear to me and I understand why CfgEn is set in both counter assign and unassign.

Reinette

Hi Babu,

In subject, please use "()" for a function.

arch/x86/kernel/cpu/resctrl/rdtgroup.c static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of, · line 1868

Quoted 6 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

+/*
+ * Send an IPI to the domain to assign the counter to RMID, event pair.
+ */
+int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
+			     enum resctrl_event_id evtid, u32 rmid, u32 closid,
+			     u32 cntr_id, bool assign)

Looking ahead this is also called when config of existing assigned counter is changed. Should this thus perhaps be resctrl_arch_config_cntr()?

arch/x86/kernel/cpu/resctrl/rdtgroup.c static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of, · line 1874

Quoted 7 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

+{
+	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
+	union l3_qos_abmc_cfg abmc_cfg = { 0 };
+	struct arch_mbm_state *arch_mbm;
+
+	abmc_cfg.split.cfg_en = 1;

Just to confirm ... a counter remains "configured" from the hardware side whether it is assigned from resctrl perspective or not? It seems to me that once a counter is "unassigned" from resctrl perspective it needs no more context about that counter, yet it remains configured from hardware side?

Hi Reinette,

Quoted 3 lines · Reinette Chatre

Hi Babu,

In subject, please use "()" for a function.

Sure.

Quoted 11 lines · Babu Moger

arch/x86/kernel/cpu/resctrl/rdtgroup.c static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of, · line 1868

Quoted 6 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

+/*
+ * Send an IPI to the domain to assign the counter to RMID, event pair.
+ */
+int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
+			     enum resctrl_event_id evtid, u32 rmid, u32 closid,
+			     u32 cntr_id, bool assign)

Looking ahead this is also called when config of existing assigned counter is changed. Should this thus perhaps be resctrl_arch_config_cntr()?

We have a matching resctrl_arch_assign_cntr() and
resctrl_arch_unassign_cntr() pair.

If we change resctrl_arch_config_cntr() then we need to change resctrl_arch_unassign_cntr to resctrl_arch_unconfig_cntr().

Should we change both?

Quoted 12 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of, · line 1874

Quoted 7 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

+{
+	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
+	union l3_qos_abmc_cfg abmc_cfg = { 0 };
+	struct arch_mbm_state *arch_mbm;
+
+	abmc_cfg.split.cfg_en = 1;

Just to confirm ... a counter remains "configured" from the hardware side whether it is assigned from resctrl perspective or not? It seems to me that once a counter is "unassigned" from resctrl perspective it needs no more context about that counter, yet it remains configured from hardware side?

That is correct.
When unassigned, we are setting cntr_en = 0, so there is no counting. But in hardware perspective it is still configured.

Hi Babu,

Quoted 21 lines · Babu

Hi Reinette,

Quoted 3 lines · Reinette Chatre

Hi Babu,

In subject, please use "()" for a function.

Sure.

Quoted 11 lines · Babu Moger

arch/x86/kernel/cpu/resctrl/rdtgroup.c static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of, · line 1868

Quoted 6 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

+/*
+ * Send an IPI to the domain to assign the counter to RMID, event pair.
+ */
+int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
+			     enum resctrl_event_id evtid, u32 rmid, u32 closid,
+			     u32 cntr_id, bool assign)

Looking ahead this is also called when config of existing assigned counter is changed. Should this thus perhaps be resctrl_arch_config_cntr()?

Quoted 2 lines

We have a matching resctrl_arch_assign_cntr() and
resctrl_arch_unassign_cntr() pair.

hmmm ... resctrl_arch_unassign_cntr() does not exist in this version of the series.

Quoted 21 lines

If we change resctrl_arch_config_cntr() then we need to change resctrl_arch_unassign_cntr to resctrl_arch_unconfig_cntr().

Should we change both?

Quoted 12 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c static ssize_t mbm_local_bytes_config_write(struct kernfs_open_file *of, · line 1874

Quoted 7 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

+{
+	struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
+	union l3_qos_abmc_cfg abmc_cfg = { 0 };
+	struct arch_mbm_state *arch_mbm;
+
+	abmc_cfg.split.cfg_en = 1;

Just to confirm ... a counter remains "configured" from the hardware side whether it is assigned from resctrl perspective or not? It seems to me that once a counter is "unassigned" from resctrl perspective it needs no more context about that counter, yet it remains configured from hardware side?

Quoted 3 lines

That is correct.
When unassigned, we are setting cntr_en = 0, so there is no counting. But in hardware perspective it is still configured.

I think I misunderstood the "configured in hardware" to equate to "assigned by OS" when in fact it is just a bit to indicate when hardware makes changes requested by MSR write.

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

index 7ad653b4e768..1d45120ff2b5 100644
@@ -864,6 +864,13 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
 	return ret;
 }
 
+/*
+ * Get the counter index for the assignable counter
+ * 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
+ * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
+ */
+#define MBM_EVENT_ARRAY_INDEX(_event) ((_event) - 2)
+
 static int rdtgroup_mbm_assign_mode_show(struct kernfs_open_file *of,
 					 struct seq_file *s, void *v)
 {

Hi Babu,

Quoted 22 lines · Babu Moger

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

index 7ad653b4e768..1d45120ff2b5 100644
@@ -864,6 +864,13 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
 	return ret;
 }
 
+/*
+ * Get the counter index for the assignable counter
+ * 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
+ * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
+ */
+#define MBM_EVENT_ARRAY_INDEX(_event) ((_event) - 2)
+
 static int rdtgroup_mbm_assign_mode_show(struct kernfs_open_file *of,
 					 struct seq_file *s, void *v)
 {

@@ -1898,6 +1905,45 @@ int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
 	return 0;
 }
 

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, · line 1914

Quoted 7 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

+/*
+ * Assign a hardware counter to the group.
+ * Counter will be assigned to all the domains if rdt_mon_domain is NULL
+ * else the counter will be allocated to specific domain.
+ */
+int rdtgroup_assign_cntr(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+			 struct rdt_mon_domain *d, enum resctrl_event_id evtid)

Could we please review the naming of function as this series progresses? Using such a generic name for this specific function seems to result in its callers later in series having even more generic names that are hard to decipher. For example, later (very generic) "rdtgroup_assign_grp()" calls this function and I find rdtgroup_assign_grp() to be very vague making the code more difficult to follow. For example, rdtgroup_assign_cntr() could be rdtgroup_assign_cntr_event() and rdtgroup_assign_grp() could instead be rdtgroup_assign_cntr()? Please feel free to improve.

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, · line 1921

Quoted 8 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +6 -0

 +{
+	int index = MBM_EVENT_ARRAY_INDEX(evtid);
+	int cntr_id = rdtgrp->mon.cntr_id[index];
+
+	/*
+	 * Allocate a new counter id to the group if the counter id is not
+	 * is not assigned already.

"is not is not" -> "is not"

Hi Reinette,

Quoted 25 lines · Reinette Chatre

Hi Babu,

Quoted 22 lines · Babu Moger

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

index 7ad653b4e768..1d45120ff2b5 100644
@@ -864,6 +864,13 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
 	return ret;
 }
 
+/*
+ * Get the counter index for the assignable counter
+ * 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
+ * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
+ */
+#define MBM_EVENT_ARRAY_INDEX(_event) ((_event) - 2)
+
 static int rdtgroup_mbm_assign_mode_show(struct kernfs_open_file *of,
 					 struct seq_file *s, void *v)
 {

@@ -1898,6 +1905,45 @@ int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
 	return 0;
 }
 

Quoted 14 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, · line 1914

Quoted 7 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

+/*
+ * Assign a hardware counter to the group.
+ * Counter will be assigned to all the domains if rdt_mon_domain is NULL
+ * else the counter will be allocated to specific domain.
+ */
+int rdtgroup_assign_cntr(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+			 struct rdt_mon_domain *d, enum resctrl_event_id evtid)

Could we please review the naming of function as this series progresses? Using such a generic name for this specific function seems to result in its callers later in series having even more generic names that are hard to decipher. For example, later (very generic) "rdtgroup_assign_grp()" calls this function and I find rdtgroup_assign_grp() to be very vague making the code more difficult to follow. For example, rdtgroup_assign_cntr() could be rdtgroup_assign_cntr_event() and rdtgroup_assign_grp() could instead be rdtgroup_assign_cntr()? Please feel free to improve.

Sure.

How about rdtgroup_assign_cntr_event() and rdtgroup_assign_cntr_grp() ?

Added grp extension for the second one.

Hi Babu,

Quoted 46 lines · Babu

Hi Reinette,

Quoted 25 lines · Reinette Chatre

Hi Babu,

Quoted 22 lines · Babu Moger

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

index 7ad653b4e768..1d45120ff2b5 100644
@@ -864,6 +864,13 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
 	return ret;
 }
 
+/*
+ * Get the counter index for the assignable counter
+ * 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
+ * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
+ */
+#define MBM_EVENT_ARRAY_INDEX(_event) ((_event) - 2)
+
 static int rdtgroup_mbm_assign_mode_show(struct kernfs_open_file *of,
 					 struct seq_file *s, void *v)
 {

@@ -1898,6 +1905,45 @@ int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
 	return 0;
 }
 

Quoted 14 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, · line 1914

Quoted 7 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

+/*
+ * Assign a hardware counter to the group.
+ * Counter will be assigned to all the domains if rdt_mon_domain is NULL
+ * else the counter will be allocated to specific domain.
+ */
+int rdtgroup_assign_cntr(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+			 struct rdt_mon_domain *d, enum resctrl_event_id evtid)

Could we please review the naming of function as this series progresses? Using such a generic name for this specific function seems to result in its callers later in series having even more generic names that are hard to decipher. For example, later (very generic) "rdtgroup_assign_grp()" calls this function and I find rdtgroup_assign_grp() to be very vague making the code more difficult to follow. For example, rdtgroup_assign_cntr() could be rdtgroup_assign_cntr_event() and rdtgroup_assign_grp() could instead be rdtgroup_assign_cntr()? Please feel free to improve.

Sure.

Quoted 3 lines

How about rdtgroup_assign_cntr_event() and rdtgroup_assign_cntr_grp() ?

Added grp extension for the second one.

Is the "grp" extension needed? The function already has "rdtgroup_" as prefix so the "grp" extension does not seem necessary to me since I think "rdtgroup_" and "grp" intend to refer to the same?

Quoted 46 lines · Reinette Chatre

Quoted 44 lines · Babu

Quoted 23 lines · Reinette Chatre

Quoted 22 lines · Babu Moger

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

index 7ad653b4e768..1d45120ff2b5 100644
@@ -864,6 +864,13 @@ static int rdtgroup_rmid_show(struct kernfs_open_file *of,
 	return ret;
 }
 
+/*
+ * Get the counter index for the assignable counter
+ * 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID
+ * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID
+ */
+#define MBM_EVENT_ARRAY_INDEX(_event) ((_event) - 2)
+
 static int rdtgroup_mbm_assign_mode_show(struct kernfs_open_file *of,
 					 struct seq_file *s, void *v)
 {

@@ -1898,6 +1905,45 @@ int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d,
 	return 0;
 }
 

Quoted 14 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c int resctrl_arch_assign_cntr(struct rdt_resource *r, struct rdt_mon_domain *d, · line 1914

Quoted 7 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c +7 -0

+/*
+ * Assign a hardware counter to the group.
+ * Counter will be assigned to all the domains if rdt_mon_domain is NULL
+ * else the counter will be allocated to specific domain.
+ */
+int rdtgroup_assign_cntr(struct rdt_resource *r, struct rdtgroup *rdtgrp,
+			 struct rdt_mon_domain *d, enum resctrl_event_id evtid)

Could we please review the naming of function as this series progresses? Using such a generic name for this specific function seems to result in its callers later in series having even more generic names that are hard to decipher. For example, later (very generic) "rdtgroup_assign_grp()" calls this function and I find rdtgroup_assign_grp() to be very vague making the code more difficult to follow. For example, rdtgroup_assign_cntr() could be rdtgroup_assign_cntr_event() and rdtgroup_assign_grp() could instead be rdtgroup_assign_cntr()? Please feel free to improve.

Sure.

How about rdtgroup_assign_cntr_event() and rdtgroup_assign_cntr_grp() ?

Added grp extension for the second one.