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

[Swapnil Sapkal <swapnil.sapkal@amd.com>]

Hi Reinette,

On 29-10-2026 00:15, 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.
> 

Sure, will do.

>>   #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?
> 

As Babu mentioned, it's difficult for a single-threaded workload to 
reach the hardware-reported value. For that reason, I used these values 
to verify whether throttling takes effect.

>> +#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?

No, that assertion can't realistically 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?
>

Sure, will update this.
>> +_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)?
> 

Yes, static_assert() is a better choice.

>> +
>> +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.
> 

Agreed on all points, thank you.

I'll switch mba_setup() to an index bounded by NUM_OF_ALLOCS and compute 
the value as mba_alloc_min() + mba_alloc_step() * alloc_idx, which 
matches how show_mba_info() already walks the sweep. That drops the 
static accumulator and the sentinel, and makes mba_alloc_max() 
unnecessary, so I'll remove it too. Will do in the next version.

>> +
>> +	if (allocation > mba_alloc_max())
>>   		return END_OF_TESTS;
>>   
>> -	sprintf(allocation_str, "%d", allocation);
>> +	sprintf(allocation_str, "%u", allocation);
>>   
>>   	ret = write_schemata(p->ctrlgrp, allocation_str, uparams->cpu, test->resource);
>>   	if (ret < 0)
>>   		return ret;
>>   
>> -	allocation += ALLOCATION_STEP;
>> +	allocation += mba_alloc_step();
>>   
>>   	return 0;
>>   }
>> @@ -75,14 +122,15 @@ static int mba_measure(const struct user_params *uparams,
>>   
>>   static bool show_mba_info(unsigned long *bw_mc, unsigned long *bw_resc)
>>   {
>> +	unsigned int alloc_min = mba_alloc_min();
>> +	unsigned int alloc_step = mba_alloc_step();
> 
> Please maintain reverse fir order.
> 
>>   	unsigned int allocation;
>>   	bool ret = false;
>>   	int runs;
>>   
>>   	ksft_print_msg("Results are displayed in (MB)\n");
>> -	/* Memory bandwidth from 100% down to 10% */
>> -	for (allocation = 0; allocation < ALLOCATION_MAX / ALLOCATION_STEP;
>> -	     allocation++) {
>> +	/* Memory bandwidth for each of the swept MBA allocations */
>> +	for (allocation = 0; allocation < NUM_OF_ALLOCS; allocation++) {
> 
> As reference to earlier comment, here allocation is an index limited by NUM_OF_ALLOCS
> 

Agreed. I'll rename the loop variable here to alloc_idx to match the 
index-based mba_setup() from your earlier comment.

>>   		unsigned long sum_bw_mc = 0, sum_bw_resc = 0;
>>   		long avg_bw_mc, avg_bw_resc;
>>   		int avg_diff_per;
>> @@ -99,7 +147,7 @@ static bool show_mba_info(unsigned long *bw_mc, unsigned long *bw_resc)
>>   		if (avg_bw_mc < THROTTLE_THRESHOLD || avg_bw_resc < THROTTLE_THRESHOLD) {
>>   			ksft_print_msg("Bandwidth below threshold (%d MiB). Dropping results from MBA schemata %u.\n",
>>   				       THROTTLE_THRESHOLD,
>> -				       ALLOCATION_MIN + ALLOCATION_STEP * allocation);
>> +				       alloc_min + alloc_step * allocation);
>>   			continue;
>>   		}
>>   
>> @@ -110,7 +158,7 @@ static bool show_mba_info(unsigned long *bw_mc, unsigned long *bw_resc)
>>   			       avg_diff_per > MAX_DIFF_PERCENT ?
>>   			       "Fail:" : "Pass:",
>>   			       MAX_DIFF_PERCENT,
>> -			       ALLOCATION_MIN + ALLOCATION_STEP * allocation);
>> +			       alloc_min + alloc_step * allocation);
>>   
>>   		ksft_print_msg("avg_diff_per: %d%%\n", avg_diff_per);
>>   		ksft_print_msg("avg_bw_mc: %lu\n", avg_bw_mc);
>> @@ -129,8 +177,8 @@ static bool show_mba_info(unsigned long *bw_mc, unsigned long *bw_resc)
>>   
>>   static int check_results(void)
>>   {
>> -	unsigned long bw_resc[NUM_OF_RUNS * ALLOCATION_MAX / ALLOCATION_STEP];
>> -	unsigned long bw_mc[NUM_OF_RUNS * ALLOCATION_MAX / ALLOCATION_STEP];
>> +	unsigned long bw_resc[NUM_OF_RUNS * NUM_OF_ALLOCS];
>> +	unsigned long bw_mc[NUM_OF_RUNS * NUM_OF_ALLOCS];
> 
> Please re-order as part of this change to maintain reverse fir.
> 
>>   	char *token_array[8], output[] = RESULT_FILE_NAME, temp[512];
>>   	int runs;
>>   	FILE *fp;
>> @@ -147,6 +195,11 @@ static int check_results(void)
>>   		char *token = strtok(temp, ":\t");
>>   		int fields = 0;
>>   
>> +		if (runs >= NUM_OF_RUNS * NUM_OF_ALLOCS) {
>> +			ksft_print_msg("Got more results than expected\n");
>> +			break;
> 
> The message is worded as a failure but the test continues. This is potentially
> confusing. If it is a failure then test is expected to stop with an error, if it
> is not then the wording of the message could be adjusted to not imply so.
> 
> Even so, this change does not seem related to enabling support needed by AMD
> systems?
> 

You're right that it's unrelated to AMD enablement. It hardens the 
existing vendor-independent result parsing, so it doesn't belong in this 
patch. I will drop this in next version.

>> +		}
>> +
>>   		while (token) {
>>   			token_array[fields++] = token;
>>   			token = strtok(NULL, ":\t");
> 
> Reinette
--
Thanks and Regards,
Swapnil