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

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 8/25/2022 8:11 AM, Moger, Babu wrote:
> On 8/24/22 16:15, Reinette Chatre wrote:
>> 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.
>>>

...

>>> 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.

I understand the motivation. My concern is that this is a resource wide
file that will display a binary value that, if true, currently means two
events are configurable. We need to consider how things can change in the
future. We should consider that this is only the beginning of monitoring
configuration and need this interface to be ready for future changes. For
example, what if all of the monitoring events are configurable? Let's say,
for example, in future AMD hardware the "llc_occupancy" event also becomes
configurable, how should info/L3_MON/configurable be interpreted? On some
machines it would thus mean that mbm_total_bytes and mbm_local_bytes are
configurable and on some machines it would mean that mbm_total_bytes,
mbm_local_bytes, and llc_occupancy are configurable. This does not make
it easy for utilities.

So, in this series the info directory on a system that supports BMEC
would look like:

info/L3_MON/mon_features:llc_occupancy
info/L3_MON/mon_features:mbm_total_bytes
info/L3_MON/mon_features:mbm_local_bytes
info/L3_MON/configurable:1

Would information like below not be more specific?
info/L3_MON/mon_features:llc_occupancy
info/L3_MON/mon_features:mbm_total_bytes
info/L3_MON/mon_features:mbm_local_bytes
info/L3_MON/mon_features:mbm_total_config
info/L3_MON/mon_features:mbm_local_config

Reinette