Hi Reinette, On 7/12/24 17:10, Reinette Chatre wrote: > 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. 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. >> >> 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? Sure. > >> + >> +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? Yes. > >> + 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. Sure. > >> + } >> +} >> + >> #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. > Sure. Will do. >> + 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? Ok. Will remove the check and the message. > > >> + 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. Ok. Will remove the check and the message. > >> + 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 > -- Thanks Babu Moger
