Re: [PATCH v16 25/34] fs/resctrl: Add event configuration directory under info/L3_MON/

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

Hi Babu,

On 8/8/25 10:47 AM, Moger, Babu wrote:
> On 8/8/2025 10:12 AM, Reinette Chatre wrote:
>> On 8/8/25 6:56 AM, Moger, Babu wrote:
>>> On 7/30/2025 3:04 PM, Reinette Chatre wrote:
>>>> On 7/25/25 11:29 AM, Babu Moger wrote:
>>>>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>>>>> index 16bcfeeb89e6..fa5f63126682 100644
>>>>> --- a/fs/resctrl/monitor.c
>>>>> +++ b/fs/resctrl/monitor.c
>>>>> @@ -929,6 +929,29 @@ struct mbm_transaction mbm_transactions[NUM_MBM_TRANSACTIONS] = {
>>>>>        {"dirty_victim_writes_all", DIRTY_VICTIMS_TO_ALL_MEM},
>>>>>    };
>>>>>    +int event_filter_show(struct kernfs_open_file *of, struct seq_file *seq, void *v)
>>>>> +{
>>>>> +    struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
>>>>> +    bool sep = false;
>>>>> +    int i;
>>>>> +
>>>>> +    mutex_lock(&rdtgroup_mutex);
>>>>> +
>>>> There is inconsistency among the files introduced on how
>>>> "mbm_event mode disabled" case is handled. Some files return failure
>>>> from their _show()/_write() when "mbm_event mode is disabled", some don't.
>>>>
>>>> The "event_filter" file always prints the MBM transactions monitored
>>>> when assignable counters are supported, whether mbm_event mode is enabled
>>>> or not. This means that the MBM event's configuration values are printed
>>>> when "default" mode is enabled.  I have two concerns about this
>>>> 1) This is potentially very confusing since switching to "default" will
>>>>      make the BMEC files visible that will enable the user to modify the
>>>>      event configurations per domain. Having this file print a global event
>>>>      configuration while there are potentially various different domain-specific
>>>>      configuration active will be confusing.
>>> Yes. I agree.
>> hmmm ... ok, you say that you agree but I cannot tell if you are going to
>> do anything about it.
>>
>> On a system with BMEC enabled the mbm_total_bytes_config and mbm_local_bytes_config
>> files should be the *only* source of MBM event configuration information, no?
> 
> That is correct.
> 
> 
>>
>> It may be ok to have event_filter print configuration when assignable counters are disabled
>> if BMEC is not supported but that would require that this information will always be
>> known for a "default" monitoring setup. While this may be true for AMD it is not obvious
>> to me that this is universally true. Once this file exists in this form resctrl will always
>> be required to provide data for the event configuration and it is not clear to me that
>> this can always be guaranteed.
> 
> Yea. It is not true universally true. I don't know what these values are for Intel and ARM.
> 
>>
>>>> 2) Can it be guaranteed that the MBM events will monitor the default
>>>>      assignable counter memory transactions when in "default" mode? It has
>>>>      never been possible to query which memory transactions are monitored by
>>>>      the default X86_FEATURE_CQM_MBM_TOTAL and X86_FEATURE_CQM_MBM_LOCAL features
>>>>      so this seems to use one feature to deduce capabilities or another?
>>> So, initialize both total and local event configuration to default values when switched to "default mode"?
>>>
>>> Something like this?
>>>
>>> mon_event_all[QOS_L3_MBM_TOTAL_EVENT_ID].evt_cfg = r->mon.mbm_cfg_mask;
>>>
>>> mon_event_all[QOS_L3_MBM_LOCAL_EVENT_ID].evt_cfg = READS_TO_LOCAL_MEM | READS_TO_LOCAL_S_MEM | NON_TEMP_WRITE_TO_LOCAL_MEM;
>>>
>>> We are already doing that right (in patch 32)?
>> Yes, but it creates this strange dependency where the "default" monitoring mode
>> (that has been supported long before configurable events and assignable counters came
>> along) requires support of "assignable counter mode".
>>
>> Consider it from another view, if resctrl wants to make event configuration available
>> for the "default" mode then the "event_filter" file could always be visible when MBM
>> local/total is supported to give users insight into what is monitored, whether
>> assignable counters are supported or not. This is not possible on systems that do
>> not support ABMC though, right?
> 
> That is correct. With BMEC, each domain can have its own settings.  So, printing the default will not be accurate.
> 
> What options do we have here.
> 
> How about adding the check if (resctrl_arch_mbm_cntr_assign_enabled())?  Only print the values when ABMC is supported else print information in "last_cmd_status".
> 

Did you perhaps intend to write "Only print the values when ABMC is *enabled* else print
information in "last_cmd_status".? If this is what you actually meant then I agree. I think
doing so places clear boundary on this feature that gives us more options/flexibility for
future changes.

Reinette