Hi Reinette,
On 2/5/2025 5:58 PM, Reinette Chatre wrote:
Hi Babu,
On 1/22/25 12:20 PM, Babu Moger wrote:
The event configuration is domain specific and initialized during domain
initialization. The values are stored in struct rdt_hw_mon_domain.
It is not required to read the configuration register every time user asks
for it. Use the value stored in struct rdt_hw_mon_domain instead.
Introduce resctrl_arch_mon_event_config_get() and
resctrl_arch_mon_event_config_set() to get/set architecture domain specific
mbm_total_cfg/mbm_local_cfg values.
Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
---
---
arch/x86/kernel/cpu/resctrl/internal.h | 15 +++++++
arch/x86/kernel/cpu/resctrl/monitor.c | 46 +++++++++++++++++++
arch/x86/kernel/cpu/resctrl/rdtgroup.c | 61 +++++---------------------
3 files changed, 72 insertions(+), 50 deletions(-)
diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index ab28b9340ee7..cfaea20145d0 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -605,6 +605,18 @@ union cpuid_0x10_x_edx {
unsigned int full;
};
+/**
+ * struct mon_config_info - Monitoring event configuratiin details
Same typo as previous version.
I am really sorry about this.
+ * @d: Domain for the event
+ * @evtid: Event type
+ * @mon_config: Event configuration value
+ */
+struct mon_config_info {
+ struct rdt_mon_domain *d;
+ enum resctrl_event_id evtid;
+ u32 mon_config;
+};
+
void rdt_last_cmd_clear(void);
void rdt_last_cmd_puts(const char *s);
__printf(1, 2)
@@ -674,4 +686,7 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable);
bool resctrl_arch_mbm_cntr_assign_enabled(struct rdt_resource *r);
void arch_mbm_evt_config_init(struct rdt_hw_mon_domain *hw_dom);
unsigned int mon_event_config_index_get(u32 evtid);
+void resctrl_arch_mon_event_config_set(void *info);
+u32 resctrl_arch_mon_event_config_get(struct rdt_mon_domain *d,
+ enum resctrl_event_id eventid);
#endif /* _ASM_X86_RESCTRL_INTERNAL_H */
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index 8917c7261680..6fe9e610e9a0 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -1324,3 +1324,49 @@ int resctrl_arch_mbm_cntr_assign_set(struct rdt_resource *r, bool enable)
return 0;
}
+
+u32 resctrl_arch_mon_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_mon_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_INDEX)
+ 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_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;
+ break;
+ default:
+ break;
+ }
+}
This new arch API has sharp corners because of asymmetry of where resctrl
runs the arch function. I do not think it is required to change this since we
can only speculate about how this may be used in the future but I do think
it will be helpful to add comments that highlight:
resctrl_arch_mon_event_config_get() -> May run on CPU that does not belong to domain.
resctrl_arch_mon_event_config_set() -> Runs on CPU that belongs to domain.
Sure. will do.
...
@@ -1683,33 +1653,23 @@ 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};
As discussed in previous version it is unnecessary to explicitly initialize
the structure if it is fully initialized in the code. This avoids need for
future cleanups like commit 29eaa7958367 ("x86/resctrl: Slightly clean-up mbm_config_show()")
Yes. Need to change it.
thanks
Babu
