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

The function resctrl_late_init() has the __init attribute, but some

functions it calls do not. Add the __init attribute to all the functions

to maintain consistency throughout the call sequence.

Fixes 6a445edce657 ("x86/intel_rdt/cqm: Add RDT monitoring initialization")

Fixes def10853930a ("x86/intel_rdt: Add two new resources for L2 Code and Data Prioritization (CDP)")

Fixes bd334c86b5d7 ("x86/resctrl: Add __init attribute to rdt_get_mon_l3_config()")

Suggested-by Reinette Chatre <[email protected]>

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

---
v9: Moved the patch to the begining of the series.
Fixed all the call sequences. Added additional Fixed tags.

v8: New patch.

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

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

index b681c2e07dbf..f845d0590429 100644
@@ -275,7 +275,7 @@ static __init bool __rdt_get_mem_config_amd(struct rdt_resource *r)
 	return true;
 }
 
-static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
+static __init void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
 {
 	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
 	union cpuid_0x10_1_eax eax;

@@ -294,7 +294,7 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
 	r->alloc_capable = true;
 }
 
-static void rdt_get_cdp_config(int level)
+static __init void rdt_get_cdp_config(int level)
 {
 	/*
 	 * By default, CDP is disabled. CDP can be enabled by mount parameter

@@ -304,12 +304,12 @@ static void rdt_get_cdp_config(int level)
 	rdt_resources_all[level].r_resctrl.cdp_capable = true;
 }
 
-static void rdt_get_cdp_l3_config(void)
+static __init void rdt_get_cdp_l3_config(void)
 {
 	rdt_get_cdp_config(RDT_RESOURCE_L3);
 }
 
-static void rdt_get_cdp_l2_config(void)
+static __init void rdt_get_cdp_l2_config(void)
 {
 	rdt_get_cdp_config(RDT_RESOURCE_L2);
 }

arch/x86/kernel/cpu/resctrl/internal.h +1 -1

index 955999aecfca..16181b90159a 100644
@@ -627,7 +627,7 @@ int closids_supported(void);
 void closid_free(int closid);
 int alloc_rmid(u32 closid);
 void free_rmid(u32 closid, u32 rmid);
-int rdt_get_mon_l3_config(struct rdt_resource *r);
+int __init rdt_get_mon_l3_config(struct rdt_resource *r);
 void __exit rdt_put_mon_l3_config(void);
 bool __init rdt_cpu_has(int flag);
 void mon_event_count(void *info);

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

index 851b561850e0..17790f92ef51 100644
@@ -983,7 +983,7 @@ void mbm_setup_overflow_handler(struct rdt_mon_domain *dom, unsigned long delay_
 		schedule_delayed_work_on(cpu, &dom->mbm_over, delay);
 }
 
-static int dom_data_init(struct rdt_resource *r)
+static __init int dom_data_init(struct rdt_resource *r)
 {
 	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
 	u32 num_closid = resctrl_arch_get_num_closid(r);

Hi Babu,

In subject please use () to indicate a function, writing resctrl_late_init()

Quoted 1 line

The function resctrl_late_init() has the __init attribute, but some

No need to say "The function" when using ().

Quoted 1 line

functions it calls do not. Add the __init attribute to all the functions

None of the functions changed are actually called by resctrl_late_init(). If this is indeed the goal then I think cache_alloc_hsw_probe() was missed.

Quoted 77 lines

to maintain consistency throughout the call sequence.

Fixes 6a445edce657 ("x86/intel_rdt/cqm: Add RDT monitoring initialization")

Fixes def10853930a ("x86/intel_rdt: Add two new resources for L2 Code and Data Prioritization (CDP)")

Fixes bd334c86b5d7 ("x86/resctrl: Add __init attribute to rdt_get_mon_l3_config()")

Suggested-by Reinette Chatre <[email protected]>

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

---
v9: Moved the patch to the begining of the series.
Fixed all the call sequences. Added additional Fixed tags.

v8: New patch.

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

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

index b681c2e07dbf..f845d0590429 100644
@@ -275,7 +275,7 @@ static __init bool __rdt_get_mem_config_amd(struct rdt_resource *r)
 	return true;
 }
 
-static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
+static __init void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
 {
 	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
 	union cpuid_0x10_1_eax eax;

@@ -294,7 +294,7 @@ static void rdt_get_cache_alloc_cfg(int idx, struct rdt_resource *r)
 	r->alloc_capable = true;
 }
 
-static void rdt_get_cdp_config(int level)
+static __init void rdt_get_cdp_config(int level)
 {
 	/*
 	 * By default, CDP is disabled. CDP can be enabled by mount parameter

@@ -304,12 +304,12 @@ static void rdt_get_cdp_config(int level)
 	rdt_resources_all[level].r_resctrl.cdp_capable = true;
 }
 
-static void rdt_get_cdp_l3_config(void)
+static __init void rdt_get_cdp_l3_config(void)
 {
 	rdt_get_cdp_config(RDT_RESOURCE_L3);
 }
 
-static void rdt_get_cdp_l2_config(void)
+static __init void rdt_get_cdp_l2_config(void)
 {
 	rdt_get_cdp_config(RDT_RESOURCE_L2);
 }

arch/x86/kernel/cpu/resctrl/internal.h +1 -1

index 955999aecfca..16181b90159a 100644
@@ -627,7 +627,7 @@ int closids_supported(void);
 void closid_free(int closid);
 int alloc_rmid(u32 closid);
 void free_rmid(u32 closid, u32 rmid);
-int rdt_get_mon_l3_config(struct rdt_resource *r);
+int __init rdt_get_mon_l3_config(struct rdt_resource *r);
 void __exit rdt_put_mon_l3_config(void);
 bool __init rdt_cpu_has(int flag);
 void mon_event_count(void *info);

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

index 851b561850e0..17790f92ef51 100644
@@ -983,7 +983,7 @@ void mbm_setup_overflow_handler(struct rdt_mon_domain *dom, unsigned long delay_
 		schedule_delayed_work_on(cpu, &dom->mbm_over, delay);
 }
 

arch/x86/kernel/cpu/resctrl/monitor.c static struct mon_evt mbm_local_event = { · line 1084

Quoted 11 lines

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

-static int dom_data_init(struct rdt_resource *r)
+static __init int dom_data_init(struct rdt_resource *r)
 {
 	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
 	u32 num_closid = resctrl_arch_get_num_closid(r);

@@ -1081,7 +1081,7 @@ static struct mon_evt mbm_local_event = {
  * because as per the SDM the total and local memory bandwidth
  * are enumerated as part of L3 monitoring.
  */
-static void l3_mon_evt_init(struct rdt_resource *r)
+static void __init l3_mon_evt_init(struct rdt_resource *r)

This change follows a different order from the other changes in this patch. "Function prototypes" in Documentation/process/coding-style.rst indicates the preferred order is storage class before return type. I acknowledge that resctrl is not consistent in this regard but we can work towards the preferred order while keeping this patch consistent?

Hi Reinette,

Quoted 3 lines · Reinette Chatre

Hi Babu,

In subject please use () to indicate a function, writing resctrl_late_init()

Will change it to

x86/resctrl: Add __init attribute for all the call sequences in resctrl_late_init()

Quoted 11 lines · Babu Moger

Quoted 1 line

The function resctrl_late_init() has the __init attribute, but some

No need to say "The function" when using ().

Quoted 1 line

functions it calls do not. Add the __init attribute to all the functions

None of the functions changed are actually called by resctrl_late_init(). If this is indeed the goal then I think cache_alloc_hsw_probe() was missed.

Will change the function to.

static inline __init void cache_alloc_hsw_probe(void)

How about this description?

"resctrl_late_init() has the __init attribute, but some of the call sequences of it do not have the __init attribute.

Add the __init attribute to all the functions in the call sequences to maintain consistency throughout."

Hi Babu,

Quoted 9 lines · Babu

Hi Reinette,

Quoted 3 lines · Reinette Chatre

Hi Babu,

In subject please use () to indicate a function, writing resctrl_late_init()

Will change it to

Quoted 2 lines

x86/resctrl: Add __init attribute for all the call sequences in resctrl_late_init()

I am not sure how to interpret "call sequences". The original is ok now that cache_alloc_hsw_probe() is also included. Specifically, this can be:

x86/resctrl: Add __init attribute to functions called from resctrl_late_init()

Quoted 17 lines

Quoted 12 lines · Babu Moger

Quoted 1 line

The function resctrl_late_init() has the __init attribute, but some

No need to say "The function" when using ().

Quoted 1 line

functions it calls do not. Add the __init attribute to all the functions

None of the functions changed are actually called by resctrl_late_init(). If this is indeed the goal then I think cache_alloc_hsw_probe() was missed.

Will change the function to.

static inline __init void cache_alloc_hsw_probe(void)

Quoted 4 lines

How about this description?

"resctrl_late_init() has the __init attribute, but some of the call sequences of it do not have the __init attribute.

"some of the call sequences of it" sounds strange. It can be simplified with "some of the functions called from it"?

Quoted 4 lines

Add the __init attribute to all the functions in the call sequences to maintain consistency throughout."

Sounds good, thank you.

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

The "mbm_cntr_assign" mode provides the option to assign a counter to an RMID, event pair 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 and is enabled by default.

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

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

---
v9: Updated user documentation based on comments.

v8: Commit message update.

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

index 30586728a4cd..a93d7980e25f 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
 
+"mbm_assign_mode":
+	Reports the list of monitoring modes supported. The enclosed brackets
+	indicate which mode is enabled.
+	::
+
+	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
+	  [mbm_cntr_assign]
+	  default
+

Hi Babu,

Quoted 67 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 counter to an RMID, event pair 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 and is enabled by default.

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

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

---
v9: Updated user documentation based on comments.

v8: Commit message update.

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

index 30586728a4cd..a93d7980e25f 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
 
+"mbm_assign_mode":
+	Reports the list of monitoring modes supported. The enclosed brackets
+	indicate which mode is enabled.
+	::
+
+	  # 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 of the
+	events in CTRL_MON or MON groups should have a counter assigned using the

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

Quoted 6 lines

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

+	"mbm_assign_control" file. The number of counters available is described
+	in the "num_mbm_cntrs" file. Changing the mode may cause all counters on
+	a resource to reset.
+
+	The mode is useful on platforms which support more CTRL_MON and MON
+	groups than the hardware counters, meaning 'unassigned' events on CTRL_MON or

" than the hardware counters" -> " than hardware counters"?

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

Quoted 3 lines

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

+	MON groups will report 'Unavailable' or count the traffic in an unpredictable
+	way.

I think the above can be confusing to users. It mentioned "*will* report Unavailable" and then "*or* count the traffic in an unpredictable way". It is not possible for counter to report "Unavailable" while also reporting unpredictable data.

My concern is that there is no way for a user to know if the platform supports more CTRL_MON and MON groups than hardware counters and the above seems to imply that counters may be unreliable ... so how does a user know if counters are unreliable or not?

Can this be made specific to help users know if their platforms are impacted? From what I know all AMD platforms are impacted so perhaps a straight-forward:

"The mode is useful on AMD platforms which support more CTRL_MON and MON ..."

I'm concerned that users with Intel platforms may want to use the "mbm_cntr_assign" mode to make the event data "more predictable" and then be concerned when the mode does not exist.

As an alternative, is it possible to know the number of hardware counters on AMD systems without ABMC? I wonder if we could perhaps always expose num_mbm_cntrs as a way for users to know if their platform may be impacted by this type of "unpredictability" (by comparing num_mbm_cntrs to num_rmids).

Quoted 11 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":
+
+	In default mode resctrl assumes there is a hardware counter for each
+	event within every CTRL_MON and MON group. Reading mbm_total_bytes or
+	mbm_local_bytes may report 'Unavailable' if there is no counter associated

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

Quoted 1 line

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

+	with that event.

If I understand correctly, on AMD platforms without ABMC the events only report "Unavailable" if there is no counter assigned at the time of the query. If a counter is unassigned and then reassigned then the event count will reset and the user will get some data back but it may thus be unpredictable (to match earlier language). Is this correct? Any AMD platform in "default" mode may thus be vulnerable to "unpredictable" event counts (not just "Unavailable") ... this gets complicated

because users should be steered to avoid "default" mode if mbm_assign_mode is available, while not be made concerned to use "default" mode on Intel where mbm_assign_mode is not available.

Reinette

Hi Reinette,

Quoted 24 lines · Reinette Chatre

Hi Babu,

Quoted 21 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 counter to an RMID, event pair 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 and is enabled by default.

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

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

---

Quoted 8 lines

I'm concerned that users with Intel platforms may want to use the "mbm_cntr_assign" mode to make the event data "more predictable" and then be concerned when the mode does not exist.

As an alternative, is it possible to know the number of hardware counters on AMD systems without ABMC? I wonder if we could perhaps always expose num_mbm_cntrs as a way for users to know if their platform may be impacted by this type of "unpredictability" (by comparing num_mbm_cntrs to num_rmids).

There is some round about(or hacky) way to find that out number of RMIDs that can be active.

Quoted 10 lines

Quoted 10 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":
+
+	In default mode resctrl assumes there is a hardware counter for each
+	event within every CTRL_MON and MON group. Reading mbm_total_bytes or
+	mbm_local_bytes may report 'Unavailable' if there is no counter associated

Quoted 8 lines

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

Quoted 1 line

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

+	with that event.

If I understand correctly, on AMD platforms without ABMC the events only report "Unavailable" if there is no counter assigned at the time of the query. If a counter is unassigned and then reassigned then the event count will reset and the user will get some data back but it may thus be unpredictable (to match earlier language). Is this correct? Any AMD platform in "default" mode may thus be vulnerable to "unpredictable" event counts (not just "Unavailable") ... this gets complicated

Yes. All the AMD systems without ABMC are affected by this problem.

Quoted 3 lines

because users should be steered to avoid "default" mode if mbm_assign_mode is available, while not be made concerned to use "default" mode on Intel where mbm_assign_mode is not available.

Can we add text to clarify this?

Hi Babu,

Quoted 27 lines · Babu

Hi Reinette,

Quoted 24 lines · Reinette Chatre

Hi Babu,

Quoted 21 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 counter to an RMID, event pair 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 and is enabled by default.

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

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

---

...

Quoted 11 lines

Quoted 8 lines

I'm concerned that users with Intel platforms may want to use the "mbm_cntr_assign" mode to make the event data "more predictable" and then be concerned when the mode does not exist.

As an alternative, is it possible to know the number of hardware counters on AMD systems without ABMC? I wonder if we could perhaps always expose num_mbm_cntrs as a way for users to know if their platform may be impacted by this type of "unpredictability" (by comparing num_mbm_cntrs to num_rmids).

There is some round about(or hacky) way to find that out number of RMIDs that can be active.

Does this give consistent and accurate data? Is this something that can be added to resctrl? (Reading your other message [1] it does not sound as though it can produce an accurate number on boot.)
If not then it will be up to the documentation to be accurate.

Quoted 21 lines

Quoted 10 lines

Quoted 10 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":
+
+	In default mode resctrl assumes there is a hardware counter for each
+	event within every CTRL_MON and MON group. Reading mbm_total_bytes or
+	mbm_local_bytes may report 'Unavailable' if there is no counter associated

Quoted 8 lines

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

Quoted 1 line

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

+	with that event.

If I understand correctly, on AMD platforms without ABMC the events only report "Unavailable" if there is no counter assigned at the time of the query. If a counter is unassigned and then reassigned then the event count will reset and the user will get some data back but it may thus be unpredictable (to match earlier language). Is this correct? Any AMD platform in "default" mode may thus be vulnerable to "unpredictable" event counts (not just "Unavailable") ... this gets complicated

Yes. All the AMD systems without ABMC are affected by this problem.

Quoted 5 lines

Quoted 3 lines

because users should be steered to avoid "default" mode if mbm_assign_mode is available, while not be made concerned to use "default" mode on Intel where mbm_assign_mode is not available.

Can we add text to clarify this?

Please do.

Reinette

[1] https://lore.kernel.org/all/[email protected]/

Hi Reinette,

Quoted 78 lines · Reinette Chatre

Hi Babu,

Quoted 27 lines · Babu

Hi Reinette,

Quoted 24 lines · Reinette Chatre

Hi Babu,

Quoted 21 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 counter to an RMID, event pair 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 and is enabled by default.

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

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

---

...

Quoted 11 lines

Quoted 8 lines

I'm concerned that users with Intel platforms may want to use the "mbm_cntr_assign" mode to make the event data "more predictable" and then be concerned when the mode does not exist.

As an alternative, is it possible to know the number of hardware counters on AMD systems without ABMC? I wonder if we could perhaps always expose num_mbm_cntrs as a way for users to know if their platform may be impacted by this type of "unpredictability" (by comparing num_mbm_cntrs to num_rmids).

There is some round about(or hacky) way to find that out number of RMIDs that can be active.

Does this give consistent and accurate data? Is this something that can be added to resctrl? (Reading your other message [1] it does not sound as though it can produce an accurate number on boot.)
If not then it will be up to the documentation to be accurate.

Quoted 26 lines

Quoted 10 lines

Quoted 10 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":
+
+    In default mode resctrl assumes there is a hardware counter for each
+    event within every CTRL_MON and MON group. Reading mbm_total_bytes or
+    mbm_local_bytes may report 'Unavailable' if there is no counter associated

Quoted 8 lines

Quoted 1 line

+    with that event.

If I understand correctly, on AMD platforms without ABMC the events only report "Unavailable" if there is no counter assigned at the time of the query. If a counter is unassigned and then reassigned then the event count will reset and the user will get some data back but it may thus be unpredictable (to match earlier language). Is this correct? Any AMD platform in "default" mode may thus be vulnerable to "unpredictable" event counts (not just "Unavailable") ... this gets complicated

Yes. All the AMD systems without ABMC are affected by this problem.

Quoted 3 lines

because users should be steered to avoid "default" mode if mbm_assign_mode is available, while not be made concerned to use "default" mode on Intel where mbm_assign_mode is not available.

Can we add text to clarify this?

Quoted 1 line

Please do.

I think we need to add text about AMD systems. How about this?

"default":
In default mode resctrl assumes there is a hardware counter for each
event within every CTRL_MON and MON group. On AMD systems with 16 more monitoring groups, reading mbm_total_bytes or mbm_local_bytes may report 'Unavailable' if there is no counter associated with that event. It is therefore recommended to use the 'mbm_cntr_assign' mode, if supported."

Hi Babu,

Quoted 85 lines · Babu

Hi Reinette,

Quoted 72 lines · Reinette Chatre

Hi Babu,

Quoted 27 lines · Babu

Hi Reinette,

Quoted 24 lines · Reinette Chatre

Hi Babu,

Quoted 21 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 counter to an RMID, event pair 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 and is enabled by default.

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

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

---

...

Quoted 11 lines

Quoted 8 lines

I'm concerned that users with Intel platforms may want to use the "mbm_cntr_assign" mode to make the event data "more predictable" and then be concerned when the mode does not exist.

As an alternative, is it possible to know the number of hardware counters on AMD systems without ABMC? I wonder if we could perhaps always expose num_mbm_cntrs as a way for users to know if their platform may be impacted by this type of "unpredictability" (by comparing num_mbm_cntrs to num_rmids).

There is some round about(or hacky) way to find that out number of RMIDs that can be active.

Does this give consistent and accurate data? Is this something that can be added to resctrl? (Reading your other message [1] it does not sound as though it can produce an accurate number on boot.)
If not then it will be up to the documentation to be accurate.

Quoted 21 lines

Quoted 10 lines

Quoted 10 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":
+
+    In default mode resctrl assumes there is a hardware counter for each
+    event within every CTRL_MON and MON group. Reading mbm_total_bytes or
+    mbm_local_bytes may report 'Unavailable' if there is no counter associated

Quoted 8 lines

Quoted 1 line

+    with that event.

If I understand correctly, on AMD platforms without ABMC the events only report "Unavailable" if there is no counter assigned at the time of the query. If a counter is unassigned and then reassigned then the event count will reset and the user will get some data back but it may thus be unpredictable (to match earlier language). Is this correct? Any AMD platform in "default" mode may thus be vulnerable to "unpredictable" event counts (not just "Unavailable") ... this gets complicated

Yes. All the AMD systems without ABMC are affected by this problem.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

because users should be steered to avoid "default" mode if mbm_assign_mode is available, while not be made concerned to use "default" mode on Intel where mbm_assign_mode is not available.

Can we add text to clarify this?

Please do.

I think we need to add text about AMD systems. How about this?

Quoted 3 lines

"default":
In default mode resctrl assumes there is a hardware counter for each
event within every CTRL_MON and MON group. On AMD systems with 16 more monitoring groups, reading mbm_total_bytes or mbm_local_bytes may report 'Unavailable' if there is no counter associated with that event. It is therefore recommended to use the 'mbm_cntr_assign' mode, if supported."

What is meant with "On AMD systems with 16 more monitoring groups"? First, the language is not clear, second, you mentioned earlier that there is just a "hacky" way to determine number of RMIDs that can be active but here "16" is made official in the documentation?

Hi Reinette,

Quoted 88 lines · Reinette Chatre

Hi Babu,

Quoted 85 lines · Babu

Hi Reinette,

Quoted 72 lines · Reinette Chatre

Hi Babu,

Quoted 27 lines · Babu

Hi Reinette,

Quoted 24 lines · Reinette Chatre

Hi Babu,

Quoted 21 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 counter to an RMID, event pair 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 and is enabled by default.

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

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

---

...

Quoted 11 lines

Quoted 8 lines

I'm concerned that users with Intel platforms may want to use the "mbm_cntr_assign" mode to make the event data "more predictable" and then be concerned when the mode does not exist.

As an alternative, is it possible to know the number of hardware counters on AMD systems without ABMC? I wonder if we could perhaps always expose num_mbm_cntrs as a way for users to know if their platform may be impacted by this type of "unpredictability" (by comparing num_mbm_cntrs to num_rmids).

There is some round about(or hacky) way to find that out number of RMIDs that can be active.

Does this give consistent and accurate data? Is this something that can be added to resctrl? (Reading your other message [1] it does not sound as though it can produce an accurate number on boot.)
If not then it will be up to the documentation to be accurate.

Quoted 21 lines

Quoted 10 lines

Quoted 10 lines

(quoted diff) +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":
+
+    In default mode resctrl assumes there is a hardware counter for each
+    event within every CTRL_MON and MON group. Reading mbm_total_bytes or
+    mbm_local_bytes may report 'Unavailable' if there is no counter associated

Quoted 8 lines

Quoted 1 line

+    with that event.

If I understand correctly, on AMD platforms without ABMC the events only report "Unavailable" if there is no counter assigned at the time of the query. If a counter is unassigned and then reassigned then the event count will reset and the user will get some data back but it may thus be unpredictable (to match earlier language). Is this correct? Any AMD platform in "default" mode may thus be vulnerable to "unpredictable" event counts (not just "Unavailable") ... this gets complicated

Yes. All the AMD systems without ABMC are affected by this problem.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

because users should be steered to avoid "default" mode if mbm_assign_mode is available, while not be made concerned to use "default" mode on Intel where mbm_assign_mode is not available.

Can we add text to clarify this?

Please do.

I think we need to add text about AMD systems. How about this?

Quoted 9 lines

Quoted 3 lines

"default":
In default mode resctrl assumes there is a hardware counter for each
event within every CTRL_MON and MON group. On AMD systems with 16 more monitoring groups, reading mbm_total_bytes or mbm_local_bytes may report 'Unavailable' if there is no counter associated with that event. It is therefore recommended to use the 'mbm_cntr_assign' mode, if supported."

What is meant with "On AMD systems with 16 more monitoring groups"? First, the language is not clear, second, you mentioned earlier that there is just a "hacky" way to determine number of RMIDs that can be active but here "16" is made official in the documentation?

The lowest active RMID is 16. I could not get it using the hacky way.
I have verified testing on all the previous generation of AMD systems by creating the monitoring groups until it reports "Unavailable". In recent systems it is 32.  We can drop the exact number to be generic.

There is no clear documentation on that.  Here is what the doc says.

A given implementation may have insufficient hardware to simultaneously track the bandwidth for all RMID values which the hardware supports. If an attempt is made to read a Bandwidth Count for an RMID that has been impacted by these hardware limitations, the “U” bit of the QM_CTR will be set when the counter is read. Subsequent QM_CTR reads for that RMID and Event may return a value with the "U" bit clear. Potential causes of the “U” bit being set include (but are not limited to)

• RMID is not currently tracked by the hardware.
• RMID was not tracked by the hardware at some time since it was last read.
• RMID has not been read since it started being tracked by the hardware.

All RMIDs which are currently in use by one or more processors in the QOS domain will be tracked. The hardware will always begin tracking a new RMID value when it gets written to the PQR_ASSOC register of any of the processors in the QOS domain and it is not already being tracked. When the hardware begins tracking an RMID that it was not previously tracking, it will clear the QM_CTR for all events in the new RMID

- Babu Moger

Hi Babu,

Quoted 111 lines · Babu

Hi Reinette,

Quoted 88 lines · Reinette Chatre

Hi Babu,

Quoted 85 lines · Babu

Hi Reinette,

Quoted 72 lines · Reinette Chatre

Hi Babu,

Quoted 27 lines · Babu

Hi Reinette,

Quoted 24 lines · Reinette Chatre

Hi Babu,

Quoted 21 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 counter to an RMID, event pair 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 and is enabled by default.

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

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

---

...

Quoted 11 lines

Quoted 8 lines

I'm concerned that users with Intel platforms may want to use the "mbm_cntr_assign" mode to make the event data "more predictable" and then be concerned when the mode does not exist.

As an alternative, is it possible to know the number of hardware counters on AMD systems without ABMC? I wonder if we could perhaps always expose num_mbm_cntrs as a way for users to know if their platform may be impacted by this type of "unpredictability" (by comparing num_mbm_cntrs to num_rmids).

There is some round about(or hacky) way to find that out number of RMIDs that can be active.

Does this give consistent and accurate data? Is this something that can be added to resctrl? (Reading your other message [1] it does not sound as though it can produce an accurate number on boot.)
If not then it will be up to the documentation to be accurate.

Quoted 21 lines

Quoted 10 lines

Quoted 10 lines

(quoted diff) +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":
+
+    In default mode resctrl assumes there is a hardware counter for each
+    event within every CTRL_MON and MON group. Reading mbm_total_bytes or
+    mbm_local_bytes may report 'Unavailable' if there is no counter associated

Quoted 8 lines

Quoted 1 line

+    with that event.

If I understand correctly, on AMD platforms without ABMC the events only report "Unavailable" if there is no counter assigned at the time of the query. If a counter is unassigned and then reassigned then the event count will reset and the user will get some data back but it may thus be unpredictable (to match earlier language). Is this correct? Any AMD platform in "default" mode may thus be vulnerable to "unpredictable" event counts (not just "Unavailable") ... this gets complicated

Yes. All the AMD systems without ABMC are affected by this problem.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

because users should be steered to avoid "default" mode if mbm_assign_mode is available, while not be made concerned to use "default" mode on Intel where mbm_assign_mode is not available.

Can we add text to clarify this?

Please do.

I think we need to add text about AMD systems. How about this?

Quoted 9 lines

Quoted 3 lines

"default":
In default mode resctrl assumes there is a hardware counter for each
event within every CTRL_MON and MON group. On AMD systems with 16 more monitoring groups, reading mbm_total_bytes or mbm_local_bytes may report 'Unavailable' if there is no counter associated with that event. It is therefore recommended to use the 'mbm_cntr_assign' mode, if supported."

What is meant with "On AMD systems with 16 more monitoring groups"? First, the language is not clear, second, you mentioned earlier that there is just a "hacky" way to determine number of RMIDs that can be active but here "16" is made official in the documentation?

The lowest active RMID is 16. I could not get it using the hacky way.
I have verified testing on all the previous generation of AMD systems by creating the monitoring groups until it reports "Unavailable". In recent systems it is 32.  We can drop the exact number to be generic.

There is no clear documentation on that.  Here is what the doc says.

A given implementation may have insufficient hardware to simultaneously track the bandwidth for all RMID values which the hardware supports. If an attempt is made to read a Bandwidth Count for an RMID that has been impacted by these hardware limitations, the “U” bit of the QM_CTR will be set when the counter is read. Subsequent QM_CTR reads for that RMID and Event may return a value with the "U" bit clear. Potential causes of the “U” bit being set include (but are not limited to)

Quoted 8 lines

• RMID is not currently tracked by the hardware.
• RMID was not tracked by the hardware at some time since it was last read.
• RMID has not been read since it started being tracked by the hardware.

All RMIDs which are currently in use by one or more processors in the QOS domain will be tracked. The hardware will always begin tracking a new RMID value when it gets written to the PQR_ASSOC register of any of the processors in the QOS domain and it is not already being tracked. When the hardware begins tracking an RMID that it was not previously tracking, it will clear the QM_CTR for all events in the new RMID

- Babu Moger

I think I am starting to understand what is meant with the "count the traffic in an unpredictable way". From what I understand the hardware uses the "U" bit to indicate that an RMID was not tracked for a while, but it only sets this bit on the first read. After that the "U" bit may be cleared if a counter can be assigned to an RMID afterwards.
If it was only user space that reads the data then it should be clear to the user when the hardware limitation is encountered and thus hardware behavior can be "predictable", but since the overflow handler runs once per second it may indeed be the overflow handler that encounters the "U" bit and that bit is not currently handled. This could leave user space with impression that events are always returning data but that data may indeed be wrong.

In another thread [1] Tony confirmed that "U" bit is not returned by Intel systems so this issue only impacts AMD. As I understand the other scenarios in which AMD systems can return "U" (the first read after assigning an RMID and the first read after changing the memory config) are all scenarios that can be controlled by resctrl.

I do not see why unpredictable data should be addressed with documentation. Could this not be fixed instead? Essentially stating "AMD systems without ABMC count the traffic in an unpredictable way" seems like a poor user experience.
What if instead resctrl handles the "U" bit better? For example, when the overflow handler encounters the "U" bit the RMID can be permanently marked as "Unavailable"? Would that not be better than the counter behaving unpredictably with users never knowing if they can trust the event counters?

Reinette

[1] https://lore.kernel.org/all/ZzUvA2XE01U25A38@agluck-desk3/

Quoted 102 lines · Reinette Chatre

Quoted 76 lines · Babu

Quoted 55 lines · Reinette Chatre

Quoted 54 lines · Babu

Quoted 43 lines · Reinette Chatre

Quoted 7 lines · Babu

Quoted 6 lines · Reinette Chatre

On 10/29/24 4:21 PM, Babu Moger wrote:

I'm concerned that users with Intel platforms may want to use the "mbm_cntr_assign" mode to make the event data "more predictable" and then be concerned when the mode does not exist.

Quoted 7 lines

Quoted 4 lines

As an alternative, is it possible to know the number of hardware counters on AMD systems without ABMC? I wonder if we could perhaps always expose num_mbm_cntrs as a way for users to know if their platform may be impacted by this type of "unpredictability" (by comparing num_mbm_cntrs to num_rmids).

There is some round about(or hacky) way to find that out number of RMIDs that can be active.

Does this give consistent and accurate data? Is this something that can be added to resctrl? (Reading your other message [1] it does not sound as though it can produce an accurate number on boot.)
If not then it will be up to the documentation to be accurate.

Quoted 21 lines

Quoted 10 lines

Quoted 10 lines

(quoted diff) +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":
+
+    In default mode resctrl assumes there is a hardware counter for each
+    event within every CTRL_MON and MON group. Reading mbm_total_bytes or
+    mbm_local_bytes may report 'Unavailable' if there is no counter associated

Quoted 8 lines

Quoted 1 line

+    with that event.

If I understand correctly, on AMD platforms without ABMC the events only report "Unavailable" if there is no counter assigned at the time of the query. If a counter is unassigned and then reassigned then the event count will reset and the user will get some data back but it may thus be unpredictable (to match earlier language). Is this correct? Any AMD platform in "default" mode may thus be vulnerable to "unpredictable" event counts (not just "Unavailable") ... this gets complicated

Yes. All the AMD systems without ABMC are affected by this problem.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

because users should be steered to avoid "default" mode if mbm_assign_mode is available, while not be made concerned to use "default" mode on Intel where mbm_assign_mode is not available.

Can we add text to clarify this?

Please do.

I think we need to add text about AMD systems. How about this?

Quoted 9 lines

Quoted 3 lines

"default":
In default mode resctrl assumes there is a hardware counter for each
event within every CTRL_MON and MON group. On AMD systems with 16 more monitoring groups, reading mbm_total_bytes or mbm_local_bytes may report 'Unavailable' if there is no counter associated with that event. It is therefore recommended to use the 'mbm_cntr_assign' mode, if supported."

What is meant with "On AMD systems with 16 more monitoring groups"? First, the language is not clear, second, you mentioned earlier that there is just a "hacky" way to determine number of RMIDs that can be active but here "16" is made official in the documentation?

The lowest active RMID is 16. I could not get it using the hacky way.
I have verified testing on all the previous generation of AMD systems by creating the monitoring groups until it reports "Unavailable". In recent systems it is 32.  We can drop the exact number to be generic.

There is no clear documentation on that.  Here is what the doc says.

A given implementation may have insufficient hardware to simultaneously track the bandwidth for all RMID values which the hardware supports. If an attempt is made to read a Bandwidth Count for an RMID that has been impacted by these hardware limitations, the “U” bit of the QM_CTR will be set when the counter is read. Subsequent QM_CTR reads for that RMID and Event may return a value with the "U" bit clear. Potential causes of the “U” bit being set include (but are not limited to)

Quoted 8 lines

• RMID is not currently tracked by the hardware.
• RMID was not tracked by the hardware at some time since it was last read.
• RMID has not been read since it started being tracked by the hardware.

All RMIDs which are currently in use by one or more processors in the QOS domain will be tracked. The hardware will always begin tracking a new RMID value when it gets written to the PQR_ASSOC register of any of the processors in the QOS domain and it is not already being tracked. When the hardware begins tracking an RMID that it was not previously tracking, it will clear the QM_CTR for all events in the new RMID

- Babu Moger

I think I am starting to understand what is meant with the "count the traffic in an unpredictable way". From what I understand the hardware uses the "U" bit to indicate that an RMID was not tracked for a while, but it only sets this bit on the first read. After that the "U" bit may be cleared if a counter can be assigned to an RMID afterwards.
If it was only user space that reads the data then it should be clear to the user when the hardware limitation is encountered and thus hardware behavior can be "predictable", but since the overflow handler runs once per second it may indeed be the overflow handler that encounters the "U" bit and that bit is not currently handled. This could leave user space with impression that events are always returning data but that data may indeed be wrong.

In another thread [1] Tony confirmed that "U" bit is not returned by Intel systems so this issue only impacts AMD. As I understand the other scenarios in which AMD systems can return "U" (the first read after assigning an RMID and the first read after changing the memory config) are all scenarios that can be controlled by resctrl.

Quoted 7 lines

I do not see why unpredictable data should be addressed with documentation. Could this not be fixed instead? Essentially stating "AMD systems without ABMC count the traffic in an unpredictable way" seems like a poor user experience.
What if instead resctrl handles the "U" bit better? For example, when the overflow handler encounters the "U" bit the RMID can be permanently marked as "Unavailable"? Would that not be better than the counter behaving unpredictably with users never knowing if they can trust the event counters?

Actually, I was looking at handling "Unavailable" in little bit better way. Right now, I see it reports "Unavailable" first then it goes into overflow and stays in overflow forever.

Also setting the RMID Unavailable permanently is not a good option. We should have a way to reset it. At some later point the RMID can become active and report the correct numbers.

I was thinking of introducing a new arch state(in arch_mbm_state) to handle this case. Need to investigate more on this. What do you think?

Hi Babu,

Quoted 111 lines · Babu

Quoted 109 lines · Reinette Chatre

Quoted 76 lines · Babu

Quoted 55 lines · Reinette Chatre

Quoted 54 lines · Babu

Quoted 43 lines · Reinette Chatre

Quoted 7 lines · Babu

Quoted 6 lines · Reinette Chatre

On 10/29/24 4:21 PM, Babu Moger wrote:

I'm concerned that users with Intel platforms may want to use the "mbm_cntr_assign" mode to make the event data "more predictable" and then be concerned when the mode does not exist.

Quoted 7 lines

Quoted 4 lines

As an alternative, is it possible to know the number of hardware counters on AMD systems without ABMC? I wonder if we could perhaps always expose num_mbm_cntrs as a way for users to know if their platform may be impacted by this type of "unpredictability" (by comparing num_mbm_cntrs to num_rmids).

There is some round about(or hacky) way to find that out number of RMIDs that can be active.

Does this give consistent and accurate data? Is this something that can be added to resctrl? (Reading your other message [1] it does not sound as though it can produce an accurate number on boot.)
If not then it will be up to the documentation to be accurate.

Quoted 21 lines

Quoted 10 lines

Quoted 10 lines

(quoted diff) +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":
+
+    In default mode resctrl assumes there is a hardware counter for each
+    event within every CTRL_MON and MON group. Reading mbm_total_bytes or
+    mbm_local_bytes may report 'Unavailable' if there is no counter associated

Quoted 8 lines

Quoted 1 line

+    with that event.

If I understand correctly, on AMD platforms without ABMC the events only report "Unavailable" if there is no counter assigned at the time of the query. If a counter is unassigned and then reassigned then the event count will reset and the user will get some data back but it may thus be unpredictable (to match earlier language). Is this correct? Any AMD platform in "default" mode may thus be vulnerable to "unpredictable" event counts (not just "Unavailable") ... this gets complicated

Yes. All the AMD systems without ABMC are affected by this problem.

Quoted 7 lines

Quoted 5 lines

Quoted 3 lines

because users should be steered to avoid "default" mode if mbm_assign_mode is available, while not be made concerned to use "default" mode on Intel where mbm_assign_mode is not available.

Can we add text to clarify this?

Please do.

I think we need to add text about AMD systems. How about this?

Quoted 9 lines

Quoted 3 lines

"default":
In default mode resctrl assumes there is a hardware counter for each
event within every CTRL_MON and MON group. On AMD systems with 16 more monitoring groups, reading mbm_total_bytes or mbm_local_bytes may report 'Unavailable' if there is no counter associated with that event. It is therefore recommended to use the 'mbm_cntr_assign' mode, if supported."

What is meant with "On AMD systems with 16 more monitoring groups"? First, the language is not clear, second, you mentioned earlier that there is just a "hacky" way to determine number of RMIDs that can be active but here "16" is made official in the documentation?

The lowest active RMID is 16. I could not get it using the hacky way.
I have verified testing on all the previous generation of AMD systems by creating the monitoring groups until it reports "Unavailable". In recent systems it is 32.  We can drop the exact number to be generic.

There is no clear documentation on that.  Here is what the doc says.

A given implementation may have insufficient hardware to simultaneously track the bandwidth for all RMID values which the hardware supports. If an attempt is made to read a Bandwidth Count for an RMID that has been impacted by these hardware limitations, the “U” bit of the QM_CTR will be set when the counter is read. Subsequent QM_CTR reads for that RMID and Event may return a value with the "U" bit clear. Potential causes of the “U” bit being set include (but are not limited to)

Quoted 8 lines

• RMID is not currently tracked by the hardware.
• RMID was not tracked by the hardware at some time since it was last read.
• RMID has not been read since it started being tracked by the hardware.

All RMIDs which are currently in use by one or more processors in the QOS domain will be tracked. The hardware will always begin tracking a new RMID value when it gets written to the PQR_ASSOC register of any of the processors in the QOS domain and it is not already being tracked. When the hardware begins tracking an RMID that it was not previously tracking, it will clear the QM_CTR for all events in the new RMID

- Babu Moger

I think I am starting to understand what is meant with the "count the traffic in an unpredictable way". From what I understand the hardware uses the "U" bit to indicate that an RMID was not tracked for a while, but it only sets this bit on the first read. After that the "U" bit may be cleared if a counter can be assigned to an RMID afterwards.
If it was only user space that reads the data then it should be clear to the user when the hardware limitation is encountered and thus hardware behavior can be "predictable", but since the overflow handler runs once per second it may indeed be the overflow handler that encounters the "U" bit and that bit is not currently handled. This could leave user space with impression that events are always returning data but that data may indeed be wrong.

In another thread [1] Tony confirmed that "U" bit is not returned by Intel systems so this issue only impacts AMD. As I understand the other scenarios in which AMD systems can return "U" (the first read after assigning an RMID and the first read after changing the memory config) are all scenarios that can be controlled by resctrl.

I do not see why unpredictable data should be addressed with documentation. Could this not be fixed instead? Essentially stating "AMD systems without ABMC count the traffic in an unpredictable way" seems like a poor user experience.
What if instead resctrl handles the "U" bit better? For example, when the overflow handler encounters the "U" bit the RMID can be permanently marked as "Unavailable"? Would that not be better than the counter behaving unpredictably with users never knowing if they can trust the event counters?

Quoted 1 line

Actually, I was looking at handling "Unavailable" in little bit better way. Right now, I see it reports "Unavailable" first then it goes into overflow and stays in overflow forever.

Could you please elaborate what you mean with "stays in overflow forever"?

Quoted 3 lines

Also setting the RMID Unavailable permanently is not a good option. We should have a way to reset it. At some later point the RMID can become active and report the correct numbers.

I assume that when an RMID becomes active cannot be the trigger to reset it since user space cannot then be aware that a counter was not available for a while.

Quoted 3 lines

I was thinking of introducing a new arch state(in arch_mbm_state) to handle this case. Need to investigate more on this. What do you think?

Some new state is surely needed to reflect that the RMID's data may be wrong. It is not clear to me how you envision the reset of the state. If it is driven from user space then I expect that resctrl needs to be taught something about this and it cannot just be buried in arch code.

The mbm_cntr_assign mode provides an option to the user to assign a 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. The interface file 'num_mbm_cntrs' is available when an architecture supports mbm_cntr_assign mode.

Hi Babu,

Quoted 9 lines · Babu Moger

The mbm_cntr_assign mode provides an option to the user to assign a 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. The interface file 'num_mbm_cntrs' is available when an architecture supports mbm_cntr_assign mode.

As mentioned in previous patch, do you think it may be possible to have a value for num_mbm_cntrs for non-ABMC AMD systems? If that is available and always exposed to user space (irrespective of mbm_cntr_assign mode) then it would be clear to user space on benefits/risks of running a "default" mode.

Hi Reinette,

Quoted 13 lines · Reinette Chatre

Hi Babu,

Quoted 9 lines · Babu Moger

The mbm_cntr_assign mode provides an option to the user to assign a 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. The interface file 'num_mbm_cntrs' is available when an architecture supports mbm_cntr_assign mode.

Quoted 5 lines

As mentioned in previous patch, do you think it may be possible to have a value for num_mbm_cntrs for non-ABMC AMD systems? If that is available and always exposed to user space (irrespective of mbm_cntr_assign mode) then it would be clear to user space on benefits/risks of running a "default" mode.

I am trying the work-around to get the number of max active RMIDs in default mode. The method is to loop through all of the recently assigned RMID's to see if any of their QM_CTR.U bits transition from 0->1.

I am not successful in getting it to work so far. I remember Peter was trying this before in soft-ABMC. Peter, Any success with that?

Hardware provides a set of counters when mbm_assign_mode is supported. These counters are assigned to the MBM monitoring events of a CTRL_MON or MON group that needs to be tracked. The kernel must manage and track the available counters.

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

dom_data_init() requires mbm_cntr_assign state to initialize mbm_cntr_free_map bitmap. Move dom_data_init() after mbm_cntr_assign detection.

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

---

Hi Babu,

Quoted 6 lines · Babu Moger

include/linux/resctrl.h +0 -0

@@ -354,6 +354,10 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
  */
 void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *d);
 

include/linux/resctrl.h void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d, · line 357

Quoted 1 line

include/linux/resctrl.h +1 -0

+void resctrl_arch_mon_event_config_set(void *info);

An architecture that may want to use this would need to know how to interpret the info passed. For an API I thus expect the struct it points to to also be available in this header file.

include/linux/resctrl.h void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d, · line 363

Quoted 8 lines

include/linux/resctrl.h +3 -0

+u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
+				      enum resctrl_event_id eventid);
+
 extern unsigned int resctrl_rmid_realloc_threshold;
 extern unsigned int resctrl_rmid_realloc_limit;
 

Reinette

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

Quoted 2 lines

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

s/doamin/domain/

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.

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

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

index 2f3a86278e84..2bc58d974934 100644

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

 mkdir mon_groups/m${i}

done

Hi Babu,

Quoted 21 lines · Babu Moger

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.

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

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

index 2f3a86278e84..2bc58d974934 100644
@@ -302,6 +302,10 @@ with the following files:
        memory bandwidth tracking to a single memory bandwidth event per
        monitoring group.
 

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

Quoted 3 lines

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

+"available_mbm_cntrs":
+       The number of free monitoring counters available assignment in each domain
+       when the architecture supports mbm_cntr_assign mode.

It seems you need to clarify that counters are only available to a domain when they're available in all domains:

resctrl# for i in seq 100; do

Quoted 2 lines

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

 mkdir mon_groups/m${i}

done

resctrl# cat info/L3_MON/available_mbm_cntrs 0=0;1=0;2=0;3=0;4=0;5=0;6=0;7=0;8=0;9=0;10=0;11=0;12=0;16=0;17=0;18=0;19=0;20=0;21=0;22=0;23=0;24=0;25=0;26=0;27=0;28=0

resctrl# cd info/L3_MON/
L3_MON# echo '/m1/0=_' > mbm_assign_control
L3_MON# cat available_mbm_cntrs 0=2;1=0;2=0;3=0;4=0;5=0;6=0;7=0;8=0;9=0;10=0;11=0;12=0;16=0;17=0;18=0;19=0;20=0;21=0;22=0;23=0;24=0;25=0;26=0;27=0;28=0 L3_MON# echo '/m16/0+t' > mbm_assign_control
-bash: echo: write error: Invalid argument
L3_MON# cat ../last_cmd_status
Out of MBM assignable counters
Assign operation '+t' failed on the group /m16/

L3_MON# rmdir ../../mon_groups/m1
L3_MON# cat available_mbm_cntrs 0=2;1=2;2=2;3=2;4=2;5=2;6=2;7=2;8=2;9=2;10=2;11=2;12=2;16=2;17=2;18=2;19=2;20=2;21=2;22=2;23=2;24=2;25=2;26=2;27=2;28=2 L3_MON# echo '/m16/0+t' > mbm_assign_control
L3_MON#

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.

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

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

index 2f3a86278e84..2bc58d974934 100644

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

+	when the architecture supports mbm_cntr_assign mode.
+
 "max_threshold_occupancy":
 		Read/write file provides the largest value (in
 		bytes) at which a previously used LLC_occupancy

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

index 3996f7528b66..e8d38a963f39 100644
@@ -1268,6 +1268,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 			cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
 			r->mon.num_mbm_cntrs = (ebx & GENMASK(15, 0)) + 1;
 			resctrl_file_fflags_init("num_mbm_cntrs", RFTYPE_MON_INFO);
+			resctrl_file_fflags_init("available_mbm_cntrs", RFTYPE_MON_INFO);
 		}
 	}
 

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

index 654cdfee1b00..ef0c1246fa2a 100644
@@ -898,6 +898,33 @@ static int rdtgroup_num_mbm_cntrs_show(struct kernfs_open_file *of,
 	return 0;
 }
 
+static int rdtgroup_available_mbm_cntrs_show(struct kernfs_open_file *of,
+					     struct seq_file *s, void *v)
+{
+	struct rdt_resource *r = of->kn->parent->priv;
+	struct rdt_mon_domain *dom;
+	bool sep = false;

Quoted 1 line

arch/x86/kernel/cpu/resctrl/rdtgroup.c:916:32: warning: shift count >= width of type [-Wshift-count-overflow]

Hi Babu,

Quoted 20 lines · Babu Moger

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.

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

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

index 2f3a86278e84..2bc58d974934 100644
@@ -302,6 +302,10 @@ with the following files:
 	memory bandwidth tracking to a single memory bandwidth event per
 	monitoring group.
 

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

Quoted 2 lines

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

+"available_mbm_cntrs":
+	The number of free monitoring counters available assignment in each domain

"The number of free monitoring counters available assignment" -> "The number of monitoring counters available for assignment"?

(not taking into account how text may change after addressing Peter's feedback)

Quoted 39 lines

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

+	when the architecture supports mbm_cntr_assign mode.
+
 "max_threshold_occupancy":
 		Read/write file provides the largest value (in
 		bytes) at which a previously used LLC_occupancy

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

index 3996f7528b66..e8d38a963f39 100644
@@ -1268,6 +1268,7 @@ int __init rdt_get_mon_l3_config(struct rdt_resource *r)
 			cpuid_count(0x80000020, 5, &eax, &ebx, &ecx, &edx);
 			r->mon.num_mbm_cntrs = (ebx & GENMASK(15, 0)) + 1;
 			resctrl_file_fflags_init("num_mbm_cntrs", RFTYPE_MON_INFO);
+			resctrl_file_fflags_init("available_mbm_cntrs", RFTYPE_MON_INFO);
 		}
 	}
 

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

index 654cdfee1b00..ef0c1246fa2a 100644
@@ -898,6 +898,33 @@ static int rdtgroup_num_mbm_cntrs_show(struct kernfs_open_file *of,
 	return 0;
 }
 
+static int rdtgroup_available_mbm_cntrs_show(struct kernfs_open_file *of,
+					     struct seq_file *s, void *v)
+{
+	struct rdt_resource *r = of->kn->parent->priv;
+	struct rdt_mon_domain *dom;
+	bool sep = false;
+	u32 val;
+
+	cpus_read_lock();
+	mutex_lock(&rdtgroup_mutex);
+
+	list_for_each_entry(dom, &r->mon_domains, hdr.list) {
+		if (sep)

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

Quoted 3 lines

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

+			seq_puts(s, ";");
+
+		val = r->mon.num_mbm_cntrs - hweight64(*dom->mbm_cntr_map);

This should probably be bitmap_weight() to address warnings like below that are encountered by build testing with various configs (32bit in this case). 0day does not seem to automatically pick up patches just based on submission but it sure will when these are merged to tip so this needs a clean slate.

Quoted 2 lines

Quoted 2 lines

arch/x86/kernel/cpu/resctrl/rdtgroup.c:916:32: warning: shift count >= width of type [-Wshift-count-overflow]

     916 |                 val = r->mon.num_mbm_cntrs - hweight64(*dom->mbm_cntr_map);
         |                                              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/asm-generic/bitops/const_hweight.h:29:49: note: expanded from macro 'hweight64'
      29 | #define hweight64(w) (__builtin_constant_p(w) ? __const_hweight64(w) : __arch_hweight64(w))
         |                                                 ^~~~~~~~~~~~~~~~~~~~
   include/asm-generic/bitops/const_hweight.h:21:76: note: expanded from macro '__const_hweight64'
      21 | #define __const_hweight64(w) (__const_hweight32(w) + __const_hweight32((w) >> 32))
         |                                                                            ^  ~~
   include/asm-generic/bitops/const_hweight.h:20:49: note: expanded from macro '__const_hweight32'
      20 | #define __const_hweight32(w) (__const_hweight16(w) + __const_hweight16((w) >> 16))
         |                                                 ^
   include/asm-generic/bitops/const_hweight.h:19:48: note: expanded from macro '__const_hweight16'
      19 | #define __const_hweight16(w) (__const_hweight8(w)  + __const_hweight8((w)  >> 8 ))
         |                                                ^
   include/asm-generic/bitops/const_hweight.h:10:9: note: expanded from macro '__const_hweight8'
      10 |          ((!!((w) & (1ULL << 0))) +     \
         |                ^

Reinette

Hi Reinette,

Quoted 23 lines · Reinette Chatre

Hi Babu,

Quoted 20 lines · Babu Moger

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.

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

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

index 2f3a86278e84..2bc58d974934 100644
@@ -302,6 +302,10 @@ with the following files:
 	memory bandwidth tracking to a single memory bandwidth event per
 	monitoring group.
 

Quoted 7 lines

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

Quoted 2 lines

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

+"available_mbm_cntrs":
+	The number of free monitoring counters available assignment in each domain

"The number of free monitoring counters available assignment" -> "The number of monitoring counters available for assignment"?

(not taking into account how text may change after addressing Peter's feedback)

How about this?

"The number of monitoring counters available for assignment in each domain when the architecture supports mbm_cntr_assign mode. There are a total of "num_mbm_cntrs" counters are available for assignment. Counters can be assigned or unassigned individually in each domain. A counter is available for new assignment if it is unassigned in all domains."

Hi Babu,

Quoted 34 lines · Babu

Hi Reinette,

Quoted 23 lines · Reinette Chatre

Hi Babu,

Quoted 20 lines · Babu Moger

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.

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

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

index 2f3a86278e84..2bc58d974934 100644
@@ -302,6 +302,10 @@ with the following files:
  	memory bandwidth tracking to a single memory bandwidth event per
  	monitoring group.
 

Quoted 7 lines

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

Quoted 2 lines

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

+"available_mbm_cntrs":
+	The number of free monitoring counters available assignment in each domain

"The number of free monitoring counters available assignment" -> "The number of monitoring counters available for assignment"?

(not taking into account how text may change after addressing Peter's feedback)

Quoted 7 lines

How about this?

"The number of monitoring counters available for assignment in each domain when the architecture supports mbm_cntr_assign mode. There are a total of "num_mbm_cntrs" counters are available for assignment. Counters can be assigned or unassigned individually in each domain. A counter is available for new assignment if it is unassigned in all domains."

Please consider the context of this paragraph. It follows right after the description of "num_mbm_cntrs" that states "Up to two counters can be assigned per monitoring group". I think it is confusing to follow that with a paragraph that states "Counters can be assigned or unassigned individually in each domain." I wonder if it may be helpful to use a different term ... for example a counter is *assigned* to an event of a monitoring group but this assignment may be to specified (not yet supported) or all (this work) domains while it is only *programmed*/*activated* to specified domains. Of course, all of this documentation needs to remain coherent if future work decides to indeed support per-domain assignment.

Hi Reinette,

Quoted 44 lines · Reinette Chatre

Hi Babu,

Quoted 33 lines · Babu

Hi Reinette,

Quoted 28 lines · Reinette Chatre

Hi Babu,

Quoted 17 lines · Babu Moger

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.
---
  Documentation/arch/x86/resctrl.rst     |  4 ++++ arch/x86/kernel/cpu/resctrl/monitor.c  |  1 + arch/x86/kernel/cpu/resctrl/rdtgroup.c | 33 ++++++++++++++++++++++++++

  3 files changed, 38 insertions(+)

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

index 2f3a86278e84..2bc58d974934 100644
@@ -302,6 +302,10 @@ with the following files:

Quoted 4 lines

      memory bandwidth tracking to a single memory bandwidth event per monitoring group.
  +"available_mbm_cntrs":
+    The number of free monitoring counters available assignment in each domain

"The number of free monitoring counters available assignment" -> "The number of monitoring counters available for assignment"?

Quoted 1 line

(not taking into account how text may change after addressing Peter's feedback)

Quoted 7 lines

How about this?

"The number of monitoring counters available for assignment in each domain when the architecture supports mbm_cntr_assign mode. There are a total of "num_mbm_cntrs" counters are available for assignment. Counters can be assigned or unassigned individually in each domain. A counter is available for new assignment if it is unassigned in all domains."

Quoted 9 lines

Please consider the context of this paragraph. It follows right after the description of "num_mbm_cntrs" that states "Up to two counters can be assigned per monitoring group". I think it is confusing to follow that with a paragraph that states "Counters can be assigned or unassigned individually in each domain." I wonder if it may be helpful to use a different term ... for example a counter is *assigned* to an event of a monitoring group but this assignment may be to specified (not yet supported) or all (this work) domains while it is only *programmed*/*activated* to specified domains. Of course, all of this documentation needs to remain coherent if future work decides to indeed support per-domain assignment.

Little bit lost here. Please help me.

"available_mbm_cntrs":
"The number of monitoring counters available for assignment in each domain when the architecture supports "mbm_cntr_assign" mode. There are a total of "num_mbm_cntrs" counters are available for assignment. A counter is assigned to an event within a monitoring group and is available for activation across all domains. Users have the flexibility to activate it selectively within specific domains."

Hi Babu,

Quoted 57 lines · Babu

Hi Reinette,

Quoted 44 lines · Reinette Chatre

Hi Babu,

Quoted 33 lines · Babu

Hi Reinette,

Quoted 28 lines · Reinette Chatre

Hi Babu,

Quoted 17 lines · Babu Moger

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.

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

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

index 2f3a86278e84..2bc58d974934 100644

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

Quoted 4 lines

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

@@ -302,6 +302,10 @@ with the following files:
       memory bandwidth tracking to a single memory bandwidth event per
       monitoring group.
   +"available_mbm_cntrs":
+    The number of free monitoring counters available assignment in each domain

"The number of free monitoring counters available assignment" -> "The number of monitoring counters available for assignment"?

Quoted 1 line

(not taking into account how text may change after addressing Peter's feedback)

Quoted 7 lines

How about this?

"The number of monitoring counters available for assignment in each domain when the architecture supports mbm_cntr_assign mode. There are a total of "num_mbm_cntrs" counters are available for assignment. Counters can be assigned or unassigned individually in each domain. A counter is available for new assignment if it is unassigned in all domains."

Quoted 9 lines

Please consider the context of this paragraph. It follows right after the description of "num_mbm_cntrs" that states "Up to two counters can be assigned per monitoring group". I think it is confusing to follow that with a paragraph that states "Counters can be assigned or unassigned individually in each domain." I wonder if it may be helpful to use a different term ... for example a counter is *assigned* to an event of a monitoring group but this assignment may be to specified (not yet supported) or all (this work) domains while it is only *programmed*/*activated* to specified domains. Of course, all of this documentation needs to remain coherent if future work decides to indeed support per-domain assignment.

Quoted 1 line

Little bit lost here. Please help me.

I think this highlights the uncertainty this interface brings. How do you expect users to use this interface? At this time I think this interface can create a lot of confusion. For example, consider a hypothetical system with three domains and four counters that has the following state per mbm_assign_control:

//0=tl;1=_;2=l #default group uses counters 0 and 1 to monitor total and local MBM /m1/0=_;1=t;2=t #monitor group m1 uses counter 2, just for total MBM /m2/0=l;1=_;2=l #monitor group m2 uses counter 3, just for local MBM /m3/0=_;1=_;2=_

Since, in this system there are only four counters available, and they have all been assigned, then there are no new counters available for assignment.

If I understand correctly, available_mbm_cntrs will read:
0=1;1=3;2=1

How is a user to interpret the above numbers? It does not reflect that no counter can be assigned to m3, instead it reflects which of the already assigned counters still need to be activated on domains. If, for example, a user is expected to use this file to know how many counters can still be assigned, should it not reflect the actual available counters. In the above scenario it will then be:
0=0;1=0;2=0

Of course, when doing the above the user may get impression that a counter that has already been assigned, just not activated, is no longer available for use.

Quoted 4 lines

"available_mbm_cntrs":
"The number of monitoring counters available for assignment in each domain when the architecture supports "mbm_cntr_assign" mode. There are a total of "num_mbm_cntrs" counters are available for assignment. A counter is assigned to an event within a monitoring group and is available for activation across all domains. Users have the flexibility to activate it selectively within specific domains."

Once we understand how users are to use this file the documentation should be easier to create.

Reinette

Quoted 62 lines · Reinette Chatre

Quoted 49 lines · Babu

Quoted 38 lines · Reinette Chatre

Quoted 29 lines · Babu

Quoted 26 lines · Reinette Chatre

Quoted 17 lines · Babu Moger

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.
---
   Documentation/arch/x86/resctrl.rst     |  4 ++++ arch/x86/kernel/cpu/resctrl/monitor.c  |  1 + arch/x86/kernel/cpu/resctrl/rdtgroup.c | 33 ++++++++++++++++++++++++++

   3 files changed, 38 insertions(+)

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

index 2f3a86278e84..2bc58d974934 100644
@@ -302,6 +302,10 @@ with the following files:

Quoted 4 lines

       memory bandwidth tracking to a single memory bandwidth event per monitoring group.
   +"available_mbm_cntrs":
+    The number of free monitoring counters available assignment in each domain

"The number of free monitoring counters available assignment" -> "The number of monitoring counters available for assignment"?

Quoted 1 line

(not taking into account how text may change after addressing Peter's feedback)

Quoted 7 lines

How about this?

"The number of monitoring counters available for assignment in each domain when the architecture supports mbm_cntr_assign mode. There are a total of "num_mbm_cntrs" counters are available for assignment. Counters can be assigned or unassigned individually in each domain. A counter is available for new assignment if it is unassigned in all domains."

Quoted 9 lines

Please consider the context of this paragraph. It follows right after the description of "num_mbm_cntrs" that states "Up to two counters can be assigned per monitoring group". I think it is confusing to follow that with a paragraph that states "Counters can be assigned or unassigned individually in each domain." I wonder if it may be helpful to use a different term ... for example a counter is *assigned* to an event of a monitoring group but this assignment may be to specified (not yet supported) or all (this work) domains while it is only *programmed*/*activated* to specified domains. Of course, all of this documentation needs to remain coherent if future work decides to indeed support per-domain assignment.

Quoted 1 line

Little bit lost here. Please help me.

I think this highlights the uncertainty this interface brings. How do you expect users to use this interface? At this time I think this interface can create a lot of confusion. For example, consider a hypothetical system with three domains and four counters that has the following state per mbm_assign_control:

//0=tl;1=_;2=l #default group uses counters 0 and 1 to monitor total and local MBM /m1/0=_;1=t;2=t #monitor group m1 uses counter 2, just for total MBM /m2/0=l;1=_;2=l #monitor group m2 uses counter 3, just for local MBM /m3/0=_;1=_;2=_

Quoted 6 lines

Since, in this system there are only four counters available, and they have all been assigned, then there are no new counters available for assignment.

If I understand correctly, available_mbm_cntrs will read:
0=1;1=3;2=1

Yes. Exactly. This causes confusion to the user.

Quoted 8 lines

How is a user to interpret the above numbers? It does not reflect that no counter can be assigned to m3, instead it reflects which of the already assigned counters still need to be activated on domains. If, for example, a user is expected to use this file to know how many counters can still be assigned, should it not reflect the actual available counters. In the above scenario it will then be:
0=0;1=0;2=0

We can also just print
#cat available_mbm_cntrs
0

The domain specific information is not important here.
That was my original idea. We can go back to that definition. That is more clear to the user.

Hi Babu,

Quoted 84 lines · Babu

Quoted 62 lines · Reinette Chatre

Quoted 49 lines · Babu

Quoted 38 lines · Reinette Chatre

Quoted 29 lines · Babu

Quoted 26 lines · Reinette Chatre

Quoted 17 lines · Babu Moger

Provide the interface to display the number of free monitoring counters available for assignment in each doamin when mbm_cntr_assign is supported.

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

---
v9: New patch.

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

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

index 2f3a86278e84..2bc58d974934 100644

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

Quoted 4 lines

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

@@ -302,6 +302,10 @@ with the following files:
        memory bandwidth tracking to a single memory bandwidth event per
        monitoring group.
    +"available_mbm_cntrs":
+    The number of free monitoring counters available assignment in each domain

"The number of free monitoring counters available assignment" -> "The number of monitoring counters available for assignment"?

Quoted 1 line

(not taking into account how text may change after addressing Peter's feedback)

Quoted 7 lines

How about this?

"The number of monitoring counters available for assignment in each domain when the architecture supports mbm_cntr_assign mode. There are a total of "num_mbm_cntrs" counters are available for assignment. Counters can be assigned or unassigned individually in each domain. A counter is available for new assignment if it is unassigned in all domains."

Quoted 9 lines

Please consider the context of this paragraph. It follows right after the description of "num_mbm_cntrs" that states "Up to two counters can be assigned per monitoring group". I think it is confusing to follow that with a paragraph that states "Counters can be assigned or unassigned individually in each domain." I wonder if it may be helpful to use a different term ... for example a counter is *assigned* to an event of a monitoring group but this assignment may be to specified (not yet supported) or all (this work) domains while it is only *programmed*/*activated* to specified domains. Of course, all of this documentation needs to remain coherent if future work decides to indeed support per-domain assignment.

Quoted 1 line

Little bit lost here. Please help me.

I think this highlights the uncertainty this interface brings. How do you expect users to use this interface? At this time I think this interface can create a lot of confusion. For example, consider a hypothetical system with three domains and four counters that has the following state per mbm_assign_control:

//0=tl;1=_;2=l #default group uses counters 0 and 1 to monitor total and local MBM /m1/0=_;1=t;2=t #monitor group m1 uses counter 2, just for total MBM /m2/0=l;1=_;2=l #monitor group m2 uses counter 3, just for local MBM /m3/0=_;1=_;2=_

Quoted 6 lines

Since, in this system there are only four counters available, and they have all been assigned, then there are no new counters available for assignment.

If I understand correctly, available_mbm_cntrs will read:
0=1;1=3;2=1

Yes. Exactly. This causes confusion to the user.

Quoted 8 lines

How is a user to interpret the above numbers? It does not reflect that no counter can be assigned to m3, instead it reflects which of the already assigned counters still need to be activated on domains. If, for example, a user is expected to use this file to know how many counters can still be assigned, should it not reflect the actual available counters. In the above scenario it will then be:
0=0;1=0;2=0

We can also just print
#cat available_mbm_cntrs
0