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

[Reinette Chatre]

Hi Babu,

On 8/8/25 11:48 AM, Moger, Babu wrote:
> On 8/8/2025 1:23 PM, Reinette Chatre wrote:
>> 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.
> 
> Yes. That is what I meant.  We have same check in event_filer_write(). Will add the same check in event_filter_show().
> 

Thank you. This makes this specific behavior consistent and addresses the
topic that started this thread:
	> There is inconsistency among the files introduced on how
	> "mbm_event mode disabled" case is handled."
Could you please check the final work to confirm that the new resctrl
files are consistent in handling of "mbm_event mode supported" and "mbm_event mode"
enabled vs disabled cases? For example, they consider same scenarios as valid/invalid
and return same error code in invalid cases.

Reinette