Hi Tony, On 10/25/24 1:42 PM, Luck, Tony wrote: > Plucking out just the big, direction change, comment for discussion (which may make > several of the code comments moot). > >> I needed to refresh my understanding of this work by re-reading the previous discussions. >> You mentioned in [2]: >> I tried out some code to make the event runtime selectable via a r/w file in the >> resctrl/info directories. But that got complicated because of the amount of state >> that needs to be updated when switching events. >> >> Could you please clarify which state you referred to? I wonder if it may be the >> struct mbm_state state maintained by mbm_bw_count()? mbm_bw_count() is lightweight >> and I see no problem with it being called for all supported MBM events when >> the software controller is enabled. With state for all supported events always available >> it seems simpler to runtime switch between which events guide the software controller? >> >> Thinking about it more, it seems possible for the user to use different >> MBM events to guide the software controller for different resource groups. >> >> If it is possible to do runtime switching in this way I do think it will simplify this >> implementation while not requiring the user to remount resctrl to make changes. You >> mentioned [3] that "a separate patch series" may be coming to address this but doing this >> now seems simpler while avoiding any future work as well as confusing duplicate ABI >> ... unless you were referring to other issues that needs to be addressed separately? > > Yes, the state maintained by mbm_bw_count() was the piece that worried me. After > a user switch to a different event there would be no bandwidth data until two updates > passed by (one to get a baseline, second to compute bandwidth). So update_mba_bw() > would need to be aware of this liminal period to avoid making updates with no data to > back them up. > > Your solution is elegant. The cost to maintain bandwidth data for each event is indeed > very low. So there are no weird transition cases. update_mba_bw() can immediately > compare bandwidth for the new event against the target bandwidth and make appropriate > adjustments. Thank you for considering it. > > This requires a new file in each CTRL_MON directory when mba_sc is enabled so > the user can make their selection. > > Note that technically it would be possible to make a different selection for each domain. > But that seems like an option without an obvious use case and would just complicate > the syntax of the new file. I did not consider this possibility. I agree with your assessment. > > Maybe name this new file "mba_sc_event"[1] with contents that match the names of > the mbm_monitor events as listed in /sys/fs/resctrl/info/L3_MON/mon_features? I do like that the content is connected to existing user interface by using events from mon_features. What do you think of connecting the filename to existing user interface (the mount option) also by, for example, being named "mba_MBps_event"? > > So default state when resctrl is mounted with the software controller enabled would > have: > > $ cat /sys/fs/resctrl/mba_sc_event > mbm_local_bytes > > User could switch to total with > > # echo mbm_total_bytes > /sys/fs/resctrl/mba_sc_event > > On systems where mbm_local_bytes is not supported default would be mbm_total_bytes. > > New CTRL_MON directories would also default to mbm_local_bytes if it is supported. This sounds good to me. Thank you very much for considering the change. Reinette
