x86,fs/resctrl: Keep default MBM mode at boot and fix ABMC programming
Subsystems (4)
Babu Moger
reviewerv4 cover x86,fs/resctrl: Keep default MBM mode at boot and fix ABMC programming
Hi All,
This series restores default MBM assignment mode at boot, assigns counters when switching to mbm_event, and fixes ABMC programming for more than 32 counters.
Commit 0f1576e43adc ("x86/resctrl: Configure mbm_event mode if supported") enabled ABMC at boot. That breaks the pqos tool from intel-cmt-cat [1], which treats a non-numeric event read as zero bandwidth. pqos creates 16 or more groups and uses two counters per group. On a platform with 32 counters per domain, that consumes the pool, so further groups read "Unassigned" and pqos reports 0 MB/s.
Leaving the mode at default does not make pqos correct. pqos still treats "Unavailable" as zero. Once the default-mode counter pool is exceeded, hardware can re-allocate a counter between reads and pqos can report that as a wraparound. That shows up only after the pool is exceeded. Default mode uses one counter per monitoring group. On existing AMD platforms that pool is 64 counters and may be larger on newer hardware, so common deployments stay within it. mbm_event mode uses two counters per group, so the same 32 counters cover only 16 groups.
mbm_event remains available as an opt-in. Users that need stable measurements beyond the default-mode pool should enable it and rotate assignments. The limitation of default mode is documented.
Patch 1: widen cntr_id in L3_QOS_ABMC_CFG from 5 bits to 12 bits, and limit the enumerated counter count to 4096, which is what that field can encode. The APM [2] at [3] will be updated for the expanded cntr_id field.
Patch 2: when switching to mbm_event, assign counters to existing groups, including the default group, matching mkdir auto-assignment.
Patch 3: leave mbm_assign_mode as "default" at boot. This seems to be the patch that has generated the most discussion. Let me know, if additional justification in the documentation would be helpful, I'd be happy to add it.
Based on tip/master:
6b27d3127c05 Merge branch into tip/master: 'x86/tdx'
[1] https://github.com/intel/intel-cmt-cat/issues/311
[2] AMD64 Architecture Programmer's Manual Volume 2: System Programming, Publication #24593, Revision 3.41, Section 19.3.3.3 "Assignable Bandwidth Monitoring (ABMC)"
[3] https://bugzilla.kernel.org/show_bug.cgi?id=206537
v4:
- Patch 1: Updated changelog(from Reinette's input).
- Patch 2: Changelog update (Reinette's input).
Function comment update for rdtgroup_assign_cntrs(). Tag order update.
- Patch 3: Introduced mbm_event counter pool and default active counter pool. Removed duplicate texts in resctrl.rst.
Re-arranged the tag order.
v3:
- Patch 1: drop the 32-bit u64 change and the bw_src widening. Cap the enumerated counter count at 4096 instead of truncating the CPUID field.
- Patch 2: clean up comments. Add Reported-by, Closes, and Cc: stable.
- Patch 3: document one counter per group in default mode and two counters per group in mbm_event mode. Describe the default-mode pool and the pqos wraparound once that pool is exceeded.
v2:
- Added patch 2 to address Sashiko's comment regarding the documentation issue. In fact, it exposed a real issue. When switching to mbm_event mode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
- Combine the two v1 patches and add patch 2 (assign existing groups on mbm_event switch; Sashiko review).
- Keep boot-default separate from the encoding/truncation fixes.
- Document the default-mode counter-pool limitation.
Previous versions:
v1: https://lore.kernel.org/lkml/980f39d3a0e0d9f73925e362f835aeef070a1bc5.1784322818.git.babu.moger@amd.com/
v2: https://lore.kernel.org/lkml/[email protected]/
v3: https://lore.kernel.org/lkml/[email protected]/
Thanks
Babu Moger
Babu Moger (3):
x86/resctrl: Fix ABMC counter programming
fs/resctrl: Assign counters to existing groups when enabling mbm_event x86,fs/resctrl: Keep mbm_assign_mode in default mode at boot
Documentation/filesystems/resctrl.rst | 50 +++++++++++++++++--------- arch/x86/kernel/cpu/resctrl/internal.h | 4 +-- arch/x86/kernel/cpu/resctrl/monitor.c | 4 +-- fs/resctrl/monitor.c | 47 +++++++++++++++++++----- 4 files changed, 75 insertions(+), 30 deletions(-)
Signature · 1 line
2.43.0
Babu Moger
reviewerPATCH v4 1/3 x86/resctrl: Fix ABMC counter programming
AMD's Assignable Bandwidth Monitoring Counters (ABMC) are configured via MSR_IA32_L3_QOS_ABMC_CFG; MSR_IA32_L3_QOS_ABMC_CFG.cntr_id selects the counter the configuration applies to. The number of counters a platform supports (the number of possible values written to MSR_IA32_L3_QOS_ABMC_CFG.cntr_id) is enumerated separately via CPUID.
On platforms that enumerate more than 32 counters, the current 5-bit encoding truncates the counter ID and misprograms ABMC.
The AMD64 Architecture Programmer's Manual [1], available from [2], has been updated to widen MSR_IA32_L3_QOS_ABMC_CFG.cntr_id from 5 bits to 12 bits (the published revision 3.41 does not yet reflect this; a future revision will). The CPUID enumeration reports the maximum counter ID in a 16-bit field and may therefore report more counters than a 12-bit MSR_IA32_L3_QOS_ABMC_CFG.cntr_id can address.
Widen MSR_IA32_L3_QOS_ABMC_CFG.cntr_id to 12 bits to match the architecture. Cap the enumerated counter count at BIT(12) so every counter ID used by resctrl can be written to MSR_IA32_L3_QOS_ABMC_CFG.cntr_id without truncation.
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming, Publication #24593, Revision 3.41, Section 19.3.3.3 "Assignable Bandwidth Monitoring (ABMC)"
---
v4: Changelog update(from Reinette's input).
v3: Dropped the fix for truncation on 32-bit x86.
Removed the change bw_src field(RMID) width to 15 bits. Added new check to limit the number of counters to 12 bits.
v2: Moved the link tag to the last.
--- arch/x86/kernel/cpu/resctrl/internal.h | 4 ++-- arch/x86/kernel/cpu/resctrl/monitor.c | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-)
arch/x86/kernel/cpu/resctrl/internal.h +2 -2
@@ -214,8 +214,8 @@ union l3_qos_abmc_cfg { bw_src :12, reserved1: 3, is_clos : 1, - cntr_id : 5, - reserved : 9, + cntr_id :12, + reserved : 2, cntr_en : 1, cfg_en : 1; } split;
arch/x86/kernel/cpu/resctrl/monitor.c +2 -1
@@ -470,7 +470,8 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r) r->mon.mbm_cntr_assignable = true; r->mon.mbm_cntr_configurable = true;cpuid_count (0x80000020 ,5 , &eax, &ebx, &ecx, &edx); - r->mon.num_mbm_cntrs = (ebx &GENMASK (15 ,0 )) +1 ; +/* cntr_id is 12 bits and can only encode 4096 counters. */ + r->mon.num_mbm_cntrs =min ((ebx &GENMASK (15 ,0 )) +1 ,BIT (12 )); hw_res->mbm_cntr_assign_enabled = true; }
Signature · 1 line
2.43.0
Re: [PATCH v4 1/3] x86/resctrl: Fix ABMC counter programming
Hi Babu,
Quoted 25 lines · Babu Moger
AMD's Assignable Bandwidth Monitoring Counters (ABMC) are configured via
MSR_IA32_L3_QOS_ABMC_CFG; MSR_IA32_L3_QOS_ABMC_CFG.cntr_id selects the counter the configuration applies to. The number of counters a platform supports (the number of possible values written to MSR_IA32_L3_QOS_ABMC_CFG.cntr_id) is enumerated separately via CPUID.On platforms that enumerate more than 32 counters, the current 5-bit encoding truncates the counter ID and misprograms ABMC.
The AMD64 Architecture Programmer's Manual [1], available from [2], has been updated to widen MSR_IA32_L3_QOS_ABMC_CFG.cntr_id from 5 bits to 12 bits (the published revision 3.41 does not yet reflect this; a future revision will). The CPUID enumeration reports the maximum counter ID in a 16-bit field and may therefore report more counters than a 12-bit MSR_IA32_L3_QOS_ABMC_CFG.cntr_id can address.
Widen MSR_IA32_L3_QOS_ABMC_CFG.cntr_id to 12 bits to match the architecture. Cap the enumerated counter count at BIT(12) so every counter ID used by resctrl can be written to MSR_IA32_L3_QOS_ABMC_CFG.cntr_id without truncation.
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming, Publication #24593, Revision 3.41, Section 19.3.3.3 "Assignable Bandwidth Monitoring (ABMC)"
Quoted 5 lines
Fixes 84ecefb76674 ("x86/resctrl: Add data structures and definitions for ABMC assignment")Signed-off-by Babu Moger <[email protected]>---
Reinette
Re: [PATCH v4 1/3] x86/resctrl: Fix ABMC counter programming
Hi Reinette,
Thanks for quick review.
Quoted 28 lines · Reinette Chatre
Hi Babu,
Quoted 25 lines · Babu Moger
AMD's Assignable Bandwidth Monitoring Counters (ABMC) are configured via
MSR_IA32_L3_QOS_ABMC_CFG; MSR_IA32_L3_QOS_ABMC_CFG.cntr_id selects the counter the configuration applies to. The number of counters a platform supports (the number of possible values written to MSR_IA32_L3_QOS_ABMC_CFG.cntr_id) is enumerated separately via CPUID.On platforms that enumerate more than 32 counters, the current 5-bit encoding truncates the counter ID and misprograms ABMC.
The AMD64 Architecture Programmer's Manual [1], available from [2], has been updated to widen MSR_IA32_L3_QOS_ABMC_CFG.cntr_id from 5 bits to 12 bits (the published revision 3.41 does not yet reflect this; a future revision will). The CPUID enumeration reports the maximum counter ID in a 16-bit field and may therefore report more counters than a 12-bit MSR_IA32_L3_QOS_ABMC_CFG.cntr_id can address.
Widen MSR_IA32_L3_QOS_ABMC_CFG.cntr_id to 12 bits to match the architecture. Cap the enumerated counter count at BIT(12) so every counter ID used by resctrl can be written to MSR_IA32_L3_QOS_ABMC_CFG.cntr_id without truncation.
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming, Publication #24593, Revision 3.41, Section 19.3.3.3 "Assignable Bandwidth Monitoring (ABMC)"
Quoted 8 lines
Quoted 5 lines
Fixes 84ecefb76674 ("x86/resctrl: Add data structures and definitions for ABMC assignment")Signed-off-by Babu Moger <[email protected]>---
Reviewed-by Reinette Chatre <[email protected]>
Thanks
Babu
Babu Moger
reviewerPATCH v4 2/3 fs/resctrl: Assign counters to existing groups when enabling mbm_event
In reply to Babu MogerWhen the user switches counter assignment mode by writing "mbm_event" to /sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space as mbm_assign_on_mkdir.
Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing CTRL_MON and its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise the rdtgroup_assign_cntrs() function comments to account for the new callers.
---
v4: Changelog update (Reinette's input).
Function comment update for rdtgroup_assign_cntrs().
v3: Changelog update.
Code comment cleanup.
Added Cc to stable.
Added Reported-by and Closes tags.
v2: New patch.
This patch addresses the Sashiko comment about documentation issue where counters are not assigned automatically when mode is switched to mbm_event.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
In fact, it exposed a real issue. When switching to mbm_event mode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.
--- Documentation/filesystems/resctrl.rst | 7 ++-- fs/resctrl/monitor.c | 47 ++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 11 deletions(-)
Documentation/filesystems/resctrl.rst +5 -2
@@ -370,8 +370,11 @@ with the following files: of counters available is described in the "num_mbm_cntrs" file. Changing the mode may cause all counters on the resource to reset. - Moving to mbm_event counter assignment mode requires users to assign the counters - to the events. Otherwise, the MBM event counters will return 'Unassigned' when read. + Moving to mbm_event counter assignment mode enables "mbm_assign_on_mkdir" and + assigns counters to the events of all existing monitoring groups, including + the default group, while counters remain available. Consult + "mbm_L3_assignments" after switching to "mbm_event" mode for counter + assignment states of all monitoring groups. The mode is beneficial for AMD platforms that support more CTRL_MON and MON groups than available hardware counters. By default, this
fs/resctrl/monitor.c +38 -9
73 lines not on this page · raw message
Signature · 1 line
2.43.0
[email protected]
AI reviewRe: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [High] x86/resctrl: MBM counter leak and hardware misconfiguration for pseudo-locked groups --
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] x86/resctrl: MBM counter leak and hardware misconfiguration for pseudo-locked groups --
commit b647b3c26df799d4d9b84cff6fe64eee2efa36ad
Author: Babu Moger <[email protected]>
fs/resctrl: Assign counters to existing groups when enabling mbm_event
When switching to mbm_event counter assignment mode, existing monitoring groups are not automatically assigned counters, leaving them unassigned. This commit walks every existing group and assigns available counters to match the auto-assignment behavior that occurs for new groups.
Quoted 4 lines
fs/resctrl/monitor.c +0
-0
[ ... ]
Quoted 4 lines
fs/resctrl/monitor.c +0
-0@@ -1272,6 +1275,22 @@ void rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp) &mon_event_all[QOS_L3_MBM_LOCAL_EVENT_ID]); }
fs/resctrl/monitor.c void rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp) · line 1287
Quoted 10 lines
fs/resctrl/monitor.c +10
-0+/* +* resctrl_assign_cntrs_allrdtgrp() - Assign counters to the MBM events of +* every existing group. +*/ +static void resctrl_assign_cntrs_allrdtgrp (void ) +{ +struct rdtgroup *prgrp, *crgrp; + +list_for_each_entry (prgrp, &rdt_all_groups, rdtgroup_list) { +rdtgroup_assign_cntrs (prgrp);
[Severity: High]
Does this code inadvertently leak hardware counters to pseudo-locked groups?
When a user creates a control group and transitions it to pseudo-locked mode, its rmid and closid are explicitly freed. If 'mbm_event' assignment mode is then enabled globally, this traversal loop captures those pseudo-locked groups because they are retained in rdt_all_groups.
Since rdtgroup_assign_cntrs() lacks a check forrdtgrp->mode == RDT_MODE_PSEUDO_LOCKED, the counter allocation proceeds.
Could this cause rdtgroup_alloc_assign_cntr() to program the hardware using the stale, freed rmid and closid of the pseudo-locked group? This appears to permanently leak scarce hardware counters and potentially corrupt the hardware monitoring state of other active groups that have been re-allocated those IDs.
Quoted 5 lines
fs/resctrl/monitor.c +5
-0+ +list_for_each_entry (crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) +rdtgroup_assign_cntrs (crgrp); + } +}
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Reinette Chatre
maintainerRe: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
In reply to Babu MogerHi Babu,
Quoted 5 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.
Quoted 8 lines
Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be
nit: Related to more detailed comments later the above "that auto-assignment would have produced" may not always be accurate. If the patch behavior is maintained (more later) then one way around this may be to just instead say "that auto-assignment could have produced".
Quoted 12 lines
fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise the
rdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
...
Quoted 10 lines
--- Documentation/filesystems/resctrl.rst | 7 ++-- fs/resctrl/monitor.c | 47 ++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 11 deletions(-)Documentation/filesystems/resctrl.rst +0
-0
Documentation/filesystems/resctrl.rst with the following files: · line 375
Quoted 8 lines
Documentation/filesystems/resctrl.rst +3
-2@@ -370,8 +370,11 @@ with the following files: of counters available is described in the "num_mbm_cntrs" file. Changing the mode may cause all counters on the resource to reset. - Moving to mbm_event counter assignment mode requires users to assign the counters - to the events. Otherwise, the MBM event counters will return 'Unassigned' when read. + Moving to mbm_event counter assignment mode enables "mbm_assign_on_mkdir" and + assigns counters to the events of all existing monitoring groups, including + the default group, while counters remain available. Consult
This also adds the "including the default group" caveat that I will refer to later.
Quoted 10 lines
Documentation/filesystems/resctrl.rst +2
-0+ "mbm_L3_assignments" after switching to "mbm_event" mode for counter + assignment states of all monitoring groups. The mode is beneficial for AMD platforms that support more CTRL_MON and MON groups than available hardware counters. By default, thisfs/resctrl/monitor.c +0
-0
fs/resctrl/monitor.c static int rdtgroup_assign_cntr_event(struct rdt_l3_mon_domain *d, struct rdtgro · line 1306
Quoted 7 lines
fs/resctrl/monitor.c +2
-2@@ -1302,14 +1302,17 @@ static int rdtgroup_assign_cntr_event(struct rdt_l3_mon_domain *d, struct rdtgro }/* -* rdtgroup_assign_cntrs() - Assign counters to MBM events. Called when -* a new group is created. +* rdtgroup_assign_cntrs() - Assign counters to enabled MBM events of a +* resource group
nit: please fix alignment. This patch inconsistently uses tabs vs spaces for this alignment. That is not major since this file itself is not consistent in this regard, but the file does consistently align the description irrespective of character used.
Quoted 50 lines
fs/resctrl/monitor.c +23
-7* * Each group can accommodate two counters per domain: one for the total -* event and one for the local event. Assignments may fail due to the limited -* number of counters. However, it is not necessary to fail the group creation -* and thus no failure is returned. Users have the option to modify the -* counter assignments after the group has been created. +* event and one for the local event. Assignments may fail due to the +* limited number of counters. +* +* Intended to be called in auto assignment scenarios ("mbm_assign_on_mkdir" +* enabled in "mbm_event" counter assignment mode) where counter assignment +* is not required to succeed. Thus no failure is returned. Users can modify +* the counter assignments afterwards if needed. */ void rdtgroup_assign_cntrs (struct rdtgroup *rdtgrp) {@@ -1328,6 +1331,22 @@ void rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp) &mon_event_all[QOS_L3_MBM_LOCAL_EVENT_ID]); } +/* +* resctrl_assign_cntrs_allrdtgrp() - Assign counters to the MBM events of +* every existing group. +*/ +static void resctrl_assign_cntrs_allrdtgrp (void ) +{ +struct rdtgroup *prgrp, *crgrp; + +list_for_each_entry (prgrp, &rdt_all_groups, rdtgroup_list) { +rdtgroup_assign_cntrs (prgrp); + +list_for_each_entry (crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) +rdtgroup_assign_cntrs (crgrp); + } +} +/* * rdtgroup_free_unassign_cntr() - Unassign and reset the counter ID configuration * for the event pointed to by @mevt within the domain @d and resctrl group @rdtgrp. @@ -1601,9 +1620,6 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, (READS_TO_LOCAL_MEM |READS_TO_LOCAL_S_MEM |NON_TEMP_WRITE_TO_LOCAL_MEM ); -/* Enable auto assignment when switching to "mbm_event" mode */ -if (enable) - r->mon.mbm_assign_on_mkdir = true;/* * Reset all the non-achitectural RMID state and assignable counters. */
fs/resctrl/monitor.c ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1635
Quoted 9 lines
fs/resctrl/monitor.c +6
-0@@ -1611,6 +1627,19 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,mbm_cntr_free_all (r, d);resctrl_reset_rmid_all (r, d); } + +/* +* Counters were freed above, so both new groups (via mkdir) and +* the groups that already exist need assignments. Groups created +* while in "default" mode have no counter assigned, including the +* default group created when resctrl is mounted. Assign counters
This patch refers three times to how "default group" receives counters during auto assignment.
How important is it that the default group is assigned counters when switching to "mbm_event" mode?
This patch implies that auto assignment will always include the default group but that cannot be guaranteed since the default group is always at the end of the rdt_all_groups list. This means that when traversing rdt_all_groups in resctrl_assign_cntrs_allrdtgrp(), whether default group receives counters would depend on how many other CTRL_MON and MON groups there are.
fs/resctrl/monitor.c ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1637
Quoted 2 lines
fs/resctrl/monitor.c +2
-0+* to them so that enabling the mode leaves the same assignments +* that mkdir would have made.
Related to previous comment is that the changelog mentions "same per-group state that auto-assignment would have produced" that repeats a similat claim made above that states "same assignments that mkdir would have made". This behavior assumes that a CTRL_MON group is created with all its MON groups before the next CTRL_MON group is created *and* that enough counters are available for all groups. If there are not enough counters then the most recent CTRL_MON groups will receive counters first potentially leaving the first "mkdir" created CTRL_MON groups without counters.
It is not clear to me whether default group requires counters. Separately, I do not see whether resctrl can or should support the same ordering as what "mkdir" would have produced. If these are not actually requirements then the patch is fine but the comments, documentation change, and changelog need to change to match the code.
If the default group does require counters then we either need to change how groups are added to rdt_all_groups or how resctrl_assign_cntrs_allrdtgrp() traverses the list.
fs/resctrl/monitor.c ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1645
Quoted 8 lines
fs/resctrl/monitor.c +5
-0+*/ +if (enable) { + r->mon.mbm_assign_on_mkdir = true; +resctrl_assign_cntrs_allrdtgrp (); + } } out_unlock:
Reinette
Re: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Reinette,
Quoted 13 lines · Reinette Chatre
Hi Babu,
Quoted 10 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Quoted 8 lines
Quoted 3 lines
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may benit: Related to more detailed comments later the above "that auto-assignment would have produced" may not always be accurate. If the patch behavior is maintained (more later) then one way around this may be to just instead say "that auto-assignment could have produced".
Ack.
Quoted 29 lines
Quoted 13 lines
fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise the
rdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
...
Quoted 13 lines
--- Documentation/filesystems/resctrl.rst | 7 ++-- fs/resctrl/monitor.c | 47 ++++++++++++++++++++++----- 2 files changed, 43 insertions(+), 11 deletions(-)Documentation/filesystems/resctrl.rst +0
-0@@ -370,8 +370,11 @@ with the following files: of counters available is described in the "num_mbm_cntrs" file. Changing the mode may cause all counters on the resource to reset.
Quoted 7 lines
Documentation/filesystems/resctrl.rst with the following files: · line 375
Quoted 5 lines
Documentation/filesystems/resctrl.rst +3
-2- Moving to mbm_event counter assignment mode requires users to assign the counters - to the events. Otherwise, the MBM event counters will return 'Unassigned' when read. + Moving to mbm_event counter assignment mode enables "mbm_assign_on_mkdir" and + assigns counters to the events of all existing monitoring groups, including + the default group, while counters remain available. ConsultThis also adds the "including the default group" caveat that I will refer to later.
ack
Quoted 13 lines
Quoted 13 lines
Documentation/filesystems/resctrl.rst +2
-0+ "mbm_L3_assignments" after switching to "mbm_event" mode for counter + assignment states of all monitoring groups. The mode is beneficial for AMD platforms that support more CTRL_MON and MON groups than available hardware counters. By default, thisfs/resctrl/monitor.c +0
-0@@ -1302,14 +1302,17 @@ static int rdtgroup_assign_cntr_event(struct rdt_l3_mon_domain *d, struct rdtgro }
Quoted 10 lines
fs/resctrl/monitor.c static int rdtgroup_assign_cntr_event(struct rdt_l3_mon_domain *d, struct rdtgro · line 1306
Quoted 5 lines
fs/resctrl/monitor.c +2
-2/* -* rdtgroup_assign_cntrs() - Assign counters to MBM events. Called when -* a new group is created. +* rdtgroup_assign_cntrs() - Assign counters to enabled MBM events of a +* resource group nit: please fix alignment. This patch inconsistently uses tabs vs spaces for this alignment. That is not major since this file itself is not consistent in this regard, but the file does consistently align the description irrespective of character used.
Sure.
Quoted 63 lines
Quoted 51 lines
fs/resctrl/monitor.c +23
-7* * Each group can accommodate two counters per domain: one for the total -* event and one for the local event. Assignments may fail due to the limited -* number of counters. However, it is not necessary to fail the group creation -* and thus no failure is returned. Users have the option to modify the -* counter assignments after the group has been created. +* event and one for the local event. Assignments may fail due to the +* limited number of counters. +* +* Intended to be called in auto assignment scenarios ("mbm_assign_on_mkdir" +* enabled in "mbm_event" counter assignment mode) where counter assignment +* is not required to succeed. Thus no failure is returned. Users can modify +* the counter assignments afterwards if needed. */ void rdtgroup_assign_cntrs (struct rdtgroup *rdtgrp) {@@ -1328,6 +1331,22 @@ void rdtgroup_assign_cntrs(struct rdtgroup *rdtgrp) &mon_event_all[QOS_L3_MBM_LOCAL_EVENT_ID]); } +/* +* resctrl_assign_cntrs_allrdtgrp() - Assign counters to the MBM events of +* every existing group. +*/ +static void resctrl_assign_cntrs_allrdtgrp (void ) +{ +struct rdtgroup *prgrp, *crgrp; + +list_for_each_entry (prgrp, &rdt_all_groups, rdtgroup_list) { +rdtgroup_assign_cntrs (prgrp); + +list_for_each_entry (crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) +rdtgroup_assign_cntrs (crgrp); + } +} +/* * rdtgroup_free_unassign_cntr() - Unassign and reset the counter ID configuration * for the event pointed to by @mevt within the domain @d and resctrl group @rdtgrp. @@ -1601,9 +1620,6 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, (READS_TO_LOCAL_MEM |READS_TO_LOCAL_S_MEM |NON_TEMP_WRITE_TO_LOCAL_MEM ); -/* Enable auto assignment when switching to "mbm_event" mode */ -if (enable) - r->mon.mbm_assign_on_mkdir = true;/* * Reset all the non-achitectural RMID state and assignable counters. */ fs/resctrl/monitor.c ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1635
Quoted 9 lines
fs/resctrl/monitor.c +6
-0@@ -1611,6 +1627,19 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf,mbm_cntr_free_all (r, d);resctrl_reset_rmid_all (r, d); } + +/* +* Counters were freed above, so both new groups (via mkdir) and +* the groups that already exist need assignments. Groups created +* while in "default" mode have no counter assigned, including the +* default group created when resctrl is mounted. Assign counters This patch refers three times to how "default group" receives counters during auto assignment.
Quoted 6 lines
How important is it that the default group is assigned counters when switching to "
mbm_event" mode?This patch implies that auto assignment will always include the default group but that cannot be guaranteed since the default group is always at the end of the
rdt_all_groupslist. This means that when traversingrdt_all_groupsinresctrl_assign_cntrs_allrdtgrp(), whether default group receives counters would depend on how many otherCTRL_MONand MON groups there are.
Good point. I recreated the problem. It is not always guaranteed that the default group gets the counter with the current logic.
Quoted 12 lines
fs/resctrl/monitor.c ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1637
Quoted 4 lines
fs/resctrl/monitor.c +2
-0+* to them so that enabling the mode leaves the same assignments +* that mkdir would have made. Related to previous comment is that the changelog mentions "same per-group state that auto-assignment would have produced" that repeats a similat claim made above that states "same assignments that mkdir would have made". This behavior assumes that a
CTRL_MONgroup is created with all its MON groups before the nextCTRL_MONgroup is created *and* that enough counters are available for all groups. If there are not enough counters then the most recentCTRL_MONgroups will receive counters first potentially leaving the first "mkdir" createdCTRL_MONgroups without counters.
Quoted 7 lines
It is not clear to me whether default group requires counters. Separately, I do not see whether resctrl can or should support the same ordering as what "mkdir" would have produced. If these are not actually requirements then the patch is fine but the comments, documentation change, and changelog need to change to match the code.
If the default group does require counters then we either need to change how groups are added to
rdt_all_groupsor howresctrl_assign_cntrs_allrdtgrp()traverses the list.
We have always treated the default group as a special case. I think we should ensure that the default group gets counters before any other group. How about adding logic to give the default group priority when assigning counters?
How about something like this?
fs/resctrl/monitor.c +3 -1
@@ -1339,8 +1339,10 @@ static void resctrl_assign_cntrs_allrdtgrp(void) {struct rdtgroup *prgrp, *crgrp; +rdtgroup_assign_cntrs (&rdtgroup_default);list_for_each_entry (prgrp, &rdt_all_groups, rdtgroup_list) { -rdtgroup_assign_cntrs (prgrp); +if (prgrp != &rdtgroup_default) +rdtgroup_assign_cntrs (prgrp); list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list,
mon.crdtgrp_list)
rdtgroup_assign_cntrs(crgrp);
Quoted 8 lines
Quoted 8 lines
fs/resctrl/monitor.c +5
-0+*/ +if (enable) { + r->mon.mbm_assign_on_mkdir = true; +resctrl_assign_cntrs_allrdtgrp (); + } }
Quoted 4 lines
fs/resctrl/monitor.c ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1645
Quoted 1 line
fs/resctrl/monitor.c +0
-0out_unlock:Reinette
Thanks
Babu
Reinette Chatre
maintainerRe: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Babu,
Quoted 2 lines · Babu
Quoted 1 line · Reinette Chatre
On 10/9/26 4:44 PM, Babu Moger wrote:
...
Quoted 39 lines
Quoted 13 lines
Review ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1635
Quoted 10 lines
(quoted diff) +6
-0@@ -1611,6 +1627,19 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, mbm_cntr_free_all(r, d); resctrl_reset_rmid_all(r, d); } + + /* + * Counters were freed above, so both new groups (via mkdir) and + * the groups that already exist need assignments. Groups created + * while in "default" mode have no counter assigned, including the + * default group created when resctrl is mounted. Assign countersThis patch refers three times to how "default group" receives counters during auto assignment.
Quoted 6 lines
How important is it that the default group is assigned counters when switching to "
mbm_event" mode?This patch implies that auto assignment will always include the default group but that cannot be guaranteed since the default group is always at the end of the
rdt_all_groupslist. This means that when traversingrdt_all_groupsinresctrl_assign_cntrs_allrdtgrp(), whether default group receives counters would depend on how many otherCTRL_MONand MON groups there are.Good point. I recreated the problem. It is not always guaranteed that the default group gets the counter with the current logic.
Quoted 17 lines
Review ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1637
Quoted 4 lines
(quoted diff) +2
-0+ * to them so that enabling the mode leaves the same assignments + * that mkdir would have made.Related to previous comment is that the changelog mentions "same per-group state that auto-assignment would have produced" that repeats a similat claim made above that states "same assignments that mkdir would have made". This behavior assumes that a
CTRL_MONgroup is created with all its MON groups before the nextCTRL_MONgroup is created *and* that enough counters are available for all groups. If there are not enough counters then the most recentCTRL_MONgroups will receive counters first potentially leaving the first "mkdir" createdCTRL_MONgroups without counters.It is not clear to me whether default group requires counters. Separately, I do not see whether resctrl can or should support the same ordering as what "mkdir" would have produced. If these are not actually requirements then the patch is fine but the comments, documentation change, and changelog need to change to match the code.
Quoted 5 lines
Quoted 2 lines
If the default group does require counters then we either need to change how groups are added to
rdt_all_groupsor howresctrl_assign_cntrs_allrdtgrp()traverses the list.We have always treated the default group as a special case. I think
In general I think the special treatment results from the default group always being present. It is not clear to me that this implies that the default group needs to have counters assigned when switching to mbm_event mode.
Quoted 3 lines
we should ensure that the default group gets counters before any other group. How about adding logic to give the default group priority when assigning counters?
I do not find there to be a strong motivation for this assignment but it should be fine to establish such expectation at this point. Just to confirm, you explicitly only consider the default group and not also the monitoring groups belonging to the default group?
Quoted 10 lines
How about something like this?
fs/resctrl/monitor.c +0
-0@@ -1339,8 +1339,10 @@ static void resctrl_assign_cntrs_allrdtgrp(void) {
fs/resctrl/monitor.c static void resctrl_assign_cntrs_allrdtgrp(void) · line 1348
Quoted 10 lines
fs/resctrl/monitor.c +3
-1struct rdtgroup *prgrp, *crgrp; + rdtgroup_assign_cntrs(&rdtgroup_default); list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) { - rdtgroup_assign_cntrs(prgrp); + if(prgrp != &rdtgroup_default) + rdtgroup_assign_cntrs(prgrp); list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) rdtgroup_assign_cntrs(crgrp);
ok. If you go this route, please add a small comment to highlight why the list traversal is done in this unexpected way. If you do so, please take care with potential duplicate comments between this function and its caller.
I do not have a preference about which groups need counters assigned when switching to mbm_event mode. Please just ensure that the documentation (resctrl.rst, changelog, code comments) match the behavior and explains why.
Thank you very much.
Reinette
Babu Moger
reviewerRe: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Reinette,
Quoted 58 lines · Reinette Chatre
Hi Babu,
Quoted 2 lines · Babu
Quoted 1 line · Reinette Chatre
On 10/9/26 4:44 PM, Babu Moger wrote:
...
Quoted 39 lines
Quoted 13 lines
Review ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1635
Quoted 10 lines
(quoted diff) +6
-0@@ -1611,6 +1627,19 @@ ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, mbm_cntr_free_all(r, d); resctrl_reset_rmid_all(r, d); } + + /* + * Counters were freed above, so both new groups (via mkdir) and + * the groups that already exist need assignments. Groups created + * while in "default" mode have no counter assigned, including the + * default group created when resctrl is mounted. Assign countersThis patch refers three times to how "default group" receives counters during auto assignment.
Quoted 6 lines
How important is it that the default group is assigned counters when switching to "
mbm_event" mode?This patch implies that auto assignment will always include the default group but that cannot be guaranteed since the default group is always at the end of the
rdt_all_groupslist. This means that when traversingrdt_all_groupsinresctrl_assign_cntrs_allrdtgrp(), whether default group receives counters would depend on how many otherCTRL_MONand MON groups there are.Good point. I recreated the problem. It is not always guaranteed that the default group gets the counter with the current logic.
Quoted 17 lines
Review ssize_t resctrl_mbm_assign_mode_write(struct kernfs_open_file *of, char *buf, · line 1637
Quoted 4 lines
(quoted diff) +2
-0+ * to them so that enabling the mode leaves the same assignments + * that mkdir would have made.Related to previous comment is that the changelog mentions "same per-group state that auto-assignment would have produced" that repeats a similat claim made above that states "same assignments that mkdir would have made". This behavior assumes that a
CTRL_MONgroup is created with all its MON groups before the nextCTRL_MONgroup is created *and* that enough counters are available for all groups. If there are not enough counters then the most recentCTRL_MONgroups will receive counters first potentially leaving the first "mkdir" createdCTRL_MONgroups without counters.It is not clear to me whether default group requires counters. Separately, I do not see whether resctrl can or should support the same ordering as what "mkdir" would have produced. If these are not actually requirements then the patch is fine but the comments, documentation change, and changelog need to change to match the code.
Quoted 5 lines
Quoted 2 lines
If the default group does require counters then we either need to change how groups are added to
rdt_all_groupsor howresctrl_assign_cntrs_allrdtgrp()traverses the list.We have always treated the default group as a special case. I think
In general I think the special treatment results from the default group always being present. It is not clear to me that this implies that the default group needs to have counters assigned when switching to
mbm_eventmode.
Quoted 8 lines
Quoted 3 lines
we should ensure that the default group gets counters before any other group. How about adding logic to give the default group priority when assigning counters?
I do not find there to be a strong motivation for this assignment but it should be fine to establish such expectation at this point. Just to confirm, you explicitly only consider the default group and not also the monitoring groups belonging to the default group?
It is just the default group. Not the monitoring group under the default group.
Quoted 17 lines
Quoted 17 lines
How about something like this?
fs/resctrl/monitor.c +3
-1@@ -1339,8 +1339,10 @@ static void resctrl_assign_cntrs_allrdtgrp(void) { struct rdtgroup *prgrp, *crgrp; + rdtgroup_assign_cntrs(&rdtgroup_default); list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) { - rdtgroup_assign_cntrs(prgrp); + if(prgrp != &rdtgroup_default) + rdtgroup_assign_cntrs(prgrp);
Quoted 8 lines
fs/resctrl/monitor.c static void resctrl_assign_cntrs_allrdtgrp(void) · line 1348
Quoted 4 lines
fs/resctrl/monitor.c +0
-0list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list) rdtgroup_assign_cntrs(crgrp);ok. If you go this route, please add a small comment to highlight why the list traversal is done in this unexpected way. If you do so, please take care with potential duplicate comments between this function and its caller.
ok.
Quoted 4 lines
I do not have a preference about which groups need counters assigned when switching to
mbm_eventmode. Please just ensure that the documentation (resctrl.rst, changelog, code comments) match the behavior and explains why.
Ok.
Thanks
Babu
Re: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
In reply to Babu MogerHi Babu,
Quoted 33 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise therdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
v4: Changelog update (Reinette's input).
Function comment update forrdtgroup_assign_cntrs().v3: Changelog update.
Code comment cleanup.
Added Cc to stable.
Added Reported-by and Closes tags.
Quoted 8 lines
v2: New patch.
This patch addresses the Sashiko comment about documentation issue where counters are not assigned automatically when mode is switched tombm_event.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
In fact, it exposed a real issue. When switching tombm_eventmode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.
Given the name of the option is 'mbm_assign_on_mkdir' I expected the auto assignment to only occur on the creation of ctrl groups or monitor groups. That's not my concern though. As you are now treating this as more of a general auto assignment should the assignment also be done when a resctrl domain is brought back online.
For illustration on an MPAM model I see:
/sys/fs/resctrl# cat mbm_L3_assignments
mbm_total_bytes:2=e;3=e
# offline one domain /sys/fs/resctrl# for i in $(seq 0 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:3=e
# bring the domain back online /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:2=_;3=e # Would it be better, or worse, for auto-assignment to occur here?
Thanks,
Ben
Babu Moger
reviewerRe: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Ben,
Quoted 56 lines · Ben Horgan
Hi Babu,
Quoted 33 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise therdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
v4: Changelog update (Reinette's input).
Function comment update forrdtgroup_assign_cntrs().v3: Changelog update.
Code comment cleanup.
Added Cc to stable.
Added Reported-by and Closes tags.Quoted 8 lines
v2: New patch.
This patch addresses the Sashiko comment about documentation issue where counters are not assigned automatically when mode is switched tombm_event.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
In fact, it exposed a real issue. When switching tombm_eventmode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.Given the name of the option is '
mbm_assign_on_mkdir' I expected the auto assignment to only occur on the creation of ctrl groups or monitor groups. That's not my concern though. As you are now treating this as more of a general auto assignment should the assignment also be done when a resctrl domain is brought back online.For illustration on an MPAM model I see:
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=e;3=e
Quoted 9 lines
# offline one domain /sys/fs/resctrl# for i in $(seq 0 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:3=e# bring the domain back online /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:2=_;3=e # Would it be better, or worse, for auto-assignment to occur here?
Interesting.
We don't know the previous state. What if it looked like this before the CPU was taken offline?
/sys/fs/resctrl# cat mbm_L3_assignments
mbm_total_bytes:2=_;3=e
In that case, forcing auto-assignment may not be the right thing to do. Since we don't know the previous state, we can only speculate.
It may be best to leave this decision to the user. Thanks,
Babu
Reinette Chatre
maintainerRe: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Babu,
Quoted 68 lines · Babu Moger
Quoted 54 lines · Ben Horgan
Quoted 33 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise therdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
v4: Changelog update (Reinette's input). Function comment update forrdtgroup_assign_cntrs().v3: Changelog update.
Code comment cleanup.
Added Cc to stable.
Added Reported-by and Closes tags.Quoted 8 lines
v2: New patch.
This patch addresses the Sashiko comment about documentation issue where counters are not assigned automatically when mode is switched tombm_event.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
In fact, it exposed a real issue. When switching tombm_eventmode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.Given the name of the option is '
mbm_assign_on_mkdir' I expected the auto assignment to only occur on the creation of ctrl groups or monitor groups. That's not my concern though. As you are now treating this as more of a general auto assignment should the assignment also be done when a resctrl domain is brought back online.For illustration on an MPAM model I see:
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=e;3=eQuoted 9 lines
# offline one domain /sys/fs/resctrl# for i in $(seq 0 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:3=e# bring the domain back online /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:2=_;3=e # Would it be better, or worse, for auto-assignment to occur here?Interesting.
We don't know the previous state. What if it looked like this before the CPU was taken offline?
Quoted 7 lines
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=_;3=eIn that case, forcing auto-assignment may not be the right thing to do. Since we don't know the previous state, we can only speculate.
It may be best to leave this decision to the user.
I think this raises an important point. To get a better idea I looked back at the original bug report referenced by the "Closes:" tag and I do not think this is a relevant fix for that bug.
Let me elaborate ...
The bug reported by Sashiko was in response to the patch that added a step to "Examples on working with mbm_assign_mode" to switch to mbm_event mode from default mode. The rest of the example was not changed to accommodate the assign mode change and the documentation thus reflected the state of a system that was in mbm_event mode when the resource groups were created, not in the new default mode.
In this new version of the series the documentation change looks different - it does not imply that the system started out in default mode and thus does not imply any mode under which those resource groups were created.
I agree with your final statement "It may be best to leave this decision to the user.". It is not possible for resctrl to know how user intends to assign counters. Why not just leave them all unassigned when switching to mbm_event mode?
Reinette
Babu Moger
reviewerRe: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Reinette,
Quoted 90 lines · Reinette Chatre
Hi Babu,
Quoted 68 lines · Babu Moger
Quoted 54 lines · Ben Horgan
Quoted 33 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise therdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
v4: Changelog update (Reinette's input).
Function comment update forrdtgroup_assign_cntrs().v3: Changelog update.
Code comment cleanup.
Added Cc to stable.
Added Reported-by and Closes tags.Quoted 8 lines
v2: New patch.
This patch addresses the Sashiko comment about documentation issue where counters are not assigned automatically when mode is switched tombm_event.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
In fact, it exposed a real issue. When switching tombm_eventmode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.Given the name of the option is '
mbm_assign_on_mkdir' I expected the auto assignment to only occur on the creation of ctrl groups or monitor groups. That's not my concern though. As you are now treating this as more of a general auto assignment should the assignment also be done when a resctrl domain is brought back online.For illustration on an MPAM model I see:
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=e;3=eQuoted 9 lines
# offline one domain /sys/fs/resctrl# for i in $(seq 0 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:3=e# bring the domain back online /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:2=_;3=e # Would it be better, or worse, for auto-assignment to occur here?Interesting.
We don't know the previous state. What if it looked like this before the CPU was taken offline?
Quoted 7 lines
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=_;3=eIn that case, forcing auto-assignment may not be the right thing to do. Since we don't know the previous state, we can only speculate.
It may be best to leave this decision to the user.
I think this raises an important point. To get a better idea I looked back at the original bug report referenced by the "Closes:" tag and I do not think this is a relevant fix for that bug.
Let me elaborate ...
The bug reported by Sashiko was in response to the patch that added a step to "Examples on working with
mbm_assign_mode" to switch tombm_eventmode from default mode. The rest of the example was not changed to accommodate the assign mode change and the documentation thus reflected the state of a system that was inmbm_eventmode when the resource groups were created, not in the new default mode.
Quoted 8 lines
In this new version of the series the documentation change looks different - it does not imply that the system started out in default mode and thus does not imply any mode under which those resource groups were created.
I agree with your final statement "It may be best to leave this decision to the user.". It is not possible for resctrl to know how user intends to assign counters. Why not just leave them all unassigned when switching to
mbm_eventmode?
That is a good point. We can do that.
Should we treat the default group as a special case and handle it differently?
Thanks,
Babu
Reinette Chatre
maintainerRe: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Babu,
Quoted 93 lines · Babu Moger
Quoted 92 lines · Reinette Chatre
Quoted 68 lines · Babu Moger
Quoted 54 lines · Ben Horgan
Quoted 33 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise therdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
v4: Changelog update (Reinette's input).
Function comment update forrdtgroup_assign_cntrs().v3: Changelog update.
Code comment cleanup.
Added Cc to stable.
Added Reported-by and Closes tags.Quoted 8 lines
v2: New patch.
This patch addresses the Sashiko comment about documentation issue where counters are not assigned automatically when mode is switched tombm_event.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
In fact, it exposed a real issue. When switching tombm_eventmode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.Given the name of the option is '
mbm_assign_on_mkdir' I expected the auto assignment to only occur on the creation of ctrl groups or monitor groups. That's not my concern though. As you are now treating this as more of a general auto assignment should the assignment also be done when a resctrl domain is brought back online.For illustration on an MPAM model I see:
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=e;3=eQuoted 9 lines
# offline one domain /sys/fs/resctrl# for i in $(seq 0 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:3=e# bring the domain back online /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:2=_;3=e # Would it be better, or worse, for auto-assignment to occur here?Interesting.
We don't know the previous state. What if it looked like this before the CPU was taken offline?
Quoted 7 lines
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=_;3=eIn that case, forcing auto-assignment may not be the right thing to do. Since we don't know the previous state, we can only speculate.
It may be best to leave this decision to the user.
I think this raises an important point. To get a better idea I looked back at the original bug report referenced by the "Closes:" tag and I do not think this is a relevant fix for that bug.
Let me elaborate ...
The bug reported by Sashiko was in response to the patch that added a step to "Examples on working with
mbm_assign_mode" to switch tombm_eventmode from default mode. The rest of the example was not changed to accommodate the assign mode change and the documentation thus reflected the state of a system that was inmbm_eventmode when the resource groups were created, not in the new default mode.In this new version of the series the documentation change looks different - it does not imply that the system started out in default mode and thus does not imply any mode under which those resource groups were created.
Quoted 8 lines
Quoted 4 lines
I agree with your final statement "It may be best to leave this decision to the user.". It is not possible for resctrl to know how user intends to assign counters. Why not just leave them all unassigned when switching to
mbm_eventmode?That is a good point. We can do that.
Should we treat the default group as a special case and handle it differently?
I am missing why the default group is special here. As I understand user space can allocate counters to the events belonging to the default group just the same as any other group. Could you please elaborate what sets the default group apart here?
Reinette
Re: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Reinette,
Quoted 101 lines · Reinette Chatre
Hi Babu,
Quoted 98 lines · Babu Moger
Quoted 96 lines · Reinette Chatre
Quoted 68 lines · Babu Moger
Quoted 54 lines · Ben Horgan
Quoted 33 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise therdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
v4: Changelog update (Reinette's input).
Function comment update forrdtgroup_assign_cntrs().v3: Changelog update.
Code comment cleanup.
Added Cc to stable.
Added Reported-by and Closes tags.Quoted 8 lines
v2: New patch.
This patch addresses the Sashiko comment about documentation issue where counters are not assigned automatically when mode is switched tombm_event.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
In fact, it exposed a real issue. When switching tombm_eventmode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.Given the name of the option is '
mbm_assign_on_mkdir' I expected the auto assignment to only occur on the creation of ctrl groups or monitor groups. That's not my concern though. As you are now treating this as more of a general auto assignment should the assignment also be done when a resctrl domain is brought back online.For illustration on an MPAM model I see:
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=e;3=eQuoted 9 lines
# offline one domain /sys/fs/resctrl# for i in $(seq 0 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:3=e# bring the domain back online /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:2=_;3=e # Would it be better, or worse, for auto-assignment to occur here?Interesting.
We don't know the previous state. What if it looked like this before the CPU was taken offline?
Quoted 7 lines
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=_;3=eIn that case, forcing auto-assignment may not be the right thing to do. Since we don't know the previous state, we can only speculate.
It may be best to leave this decision to the user.
I think this raises an important point. To get a better idea I looked back at the original bug report referenced by the "Closes:" tag and I do not think this is a relevant fix for that bug.
Let me elaborate ...
The bug reported by Sashiko was in response to the patch that added a step to "Examples on working with
mbm_assign_mode" to switch tombm_eventmode from default mode. The rest of the example was not changed to accommodate the assign mode change and the documentation thus reflected the state of a system that was inmbm_eventmode when the resource groups were created, not in the new default mode.In this new version of the series the documentation change looks different - it does not imply that the system started out in default mode and thus does not imply any mode under which those resource groups were created.
I agree with your final statement "It may be best to leave this decision to the user.". It is not possible for resctrl to know how user intends to assign counters. Why not just leave them all unassigned when switching to
mbm_eventmode?
Quoted 7 lines
Quoted 3 lines
That is a good point. We can do that.
Should we treat the default group as a special case and handle it differently?
I am missing why the default group is special here. As I understand user space can allocate counters to the events belonging to the default group just the same as any other group. Could you please elaborate what sets the default group apart here?
As you know, the default group is the "catch-all" container for the system, and users may expect it to report valid bandwidth measurements. That said, I'm not sure those considerations alone justify giving it guaranteed counter assignment.
Thanks,
Babu
Reinette Chatre
maintainerRe: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Babu,
Quoted 108 lines · Babu
Quoted 99 lines · Reinette Chatre
Quoted 98 lines · Babu Moger
Quoted 96 lines · Reinette Chatre
Quoted 68 lines · Babu Moger
Quoted 54 lines · Ben Horgan
Quoted 33 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise therdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
v4: Changelog update (Reinette's input).
Function comment update forrdtgroup_assign_cntrs().v3: Changelog update.
Code comment cleanup.
Added Cc to stable.
Added Reported-by and Closes tags.Quoted 8 lines
v2: New patch.
This patch addresses the Sashiko comment about documentation issue where counters are not assigned automatically when mode is switched tombm_event.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
In fact, it exposed a real issue. When switching tombm_eventmode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.Given the name of the option is '
mbm_assign_on_mkdir' I expected the auto assignment to only occur on the creation of ctrl groups or monitor groups. That's not my concern though. As you are now treating this as more of a general auto assignment should the assignment also be done when a resctrl domain is brought back online.For illustration on an MPAM model I see:
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=e;3=eQuoted 9 lines
# offline one domain /sys/fs/resctrl# for i in $(seq 0 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:3=e# bring the domain back online /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:2=_;3=e # Would it be better, or worse, for auto-assignment to occur here?Interesting.
We don't know the previous state. What if it looked like this before the CPU was taken offline?
Quoted 7 lines
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=_;3=eIn that case, forcing auto-assignment may not be the right thing to do. Since we don't know the previous state, we can only speculate.
It may be best to leave this decision to the user.
I think this raises an important point. To get a better idea I looked back at the original bug report referenced by the "Closes:" tag and I do not think this is a relevant fix for that bug.
Let me elaborate ...
The bug reported by Sashiko was in response to the patch that added a step to "Examples on working with
mbm_assign_mode" to switch tombm_eventmode from default mode. The rest of the example was not changed to accommodate the assign mode change and the documentation thus reflected the state of a system that was inmbm_eventmode when the resource groups were created, not in the new default mode.In this new version of the series the documentation change looks different - it does not imply that the system started out in default mode and thus does not imply any mode under which those resource groups were created.
I agree with your final statement "It may be best to leave this decision to the user.". It is not possible for resctrl to know how user intends to assign counters. Why not just leave them all unassigned when switching to
mbm_eventmode?Quoted 7 lines
Quoted 3 lines
That is a good point. We can do that.
Should we treat the default group as a special case and handle it differently?
I am missing why the default group is special here. As I understand user space can allocate counters to the events belonging to the default group just the same as any other group. Could you please elaborate what sets the default group apart here?
Quoted 4 lines
As you know, the default group is the "catch-all" container for the system, and users may expect it to report valid bandwidth measurements. That said, I'm not sure those considerations alone justify giving it guaranteed counter assignment.
The current documentation states following about impact of switching to mbm_event mode:
- "Changing the mode may cause all counters on the resource to reset."
- "Moving to mbm_event counter assignment mode requires users to assign the counters to the events. Otherwise, the MBM event counters will return 'Unassigned' when read."
The second is clear about expected behavior and there is no "except default group" caveat.
At this point I do not see a motivation to change the existing behavior.
My vote is for this patch to be dropped.
Reinette
Re: [PATCH v4 2/3] fs/resctrl: Assign counters to existing groups when enabling mbm_event
Hi Reinette,
Quoted 121 lines · Reinette Chatre
Hi Babu,
Quoted 108 lines · Babu
Quoted 99 lines · Reinette Chatre
Quoted 98 lines · Babu Moger
Quoted 96 lines · Reinette Chatre
Quoted 68 lines · Babu Moger
Quoted 54 lines · Ben Horgan
Quoted 33 lines · Babu Moger
When the user switches counter assignment mode by writing "
mbm_event" to/sys/fs/resctrl/info/L3_MON/mbm_assign_mode, resctrl frees all assignable counters, resets per-domain RMID state, and enables counter auto-assignment exposed to user space asmbm_assign_on_mkdir.Even though counter auto-assignment is enabled, groups that already exist at the time of the switch, including the default group created at mount, are not assigned a counter. All MBM events read "Unassigned" until the user assigns a counter by hand.
Walk every existing
CTRL_MONand its MON children after the reset and assign counters to their MBM events. Enabling "mbm_event" now leaves the same per-group state that auto-assignment would have produced. There may be fewer available counters than MBM events across the existing groups; in that case stop assignment when no counters remain. Events in the remaining groups read "Unassigned", matching the behavior of creating a group when no counters are available. Also revise therdtgroup_assign_cntrs()function comments to account for the new callers.Fixes 8004ea01cf63 ("fs/resctrl: Introduce the interface to switch between monitor modes")Reported-by Sashiko <[email protected]>Signed-off-by Babu Moger <[email protected]>---
v4: Changelog update (Reinette's input).
Function comment update forrdtgroup_assign_cntrs().v3: Changelog update.
Code comment cleanup.
Added Cc to stable.
Added Reported-by and Closes tags.Quoted 8 lines
v2: New patch.
This patch addresses the Sashiko comment about documentation issue where counters are not assigned automatically when mode is switched tombm_event.
https://sashiko.dev/#/patchset/8cb66e18e32e4087a9712c1e68ee6da614efe244.1784322818.git.babu.moger%40amd.com
In fact, it exposed a real issue. When switching tombm_eventmode, existing monitoring groups should be assigned counters whenever counters are available. This provides a smooth transition between modes and aligns the behavior with the existing auto-assignment mechanism.Given the name of the option is '
mbm_assign_on_mkdir' I expected the auto assignment to only occur on the creation of ctrl groups or monitor groups. That's not my concern though. As you are now treating this as more of a general auto assignment should the assignment also be done when a resctrl domain is brought back online.For illustration on an MPAM model I see:
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=e;3=eQuoted 9 lines
# offline one domain /sys/fs/resctrl# for i in $(seq 0 3); do echo 0 > /sys/devices/system/cpu/cpu$i/online; done /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:3=e# bring the domain back online /sys/fs/resctrl# cat mbm_L3_assignments mbm_total_bytes:2=_;3=e # Would it be better, or worse, for auto-assignment to occur here?Interesting.
We don't know the previous state. What if it looked like this before the CPU was taken offline?
Quoted 7 lines
/sys/fs/resctrl# cat
mbm_L3_assignments
mbm_total_bytes:2=_;3=eIn that case, forcing auto-assignment may not be the right thing to do. Since we don't know the previous state, we can only speculate.
It may be best to leave this decision to the user.
I think this raises an important point. To get a better idea I looked back at the original bug report referenced by the "Closes:" tag and I do not think this is a relevant fix for that bug.
Let me elaborate ...
The bug reported by Sashiko was in response to the patch that added a step to "Examples on working with
mbm_assign_mode" to switch tombm_eventmode from default mode. The rest of the example was not changed to accommodate the assign mode change and the documentation thus reflected the state of a system that was inmbm_eventmode when the resource groups were created, not in the new default mode.In this new version of the series the documentation change looks different - it does not imply that the system started out in default mode and thus does not imply any mode under which those resource groups were created.
I agree with your final statement "It may be best to leave this decision to the user.". It is not possible for resctrl to know how user intends to assign counters. Why not just leave them all unassigned when switching to
mbm_eventmode?Quoted 7 lines
Quoted 3 lines
That is a good point. We can do that.
Should we treat the default group as a special case and handle it differently?
I am missing why the default group is special here. As I understand user space can allocate counters to the events belonging to the default group just the same as any other group. Could you please elaborate what sets the default group apart here?
Quoted 4 lines
As you know, the default group is the "catch-all" container for the system, and users may expect it to report valid bandwidth measurements. That said, I'm not sure those considerations alone justify giving it guaranteed counter assignment.
The current documentation states following about impact of switching to
mbm_eventmode:
- "Changing the mode may cause all counters on the resource to reset."
- "Moving tombm_eventcounter assignment mode requires users to assign the counters to the events. Otherwise, the MBM event counters will return 'Unassigned' when read."
Quoted 5 lines
The second is clear about expected behavior and there is no "except default group" caveat.
At this point I do not see a motivation to change the existing behavior.
My vote is for this patch to be dropped.
Sure. Will drop this patch.
Thank you,
Babu
Babu Moger
reviewerPATCH v4 3/3 x86,fs/resctrl: Keep mbm_assign_mode in default mode at boot
In reply to Babu MogerThe mbm_event mode assigns hardware MBM counters to RMID/event pairs. It is intended for deployments that need to manage counter assignment on platforms where the number of monitoring groups exceeds the available hardware counters. Hardware counters are scarce on these platforms, so mbm_event mode suits workflows that monitor a subset of groups at a time and rotate assignments as needed.
resctrl enables the mbm_event mode by default on hardware that supports it. This breaks the pqos tool [1], which assumes the historical default mode. It creates 16 or more monitoring groups and uses two counters per group. On a platform with 32 counters per domain, that exhausts the mbm_event counters pool, causing additional groups to read "Unassigned". pqos interprets a non-numeric event read as zero bandwidth and reports 0 MB/s for those groups:
$sudo pqos -m all:0-4
CORE IPC MISSES LLC[KB] MBL[MB/s] MBR[MB/s]
0 0.69 16k 192.0 0.0 0.0
1 0.40 3k 0.0 0.0 0.0
2 0.44 1k 64.0 0.0 0.0
3 0.39 1k 64.0 0.0 0.0
4 0.43 1k 0.0 0.0 0.0Leave mbm_assign_mode in default mode during initialization. Default mode uses one counter per monitoring group. On existing AMD platforms, the default active counters pool is 64 and may be larger on newer hardware. The active counters pool is the number of counters that the hardware can actively track in default mode.
Deployments within the active counter pool receive accurate bandwidth measurements in the default mode. Users can create more monitoring groups (4096) than there are active counters in the pool. Beyond that pool, readings may be misleading or report as "Unavailable", with no user-visible indication.
Default mode is still limited once the number of monitoring groups exceeds that active counters pool. After hardware re-allocates a counter, a read may return "Unavailable". pqos still treats "Unavailable" as zero. Successive reads can be a count, "Unavailable", then another count. pqos treats those jumps as wraparound and can report an inconsistent rate:
$sudo pqos -m all:0-4
CORE IPC MISSES LLC[KB] MBL[MB/s] MBR[MB/s] 0 0.76 60k 32.0 0.0 0.0 1 0.45 1k 32.0 0.0 0.0 2 1.57 107k 64.0 0.0 17592186044184.9 3 1.62 276k 4928.0 3.2 0.5
Users that need stable measurements beyond the active countes pool should use mbm_event mode and rotate assignments as needed. Enable it with:
$echo mbm_event > /sys/fs/resctrl/info/L3_MON/mbm_assign_mode
---
v4: Introduced mbm_event counter pool and default active counter pool.
Removed duplicate texts in resctrl.rst.
Re-arranged the tag order.
v3: Added pqos output to show the problem.
More changelog to detail the issue.
Updated the resctrl.rst to mention changes.
v2:
Added documentation describing the known issue with the default mode. Will add cc to stable once we have all the things in order. Let me know if I missed anything.
--- Documentation/filesystems/resctrl.rst | 43 +++++++++++++++++---------- arch/x86/kernel/cpu/resctrl/monitor.c | 1 - 2 files changed, 28 insertions(+), 16 deletions(-)
Documentation/filesystems/resctrl.rst +28 -15
@@ -354,8 +354,8 @@ with the following files: :: # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode - [mbm_event] - default + [default] + mbm_event "mbm_event":
@@ -376,20 +376,35 @@ with the following files: "mbm_L3_assignments" after switching to "mbm_event" mode for counter assignment states of all monitoring groups. - The mode is beneficial for AMD platforms that support more CTRL_MON - and MON groups than available hardware counters. By default, this - feature is enabled on AMD platforms with the ABMC (Assignable Bandwidth - Monitoring Counters) capability, ensuring counters remain assigned even - when the corresponding RMID is not actively used by any processor. + It is intended for deployments that need to manage counter assignment on + platforms where the number of monitoring groups exceeds the available + hardware counters. Hardware counters are scarce on these platforms, so + mbm_event mode suits workflows that monitor a subset of groups at a time and + rotate assignments as needed. The mbm_event mode ensures counters remain + assigned even when the corresponding RMID is not actively monitored. "default": In default mode, resctrl assumes there is a hardware counter for each - event within every CTRL_MON and MON group. On AMD platforms, it is - recommended to use the mbm_event mode, if supported, to prevent reset of MBM - events between reads resulting from hardware re-allocating counters. This can - result in misleading values or display "Unavailable" if no counter is assigned - to the event. + event within every CTRL_MON and MON group. This mode is enabled by default. + + On AMD platforms that support more CTRL_MON and MON groups than hardware + counters, hardware dynamically shares a smaller pool of counters among + RMIDs. One counter from this pool is used per monitoring group and counts + every MBM event of that group's RMID. The size of the pool is not + enumerated to software (unlike "num_mbm_cntrs"), and "num_rmids" may be + much larger. For example, 64 counters in this pool monitor 64 groups. + Newer hardware may provide a larger pool. + + While the number of monitoring groups does not exceed that pool, a counter + stays attached to each RMID and readings remain accurate. Creating more + groups than the pool can cause hardware to re-allocate those counters + between successive reads of an event. Bandwidth values may then be + misleading, or a read may return "Unavailable" if no counter is allocated + to the RMID. There is no user-visible indication when this begins. + Users who need stable readings beyond that pool should switch to mbm_event + mode, if supported, and assign counters to the groups of interest + (rotating assignments as needed). * To enable "mbm_event" counter assignment mode: ::
@@ -1787,11 +1802,9 @@ View the llc occupancy snapshot:: Examples on working with mbm_assign_mode ======================================== -a. Check if MBM counter assignment mode is supported. +a. Check if MBM counter assignment mode is supported and enabled. :: - # mount -t resctrl resctrl /sys/fs/resctrl/ - # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode [mbm_event] default
arch/x86/kernel/cpu/resctrl/monitor.c +0 -1
@@ -472,7 +472,6 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r)cpuid_count (0x80000020 ,5 , &eax, &ebx, &ecx, &edx);/* cntr_id is 12 bits and can only encode 4096 counters. */ r->mon.num_mbm_cntrs =min ((ebx &GENMASK (15 ,0 )) +1 ,BIT (12 )); - hw_res->mbm_cntr_assign_enabled = true; } r->mon_capable = true;
Signature · 1 line
2.43.0
Reinette Chatre
maintainerRe: [PATCH v4 3/3] x86,fs/resctrl: Keep mbm_assign_mode in default mode at boot
Hi Babu,
Quoted 23 lines · Babu Moger
The
mbm_eventmode assigns hardware MBM counters to RMID/event pairs. It is intended for deployments that need to manage counter assignment on platforms where the number of monitoring groups exceeds the available hardware counters. Hardware counters are scarce on these platforms, sombm_eventmode suits workflows that monitor a subset of groups at a time and rotate assignments as needed.resctrl enables the
mbm_eventmode by default on hardware that supports it. This breaks the pqos tool [1], which assumes the historical default mode. It creates 16 or more monitoring groups and uses two counters per group. On a platform with 32 counters per domain, that exhausts thembm_eventcounters pool, causing additional groups to read "Unassigned". pqos interprets a non-numeric event read as zero bandwidth and reports 0 MB/s for those groups:$sudo pqos -m all:0-4
CORE IPC MISSES LLC[KB] MBL[MB/s] MBR[MB/s]0 0.69 16k 192.0 0.0 0.0 1 0.40 3k 0.0 0.0 0.0 2 0.44 1k 64.0 0.0 0.0 3 0.39 1k 64.0 0.0 0.0 4 0.43 1k 0.0 0.0 0.0
Quoted 7 lines
Leave
mbm_assign_modein default mode during initialization. Default mode uses one counter per monitoring group. On existing AMD platforms, the default active counters pool is 64 and may be larger on newer hardware. The active counters pool is the number of counters that the hardware can actively track in default mode.Deployments within the active counter pool receive accurate bandwidth
nit: "active counter pool" -> "active counters pool" (to be consistent with all the other instances)
Quoted 2 lines
measurements in the default mode. Users can create more monitoring groups (4096) than there are active counters in the pool. Beyond that pool,
Unexpected where the hardcoded 4096 comes from. Could this be "(for example, 4096 on current hardware)"?
Quoted 9 lines
readings may be misleading or report as "Unavailable", with no user-visible indication.
Default mode is still limited once the number of monitoring groups exceeds that active counters pool. After hardware re-allocates a counter, a read may return "Unavailable". pqos still treats "Unavailable" as zero. Successive reads can be a count, "Unavailable", then another count. pqos treats those jumps as wraparound and can report an inconsistent rate:
Quoted 8 lines
$sudo pqos -m all:0-4
CORE IPC MISSES LLC[KB] MBL[MB/s] MBR[MB/s] 0 0.76 60k 32.0 0.0 0.0 1 0.45 1k 32.0 0.0 0.0 2 1.57 107k 64.0 0.0 17592186044184.9 3 1.62 276k 4928.0 3.2 0.5Users that need stable measurements beyond the active countes pool should
"countes" -> "counters"
Quoted 10 lines
use
mbm_eventmode and rotate assignments as needed. Enable it with:$echo
mbm_event>/sys/fs/resctrl/info/L3_MON/mbm_assign_modeFixes 0f1576e43adc ("x86/resctrl: Configure mbm_event mode if supported")Signed-off-by Babu Moger <[email protected]>Link https://github.com/intel/intel-cmt-cat # [1]---
...
Quoted 69 lines
--- Documentation/filesystems/resctrl.rst | 43 +++++++++++++++++---------- arch/x86/kernel/cpu/resctrl/monitor.c | 1 - 2 files changed, 28 insertions(+), 16 deletions(-)Documentation/filesystems/resctrl.rst +27
-12@@ -354,8 +354,8 @@ with the following files: :: # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode - [mbm_event] - default + [default] + mbm_event "mbm_event":@@ -376,20 +376,35 @@ with the following files: "mbm_L3_assignments" after switching to "mbm_event" mode for counter assignment states of all monitoring groups. - The mode is beneficial for AMD platforms that support more CTRL_MON - and MON groups than available hardware counters. By default, this - feature is enabled on AMD platforms with the ABMC (Assignable Bandwidth - Monitoring Counters) capability, ensuring counters remain assigned even - when the corresponding RMID is not actively used by any processor. + It is intended for deployments that need to manage counter assignment on + platforms where the number of monitoring groups exceeds the available + hardware counters. Hardware counters are scarce on these platforms, so + mbm_event mode suits workflows that monitor a subset of groups at a time and + rotate assignments as needed. The mbm_event mode ensures counters remain + assigned even when the corresponding RMID is not actively monitored. "default": In default mode, resctrl assumes there is a hardware counter for each - event within every CTRL_MON and MON group. On AMD platforms, it is - recommended to use the mbm_event mode, if supported, to prevent reset of MBM - events between reads resulting from hardware re-allocating counters. This can - result in misleading values or display "Unavailable" if no counter is assigned - to the event. + event within every CTRL_MON and MON group. This mode is enabled by default. + + On AMD platforms that support more CTRL_MON and MON groups than hardware + counters, hardware dynamically shares a smaller pool of counters among + RMIDs. One counter from this pool is used per monitoring group and counts + every MBM event of that group's RMID. The size of the pool is not + enumerated to software (unlike "num_mbm_cntrs"), and "num_rmids" may be + much larger. For example, 64 counters in this pool monitor 64 groups. + Newer hardware may provide a larger pool. + + While the number of monitoring groups does not exceed that pool, a counter + stays attached to each RMID and readings remain accurate. Creating more + groups than the pool can cause hardware to re-allocate those counters + between successive reads of an event. Bandwidth values may then be + misleading, or a read may return "Unavailable" if no counter is allocated + to the RMID. There is no user-visible indication when this begins. + Users who need stable readings beyond that pool should switch to mbm_event + mode, if supported, and assign counters to the groups of interest + (rotating assignments as needed). * To enable "mbm_event" counter assignment mode: ::@@ -1787,11 +1802,9 @@ View the llc occupancy snapshot:: Examples on working with mbm_assign_mode ========================================
Documentation/filesystems/resctrl.rst View the llc occupancy snapshot:: · line 1810
Quoted 10 lines
Documentation/filesystems/resctrl.rst +1
-3-a. Check if MBM counter assignment mode is supported. +a. Check if MBM counter assignment mode is supported and enabled. :: - # mount -t resctrl resctrl /sys/fs/resctrl/ - # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode [mbm_event] default
After the above doc changes the mbm_assign_on_mkdir description still reads "Enabled by default on boot". This is still technically accurate, but this patch makes it unobservable by user space. Fine with me if you intentionally decide to keep it but I would just like to confirm that you considered it?
arch/x86/kernel/cpu/resctrl/monitor.c int __init rdt_get_l3_mon_config(struct rdt_resource *r) · line 477
Quoted 12 lines
arch/x86/kernel/cpu/resctrl/monitor.c +0
-1@@ -472,7 +472,6 @@ int __init rdt_get_l3_mon_config(struct rdt_resource *r)cpuid_count (0x80000020 ,5 , &eax, &ebx, &ecx, &edx);/* cntr_id is 12 bits and can only encode 4096 counters. */ r->mon.num_mbm_cntrs =min ((ebx &GENMASK (15 ,0 )) +1 ,BIT (12 )); - hw_res->mbm_cntr_assign_enabled = true; } r->mon_capable = true;
Reinette
Babu Moger
reviewerRe: [PATCH v4 3/3] x86,fs/resctrl: Keep mbm_assign_mode in default mode at boot
Hi Reinette,
Quoted 32 lines · Reinette Chatre
Hi Babu,
Quoted 29 lines · Babu Moger
The
mbm_eventmode assigns hardware MBM counters to RMID/event pairs. It is intended for deployments that need to manage counter assignment on platforms where the number of monitoring groups exceeds the available hardware counters. Hardware counters are scarce on these platforms, sombm_eventmode suits workflows that monitor a subset of groups at a time and rotate assignments as needed.resctrl enables the
mbm_eventmode by default on hardware that supports it. This breaks the pqos tool [1], which assumes the historical default mode. It creates 16 or more monitoring groups and uses two counters per group. On a platform with 32 counters per domain, that exhausts thembm_eventcounters pool, causing additional groups to read "Unassigned". pqos interprets a non-numeric event read as zero bandwidth and reports 0 MB/s for those groups:$sudo pqos -m all:0-4 CORE IPC MISSES LLC[KB] MBL[MB/s] MBR[MB/s] 0 0.69 16k 192.0 0.0 0.0 1 0.40 3k 0.0 0.0 0.0 2 0.44 1k 64.0 0.0 0.0 3 0.39 1k 64.0 0.0 0.0 4 0.43 1k 0.0 0.0 0.0Leave
mbm_assign_modein default mode during initialization. Default mode uses one counter per monitoring group. On existing AMD platforms, the default active counters pool is 64 and may be larger on newer hardware. The active counters pool is the number of counters that the hardware can actively track in default mode.
Quoted 4 lines
Quoted 1 line
Deployments within the active counter pool receive accurate bandwidth
nit: "active counter pool" -> "active counters pool" (to be consistent with all the other instances)
Sure.
Quoted 5 lines
Quoted 2 lines
measurements in the default mode. Users can create more monitoring groups (4096) than there are active counters in the pool. Beyond that pool,
Unexpected where the hardcoded 4096 comes from. Could this be "(for example, 4096 on current hardware)"?
Sure.
Quoted 16 lines
Quoted 16 lines
readings may be misleading or report as "Unavailable", with no user-visible indication.
Default mode is still limited once the number of monitoring groups exceeds that active counters pool. After hardware re-allocates a counter, a read may return "Unavailable". pqos still treats "Unavailable" as zero. Successive reads can be a count, "Unavailable", then another count. pqos treats those jumps as wraparound and can report an inconsistent rate:
$sudo pqos -m all:0-4 CORE IPC MISSES LLC[KB] MBL[MB/s] MBR[MB/s] 0 0.76 60k 32.0 0.0 0.0 1 0.45 1k 32.0 0.0 0.0 2 1.57 107k 64.0 0.0 17592186044184.9 3 1.62 276k 4928.0 3.2 0.5
Quoted 3 lines
Quoted 1 line
Users that need stable measurements beyond the active countes pool should
"countes" -> "counters"
Sure.
Quoted 88 lines
Quoted 11 lines
use
mbm_eventmode and rotate assignments as needed. Enable it with:$echo
mbm_event>/sys/fs/resctrl/info/L3_MON/mbm_assign_modeFixes 0f1576e43adc ("x86/resctrl: Configure mbm_event mode if supported")Signed-off-by Babu Moger <[email protected]>Link https://github.com/intel/intel-cmt-cat # [1]---
...
Quoted 74 lines
--- Documentation/filesystems/resctrl.rst | 43 +++++++++++++++++---------- arch/x86/kernel/cpu/resctrl/monitor.c | 1 - 2 files changed, 28 insertions(+), 16 deletions(-)Documentation/filesystems/resctrl.rst +28
-13@@ -354,8 +354,8 @@ with the following files: :: # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode - [mbm_event] - default + [default] + mbm_event "mbm_event":@@ -376,20 +376,35 @@ with the following files: "mbm_L3_assignments" after switching to "mbm_event" mode for counter assignment states of all monitoring groups. - The mode is beneficial for AMD platforms that support more CTRL_MON - and MON groups than available hardware counters. By default, this - feature is enabled on AMD platforms with the ABMC (Assignable Bandwidth - Monitoring Counters) capability, ensuring counters remain assigned even - when the corresponding RMID is not actively used by any processor. + It is intended for deployments that need to manage counter assignment on + platforms where the number of monitoring groups exceeds the available + hardware counters. Hardware counters are scarce on these platforms, so + mbm_event mode suits workflows that monitor a subset of groups at a time and + rotate assignments as needed. The mbm_event mode ensures counters remain + assigned even when the corresponding RMID is not actively monitored. "default": In default mode, resctrl assumes there is a hardware counter for each - event within every CTRL_MON and MON group. On AMD platforms, it is - recommended to use the mbm_event mode, if supported, to prevent reset of MBM - events between reads resulting from hardware re-allocating counters. This can - result in misleading values or display "Unavailable" if no counter is assigned - to the event. + event within every CTRL_MON and MON group. This mode is enabled by default. + + On AMD platforms that support more CTRL_MON and MON groups than hardware + counters, hardware dynamically shares a smaller pool of counters among + RMIDs. One counter from this pool is used per monitoring group and counts + every MBM event of that group's RMID. The size of the pool is not + enumerated to software (unlike "num_mbm_cntrs"), and "num_rmids" may be + much larger. For example, 64 counters in this pool monitor 64 groups. + Newer hardware may provide a larger pool. + + While the number of monitoring groups does not exceed that pool, a counter + stays attached to each RMID and readings remain accurate. Creating more + groups than the pool can cause hardware to re-allocate those counters + between successive reads of an event. Bandwidth values may then be + misleading, or a read may return "Unavailable" if no counter is allocated + to the RMID. There is no user-visible indication when this begins. + Users who need stable readings beyond that pool should switch to mbm_event + mode, if supported, and assign counters to the groups of interest + (rotating assignments as needed). * To enable "mbm_event" counter assignment mode: ::@@ -1787,11 +1802,9 @@ View the llc occupancy snapshot:: Examples on working with mbm_assign_mode ======================================== -a. Check if MBM counter assignment mode is supported. +a. Check if MBM counter assignment mode is supported and enabled. ::
Quoted 10 lines
Documentation/filesystems/resctrl.rst View the llc occupancy snapshot:: · line 1810
Quoted 5 lines
Documentation/filesystems/resctrl.rst +0
-2- # mount -t resctrl resctrl /sys/fs/resctrl/ - # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_mode [mbm_event] defaultAfter the above doc changes the
mbm_assign_on_mkdirdescription still reads "Enabled by default on boot". This is still technically accurate, but this patch makes it unobservable by user space. Fine with me if you intentionally decide to keep it but I would just like to confirm that you considered it?
Good catch. I noticed that a while ago, but it slipped my mind.
I also see that the MPAM driver initializes it to true at boot. I would expect that to be harmless.
Thanks
Babu