Re: [PATCH v16 27/34] fs/resctrl: Introduce mbm_assign_on_mkdir to enable assignments on mkdir

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

Hi Reinette.

On 8/8/2025 4:00 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 8/8/25 1:29 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 7/30/2025 3:08 PM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 7/25/25 11:29 AM, Babu Moger wrote:
>>>> The "mbm_event" counter assignment mode allows users to assign a hardware
>>>> counter to an RMID, event pair and monitor the bandwidth as long as it is
>>>> assigned.
>>> Above implies this addition is in support of "mbm_event" mode while the
>>> implementation applies to any and all assignable counter modes, including
>>> the "default" and for example the upcoming "soft-ABMC". It is clear to me
>>> how this is used and interpreted when "mbm_event" mode is enabled, but not
>>> for the others (more below).
>>>
>>>> Introduce a user-configurable option that determines if a counter will
>>>> automatically be assigned to an RMID, event pair when its associated
>>>> monitor group is created via mkdir.
>>>>
>>>> Suggested-by: Peter Newman <peternewman@google.com>
>>>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>>>> ---
>>> ...
>>>
>>>> ---
>>>>    Documentation/filesystems/resctrl.rst | 16 ++++++++++
>>>>    fs/resctrl/monitor.c                  |  2 ++
>>>>    fs/resctrl/rdtgroup.c                 | 43 +++++++++++++++++++++++++++
>>>>    include/linux/resctrl.h               |  3 ++
>>>>    4 files changed, 64 insertions(+)
>>>>
>>>> diff --git a/Documentation/filesystems/resctrl.rst b/Documentation/filesystems/resctrl.rst
>>>> index 37dbad4d50f7..165e0d315af7 100644
>>>> --- a/Documentation/filesystems/resctrl.rst
>>>> +++ b/Documentation/filesystems/resctrl.rst
>>>> @@ -354,6 +354,22 @@ with the following files:
>>>>          # cat /sys/fs/resctrl/info/L3_MON/event_configs/mbm_total_bytes/event_filter
>>>>           local_reads,local_non_temporal_writes
>>>>    +"mbm_assign_on_mkdir":
>>> Needs a "Exists when "mbm_event" counter assignment mode is supported."?
>>> Also needs clarification on on behavior when "mbm_event" is enabled vs. disabled.
>> I think we should allow it to modify  only when "mbm_event" is enabled.
>>>> +    Determines if a counter will automatically be assigned to an RMID, event pair
>>> "will automatically be" -> "is automatically"
>>> "RMID, event" -> "RMID, MBM event"
>> Sure.
>>>> +    when its associated monitor group is created via mkdir. It is enabled by default
>>>> +    on boot and users can disable by writing to the interface.
>>> "users can disable" -> "users can disable this capability" or "can be disabled"?
>> Sure.
>>> This implementation enables user to read/write this file/property when "mbm_event" mode is
>>> disabled. Considering this explanation I do not think it is clear how this file reflects
>>> system behavior when in "default" mode. There is no difference between mbm_assign_on_mkdir
>>> enabled/disabled when in "default" mode, no?
>> Yes. So, we should only allow modifications only when mbm_event mode is enabled.
>>> Should interactions with "mbm_assign_on_mkdir" be restricted to when
>>> "mbm_event" mode is enabled? If so, the next question would likely be whether value
>> Yes.
>>> should change during "mbm_event" enable->disable or "disable->enable". Above states
>>> clearly that it is enabled on boot and it may be reasonable to have it keep (but not always
>>> expose) user's setting when switching between modes.
>>>
>>> Restricting it to "mbm_event" mode now gives us some flexibility when soft-ABMC follows
>>> on if/how it can/should support this. What do you think?
>> Yes. We should restrict it to modify only when mbm_event mode is enabled.
> I agree. I also think it should not be displayed with mbm_event mode is disabled. This is
> because it indicates to user space that counters are automatically assigned to RMID, event
> pairs and since "default" mode depends on hardware doing this it may not be accurate when, for
> example, ABMC is disabled. Alternative is to add a third value, for example "enabled", "disabled", and
> "undefined"(?). This sounds a bit awkward though so I think simplest may be to have this file
> also be consistent with the others and return error when mbm_event mode is disabled.


Yes. We can do that.


>
>> And always enable it when switching from
>>
>> "mbm_event" disable -> enable:  r->mon.mbm_assign_on_mkdir = true;
>>
>> "mbm_event" enable -> enable: "no need to modify as the value does not affect the behavior."
>>
> ok, please note this may need an update to the doc that currently only states "enabled by
> default on boot" to indicate it is also automatically enabled when enabling mbm_event mode.


Will add texts about it.

Thanks

Babu