[AMD Official Use Only - General]
Hi Fenghua,
> -----Original Message-----
> From: Yu, Fenghua <fenghua.yu@xxxxxxxxx>
> Sent: Tuesday, September 27, 2022 5:25 PM
> To: Moger, Babu <Babu.Moger@xxxxxxx>; corbet@xxxxxxx; Chatre, Reinette
> <reinette.chatre@xxxxxxxxx>; tglx@xxxxxxxxxxxxx; mingo@xxxxxxxxxx;
> bp@xxxxxxxxx
> Cc: dave.hansen@xxxxxxxxxxxxxxx; x86@xxxxxxxxxx; hpa@xxxxxxxxx;
> paulmck@xxxxxxxxxx; akpm@xxxxxxxxxxxxxxxxxxxx; quic_neeraju@xxxxxxxxxxx;
> rdunlap@xxxxxxxxxxxxx; damien.lemoal@xxxxxxxxxxxxxxxxxx;
> songmuchun@xxxxxxxxxxxxx; peterz@xxxxxxxxxxxxx; jpoimboe@xxxxxxxxxx;
> pbonzini@xxxxxxxxxx; Bae, Chang Seok <chang.seok.bae@xxxxxxxxx>;
> pawan.kumar.gupta@xxxxxxxxxxxxxxx; jmattson@xxxxxxxxxx;
> daniel.sneddon@xxxxxxxxxxxxxxx; Das1, Sandipan <Sandipan.Das@xxxxxxx>;
> Luck, Tony <tony.luck@xxxxxxxxx>; james.morse@xxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; bagasdotme@xxxxxxxxx;
> Eranian, Stephane <eranian@xxxxxxxxxx>
> Subject: RE: [PATCH v5 06/12] x86/resctrl: Introduce data structure to support
> monitor configuration
>
> Hi, Babu,
>
> > Add couple of fields in mon_evt to support Bandwidth Monitoring Event
> > Configuratio (BMEC) and also update the "mon_features".
>
> s/Configuratio/ Configuration/
Sure.
>
> >
> > The sysfs file "mon_features" will display the monitor configuration if
> supported.
> >
> > Before the change.
> > $cat /sys/fs/resctrl/info/L3_MON/mon_features
> > llc_occupancy
> > mbm_total_bytes
> > mbm_local_bytes
> >
> > After the change if BMEC is supported.
> > $cat /sys/fs/resctrl/info/L3_MON/mon_features
> > llc_occupancy
> > mbm_total_bytes
> > mbm_total_config
> > mbm_local_bytes
> > mbm_local_config
> >
> > Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
> > ---
> > arch/x86/kernel/cpu/resctrl/core.c | 3 ++-
> > arch/x86/kernel/cpu/resctrl/internal.h | 6 +++++-
> > arch/x86/kernel/cpu/resctrl/monitor.c | 9 ++++++++-
> > arch/x86/kernel/cpu/resctrl/rdtgroup.c | 5 ++++-
> > 4 files changed, 19 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/x86/kernel/cpu/resctrl/core.c
> > b/arch/x86/kernel/cpu/resctrl/core.c
> > index 56c96607259c..513e6a00f58e 100644
> > --- a/arch/x86/kernel/cpu/resctrl/core.c
> > +++ b/arch/x86/kernel/cpu/resctrl/core.c
> > @@ -849,6 +849,7 @@ static __init bool get_rdt_alloc_resources(void)
> > static __init bool get_rdt_mon_resources(void) {
> > struct rdt_resource *r =
> > &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
> > + bool mon_configurable = rdt_cpu_has(X86_FEATURE_BMEC);
> >
> > if (rdt_cpu_has(X86_FEATURE_CQM_OCCUP_LLC))
> > rdt_mon_features |= (1 << QOS_L3_OCCUP_EVENT_ID); @@ -
> > 860,7 +861,7 @@ static __init bool get_rdt_mon_resources(void)
> > if (!rdt_mon_features)
> > return false;
> >
> > - return !rdt_get_mon_l3_config(r);
> > + return !rdt_get_mon_l3_config(r, mon_configurable);
> > }
> >
> > static __init void __check_quirks_intel(void) diff --git
> > a/arch/x86/kernel/cpu/resctrl/internal.h
> > b/arch/x86/kernel/cpu/resctrl/internal.h
> > index c049a274383c..4d03f443b353 100644
> > --- a/arch/x86/kernel/cpu/resctrl/internal.h
> > +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> > @@ -72,11 +72,15 @@ DECLARE_STATIC_KEY_FALSE(rdt_mon_enable_key);
> > * struct mon_evt - Entry in the event list of a resource
> > * @evtid: event id
> > * @name: name of the event
> > + * @configurable: true if the event is configurable
> > + * @config_name: sysfs file name of the event if configurable
> > * @list: entry in &rdt_resource->evt_list
> > */
> > struct mon_evt {
> > u32 evtid;
> > char *name;
> > + bool configurable;
> > + char *config_name;
>
> Seems config_name is only used to be shown in mon_features. Is it necessary to
> have the field?
Sure. I can remove it.
>
> > struct list_head list;
> > };
> >
> > @@ -529,7 +533,7 @@ int closids_supported(void); void closid_free(int
> > closid); int alloc_rmid(void); void free_rmid(u32 rmid); -int
> > rdt_get_mon_l3_config(struct rdt_resource *r);
> > +int rdt_get_mon_l3_config(struct rdt_resource *r, bool configurable);
> > void mon_event_count(void *info);
> > int rdtgroup_mondata_show(struct seq_file *m, void *arg); void
> > rmdir_mondata_subdir_allrdtgrp(struct rdt_resource *r, diff --git
> > a/arch/x86/kernel/cpu/resctrl/monitor.c
> > b/arch/x86/kernel/cpu/resctrl/monitor.c
> > index eaf25a234ff5..dc97aa7a3b3d 100644
> > --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> > +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> > @@ -656,11 +656,13 @@ static struct mon_evt llc_occupancy_event = {
> > static struct mon_evt mbm_total_event = {
> > .name = "mbm_total_bytes",
> > .evtid = QOS_L3_MBM_TOTAL_EVENT_ID,
> > + .config_name = "mbm_total_config",
> > };
>
> Struct mon_evt mbm_total_config_event = {
> .name = "mbm_total_config",
>
> >
> > static struct mon_evt mbm_local_event = {
> > .name = "mbm_local_bytes",
> > .evtid = QOS_L3_MBM_LOCAL_EVENT_ID,
> > + .config_name = "mbm_local_config",
> > };
> >
> > /*
> > @@ -682,7 +684,7 @@ static void l3_mon_evt_init(struct rdt_resource *r)
> > list_add_tail(&mbm_local_event.list, &r->evt_list); }
> >
> > -int rdt_get_mon_l3_config(struct rdt_resource *r)
> > +int rdt_get_mon_l3_config(struct rdt_resource *r, bool configurable)
> > {
> > unsigned int mbm_offset =
> > boot_cpu_data.x86_cache_mbm_width_offset;
> > struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r); @@ -714,6
> > +716,11 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
> > if (ret)
> > return ret;
> >
> > + if (configurable) {
> > + mbm_total_event.configurable = true;
> > + mbm_local_event.configurable = true;
> > + }
> > +
> > l3_mon_evt_init(r);
> >
> > r->mon_capable = true;
> > diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > index 04b519bca50d..834a55d78e3f 100644
> > --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> > @@ -1001,8 +1001,11 @@ static int rdt_mon_features_show(struct
> > kernfs_open_file *of,
> > struct rdt_resource *r = of->kn->parent->priv;
> > struct mon_evt *mevt;
> >
> > - list_for_each_entry(mevt, &r->evt_list, list)
> > + list_for_each_entry(mevt, &r->evt_list, list) {
> > seq_printf(seq, "%s\n", mevt->name);
> > + if (mevt->configurable)
> > + seq_printf(seq, "%s\n", mevt->config_name);
>
> If "config_name" is not defined, it could be:
> If (mevt->configurable)
> Seq_printf(seq, "%s_config\n", mevt->name);
>
Sure. Thanks
Babu
