Hi Babu, On 11/4/2022 1:00 PM, Babu Moger wrote: > The current event configuration can be viewed by the user by reading > the configuration file /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config. > The event configuration settings are domain specific and will affect all > the CPUs in the domain. > > Following are the types of events supported: > ==== =========================================================== > Bits Description > ==== =========================================================== > 6 Dirty Victims from the QOS domain to all types of memory > 5 Reads to slow memory in the non-local NUMA domain > 4 Reads to slow memory in the local NUMA domain > 3 Non-temporal writes to non-local NUMA domain > 2 Non-temporal writes to local NUMA domain > 1 Reads to memory in the non-local NUMA domain > 0 Reads to memory in the local NUMA domain > ==== =========================================================== > > By default, the mbm_total_bytes_config is set to 0x7f to count all the > event types. > > For example: > $cat /sys/fs/resctrl/info/L3_MON/mbm_total_bytes_config > 0=0x7f;1=0x7f;2=0x7f;3=0x7f > > In this case, the event mbm_total_bytes is currently configured > with 0x7f on domains 0 to 3. > > Signed-off-by: Babu Moger <babu.moger@xxxxxxx> > --- > arch/x86/kernel/cpu/resctrl/internal.h | 28 ++++++++++ > arch/x86/kernel/cpu/resctrl/monitor.c | 1 > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 87 ++++++++++++++++++++++++++++++++ > 3 files changed, 116 insertions(+) > > diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h > index 5459b5022760..c74285fd0f6e 100644 > --- a/arch/x86/kernel/cpu/resctrl/internal.h > +++ b/arch/x86/kernel/cpu/resctrl/internal.h > @@ -15,6 +15,7 @@ > #define MSR_IA32_MBA_THRTL_BASE 0xd50 > #define MSR_IA32_MBA_BW_BASE 0xc0000200 > #define MSR_IA32_SMBA_BW_BASE 0xc0000280 > +#define MSR_IA32_EVT_CFG_BASE 0xc0000400 > > #define MSR_IA32_QM_CTR 0x0c8e > #define MSR_IA32_QM_EVTSEL 0x0c8d > @@ -41,6 +42,32 @@ > */ > #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) > + > +/* Max configurable events */ > +#define MAX_CONFIG_EVENTS 2 > This max being disconnected from what it is a max of looks like a source of future confusion. > struct rdt_fs_context { > struct kernfs_fs_context kfc; > @@ -542,5 +569,6 @@ bool has_busy_rmid(struct rdt_resource *r, struct rdt_domain *d); > void __check_limbo(struct rdt_domain *d, bool force_free); > void rdt_domain_reconfigure_cdp(struct rdt_resource *r); > void __init thread_throttle_mode_init(void); > +void mbm_config_rftype_init(void); > > #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 06c2dc980855..a188dacab6c8 100644 > --- a/arch/x86/kernel/cpu/resctrl/monitor.c > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c > @@ -787,6 +787,7 @@ int rdt_get_mon_l3_config(struct rdt_resource *r) > if (mon_configurable) { > mbm_total_event.configurable = true; > mbm_local_event.configurable = true; > + mbm_config_rftype_init(); > } > > l3_mon_evt_init(r); > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > index 8342feb54a7f..dea58b6b4aa4 100644 > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c > @@ -1423,6 +1423,78 @@ static int rdtgroup_size_show(struct kernfs_open_file *of, > return ret; > } > > +struct mon_config_info { > + u32 evtid; > + u32 mon_config; > +}; > + > +/** > + * mon_event_config_index_get - get the index for the configurable event > + * @evtid: event id. > + * > + * Return: 0 for evtid == QOS_L3_MBM_TOTAL_EVENT_ID > + * 1 for evtid == QOS_L3_MBM_LOCAL_EVENT_ID > + * > 1 otherwise > + */ > +static inline unsigned int mon_event_config_index_get(u32 evtid) > +{ > + return evtid - QOS_L3_MBM_TOTAL_EVENT_ID; > +} It seems strange that the validation of the index is split from where the index is determined. I think it would be easier to understand, and reduce code duplication, it if is done together. How about: #define INVALID_CONFIG_INDEX UINT_MAX static inline unsigned int mon_event_config_index_get(u32 evtid) { switch (evtid) { case QOS_L3_MBM_TOTAL_EVENT_ID: return 0; case QOS_L3_MBM_LOCAL_EVENT_ID: return 1; default: /* WARN */ return INVALID_CONFIG_INDEX; } } What do you think? > + > +static void mon_event_config_read(void *info) > +{ > + struct mon_config_info *mon_info = info; > + u32 h, index; > + > + index = mon_event_config_index_get(mon_info->evtid); > + if (index >= MAX_CONFIG_EVENTS) { > + pr_warn_once("Invalid event id %d\n", mon_info->evtid); > + return; > + } > + rdmsr(MSR_IA32_EVT_CFG_BASE + index, mon_info->mon_config, h); > +} > + > +static void mondata_config_read(struct rdt_domain *d, struct mon_config_info *mon_info) > +{ > + smp_call_function_any(&d->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_domain *dom; > + bool sep = false; > + > + mutex_lock(&rdtgroup_mutex); > + > + list_for_each_entry(dom, &r->domains, list) { > + if (sep) > + seq_puts(s, ";"); > + > + mon_info.evtid = evtid; > + mondata_config_read(dom, &mon_info); > + > + seq_printf(s, "%d=0x%02lx", dom->id, This is a u32 ... is just x sufficient? > + mon_info.mon_config & MAX_EVT_CONFIG_BITS); Please do this masking within mondata_config_read(). It should not be required for every mon_config_read() caller to validate the data because they may forget (re. patch 10). > + sep = true; > + } > + seq_puts(s, "\n"); > + > + mutex_unlock(&rdtgroup_mutex); > + > + return 0; > +} > + > +static int mbm_total_bytes_config_show(struct kernfs_open_file *of, > + struct seq_file *seq, void *v) > +{ > + struct rdt_resource *r = of->kn->parent->priv; > + > + mbm_config_show(seq, r, QOS_L3_MBM_TOTAL_EVENT_ID); > + > + return 0; > +} > + > /* rdtgroup information files for one cache resource. */ > static struct rftype res_common_files[] = { > { > @@ -1521,6 +1593,12 @@ static struct rftype res_common_files[] = { > .seq_show = max_threshold_occ_show, > .fflags = RF_MON_INFO | RFTYPE_RES_CACHE, > }, > + { > + .name = "mbm_total_bytes_config", > + .mode = 0444, > + .kf_ops = &rdtgroup_kf_single_ops, > + .seq_show = mbm_total_bytes_config_show, > + }, > { > .name = "cpus", > .mode = 0644, > @@ -1627,6 +1705,15 @@ void __init thread_throttle_mode_init(void) > rft->fflags = RF_CTRL_INFO | RFTYPE_RES_MB; > } > > +void mbm_config_rftype_init(void) Does this need __init? > +{ > + struct rftype *rft; > + > + rft = rdtgroup_get_rftype_by_name("mbm_total_bytes_config"); > + if (rft) > + rft->fflags = RF_MON_INFO | RFTYPE_RES_CACHE; > +} > + > /** > * rdtgroup_kn_mode_restrict - Restrict user access to named resctrl file > * @r: The resource group with which the file is associated. > > Reinette