Hi Babu,
On 7/3/24 2:48 PM, Babu Moger wrote:
The event configuration is domain specific and initialized during domain
initialization. It is not required to read the configuration register
every time user asks for it. Use the value stored in rdt_mon_hw_domain
rdt_mon_hw_domain -> rdt_hw_mon_domain
instead. Also update the configuration value when user writes it.
Please separate the context/problem/solution clearly.
Introduce resctrl_arch_event_config_get() and
resctrl_arch_event_config_set() to get/set architecture domain specific
mbm_total_cfg/mbm_local_cfg values.
Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
v5: Introduced resctrl_arch_event_config_get and
resctrl_arch_event_config_get() based on our discussion.
https://lore.kernel.org/lkml/68e861f9-245d-4496-a72e-46fc57d19c62@xxxxxxx/
v4: New patch.
---
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 112 +++++++++++++++----------
include/linux/resctrl.h | 4 +
2 files changed, 72 insertions(+), 44 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
index b2b751741dd8..91c5d45ac367 100644
--- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
+++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
@@ -1591,10 +1591,59 @@ static int rdtgroup_size_show(struct kernfs_open_file *of,
}
struct mon_config_info {
+ struct rdt_mon_domain *d;
u32 evtid;
u32 mon_config;
};
as seen above, mon_config is a u32
+#define INVALID_CONFIG_VALUE UINT_MAX
So an invalid config value can be U32_MAX?
+
+unsigned int resctrl_arch_event_config_get(struct rdt_mon_domain *d,
+ enum resctrl_event_id eventid)
+{
+ struct rdt_hw_mon_domain *hw_dom = resctrl_to_arch_mon_dom(d);
+
+ switch (eventid) {
+ case QOS_L3_OCCUP_EVENT_ID:
+ break;
+ case QOS_L3_MBM_TOTAL_EVENT_ID:
+ return hw_dom->mbm_total_cfg;
+ case QOS_L3_MBM_LOCAL_EVENT_ID:
+ return hw_dom->mbm_local_cfg;
+ }
+
+ /* Never expect to get here */
+ WARN_ON_ONCE(1);
+
+ return INVALID_CONFIG_VALUE;
+}
+
+void resctrl_arch_event_config_set(void *info)
+{
+ struct mon_config_info *mon_info = info;
+ struct rdt_hw_mon_domain *hw_dom;
+ unsigned int index;
+
+ index = mon_event_config_index_get(mon_info->evtid);
+ if (index == INVALID_CONFIG_VALUE) {
INVALID_CONFIG_INDEX?
+ pr_warn_once("Invalid event id %d\n", mon_info->evtid);
+ return;
+ }
+ wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
+
+ hw_dom = resctrl_to_arch_mon_dom(mon_info->d);
+
+ switch (mon_info->evtid) {
+ case QOS_L3_OCCUP_EVENT_ID:
+ break;
+ case QOS_L3_MBM_TOTAL_EVENT_ID:
+ hw_dom->mbm_total_cfg = mon_info->mon_config;
+ break;
+ case QOS_L3_MBM_LOCAL_EVENT_ID:
+ hw_dom->mbm_local_cfg = mon_info->mon_config;
Please add a break here.
+ }
+}
+
#define INVALID_CONFIG_INDEX UINT_MAX
/**
@@ -1619,33 +1668,11 @@ unsigned int mon_event_config_index_get(u32 evtid)
}
}
-static void mon_event_config_read(void *info)
-{
- struct mon_config_info *mon_info = info;
- unsigned int index;
- u64 msrval;
-
- index = mon_event_config_index_get(mon_info->evtid);
- if (index == INVALID_CONFIG_INDEX) {
- pr_warn_once("Invalid event id %d\n", mon_info->evtid);
- return;
- }
- rdmsrl(MSR_IA32_EVT_CFG_BASE + index, msrval);
-
- /* Report only the valid event configuration bits */
- mon_info->mon_config = msrval & MAX_EVT_CONFIG_BITS;
-}
-
-static void mondata_config_read(struct rdt_mon_domain *d, struct mon_config_info *mon_info)
-{
- smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_read, mon_info, 1);
-}
-
static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid)
{
- struct mon_config_info mon_info = {0};
struct rdt_mon_domain *dom;
bool sep = false;
+ int val;
cpus_read_lock();
mutex_lock(&rdtgroup_mutex);
@@ -1654,11 +1681,13 @@ static int mbm_config_show(struct seq_file *s, struct rdt_resource *r, u32 evtid
if (sep)
seq_puts(s, ";");
- memset(&mon_info, 0, sizeof(struct mon_config_info));
- mon_info.evtid = evtid;
- mondata_config_read(dom, &mon_info);
+ val = resctrl_arch_event_config_get(dom, evtid);
There are too many types used interchangeably. The mon_config is a "u32", but the new function
returns "unsigned int", which is then assigned to an "int". Please just use one type
consistently, it is a u32 so resctrl_arch_event_config_get() can return u32 and "val" should
be u32.
+ if (val == INVALID_CONFIG_VALUE) {
+ rdt_last_cmd_puts("Invalid event configuration\n");
I do not see a reason to print message to user space here. If this error is encountered
then it is a kernel bug and resctrl_arch_event_config_get() would already have triggered
a WARN.
Since this is a "never should happen" scenario I wonder if we can not just print
the INVALID_CONFIG_VALUE to user space?
+ break;
+ }
- seq_printf(s, "%d=0x%02x", dom->hdr.id, mon_info.mon_config);
+ seq_printf(s, "%d=0x%02x", dom->hdr.id, val);
sep = true;
}
seq_puts(s, "\n");
@@ -1689,33 +1718,27 @@ static int mbm_local_bytes_config_show(struct kernfs_open_file *of,
return 0;
}
-static void mon_event_config_write(void *info)
-{
- struct mon_config_info *mon_info = info;
- unsigned int index;
-
- index = mon_event_config_index_get(mon_info->evtid);
- if (index == INVALID_CONFIG_INDEX) {
- pr_warn_once("Invalid event id %d\n", mon_info->evtid);
- return;
- }
- wrmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, 0);
-}
static void mbm_config_write_domain(struct rdt_resource *r,
struct rdt_mon_domain *d, u32 evtid, u32 val)
{
struct mon_config_info mon_info = {0};
+ int config_val;
/*
- * Read the current config value first. If both are the same then
+ * Check the current config value first. If both are the same then
* no need to write it again.
*/
- mon_info.evtid = evtid;
- mondata_config_read(d, &mon_info);
- if (mon_info.mon_config == val)
+ config_val = resctrl_arch_event_config_get(d, evtid);
+ if (config_val == INVALID_CONFIG_VALUE) {
+ rdt_last_cmd_puts("Invalid event configuration\n");
same here about unneeded print to user space. When this is encountered it is
a kernel bug.
+ return;
+ }
+ if (config_val == val)
return;
+ mon_info.d = d;
+ mon_info.evtid = evtid;
mon_info.mon_config = val;
/*
@@ -1724,7 +1747,8 @@ static void mbm_config_write_domain(struct rdt_resource *r,
* are scoped at the domain level. Writing any of these MSRs
* on one CPU is observed by all the CPUs in the domain.
*/
- smp_call_function_any(&d->hdr.cpu_mask, mon_event_config_write,
+ smp_call_function_any(&d->hdr.cpu_mask,
+ resctrl_arch_event_config_set,
&mon_info, 1);
/*
diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
index 62f0f002ef41..f017258ebf85 100644
--- a/include/linux/resctrl.h
+++ b/include/linux/resctrl.h
@@ -352,6 +352,10 @@ void resctrl_arch_reset_rmid(struct rdt_resource *r, struct rdt_mon_domain *d,
*/
void resctrl_arch_reset_rmid_all(struct rdt_resource *r, struct rdt_mon_domain *d);
+void resctrl_arch_event_config_set(void *info);
+unsigned int resctrl_arch_event_config_get(struct rdt_mon_domain *d,
+ enum resctrl_event_id eventid);
+
extern unsigned int resctrl_rmid_realloc_threshold;
extern unsigned int resctrl_rmid_realloc_limit;
Reinette
