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

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."

>
>> +
>> +	"0":
>> +		Auto assignment is disabled.
>> +	"1":
>> +		Auto assignment is enabled.
>> +
>> +	Example::
>> +
>> +	  # echo 0 > /sys/fs/resctrl/info/L3_MON/mbm_assign_on_mkdir
>> +	  # cat /sys/fs/resctrl/info/L3_MON/mbm_assign_on_mkdir
>> +	  0
>> +
>>   "max_threshold_occupancy":
>>   		Read/write file provides the largest value (in
>>   		bytes) at which a previously used LLC_occupancy
>> diff --git a/fs/resctrl/monitor.c b/fs/resctrl/monitor.c
>> index 8efbeb910f77..6205bbfe08fb 100644
>> --- a/fs/resctrl/monitor.c
>> +++ b/fs/resctrl/monitor.c
>> @@ -1077,6 +1077,8 @@ int resctrl_mon_resource_init(void)
>>   		resctrl_file_fflags_init("available_mbm_cntrs",
>>   					 RFTYPE_MON_INFO | RFTYPE_RES_CACHE);
>>   		resctrl_file_fflags_init("event_filter", RFTYPE_ASSIGN_CONFIG);
>> +		resctrl_file_fflags_init("mbm_assign_on_mkdir", RFTYPE_MON_INFO |
>> +					 RFTYPE_RES_CACHE);
>>   	}
>>   
>>   	return 0;
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index c3d6540c3280..bf04235d2603 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
> Please move resctrl_mbm_assign_on_mkdir_show() and resctrl_mbm_assign_on_mkdir_write() to monitor.c

Sure.

Thanks

Babu