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

["Moger, Babu" <babu.moger@amd.com>]

Hi Reinette,

On 8/8/2025 1:23 PM, Reinette Chatre wrote:
> 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.

Yes. That is what I meant.  We have same check in event_filer_write(). 
Will add the same check in event_filter_show().

Thanks

Babu