RE: [PATCH v5 06/12] x86/resctrl: Introduce data structure to support monitor configuration

[Moger, Babu]

[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