Hi Reinette, On 8/16/24 16:36, Reinette Chatre wrote: > Hi Babu, > > On 8/6/24 3:00 PM, Babu Moger wrote: >> The event configuration is domain specific and initialized during domain >> initialization. The values is stored in rdt_hw_mon_domain. > > "The values is stored in rdt_hw_mon_domain." -> "The values are stored > in struct rdt_hw_mon_domain." Sure. > >> >> It is not required to read the configuration register every time user asks >> for it. Use the value stored in rdt_hw_mon_domain instead. > > "rdt_hw_mon_domain" -> "struct rdt_hw_mon_domain" Sure. > >> >> 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. Also, remove unused config value >> definitions. > > hmmm ... while the config values are not used they are now established > ABI and any other architecture that wants to support configurable events > will need to follow these definitions. It is thus required to keep them > documented in the kernel in support of future changes. I > understand that they are documented in user docs, but could we keep them > in the kernel code also? Since they are unused they could perhaps be moved > to comments as a compromise? How about just keeping them as is? I will just not remove it. > >> >> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >> --- >> v6: Fixed inconstancy with types. Made all the types to u32 for config >> value. >> Removed few rdt_last_cmd_puts as it is not necessary. >> Removed unused config value definitions. >> Few more updates to commit message. >> >> 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/internal.h | 21 ----- >> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 104 ++++++++++++++----------- >> include/linux/resctrl.h | 4 + >> 3 files changed, 64 insertions(+), 65 deletions(-) >> >> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h >> b/arch/x86/kernel/cpu/resctrl/internal.h >> index 4d8cc36a8d79..1021227d8c7e 100644 >> --- a/arch/x86/kernel/cpu/resctrl/internal.h >> +++ b/arch/x86/kernel/cpu/resctrl/internal.h >> @@ -32,27 +32,6 @@ >> */ >> #define MBM_CNTR_WIDTH_OFFSET_MAX (62 - MBM_CNTR_WIDTH_BASE) >> -/* Reads to Local DRAM Memory */ >> -#define READS_TO_LOCAL_MEM BIT(0) >> - >> -/* Reads to Remote DRAM Memory */ >> -#define READS_TO_REMOTE_MEM BIT(1) >> - >> -/* Non-Temporal Writes to Local Memory */ >> -#define NON_TEMP_WRITE_TO_LOCAL_MEM BIT(2) >> - >> -/* Non-Temporal Writes to Remote Memory */ >> -#define NON_TEMP_WRITE_TO_REMOTE_MEM BIT(3) >> - >> -/* Reads to Local Memory the system identifies as "Slow Memory" */ >> -#define READS_TO_LOCAL_S_MEM BIT(4) >> - >> -/* Reads to Remote Memory the system identifies as "Slow Memory" */ >> -#define READS_TO_REMOTE_S_MEM BIT(5) >> - >> -/* Dirty Victims to All Types of Memory */ >> -#define DIRTY_VICTIMS_TO_ALL_MEM BIT(6) >> - >> /* Max event bits supported */ >> #define MAX_EVT_CONFIG_BITS GENMASK(6, 0) >> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> index 02afd3442876..0047b4eb0ff5 100644 >> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >> @@ -1605,10 +1605,57 @@ static int rdtgroup_size_show(struct >> kernfs_open_file *of, >> } >> struct mon_config_info { >> + struct rdt_mon_domain *d; >> u32 evtid; >> u32 mon_config; >> }; >> +u32 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_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_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; >> + break; >> + } >> +} >> + >> /** >> * mon_event_config_index_get - get the hardware index for the >> * configurable event >> @@ -1631,33 +1678,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; >> + u32 val; >> cpus_read_lock(); >> mutex_lock(&rdtgroup_mutex); >> @@ -1666,11 +1691,11 @@ 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); >> + if (val == INVALID_CONFIG_VALUE) > > Can this check and the "break" that follows be dropped? val being > INVALID_CONFIG_VALUE would be a kernel bug and > resctrl_arch_event_config_get() > would already have printed the WARN. In this unlikely scenario I find it > unexpected that mbm_config_show() will return success in this case and the > below seq_printf() would handle the printing of INVALID_CONFIG_VALUE without > issue anyway. Sure. I will drop the check and break. > >> + 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"); > > Reinette > > -- Thanks Babu Moger
