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 1:44 PM, Moger, Babu wrote:
> 
> On 8/25/2022 10:56 AM, Reinette Chatre wrote:
>> 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
> 
> Hi Reinette,
> 
>  Yes. That is more specific.
> 
> So, basically your idea is to print the information from mon_evt structure if mon_configarable is set in the resource structure.
> 
> Some thing like ..
> 
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index 83c8780726ff..46c6bf3f08e3 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1194,8 +1194,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 (r->mon_configurable)
> +                       seq_printf(seq, "%s\n", mevt->config);
> +       }
> 
>         return 0;
>  }
> 
> Is that the idea?


I do not see why struct rdt_resource->configurable is needed. Again, this
is a resource wide property with an implicit meaning related to only two
event counters. Again, what if AMD later makes the llc_occupancy event counter
configurable? How can resctrl know, using "r->mon_configurable" whether
it should print mevt->config? 

How about:
		if (mevt->config)
			seq_printf(seq, "%s\n", mevt->config);

As I mentioned in [1], mevt->config can be set in rdt_get_mon_l3_config()
based on a check on the BMEC feature instead of hardcoded as it is now.
Or, if the string manipulation is of concern the hardcoding of mevt->config
(perhaps then mevt->config_name) could remain and a new mevt->configurable
could be set from rdt_get_mon_l3_config() and then the above could be:

		if (mevt->configurable)
			seq_printf(seq, "%s\n", mevt->config_name);

Reinette

[1] https://lore.kernel.org/lkml/c5777707-746e-edab-2ce2-3405ff55be56@intel.com/