Re: [PATCH v20 07/18] x86/resctrl: Block use of mba_MBps mount option on Sub-NUMA Cluster (SNC) systems

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

Hi Tony,

On 6/21/24 8:24 AM, Tony Luck wrote:
> On Thu, Jun 20, 2024 at 06:56:56PM -0700, Reinette Chatre wrote:
>> On 6/20/24 3:07 PM, Luck, Tony wrote:
>>>>> When SNC is enabled there is a mismatch between the MBA control function
>>>>> which operates at L3 cache scope and the MBM monitor functions which
>>>>> measure memory bandwidth on each SNC node.
>>>>>
>>>>> Block use of the mba_MBps when scopes for MBA/MBM do not match.
>>>>>
>>>>> Signed-off-by: Tony Luck <tony.luck@intel.com>
>>>>> ---
>>>>>     arch/x86/kernel/cpu/resctrl/rdtgroup.c | 4 +++-
>>>>>     1 file changed, 3 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>> index eb3bbfa96d5a..a0a43dbe011b 100644
>>>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>>>> @@ -2339,10 +2339,12 @@ static void mba_sc_domain_destroy(struct rdt_resource *r,
>>>>>      */
>>>>>     static bool supports_mba_mbps(void)
>>>>>     {
>>>>> +   struct rdt_resource *rmbm = &rdt_resources_all[RDT_RESOURCE_L3].r_resctrl;
>>>>>       struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
>>>>>
>>>>>       return (is_mbm_local_enabled() &&
>>>>> -           r->alloc_capable && is_mba_linear());
>>>>> +           r->alloc_capable && is_mba_linear() &&
>>>>> +           r->ctrl_scope == rmbm->mon_scope);
>>>>>     }
>>>>>
>>>>>     /*
>>>>
>>>> The function comments of supports_mba_mbps() needs an update to accompany
>>>> this new requirement.
>>>
>>> Will add comment on extra requirement.
>>>
>>>> I also think that the "mba_MBps" mount option is now complicated enough to
>>>> warrant a clear error to user space when using it fails. invalfc() is
>>>> available for this and enables user space to get detailed log message
>>>> from a read() on an fd created by fsopen().
>>>>
>>>> Perhaps something like (please check line length and feel free to improve
>>>> since as is it may quite cryptic):
>>>>         rdt_parse_param(...)
>>>>         {
>>>>
>>>>
>>>>         ...
>>>>         case Opt_mba_mbps:
>>>>                 if (!supports_mba_mbps())
>>>>                         return invalfc(fc, "mba_MBps requires both MBM and (linear scale) MBA at L3 scope");
>>>>         ...
>>>>         }
>>>
>>> Line length is indeed a problem (108 characters). Usual line split methods barely help as the moving the
>>> string to the next line and aligning with the "(" only saves 4 characters.
>>>
>>> How about this (suggestions for a shorter variable name - line is 97 characters)
>>>
>>> static char mba_mbps_invalid[] = "mba_MBps requires both MBM and (linear scale) MBA at L3 scope";
>>>
>>> rdt_parse_param(...)
>>> {
>>> 	...
>>> 	case Opt_mba_mbps:
>>> 		if (!supports_mba_mbps())
>>> 			return invalfc(fc, mba_mbps_invalid);
>>> 	...
>>> }
>>
>> On 6/20/24 3:12 PM, Luck, Tony wrote:
>>>> static char mba_mbps_invalid[] = "mba_MBps requires both MBM and (linear scale) MBA at L3 scope";
>>>
>>> checkpatch recommends "static const char ..." pushing this over 100 chars :-(
>>>
>>
>> How about something like below that reaches 96:
>>
>> 	case Opt_mba_mbps:
>> 		if (!supports_mba_mbps())
>> 			return invalfc(fc,
>> 				       "mba_MBps requires both MBM and linear MBA at L3 scope");
>>
> 
> Reinette,
> 
> Alternative option. Move the messaging into supports_mba_mbps() and
> split into shorter pieces for each reason. The other callers of
> supports_mba_mbps() that are just re-checking status would pass
> a NULL argument.

This fragmentation of the mount parameter checking, splitting its error
reporting to be partially into generic code, does not look ideal to me.

Looking at the information provided in the messages you created I can think
of two more options:

rdt_parse_param(...)
{
	...
	const char *msg;
	...

  	case Opt_mba_mbps:
		msg = "mba_MBps requires local MBM and linear scale MBA at L3 scope";
		if (!supports_mba_mbps())
  			return invalfc(fc, "%s", msg);
	...
}


rdt_parse_param(...)
{
	...
  	case Opt_mba_mbps:
		if (!supports_mba_mbps()) {
			errorfc(fc,
				"mba_MBps requires local MBM and linear scale MBA at L3 scope");
			return -EINVAL;
		}
	...
}

Reinette