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

[Moger, Babu]

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