Users can create as many monitor groups as RMIDs supported by the hardware.
However, bandwidth monitoring feature on AMD system only guarantees that
RMIDs currently assigned to a processor will be tracked by hardware. The
counters of any other RMIDs which are no longer being tracked will be reset
to zero. The MBM event counters return "Unavailable" for the RMIDs that are
not tracked by hardware. So, there can be only limited number of groups
that can give guaranteed monitoring numbers. With ever changing
configurations there is no way to definitely know which of these groups are
being tracked for certain point of time. Users do not have the option to
monitor a group or set of groups for certain period of time without
worrying about RMID being reset in between.
The ABMC feature provides an option to the user to assign a hardware
counter to an RMID, event pair and monitor the bandwidth as long as it is
assigned. The assigned RMID will be tracked by the hardware until the user
unassigns it manually. There is no need to worry about counters being reset
during this period. Additionally, the user can specify a bitmask
identifying the specific bandwidth types from the given source to track
with the counter.
Without ABMC enabled, monitoring will work in current mode without
assignment option.
Linux resctrl subsystem provides the interface to count maximum of two
memory bandwidth events per group, from a combination of available total
and local events. Keeping the current interface, users can enable a maximum
of 2 ABMC counters per group. User will also have the option to enable only
one counter to the group. If the system runs out of assignable ABMC
counters, kernel will display an error. Users need to disable an already
enabled counter to make space for new assignments.
The feature can be detected via CPUID_Fn80000020_EBX_x00 bit 5.
Bits Description
5 ABMC (Assignable Bandwidth Monitoring Counters)
The feature details are documented in APM listed below [1].
[1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
Monitoring (ABMC).
Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
Signed-off-by: Babu Moger <babu.moger@amd.com>
---
Note: Checkpatch checks/warnings are ignored to maintain coding style.
v12: Removed the dependancy on X86_FEATURE_BMEC.
Removed the Reviewed-by tag as patch has changed.
v11: No changes.
v10: No changes.
v9: Took care of couple of minor merge conflicts. No other changes.
v8: No changes.
v7: Removed "" from feature flags. Not required anymore.
https://lore.kernel.org/lkml/20240817145058.GCZsC40neU4wkPXeVR@fat_crate.local/
v6: Added Reinette's Reviewed-by. Moved the Checkpatch note below ---.
v5: Minor rebase change and subject line update.
v4: Changes because of rebase. Feature word 21 has few more additions now.
Changed the text to "tracked by hardware" instead of active.
v3: Change because of rebase. Actual patch did not change.
v2: Added dependency on X86_FEATURE_BMEC.
---
arch/x86/include/asm/cpufeatures.h | 1 +
arch/x86/kernel/cpu/cpuid-deps.c | 2 ++
arch/x86/kernel/cpu/scattered.c | 1 +
3 files changed, 4 insertions(+)
diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 8b7cf13e0acb..accc1c328672 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -479,6 +479,7 @@
#define X86_FEATURE_AMD_FAST_CPPC (21*32 + 5) /* Fast CPPC */
#define X86_FEATURE_AMD_HETEROGENEOUS_CORES (21*32 + 6) /* Heterogeneous Core Topology */
#define X86_FEATURE_AMD_WORKLOAD_CLASS (21*32 + 7) /* Workload Classification */
+#define X86_FEATURE_ABMC (21*32 + 8) /* Assignable Bandwidth Monitoring Counters */
/*
* BUG word(s)
diff --git a/arch/x86/kernel/cpu/cpuid-deps.c b/arch/x86/kernel/cpu/cpuid-deps.c
index a2fbea0be535..2f54831e04e5 100644
--- a/arch/x86/kernel/cpu/cpuid-deps.c
+++ b/arch/x86/kernel/cpu/cpuid-deps.c
@@ -71,6 +71,8 @@ static const struct cpuid_dep cpuid_deps[] = {
{ X86_FEATURE_CQM_MBM_LOCAL, X86_FEATURE_CQM_LLC },
{ X86_FEATURE_BMEC, X86_FEATURE_CQM_MBM_TOTAL },
{ X86_FEATURE_BMEC, X86_FEATURE_CQM_MBM_LOCAL },
+ { X86_FEATURE_ABMC, X86_FEATURE_CQM_MBM_TOTAL },
+ { X86_FEATURE_ABMC, X86_FEATURE_CQM_MBM_LOCAL },
{ X86_FEATURE_AVX512_BF16, X86_FEATURE_AVX512VL },
{ X86_FEATURE_AVX512_FP16, X86_FEATURE_AVX512BW },
{ X86_FEATURE_ENQCMD, X86_FEATURE_XSAVES },
diff --git a/arch/x86/kernel/cpu/scattered.c b/arch/x86/kernel/cpu/scattered.c
index 16f3ca30626a..3b72b72270f1 100644
--- a/arch/x86/kernel/cpu/scattered.c
+++ b/arch/x86/kernel/cpu/scattered.c
@@ -49,6 +49,7 @@ static const struct cpuid_bit cpuid_bits[] = {
{ X86_FEATURE_MBA, CPUID_EBX, 6, 0x80000008, 0 },
{ X86_FEATURE_SMBA, CPUID_EBX, 2, 0x80000020, 0 },
{ X86_FEATURE_BMEC, CPUID_EBX, 3, 0x80000020, 0 },
+ { X86_FEATURE_ABMC, CPUID_EBX, 5, 0x80000020, 0 },
{ X86_FEATURE_AMD_WORKLOAD_CLASS, CPUID_EAX, 22, 0x80000021, 0 },
{ X86_FEATURE_PERFMON_V2, CPUID_EAX, 0, 0x80000022, 0 },
{ X86_FEATURE_AMD_LBR_V2, CPUID_EAX, 1, 0x80000022, 0 },
--
2.34.1Hi Babu, On 4/3/25 5:18 PM, Babu Moger wrote: > Users can create as many monitor groups as RMIDs supported by the hardware. > However, bandwidth monitoring feature on AMD system only guarantees that > RMIDs currently assigned to a processor will be tracked by hardware. The > counters of any other RMIDs which are no longer being tracked will be reset > to zero. The MBM event counters return "Unavailable" for the RMIDs that are > not tracked by hardware. So, there can be only limited number of groups > that can give guaranteed monitoring numbers. With ever changing > configurations there is no way to definitely know which of these groups are > being tracked for certain point of time. Users do not have the option to > monitor a group or set of groups for certain period of time without > worrying about RMID being reset in between. > > The ABMC feature provides an option to the user to assign a hardware > counter to an RMID, event pair and monitor the bandwidth as long as it is > assigned. The assigned RMID will be tracked by the hardware until the user > unassigns it manually. There is no need to worry about counters being reset > during this period. Additionally, the user can specify a bitmask > identifying the specific bandwidth types from the given source to track > with the counter. > > Without ABMC enabled, monitoring will work in current mode without > assignment option. > > Linux resctrl subsystem provides the interface to count maximum of two > memory bandwidth events per group, from a combination of available total > and local events. Keeping the current interface, users can enable a maximum > of 2 ABMC counters per group. User will also have the option to enable only > one counter to the group. If the system runs out of assignable ABMC > counters, kernel will display an error. Users need to disable an already > enabled counter to make space for new assignments. The above paragraph sounds like it is still talking about the original global assignment of counters. > > The feature can be detected via CPUID_Fn80000020_EBX_x00 bit 5. > Bits Description > 5 ABMC (Assignable Bandwidth Monitoring Counters) > > The feature details are documented in APM listed below [1]. > [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming > Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth > Monitoring (ABMC). > > Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537 > Signed-off-by: Babu Moger <babu.moger@amd.com> > --- > > Note: Checkpatch checks/warnings are ignored to maintain coding style. > > v12: Removed the dependancy on X86_FEATURE_BMEC. Considering this removal it is not clear to me how the BMEC and ABMC features are managed on a platform. Since this dependency existed I assume platforms that support both ABMC and BMEC exist and after previous discussion [1] I expected to see that BMEC support will be disabled when ABMC is detected but I do not see this done in this series. From what I can tell, looking at patch "x86/resctrl: Detect Assignable Bandwidth Monitoring feature details" BMEC and ABMC are both detected and enabled while I do not see any interactions handled. For example, a user modifying the BMEC appears to have no impact on existing ABMC assigned counters. Could you please clarify how event configuration works on platforms that support both ABMC and BMEC? Reinette [1] https://lore.kernel.org/all/4b66ea1c-2f76-4218-a67b-2232b2be6990@amd.com/
Hi Reinette,
On 4/11/25 15:52, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/3/25 5:18 PM, Babu Moger wrote:
>> Users can create as many monitor groups as RMIDs supported by the hardware.
>> However, bandwidth monitoring feature on AMD system only guarantees that
>> RMIDs currently assigned to a processor will be tracked by hardware. The
>> counters of any other RMIDs which are no longer being tracked will be reset
>> to zero. The MBM event counters return "Unavailable" for the RMIDs that are
>> not tracked by hardware. So, there can be only limited number of groups
>> that can give guaranteed monitoring numbers. With ever changing
>> configurations there is no way to definitely know which of these groups are
>> being tracked for certain point of time. Users do not have the option to
>> monitor a group or set of groups for certain period of time without
>> worrying about RMID being reset in between.
>>
>> The ABMC feature provides an option to the user to assign a hardware
>> counter to an RMID, event pair and monitor the bandwidth as long as it is
>> assigned. The assigned RMID will be tracked by the hardware until the user
>> unassigns it manually. There is no need to worry about counters being reset
>> during this period. Additionally, the user can specify a bitmask
>> identifying the specific bandwidth types from the given source to track
>> with the counter.
>>
>> Without ABMC enabled, monitoring will work in current mode without
>> assignment option.
>>
>> Linux resctrl subsystem provides the interface to count maximum of two
>> memory bandwidth events per group, from a combination of available total
>> and local events. Keeping the current interface, users can enable a maximum
>> of 2 ABMC counters per group. User will also have the option to enable only
>> one counter to the group. If the system runs out of assignable ABMC
>> counters, kernel will display an error. Users need to disable an already
>> enabled counter to make space for new assignments.
>
> The above paragraph sounds like it is still talking about the original
> global assignment of counters.
Ok. Sure. Will update it.
>
>>
>> The feature can be detected via CPUID_Fn80000020_EBX_x00 bit 5.
>> Bits Description
>> 5 ABMC (Assignable Bandwidth Monitoring Counters)
>>
>> The feature details are documented in APM listed below [1].
>> [1] AMD64 Architecture Programmer's Manual Volume 2: System Programming
>> Publication # 24593 Revision 3.41 section 19.3.3.3 Assignable Bandwidth
>> Monitoring (ABMC).
>>
>> Link: https://bugzilla.kernel.org/show_bug.cgi?id=206537
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>
>> Note: Checkpatch checks/warnings are ignored to maintain coding style.
>>
>> v12: Removed the dependancy on X86_FEATURE_BMEC.
>
> Considering this removal it is not clear to me how the BMEC and ABMC features
> are managed on a platform. Since this dependency existed I assume platforms
> that support both ABMC and BMEC exist and after previous discussion [1]
> I expected to see that BMEC support will be disabled when ABMC is detected
> but I do not see this done in this series. From what I can tell, looking at
> patch "x86/resctrl: Detect Assignable Bandwidth Monitoring feature details"
> BMEC and ABMC are both detected and enabled while I do not see any
> interactions handled. For example, a user modifying the BMEC appears
> to have no impact on existing ABMC assigned counters. Could you please clarify
> how event configuration works on platforms that support both ABMC and BMEC?
They are mutually exclusive. If ABMC is enabled then BMEC should not work.
I missed to handle it. Also, I was not very clear at that on how to
handle that.
Here is my proposal to handle this case. This can be separate patch.
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index d10cf1e5b914..772f2f77faee 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1370,7 +1370,7 @@ static int rdt_mon_features_show(struct
kernfs_open_file *of,
list_for_each_entry(mevt, &r->mon.evt_list, list) {
seq_printf(seq, "%s\n", mevt->name);
- if (mevt->configurable)
+ if (mevt->configurable &&
!resctrl_arch_mbm_cntr_assign_enabled(r))
seq_printf(seq, "%s_config\n", mevt->name);
}
@@ -1846,6 +1846,11 @@ static int mbm_config_show(struct seq_file *s,
struct rdt_resource *r, u32 evtid
cpus_read_lock();
mutex_lock(&rdtgroup_mutex);
+ if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
+ rdt_last_cmd_puts("Event configuration(BMEC) not supported
with mbm_cntr_assign mode\n");
+ return -EINVAL;
+ }
+
list_for_each_entry(dom, &r->mon_domains, hdr.list) {
if (sep)
seq_puts(s, ";");
@@ -1865,21 +1870,24 @@ static int mbm_config_show(struct seq_file *s,
struct rdt_resource *r, u32 evtid
static int mbm_total_bytes_config_show(struct kernfs_open_file *of,
struct seq_file *seq, void *v)
{
+ int ret;
struct rdt_resource *r = of->kn->parent->priv;
- mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
+ ret = mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
- return 0;
+ return ret;
}
static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
struct seq_file *seq, void *v)
{
+ int ret;
+
struct rdt_resource *r = of->kn->parent->priv;
- mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
+ ret = mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
- return 0;
+ return ret;
}
static void mbm_config_write_domain(struct rdt_resource *r,
@@ -1932,6 +1940,11 @@ static int mon_config_write(struct rdt_resource *r,
char *tok, u32 evtid)
/* Walking r->domains, ensure it can't race with cpuhp */
lockdep_assert_cpus_held();
+ if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
+ rdt_last_cmd_puts("Event configuration(BMEC) not supported
with mbm_cntr_assign mode\n");
+ return -EINVAL;
+ }
+
next:
if (!tok || tok[0] == '\0')
return 0;
>
> Reinette
>
> [1] https://lore.kernel.org/all/4b66ea1c-2f76-4218-a67b-2232b2be6990@amd.com/
>
--
Thanks
Babu Moger
Hi Babu,
On 4/14/25 10:48 AM, Moger, Babu wrote:
> Here is my proposal to handle this case. This can be separate patch.
>
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index d10cf1e5b914..772f2f77faee 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1370,7 +1370,7 @@ static int rdt_mon_features_show(struct
> kernfs_open_file *of,
>
> list_for_each_entry(mevt, &r->mon.evt_list, list) {
> seq_printf(seq, "%s\n", mevt->name);
> - if (mevt->configurable)
> + if (mevt->configurable &&
> !resctrl_arch_mbm_cntr_assign_enabled(r))
> seq_printf(seq, "%s_config\n", mevt->name);
> }
>
> @@ -1846,6 +1846,11 @@ static int mbm_config_show(struct seq_file *s,
> struct rdt_resource *r, u32 evtid
> cpus_read_lock();
> mutex_lock(&rdtgroup_mutex);
>
> + if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
> + rdt_last_cmd_puts("Event configuration(BMEC) not supported
> with mbm_cntr_assign mode\n");
> + return -EINVAL;
> + }
> +
> list_for_each_entry(dom, &r->mon_domains, hdr.list) {
> if (sep)
> seq_puts(s, ";");
> @@ -1865,21 +1870,24 @@ static int mbm_config_show(struct seq_file *s,
> struct rdt_resource *r, u32 evtid
> static int mbm_total_bytes_config_show(struct kernfs_open_file *of,
> struct seq_file *seq, void *v)
> {
> + int ret;
> struct rdt_resource *r = of->kn->parent->priv;
>
> - mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
> + ret = mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
>
> - return 0;
> + return ret;
> }
>
> static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
> struct seq_file *seq, void *v)
> {
> + int ret;
> +
> struct rdt_resource *r = of->kn->parent->priv;
>
> - mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
> + ret = mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
>
> - return 0;
> + return ret;
> }
>
> static void mbm_config_write_domain(struct rdt_resource *r,
> @@ -1932,6 +1940,11 @@ static int mon_config_write(struct rdt_resource *r,
> char *tok, u32 evtid)
> /* Walking r->domains, ensure it can't race with cpuhp */
> lockdep_assert_cpus_held();
>
> + if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
> + rdt_last_cmd_puts("Event configuration(BMEC) not supported
> with mbm_cntr_assign mode\n");
> + return -EINVAL;
> + }
> +
> next:
> if (!tok || tok[0] == '\0')
> return 0;
>
Instead of chasing every call that may involve BMEC I think it will be simpler to
disable BMEC support during initialization when ABMC is detected. Specifically,
on systems that support both BMEC and ABMC rdt_cpu_has(X86_FEATURE_BMEC) returns
false.
I would also like to consider enhancing mevt->configurable to handle all different
ways in which events can be configured. For example, making mevt->configurable an
enum that captures how event can be configured instead of keeping mevt->configurable
a boolean for BMEC support and handling ABMC completely separately. I hope this
may become clearer when using struct mon_evt for ABMC also.
Reinette
Hi Reinette,
On 4/15/25 11:09, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/14/25 10:48 AM, Moger, Babu wrote:
>
>> Here is my proposal to handle this case. This can be separate patch.
>>
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index d10cf1e5b914..772f2f77faee 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -1370,7 +1370,7 @@ static int rdt_mon_features_show(struct
>> kernfs_open_file *of,
>>
>> list_for_each_entry(mevt, &r->mon.evt_list, list) {
>> seq_printf(seq, "%s\n", mevt->name);
>> - if (mevt->configurable)
>> + if (mevt->configurable &&
>> !resctrl_arch_mbm_cntr_assign_enabled(r))
>> seq_printf(seq, "%s_config\n", mevt->name);
>> }
>>
>> @@ -1846,6 +1846,11 @@ static int mbm_config_show(struct seq_file *s,
>> struct rdt_resource *r, u32 evtid
>> cpus_read_lock();
>> mutex_lock(&rdtgroup_mutex);
>>
>> + if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
>> + rdt_last_cmd_puts("Event configuration(BMEC) not supported
>> with mbm_cntr_assign mode\n");
>> + return -EINVAL;
>> + }
>> +
>> list_for_each_entry(dom, &r->mon_domains, hdr.list) {
>> if (sep)
>> seq_puts(s, ";");
>> @@ -1865,21 +1870,24 @@ static int mbm_config_show(struct seq_file *s,
>> struct rdt_resource *r, u32 evtid
>> static int mbm_total_bytes_config_show(struct kernfs_open_file *of,
>> struct seq_file *seq, void *v)
>> {
>> + int ret;
>> struct rdt_resource *r = of->kn->parent->priv;
>>
>> - mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
>> + ret = mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>> struct seq_file *seq, void *v)
>> {
>> + int ret;
>> +
>> struct rdt_resource *r = of->kn->parent->priv;
>>
>> - mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
>> + ret = mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
>>
>> - return 0;
>> + return ret;
>> }
>>
>> static void mbm_config_write_domain(struct rdt_resource *r,
>> @@ -1932,6 +1940,11 @@ static int mon_config_write(struct rdt_resource *r,
>> char *tok, u32 evtid)
>> /* Walking r->domains, ensure it can't race with cpuhp */
>> lockdep_assert_cpus_held();
>>
>> + if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
>> + rdt_last_cmd_puts("Event configuration(BMEC) not supported
>> with mbm_cntr_assign mode\n");
>> + return -EINVAL;
>> + }
>> +
>> next:
>> if (!tok || tok[0] == '\0')
>> return 0;
>>
>
> Instead of chasing every call that may involve BMEC I think it will be simpler to
> disable BMEC support during initialization when ABMC is detected. Specifically,
> on systems that support both BMEC and ABMC rdt_cpu_has(X86_FEATURE_BMEC) returns
> false.
There is one problem with this approach. Users have the option to switch
between the assignment modes. System will boot with ABMC by default if
supported. But, users can switch to 'default' mode after the boot. By
disabling the BMEC completely, it will not be possible to do that.
>
> I would also like to consider enhancing mevt->configurable to handle all different
> ways in which events can be configured. For example, making mevt->configurable an
> enum that captures how event can be configured instead of keeping mevt->configurable
> a boolean for BMEC support and handling ABMC completely separately. I hope this
> may become clearer when using struct mon_evt for ABMC also.
Sure. I can try that.
--
Thanks
Babu Moger
Hi Babu,
On 4/15/25 12:43 PM, Moger, Babu wrote:
> Hi Reinette,
>
> On 4/15/25 11:09, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 4/14/25 10:48 AM, Moger, Babu wrote:
>>
>>> Here is my proposal to handle this case. This can be separate patch.
>>>
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index d10cf1e5b914..772f2f77faee 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -1370,7 +1370,7 @@ static int rdt_mon_features_show(struct
>>> kernfs_open_file *of,
>>>
>>> list_for_each_entry(mevt, &r->mon.evt_list, list) {
>>> seq_printf(seq, "%s\n", mevt->name);
>>> - if (mevt->configurable)
>>> + if (mevt->configurable &&
>>> !resctrl_arch_mbm_cntr_assign_enabled(r))
>>> seq_printf(seq, "%s_config\n", mevt->name);
>>> }
>>>
>>> @@ -1846,6 +1846,11 @@ static int mbm_config_show(struct seq_file *s,
>>> struct rdt_resource *r, u32 evtid
>>> cpus_read_lock();
>>> mutex_lock(&rdtgroup_mutex);
>>>
>>> + if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
>>> + rdt_last_cmd_puts("Event configuration(BMEC) not supported
>>> with mbm_cntr_assign mode\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> list_for_each_entry(dom, &r->mon_domains, hdr.list) {
>>> if (sep)
>>> seq_puts(s, ";");
>>> @@ -1865,21 +1870,24 @@ static int mbm_config_show(struct seq_file *s,
>>> struct rdt_resource *r, u32 evtid
>>> static int mbm_total_bytes_config_show(struct kernfs_open_file *of,
>>> struct seq_file *seq, void *v)
>>> {
>>> + int ret;
>>> struct rdt_resource *r = of->kn->parent->priv;
>>>
>>> - mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
>>> + ret = mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
>>>
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>>> struct seq_file *seq, void *v)
>>> {
>>> + int ret;
>>> +
>>> struct rdt_resource *r = of->kn->parent->priv;
>>>
>>> - mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
>>> + ret = mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
>>>
>>> - return 0;
>>> + return ret;
>>> }
>>>
>>> static void mbm_config_write_domain(struct rdt_resource *r,
>>> @@ -1932,6 +1940,11 @@ static int mon_config_write(struct rdt_resource *r,
>>> char *tok, u32 evtid)
>>> /* Walking r->domains, ensure it can't race with cpuhp */
>>> lockdep_assert_cpus_held();
>>>
>>> + if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
>>> + rdt_last_cmd_puts("Event configuration(BMEC) not supported
>>> with mbm_cntr_assign mode\n");
>>> + return -EINVAL;
>>> + }
>>> +
>>> next:
>>> if (!tok || tok[0] == '\0')
>>> return 0;
>>>
>>
>> Instead of chasing every call that may involve BMEC I think it will be simpler to
>> disable BMEC support during initialization when ABMC is detected. Specifically,
>> on systems that support both BMEC and ABMC rdt_cpu_has(X86_FEATURE_BMEC) returns
>> false.
>
> There is one problem with this approach. Users have the option to switch
> between the assignment modes. System will boot with ABMC by default if
> supported. But, users can switch to 'default' mode after the boot. By
> disabling the BMEC completely, it will not be possible to do that.
Good point. Thank you. Another option is to hide (see kernfs_show()) mbm_total_bytes_config
and mbm_local_bytes_config when ABMC is enabled. To me this seems like a clear
interface to user space, when user interface changes the mode the interface changes
to reflect new mode.
>
>>
>> I would also like to consider enhancing mevt->configurable to handle all different
>> ways in which events can be configured. For example, making mevt->configurable an
>> enum that captures how event can be configured instead of keeping mevt->configurable
>> a boolean for BMEC support and handling ABMC completely separately. I hope this
>> may become clearer when using struct mon_evt for ABMC also.
>
> Sure. I can try that.
Thank you.
Reinette
Hi Reinette,
On 4/16/25 11:08, Reinette Chatre wrote:
> Hi Babu,
>
> On 4/15/25 12:43 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 4/15/25 11:09, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 4/14/25 10:48 AM, Moger, Babu wrote:
>>>
>>>> Here is my proposal to handle this case. This can be separate patch.
>>>>
>>>>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index d10cf1e5b914..772f2f77faee 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -1370,7 +1370,7 @@ static int rdt_mon_features_show(struct
>>>> kernfs_open_file *of,
>>>>
>>>> list_for_each_entry(mevt, &r->mon.evt_list, list) {
>>>> seq_printf(seq, "%s\n", mevt->name);
>>>> - if (mevt->configurable)
>>>> + if (mevt->configurable &&
>>>> !resctrl_arch_mbm_cntr_assign_enabled(r))
>>>> seq_printf(seq, "%s_config\n", mevt->name);
>>>> }
>>>>
>>>> @@ -1846,6 +1846,11 @@ static int mbm_config_show(struct seq_file *s,
>>>> struct rdt_resource *r, u32 evtid
>>>> cpus_read_lock();
>>>> mutex_lock(&rdtgroup_mutex);
>>>>
>>>> + if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
>>>> + rdt_last_cmd_puts("Event configuration(BMEC) not supported
>>>> with mbm_cntr_assign mode\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> list_for_each_entry(dom, &r->mon_domains, hdr.list) {
>>>> if (sep)
>>>> seq_puts(s, ";");
>>>> @@ -1865,21 +1870,24 @@ static int mbm_config_show(struct seq_file *s,
>>>> struct rdt_resource *r, u32 evtid
>>>> static int mbm_total_bytes_config_show(struct kernfs_open_file *of,
>>>> struct seq_file *seq, void *v)
>>>> {
>>>> + int ret;
>>>> struct rdt_resource *r = of->kn->parent->priv;
>>>>
>>>> - mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
>>>> + ret = mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID);
>>>>
>>>> - return 0;
>>>> + return ret;
>>>> }
>>>>
>>>> static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
>>>> struct seq_file *seq, void *v)
>>>> {
>>>> + int ret;
>>>> +
>>>> struct rdt_resource *r = of->kn->parent->priv;
>>>>
>>>> - mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
>>>> + ret = mbm_config_show(seq, r, QOS_L3_MBM_LOCAL_EVENT_ID);
>>>>
>>>> - return 0;
>>>> + return ret;
>>>> }
>>>>
>>>> static void mbm_config_write_domain(struct rdt_resource *r,
>>>> @@ -1932,6 +1940,11 @@ static int mon_config_write(struct rdt_resource *r,
>>>> char *tok, u32 evtid)
>>>> /* Walking r->domains, ensure it can't race with cpuhp */
>>>> lockdep_assert_cpus_held();
>>>>
>>>> + if (resctrl_arch_mbm_cntr_assign_enabled(r)) {
>>>> + rdt_last_cmd_puts("Event configuration(BMEC) not supported
>>>> with mbm_cntr_assign mode\n");
>>>> + return -EINVAL;
>>>> + }
>>>> +
>>>> next:
>>>> if (!tok || tok[0] == '\0')
>>>> return 0;
>>>>
>>>
>>> Instead of chasing every call that may involve BMEC I think it will be simpler to
>>> disable BMEC support during initialization when ABMC is detected. Specifically,
>>> on systems that support both BMEC and ABMC rdt_cpu_has(X86_FEATURE_BMEC) returns
>>> false.
>>
>> There is one problem with this approach. Users have the option to switch
>> between the assignment modes. System will boot with ABMC by default if
>> supported. But, users can switch to 'default' mode after the boot. By
>> disabling the BMEC completely, it will not be possible to do that.
>
> Good point. Thank you. Another option is to hide (see kernfs_show()) mbm_total_bytes_config
> and mbm_local_bytes_config when ABMC is enabled. To me this seems like a clear
> interface to user space, when user interface changes the mode the interface changes
> to reflect new mode.
Sure. Will try this. Thanks for the pointer.
--
Thanks
Babu Moger
© 2016 - 2025 Red Hat, Inc.