Newer AMD processors support the new feature Bandwidth Monitoring Event
Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes
are configurable when this feature is present.
Set mon_configurable if the feature is available.
Signed-off-by: Babu Moger <babu.moger@amd.com>
Reviewed-by: Ingo Molnar <mingo@kernel.org>
---
arch/x86/kernel/cpu/resctrl/monitor.c | 14 ++++++++++++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 +++++++++++++++++
include/linux/resctrl.h | 1 +
3 files changed, 32 insertions(+)
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index eaf25a234ff5..b9de417dac1c 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -682,6 +682,16 @@ static void l3_mon_evt_init(struct rdt_resource *r)
list_add_tail(&mbm_local_event.list, &r->evt_list);
}
+
+void __rdt_get_mon_l3_config_amd(struct rdt_resource *r)
+{
+ /*
+ * Check if CPU supports the Bandwidth Monitoring Event Configuration
+ */
+ if (boot_cpu_has(X86_FEATURE_BMEC))
+ r->mon_configurable = true;
+}
+
int rdt_get_mon_l3_config(struct rdt_resource *r)
{
unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
@@ -714,6 +724,10 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
if (ret)
return ret;
+ if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
+ __rdt_get_mon_l3_config_amd(r);
+
+
l3_mon_evt_init(r);
r->mon_capable = true;
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index fc5286067201..855483b297a8 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
return 0;
}
+static int rdt_mon_configurable_show(struct kernfs_open_file *of,
+ struct seq_file *seq, void *v)
+{
+ struct rdt_resource *r = of->kn->parent->priv;
+
+ seq_printf(seq, "%d\n", r->mon_configurable);
+
+ return 0;
+}
+
static int rdt_mon_features_show(struct kernfs_open_file *of,
struct seq_file *seq, void *v)
{
@@ -1447,6 +1457,13 @@ static struct rftype res_common_files[] = {
.seq_show = rdt_num_rmids_show,
.fflags = RF_MON_INFO,
},
+ {
+ .name = "mon_configurable",
+ .mode = 0444,
+ .kf_ops = &rdtgroup_kf_single_ops,
+ .seq_show = rdt_mon_configurable_show,
+ .fflags = RF_MON_INFO,
+ },
{
.name = "cbm_mask",
.mode = 0444,
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 21deb5212bbd..4ee2b606ac14 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -154,6 +154,7 @@ struct rdt_resource {
bool mon_enabled;
bool alloc_capable;
bool mon_capable;
+ bool mon_configurable;
int num_rmid;
int cache_level;
struct resctrl_cache cache;
Hi Babu,
On 8/22/2022 6:43 AM, Babu Moger wrote:
> Newer AMD processors support the new feature Bandwidth Monitoring Event
> Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes
> are configurable when this feature is present.
>
> Set mon_configurable if the feature is available.
>
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> Reviewed-by: Ingo Molnar <mingo@kernel.org>
> ---
> arch/x86/kernel/cpu/resctrl/monitor.c | 14 ++++++++++++++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 +++++++++++++++++
> include/linux/resctrl.h | 1 +
> 3 files changed, 32 insertions(+)
>
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index eaf25a234ff5..b9de417dac1c 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -682,6 +682,16 @@ static void l3_mon_evt_init(struct rdt_resource *r)
> list_add_tail(&mbm_local_event.list, &r->evt_list);
> }
>
> +
> +void __rdt_get_mon_l3_config_amd(struct rdt_resource *r)
> +{
> + /*
> + * Check if CPU supports the Bandwidth Monitoring Event Configuration
> + */
> + if (boot_cpu_has(X86_FEATURE_BMEC))
> + r->mon_configurable = true;
> +}
Could this rather use rdt_cpu_has() with the added support for disabling
the feature via kernel parameter?
> +
> int rdt_get_mon_l3_config(struct rdt_resource *r)
> {
> unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
> @@ -714,6 +724,10 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
> if (ret)
> return ret;
>
> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
> + __rdt_get_mon_l3_config_amd(r);
> +
> +
Why is this vendor check needed? Is X86_FEATURE_BMEC not sufficient?
> l3_mon_evt_init(r);
>
> r->mon_capable = true;
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index fc5286067201..855483b297a8 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
> return 0;
> }
>
> +static int rdt_mon_configurable_show(struct kernfs_open_file *of,
> + struct seq_file *seq, void *v)
> +{
> + struct rdt_resource *r = of->kn->parent->priv;
> +
> + seq_printf(seq, "%d\n", r->mon_configurable);
Why is this file needed? It seems that the next patches also introduce
files in support of this new feature that will make the actual configuration
data accessible - those files are only created if this feature is supported.
Would those files not be sufficient for user space to learn about the feature
support?
Reinette
Hi Reinette,
On 8/24/22 16:15, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/22/2022 6:43 AM, Babu Moger wrote:
>> Newer AMD processors support the new feature Bandwidth Monitoring Event
>> Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes
>> are configurable when this feature is present.
>>
>> Set mon_configurable if the feature is available.
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> Reviewed-by: Ingo Molnar <mingo@kernel.org>
>> ---
>> arch/x86/kernel/cpu/resctrl/monitor.c | 14 ++++++++++++++
>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 +++++++++++++++++
>> include/linux/resctrl.h | 1 +
>> 3 files changed, 32 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index eaf25a234ff5..b9de417dac1c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -682,6 +682,16 @@ static void l3_mon_evt_init(struct rdt_resource *r)
>> list_add_tail(&mbm_local_event.list, &r->evt_list);
>> }
>>
>> +
>> +void __rdt_get_mon_l3_config_amd(struct rdt_resource *r)
>> +{
>> + /*
>> + * Check if CPU supports the Bandwidth Monitoring Event Configuration
>> + */
>> + if (boot_cpu_has(X86_FEATURE_BMEC))
>> + r->mon_configurable = true;
>> +}
> Could this rather use rdt_cpu_has() with the added support for disabling
> the feature via kernel parameter?
Yes, That is possible. We could do that.
>
>
>> +
>> int rdt_get_mon_l3_config(struct rdt_resource *r)
>> {
>> unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
>> @@ -714,6 +724,10 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>> if (ret)
>> return ret;
>>
>> + if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
>> + __rdt_get_mon_l3_config_amd(r);
>> +
>> +
> Why is this vendor check needed? Is X86_FEATURE_BMEC not sufficient?
Yes. I can remove the vendor check.
>
>> l3_mon_evt_init(r);
>>
>> r->mon_capable = true;
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index fc5286067201..855483b297a8 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
>> return 0;
>> }
>>
>> +static int rdt_mon_configurable_show(struct kernfs_open_file *of,
>> + struct seq_file *seq, void *v)
>> +{
>> + struct rdt_resource *r = of->kn->parent->priv;
>> +
>> + seq_printf(seq, "%d\n", r->mon_configurable);
> Why is this file needed? It seems that the next patches also introduce
> files in support of this new feature that will make the actual configuration
> data accessible - those files are only created if this feature is supported.
> Would those files not be sufficient for user space to learn about the feature
> support?
This is part of /sys/fs/resctrl/info/L3_MON# directory which basically has
the information about all the monitoring features. As this is one of the
mon features, I have added it there. Also, this can be used from the
utility(like pqos or rdtset) to check if the configuring the monitor is
supported without looking at individual files. It makes things easier.
Thanks
Babu
> Reinette
--
Thanks
Babu Moger
Hi Babu,
On 8/25/2022 8:11 AM, Moger, Babu wrote:
> On 8/24/22 16:15, Reinette Chatre wrote:
>> On 8/22/2022 6:43 AM, Babu Moger wrote:
>>> Newer AMD processors support the new feature Bandwidth Monitoring Event
>>> Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes
>>> are configurable when this feature is present.
>>>
>>> Set mon_configurable if the feature is available.
>>>
...
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index fc5286067201..855483b297a8 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
>>> return 0;
>>> }
>>>
>>> +static int rdt_mon_configurable_show(struct kernfs_open_file *of,
>>> + struct seq_file *seq, void *v)
>>> +{
>>> + struct rdt_resource *r = of->kn->parent->priv;
>>> +
>>> + seq_printf(seq, "%d\n", r->mon_configurable);
>> Why is this file needed? It seems that the next patches also introduce
>> files in support of this new feature that will make the actual configuration
>> data accessible - those files are only created if this feature is supported.
>> Would those files not be sufficient for user space to learn about the feature
>> support?
>
> This is part of /sys/fs/resctrl/info/L3_MON# directory which basically has
> the information about all the monitoring features. As this is one of the
> mon features, I have added it there. Also, this can be used from the
> utility(like pqos or rdtset) to check if the configuring the monitor is
> supported without looking at individual files. It makes things easier.
I understand the motivation. My concern is that this is a resource wide
file that will display a binary value that, if true, currently means two
events are configurable. We need to consider how things can change in the
future. We should consider that this is only the beginning of monitoring
configuration and need this interface to be ready for future changes. For
example, what if all of the monitoring events are configurable? Let's say,
for example, in future AMD hardware the "llc_occupancy" event also becomes
configurable, how should info/L3_MON/configurable be interpreted? On some
machines it would thus mean that mbm_total_bytes and mbm_local_bytes are
configurable and on some machines it would mean that mbm_total_bytes,
mbm_local_bytes, and llc_occupancy are configurable. This does not make
it easy for utilities.
So, in this series the info directory on a system that supports BMEC
would look like:
info/L3_MON/mon_features:llc_occupancy
info/L3_MON/mon_features:mbm_total_bytes
info/L3_MON/mon_features:mbm_local_bytes
info/L3_MON/configurable:1
Would information like below not be more specific?
info/L3_MON/mon_features:llc_occupancy
info/L3_MON/mon_features:mbm_total_bytes
info/L3_MON/mon_features:mbm_local_bytes
info/L3_MON/mon_features:mbm_total_config
info/L3_MON/mon_features:mbm_local_config
Reinette
On 8/25/2022 10:56 AM, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/25/2022 8:11 AM, Moger, Babu wrote:
>> On 8/24/22 16:15, Reinette Chatre wrote:
>>> On 8/22/2022 6:43 AM, Babu Moger wrote:
>>>> Newer AMD processors support the new feature Bandwidth Monitoring Event
>>>> Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes
>>>> are configurable when this feature is present.
>>>>
>>>> Set mon_configurable if the feature is available.
>>>>
> ...
>
>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> index fc5286067201..855483b297a8 100644
>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>> @@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
>>>> return 0;
>>>> }
>>>>
>>>> +static int rdt_mon_configurable_show(struct kernfs_open_file *of,
>>>> + struct seq_file *seq, void *v)
>>>> +{
>>>> + struct rdt_resource *r = of->kn->parent->priv;
>>>> +
>>>> + seq_printf(seq, "%d\n", r->mon_configurable);
>>> Why is this file needed? It seems that the next patches also introduce
>>> files in support of this new feature that will make the actual configuration
>>> data accessible - those files are only created if this feature is supported.
>>> Would those files not be sufficient for user space to learn about the feature
>>> support?
>> This is part of /sys/fs/resctrl/info/L3_MON# directory which basically has
>> the information about all the monitoring features. As this is one of the
>> mon features, I have added it there. Also, this can be used from the
>> utility(like pqos or rdtset) to check if the configuring the monitor is
>> supported without looking at individual files. It makes things easier.
> I understand the motivation. My concern is that this is a resource wide
> file that will display a binary value that, if true, currently means two
> events are configurable. We need to consider how things can change in the
> future. We should consider that this is only the beginning of monitoring
> configuration and need this interface to be ready for future changes. For
> example, what if all of the monitoring events are configurable? Let's say,
> for example, in future AMD hardware the "llc_occupancy" event also becomes
> configurable, how should info/L3_MON/configurable be interpreted? On some
> machines it would thus mean that mbm_total_bytes and mbm_local_bytes are
> configurable and on some machines it would mean that mbm_total_bytes,
> mbm_local_bytes, and llc_occupancy are configurable. This does not make
> it easy for utilities.
>
> So, in this series the info directory on a system that supports BMEC
> would look like:
>
> info/L3_MON/mon_features:llc_occupancy
> info/L3_MON/mon_features:mbm_total_bytes
> info/L3_MON/mon_features:mbm_local_bytes
> info/L3_MON/configurable:1
>
> Would information like below not be more specific?
> info/L3_MON/mon_features:llc_occupancy
> info/L3_MON/mon_features:mbm_total_bytes
> info/L3_MON/mon_features:mbm_local_bytes
> info/L3_MON/mon_features:mbm_total_config
> info/L3_MON/mon_features:mbm_local_config
Hi Reinette,
Yes. That is more specific.
So, basically your idea is to print the information from mon_evt
structure if mon_configarable is set in the resource structure.
Some thing like ..
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index 83c8780726ff..46c6bf3f08e3 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1194,8 +1194,11 @@ static int rdt_mon_features_show(struct
kernfs_open_file *of,
struct rdt_resource *r = of->kn->parent->priv;
struct mon_evt *mevt;
- list_for_each_entry(mevt, &r->evt_list, list)
+ list_for_each_entry(mevt, &r->evt_list, list) {
seq_printf(seq, "%s\n", mevt->name);
+ if (r->mon_configurable)
+ seq_printf(seq, "%s\n", mevt->config);
+ }
return 0;
}
Is that the idea?
Thanks
Babu
Hi Babu,
On 8/25/2022 1:44 PM, Moger, Babu wrote:
>
> On 8/25/2022 10:56 AM, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 8/25/2022 8:11 AM, Moger, Babu wrote:
>>> On 8/24/22 16:15, Reinette Chatre wrote:
>>>> On 8/22/2022 6:43 AM, Babu Moger wrote:
>>>>> Newer AMD processors support the new feature Bandwidth Monitoring Event
>>>>> Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes
>>>>> are configurable when this feature is present.
>>>>>
>>>>> Set mon_configurable if the feature is available.
>>>>>
>> ...
>>
>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>> index fc5286067201..855483b297a8 100644
>>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>> @@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
>>>>> return 0;
>>>>> }
>>>>> +static int rdt_mon_configurable_show(struct kernfs_open_file *of,
>>>>> + struct seq_file *seq, void *v)
>>>>> +{
>>>>> + struct rdt_resource *r = of->kn->parent->priv;
>>>>> +
>>>>> + seq_printf(seq, "%d\n", r->mon_configurable);
>>>> Why is this file needed? It seems that the next patches also introduce
>>>> files in support of this new feature that will make the actual configuration
>>>> data accessible - those files are only created if this feature is supported.
>>>> Would those files not be sufficient for user space to learn about the feature
>>>> support?
>>> This is part of /sys/fs/resctrl/info/L3_MON# directory which basically has
>>> the information about all the monitoring features. As this is one of the
>>> mon features, I have added it there. Also, this can be used from the
>>> utility(like pqos or rdtset) to check if the configuring the monitor is
>>> supported without looking at individual files. It makes things easier.
>> I understand the motivation. My concern is that this is a resource wide
>> file that will display a binary value that, if true, currently means two
>> events are configurable. We need to consider how things can change in the
>> future. We should consider that this is only the beginning of monitoring
>> configuration and need this interface to be ready for future changes. For
>> example, what if all of the monitoring events are configurable? Let's say,
>> for example, in future AMD hardware the "llc_occupancy" event also becomes
>> configurable, how should info/L3_MON/configurable be interpreted? On some
>> machines it would thus mean that mbm_total_bytes and mbm_local_bytes are
>> configurable and on some machines it would mean that mbm_total_bytes,
>> mbm_local_bytes, and llc_occupancy are configurable. This does not make
>> it easy for utilities.
>>
>> So, in this series the info directory on a system that supports BMEC
>> would look like:
>>
>> info/L3_MON/mon_features:llc_occupancy
>> info/L3_MON/mon_features:mbm_total_bytes
>> info/L3_MON/mon_features:mbm_local_bytes
>> info/L3_MON/configurable:1
>>
>> Would information like below not be more specific?
>> info/L3_MON/mon_features:llc_occupancy
>> info/L3_MON/mon_features:mbm_total_bytes
>> info/L3_MON/mon_features:mbm_local_bytes
>> info/L3_MON/mon_features:mbm_total_config
>> info/L3_MON/mon_features:mbm_local_config
>
> Hi Reinette,
>
> Yes. That is more specific.
>
> So, basically your idea is to print the information from mon_evt structure if mon_configarable is set in the resource structure.
>
> Some thing like ..
>
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 83c8780726ff..46c6bf3f08e3 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1194,8 +1194,11 @@ static int rdt_mon_features_show(struct kernfs_open_file *of,
> struct rdt_resource *r = of->kn->parent->priv;
> struct mon_evt *mevt;
>
> - list_for_each_entry(mevt, &r->evt_list, list)
> + list_for_each_entry(mevt, &r->evt_list, list) {
> seq_printf(seq, "%s\n", mevt->name);
> + if (r->mon_configurable)
> + seq_printf(seq, "%s\n", mevt->config);
> + }
>
> return 0;
> }
>
> Is that the idea?
I do not see why struct rdt_resource->configurable is needed. Again, this
is a resource wide property with an implicit meaning related to only two
event counters. Again, what if AMD later makes the llc_occupancy event counter
configurable? How can resctrl know, using "r->mon_configurable" whether
it should print mevt->config?
How about:
if (mevt->config)
seq_printf(seq, "%s\n", mevt->config);
As I mentioned in [1], mevt->config can be set in rdt_get_mon_l3_config()
based on a check on the BMEC feature instead of hardcoded as it is now.
Or, if the string manipulation is of concern the hardcoding of mevt->config
(perhaps then mevt->config_name) could remain and a new mevt->configurable
could be set from rdt_get_mon_l3_config() and then the above could be:
if (mevt->configurable)
seq_printf(seq, "%s\n", mevt->config_name);
Reinette
[1] https://lore.kernel.org/lkml/c5777707-746e-edab-2ce2-3405ff55be56@intel.com/
Hi Reinette, Some reason this thread did not land in my mailbox. Replying using git sendmail to the thread Yes. This should work. I will try that. Thanks Babu
© 2016 - 2026 Red Hat, Inc.