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

[Moger, Babu]

Hi Tony,

On 12/4/23 12:16, Tony Luck wrote:
> On Mon, Dec 04, 2023 at 10:24:58AM -0600, Moger, Babu wrote:
>> Hi Tony,
>>
>> You are intending to achieve two things at once here.
>> 1. Adding new mount option
>> 2. Changing behaviour for the current option.
>> I think you need to split this patch into two. Few comments below.
> 
> Hi Babu,
> 
> Thanks for looking at this patch.
> 
> You are right. I will split the patch into two as you suggest.
> 
>> On 12/1/23 15:47, Tony Luck wrote:
>>> The MBA Software Controller(mba_sc) is a feedback loop that uses
>>> measurements of local memory bandwidth to adjust MBA throttling levels to
>>> keep workloads in a resctrl group within a target bandwidth set in the
>>> schemata file.
>>>
>>> But on Intel systems the memory bandwidth monitoring events are
>>> independently enumerated. It is possible for a system to support
>>> total memory bandwidth monitoring, but not support local bandwidth
>>> monitoring. On such a system a user could not enable mba_sc mode.
>>> Users will see this highly unhelpful error message from mount:
>>>
>>>  # mount -t resctrl -o mba_MBps resctrl /sys/fs/resctrl
>>>  mount: /sys/fs/resctrl: wrong fs type, bad option, bad superblock on
>>>  resctrl, missing codepage or helper program, or other error.
>>>  dmesg(1) may have more information after failed mount system call.
>>>
>>> dmesg(1) does not provide any additional information.
>>>
>>> Add a new mount option "mba_MBps_event=[local|total]" that allows
>>> a user to specify which monitoring event to use. Also modify the
>>> existing "mba_MBps" option to switch to total bandwidth monitoring
>>> if local monitoring is not available.
>>
>> I am not sure why you need both these options. I feel you just need one of
>> these options.
> 
> I should have included "changes since v4" in with this message, and
> pasted in some parts of this earlier messge from the discussion about
> v4:
> 
> https://lore.kernel.org/all/ZWpF5m4mIeZdK8kv@agluck-desk3/
> 
> Having the option take "local" would give a way for a user to
> avoid the failover to using "total" if they really didn't want
> that to happen.

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.

Thanks
Babu


> 
> Not in that message, because I didn't think of it until later, it
> opens the door for different events in the future.
> 
> But I'm also open to other suggestions on naming and function of
> mount options here.
> 
> Thanks
> 
> -Tony

-- 
Thanks
Babu Moger