Re: [PATCH v5 4/5] selftests/resctrl: Use bandwidth values for AMD MBA schemata

["Moger, Babu" <bmoger@amd.com>]

Hi Reinette,

On 10/28/2026 1:45 PM, Reinette Chatre wrote:
> Hi Swapnil,
> 
> On 10/22/26 6:25 AM, Swapnil Sapkal wrote:
> 
>> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
>> index 17ae73d3476c..947735e26099 100644
>> --- a/tools/testing/selftests/resctrl/mba_test.c
>> +++ b/tools/testing/selftests/resctrl/mba_test.c
>> @@ -13,10 +13,51 @@
>>   #define RESULT_FILE_NAME	"result_mba"
>>   #define NUM_OF_RUNS		5
>>   #define MAX_DIFF_PERCENT	15
>> +
>> +/* Intel MBA is specified as a percentage: 10%, 20% ... 100%. */
> 
> The percentage based interface is the generic interface and not Intel specific.
> You can drop "Intel" from references to this interface.
> 
>>   #define ALLOCATION_MAX		100
>>   #define ALLOCATION_MIN		10
>>   #define ALLOCATION_STEP		10
>>   
>> +/*
>> + * AMD MBA is not a percentage but an absolute memory bandwidth value. The
>> + * test sweeps the bandwidth from 4 GB/s to 40 GB/s in steps of 4 GB/s,
>> + * which the hardware represents in units of 1/8 GB/s (i.e. 4 GB/s -> 32,
>> + * 8 GB/s -> 64, ... 40 GB/s -> 320).
>> + */
>> +#define AMD_ALLOCATION_MAX	320
> 
> The AMD maximum is enumerated from hardware. How can it be ensured this is
> the appropriate maximum for all AMD systems?

You are right.

The hardware reports a maximum bandwidth of 4096 (512 GB/s), but a 
single-threaded workload may not be able to achieve it. So, we cannot 
use the reported maximum bandwidth.


> 
>> +#define AMD_ALLOCATION_MIN	32
>> +#define AMD_ALLOCATION_STEP	32
>> +
>> +/* Number of MBA schemata allocations to sweep (same for both vendors). */
>> +#define NUM_OF_ALLOCS		(ALLOCATION_MAX / ALLOCATION_STEP)
>> +
>> +/*
>> + * The fixed-size result arrays in check_results() and the aggregation loop in
>> + * show_mba_info() are sized and driven by NUM_OF_ALLOCS, so each vendor's sweep
>> + * must produce exactly NUM_OF_ALLOCS allocations. Enforce this at build time so
>> + * that changing a vendor's min/max/step can never overflow those arrays.
>> + */
>> +_Static_assert((ALLOCATION_MAX - ALLOCATION_MIN) / ALLOCATION_STEP + 1 == NUM_OF_ALLOCS,
>> +	       "Intel MBA sweep must yield NUM_OF_ALLOCS allocations");
> 
> Considering how NUM_ALLOCS is defined, could this ever be encountered?
> Could this instead be
> 	#define NUM_ALLOCS 10
> to ensure the sanity checks is accurate in both scenarios without deriving a
> architecture neutral count from Intel counts?
> 
>> +_Static_assert((AMD_ALLOCATION_MAX - AMD_ALLOCATION_MIN) / AMD_ALLOCATION_STEP + 1 == NUM_OF_ALLOCS,
>> +	       "AMD MBA sweep must yield NUM_OF_ALLOCS allocations");
> 
> Could _Static_assert be the more flexible static_assert() (and include assert.h)?
> 
>> +
>> +static unsigned int mba_alloc_min(void)
>> +{
>> +	return get_vendor() == ARCH_AMD ? AMD_ALLOCATION_MIN : ALLOCATION_MIN;
>> +}
>> +
>> +static unsigned int mba_alloc_max(void)
>> +{
>> +	return get_vendor() == ARCH_AMD ? AMD_ALLOCATION_MAX : ALLOCATION_MAX;
>> +}
>> +
>> +static unsigned int mba_alloc_step(void)
>> +{
>> +	return get_vendor() == ARCH_AMD ? AMD_ALLOCATION_STEP : ALLOCATION_STEP;
>> +}
>> +
>>   static int mba_init(const struct resctrl_test *test,
>>   		    const struct user_params *uparams,
>>   		    const struct resctrl_val_param *param, int domain_id)
>> @@ -33,15 +74,17 @@ static int mba_init(const struct resctrl_test *test,
>>   }
>>   
>>   /*
>> - * Change schemata percentage from 100 to 10%. Write schemata to specified
>> - * con_mon grp, mon_grp in resctrl FS.
>> + * Sweep the MBA schemata from the minimum to the maximum allocation. On
>> + * Intel the allocation is a percentage (10% .. 100%); on AMD it is an
>> + * absolute memory bandwidth value (4 GB/s .. 40 GB/s). Write schemata to
>> + * the specified con_mon grp, mon_grp in resctrl FS.
>>    * For each allocation, run 5 times in order to get average values.
>>    */
>>   static int mba_setup(const struct resctrl_test *test,
>>   		     const struct user_params *uparams,
>>   		     struct resctrl_val_param *p)
>>   {
>> -	static unsigned int allocation = ALLOCATION_MIN;
>> +	static unsigned int allocation;
>>   	static int runs_per_allocation;
>>   	char allocation_str[64];
>>   	int ret;
>> @@ -53,16 +96,20 @@ static int mba_setup(const struct resctrl_test *test,
>>   	if (runs_per_allocation++ != 0)
>>   		return 0;
>>   
>> -	if (allocation > ALLOCATION_MAX)
>> +	/* Set the initial allocation once the vendor is known. */
>> +	if (allocation == 0)
>> +		allocation = mba_alloc_min();
> 
> This does not look right. "once the vendor is known" does not look to be the
> real reason here since vendor is known at this point. This just seems to be
> a workaround to assign a function return to a static? It seems risky though since
> 0 is a valid minimum for AMD and that would cause problems if ever used with this
> implementation.
> 
> show_mba_info() already treats the allocation as an index as opposed to an
> accumulated value. Could this be simplified by using an index while also making
> it consistent with show_mba_info()?
> 
> Consider, for example, something like:
> 
>    	static int runs_per_allocation;
>    	static unsigned int alloc_idx;
>    	char allocation_str[64];
>    	unsigned int allocation;
>    	int ret;
>    ...
>    	if (alloc_idx >= NUM_OF_ALLOCS)
>    		return END_OF_TESTS;
> 
>    	allocation = mba_alloc_min() + mba_alloc_step() * alloc_idx;
>    	alloc_idx++;
> 
>    	sprintf(allocation_str, "%u", allocation);
> 
> I think having everything bounded by NUM_ALLOCS instead of mba_setup()
> bounded on accumulated allocation would make this overall easier to
> understand.
> 
> This would make mba_alloc_max() unnecessary.

This sounds good to me. We don't need to depend on mba_alloc_max().

Start with a minimum bandwidth for AMD (say, 4 GB/s), then increase it 
in 4 GB/s increments and repeat the test for NUM_OF_ALLOCS.

I have not tested this, but it should work.

Thanks,
Babu