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@xxxxxxxxxx> >>> Signed-off-by: Babu Moger <babu.moger@xxxxxxx> >>> --- >> ... >> >>> --- >>> 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. > > 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. Reinette
