Re: [PATCH v3 06/10] x86/resctrl: Introduce mon_configurable to detect Bandwidth Monitoring Event Configuration

[Moger, Babu]

Hi Reinette,

On 8/24/22 16:15, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/22/2022 6:43 AM, Babu Moger wrote:
>> Newer AMD processors support the new feature Bandwidth Monitoring Event
>> Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes
>> are configurable when this feature is present.
>>
>> Set mon_configurable if the feature is available.
>>
>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx>
>> Reviewed-by: Ingo Molnar <mingo@xxxxxxxxxx>
>> ---
>>  arch/x86/kernel/cpu/resctrl/monitor.c  |   14 ++++++++++++++
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   17 +++++++++++++++++
>>  include/linux/resctrl.h                |    1 +
>>  3 files changed, 32 insertions(+)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
>> index eaf25a234ff5..b9de417dac1c 100644
>> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
>> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
>> @@ -682,6 +682,16 @@ static void l3_mon_evt_init(struct rdt_resource *r)
>>  		list_add_tail(&mbm_local_event.list, &r->evt_list);
>>  }
>>  
>> +
>> +void __rdt_get_mon_l3_config_amd(struct rdt_resource *r)
>> +{
>> +	/*
>> +	 * Check if CPU supports the Bandwidth Monitoring Event Configuration
>> +	 */
>> +	if (boot_cpu_has(X86_FEATURE_BMEC))
>> +		r->mon_configurable = true;
>> +}
> Could this rather use rdt_cpu_has() with the added support for disabling
> the feature via kernel parameter?

Yes, That is possible. We could do that.


>
>
>> +
>>  int rdt_get_mon_l3_config(struct rdt_resource *r)
>>  {
>>  	unsigned int mbm_offset = boot_cpu_data.x86_cache_mbm_width_offset;
>> @@ -714,6 +724,10 @@ int rdt_get_mon_l3_config(struct rdt_resource *r)
>>  	if (ret)
>>  		return ret;
>>  
>> +	if (boot_cpu_data.x86_vendor == X86_VENDOR_AMD)
>> +		__rdt_get_mon_l3_config_amd(r);
>> +
>> +
> Why is this vendor check needed? Is X86_FEATURE_BMEC not sufficient?
Yes. I can remove the vendor check.
>
>>  	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 fc5286067201..855483b297a8 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of,
>>  	return 0;
>>  }
>>  
>> +static int rdt_mon_configurable_show(struct kernfs_open_file *of,
>> +				     struct seq_file *seq, void *v)
>> +{
>> +	struct rdt_resource *r = of->kn->parent->priv;
>> +
>> +	seq_printf(seq, "%d\n", r->mon_configurable);
> Why is this file needed? It seems that the next patches also introduce
> files in support of this new feature that will make the actual configuration
> data accessible - those files are only created if this feature is supported.
> Would those files not be sufficient for user space to learn about the feature
> support?

This is part of /sys/fs/resctrl/info/L3_MON# directory which basically has
the information about all the monitoring features. As this is one of the
mon features, I have added it there. Also, this can be used from the
utility(like pqos or rdtset) to check if the configuring the monitor is
supported without looking at individual files. It makes things easier.

Thanks

Babu


> Reinette

-- 
Thanks
Babu Moger