Re: [PATCH v7 1/4] x86/resctrl: Make input event for MBA Software Controller configurable

[Reinette Chatre <reinette.chatre@intel.com>]

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