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

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

Hi Babu,

On 03-11-2026 06:45, Moger, Babu wrote:
> 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.
> 

Thank you for the clarification.

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

Sure, I will update this accordingly in the next version.

> I have not tested this, but it should work.
> 
> Thanks,
> Babu
--
Thanks and Regards,
Swapnil