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
