Re: [PATCH v5] x86/resctrl: Add event choices for mba_MBps

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

Hi Tony,

On 12/4/2023 11:45 AM, Luck, Tony wrote:
>> Yes. I saw the thread. Even then I feel having two similar options can
>> cause confusion. I feel it is enough just to solve the original problem.
>> Giving more options to a corner cases is a overkill in my opinion.
> 
> The "original" problem was systems without "local" bandwidth event. I
> wanted to give a way for users of mba_MBps to still have some way to
> use it (assuming that "total" bandwidth event was present).
> 
> Reinette suggested that some people might want to use "total", even
> on systems that support "local". I firmly agree with that.  It is easy to
> construct scenarios where most bandwidth is to a remote node. using
> "local" event will do nothing to throttle in these case. I'm not at all sure
> why "local" event was picked. There's nothing in the LKML threads to
> give clues.
> 
> I proposed a mount option "total" as a modifier to be used in conjunction
> with "mba_MBps". Reinette said it was too generic. Her suggestion was
> to add "mba_MBps_total" to be used instead of "mba_MBps".

No, it cannot be used instead of "mba_MBps". My intention was for it to be
in addition to existing "mba_MBps" since taking "mba_MBps" away would be
considered breaking user space ABI.

Even so ...

> 
> Is that where I should have gone, instead of "mba_MBps={local|total}"?

While I did propose "mba_MBps_total" (in addition to "mba_MBps") I do
recognize your comment that a new key of mba_MBps_event does give more
flexibility if different events become available in future. Emphasis is
on "different" since I do not believe the parsing can support multiple
events and thus mba_MBps_event cannot be treated as a general bucket for all
mba_sc options, just different events guiding the feedback loop.

"mba_MBps" must be kept and having it continue to use local bw as default,
but total bw on systems that do not support local bw seems appropriate,
(which is what this patch does).

Reinette