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
