diff mbox series

[v8,07/13] x86/resctrl: Introduce data structure to support monitor configuration

Message ID 166759204323.3281208.9744810874584175730.stgit@bmoger-ubuntu
State New
Headers
Series Support for AMD QoS new features |

Commit Message

Moger, Babu Nov. 4, 2022, 8 p.m. UTC 
  Add a new field in mon_evt to support Bandwidth Monitoring Event
Configuration(BMEC) and also update the "mon_features" display.

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 when BMEC is supported.
	$cat /sys/fs/resctrl/info/L3_MON/mon_features
	llc_occupancy
	mbm_total_bytes
	mbm_total_bytes_config
	mbm_local_bytes
	mbm_local_bytes_config

Signed-off-by: Babu Moger <babu.moger@amd.com>
---
 arch/x86/kernel/cpu/resctrl/internal.h |    2 ++
 arch/x86/kernel/cpu/resctrl/monitor.c  |    6 ++++++
 arch/x86/kernel/cpu/resctrl/rdtgroup.c |    5 ++++-
 3 files changed, 12 insertions(+), 1 deletion(-)

Comments

Reinette Chatre Nov. 23, 2022, 12:14 a.m. UTC | #1 
Hi Babu,

On 11/4/2022 1:00 PM, Babu Moger wrote:
> Add a new field in mon_evt to support Bandwidth Monitoring Event
> Configuration(BMEC) and also update the "mon_features" display.
> 
> The sysfs file "mon_features" will display the monitor configuration

sysfs -> resctrl ?

> if supported.

This is not clear. "mon_features" does not display the monitor
configuration, it displays the name of the file that can be used to 
see the monitor configuration.

> 
> Before the change.
> 	$cat /sys/fs/resctrl/info/L3_MON/mon_features
> 	llc_occupancy
> 	mbm_total_bytes
> 	mbm_local_bytes
> 
> After the change when BMEC is supported.
> 	$cat /sys/fs/resctrl/info/L3_MON/mon_features
> 	llc_occupancy
> 	mbm_total_bytes
> 	mbm_total_bytes_config
> 	mbm_local_bytes
> 	mbm_local_bytes_config
> 
> Signed-off-by: Babu Moger <babu.moger@amd.com>
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |    2 ++
>  arch/x86/kernel/cpu/resctrl/monitor.c  |    6 ++++++
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |    5 ++++-
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
> index e30e8b23f6b5..5459b5022760 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -63,11 +63,13 @@ 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
>   * @list:		entry in &rdt_resource->evt_list
>   */
>  struct mon_evt {
>  	enum resctrl_event_id	evtid;
>  	char			*name;
> +	bool			configurable;
>  	struct list_head	list;
>  };
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index efe0c30d3a12..06c2dc980855 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -750,6 +750,7 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>  {
>  	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
> +	bool mon_configurable = rdt_cpu_has(X86_FEATURE_BMEC);
>  	unsigned int threshold;
>  	int ret;
>  
> @@ -783,6 +784,11 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>  	if (ret)
>  		return ret;
>  
> +	if (mon_configurable) {
> +		mbm_total_event.configurable = true;
> +		mbm_local_event.configurable = true;
> +	}
> +

Is the local variable needed? Why not just:
	if (rdt_cpu_has(X86_FEATURE_BMEC))


Reinette
Moger, Babu Nov. 23, 2022, 6:23 p.m. UTC | #2 
Hi Reinette,

On 11/22/22 18:14, Reinette Chatre wrote:
> Hi Babu,
>
> On 11/4/2022 1:00 PM, Babu Moger wrote:
>> Add a new field in mon_evt to support Bandwidth Monitoring Event
>> Configuration(BMEC) and also update the "mon_features" display.
>>
>> The sysfs file "mon_features" will display the monitor configuration
> sysfs -> resctrl ?
Sure.
>
>> if supported.
> This is not clear. "mon_features" does not display the monitor
> configuration, it displays the name of the file that can be used to 
> see the monitor configuration.

Will change it to:

The sysfs -> resctrl file "mon_features" will display the supported events
and files that can be used to configure those events if monitor
configuration is supported.


>
>> Before the change.
>> 	$cat /sys/fs/resctrl/info/L3_MON/mon_features
>> 	llc_occupancy
>> 	mbm_total_bytes
>> 	mbm_local_bytes
>>
>> After the change when BMEC is supported.
>> 	$cat /sys/fs/resctrl/info/L3_MON/mon_features
>> 	llc_occupancy
>> 	mbm_total_bytes
>> 	mbm_total_bytes_config
>> 	mbm_local_bytes
>> 	mbm_local_bytes_config
>>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/internal.h |    2 ++
>>  arch/x86/kernel/cpu/resctrl/monitor.c  |    6 ++++++
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |    5 ++++-
>>  3 files changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
>> index e30e8b23f6b5..5459b5022760 100644
>> --- a/arch/x86/kernel/cpu/resctrl/internal.h
>> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
>> @@ -63,11 +63,13 @@ 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
>>   * @list:		entry in &rdt_resource->evt_list
>>   */
>>  struct mon_evt {
>>  	enum resctrl_event_id	evtid;
>>  	char			*name;
>> +	bool			configurable;
>>  	struct list_head	list;
>>  };
>>  
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index efe0c30d3a12..06c2dc980855 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -750,6 +750,7 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>>  {
>>  	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
>>  	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
>> +	bool mon_configurable = rdt_cpu_has(X86_FEATURE_BMEC);
>>  	unsigned int threshold;
>>  	int ret;
>>  
>> @@ -783,6 +784,11 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>>  	if (ret)
>>  		return ret;
>>  
>> +	if (mon_configurable) {
>> +		mbm_total_event.configurable = true;
>> +		mbm_local_event.configurable = true;
>> +	}
>> +
> Is the local variable needed? Why not just:
> 	if (rdt_cpu_has(X86_FEATURE_BMEC))


Local variable not requited. Will change it.

Thanks

Babu

>
>
> Reinette
Reinette Chatre Nov. 23, 2022, 7:05 p.m. UTC | #3 
Hi Babu,

On 11/23/2022 10:23 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 11/22/22 18:14, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 11/4/2022 1:00 PM, Babu Moger wrote:
>>> Add a new field in mon_evt to support Bandwidth Monitoring Event
>>> Configuration(BMEC) and also update the "mon_features" display.
>>>
>>> The sysfs file "mon_features" will display the monitor configuration
>> sysfs -> resctrl ?
> Sure.
>>
>>> if supported.
>> This is not clear. "mon_features" does not display the monitor
>> configuration, it displays the name of the file that can be used to 
>> see the monitor configuration.
> 
> Will change it to:
> 
> The sysfs -> resctrl file "mon_features" will display the supported events
> and files that can be used to configure those events if monitor
> configuration is supported.
> 

I meant that "sysfs" should be replaced by "resctrl".

Reinette
Moger, Babu Nov. 23, 2022, 9:46 p.m. UTC | #4 
[AMD Official Use Only - General]



> -----Original Message-----
> From: Reinette Chatre <reinette.chatre@intel.com>
> Sent: Wednesday, November 23, 2022 1:06 PM
> To: Moger, Babu <Babu.Moger@amd.com>; corbet@lwn.net;
> tglx@linutronix.de; mingo@redhat.com; bp@alien8.de
> Cc: fenghua.yu@intel.com; dave.hansen@linux.intel.com; x86@kernel.org;
> hpa@zytor.com; paulmck@kernel.org; akpm@linux-foundation.org;
> quic_neeraju@quicinc.com; rdunlap@infradead.org;
> damien.lemoal@opensource.wdc.com; songmuchun@bytedance.com;
> peterz@infradead.org; jpoimboe@kernel.org; pbonzini@redhat.com;
> chang.seok.bae@intel.com; pawan.kumar.gupta@linux.intel.com;
> jmattson@google.com; daniel.sneddon@linux.intel.com; Das1, Sandipan
> <Sandipan.Das@amd.com>; tony.luck@intel.com; james.morse@arm.com;
> linux-doc@vger.kernel.org; linux-kernel@vger.kernel.org;
> bagasdotme@gmail.com; eranian@google.com
> Subject: Re: [PATCH v8 07/13] x86/resctrl: Introduce data structure to support
> monitor configuration
> 
> Hi Babu,
> 
> On 11/23/2022 10:23 AM, Moger, Babu wrote:
> > Hi Reinette,
> >
> > On 11/22/22 18:14, Reinette Chatre wrote:
> >> Hi Babu,
> >>
> >> On 11/4/2022 1:00 PM, Babu Moger wrote:
> >>> Add a new field in mon_evt to support Bandwidth Monitoring Event
> >>> Configuration(BMEC) and also update the "mon_features" display.
> >>>
> >>> The sysfs file "mon_features" will display the monitor configuration
> >> sysfs -> resctrl ?
> > Sure.
> >>
> >>> if supported.
> >> This is not clear. "mon_features" does not display the monitor
> >> configuration, it displays the name of the file that can be used to
> >> see the monitor configuration.
> >
> > Will change it to:
> >
> > The sysfs -> resctrl file "mon_features" will display the supported
> > events and files that can be used to configure those events if monitor
> > configuration is supported.
> >
> 
> I meant that "sysfs" should be replaced by "resctrl".

Ok. Got it.
Thanks
Babu
diff mbox series

Patch

diff --git a/arch/x86/kernel/cpu/resctrl/internal.h b/arch/x86/kernel/cpu/resctrl/internal.h
index e30e8b23f6b5..5459b5022760 100644
--- a/arch/x86/kernel/cpu/resctrl/internal.h
+++ b/arch/x86/kernel/cpu/resctrl/internal.h
@@ -63,11 +63,13 @@  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
  * @list:		entry in &rdt_resource->evt_list
  */
 struct mon_evt {
 	enum resctrl_event_id	evtid;
 	char			*name;
+	bool			configurable;
 	struct list_head	list;
 };
 
diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
index efe0c30d3a12..06c2dc980855 100644
--- a/arch/x86/kernel/cpu/resctrl/monitor.c
+++ b/arch/x86/kernel/cpu/resctrl/monitor.c
@@ -750,6 +750,7 @@  int rdt_get_mon_l3_config(struct rdt_resource *r)
 {
 	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
 	struct rdt_hw_resource *hw_res = resctrl_to_arch_res(r);
+	bool mon_configurable = rdt_cpu_has(X86_FEATURE_BMEC);
 	unsigned int threshold;
 	int ret;
 
@@ -783,6 +784,11 @@  int rdt_get_mon_l3_config(struct rdt_resource *r)
 	if (ret)
 		return ret;
 
+	if (mon_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 8a3dafc0dbf7..8342feb54a7f 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_config\n", mevt->name);
+	}
 
 	return 0;
 }