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

[Reinette Chatre]

Hi Babu,

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?

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.

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

Reinette