Re: [PATCH v5 3/5] selftests/resctrl: Add support for MBM and MBA tests on AMD

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

Hi Reinette,

On 29-10-2026 00:11, Reinette Chatre wrote:
> Hi Swapnil,
> 
> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index fdbbe2155cea..38cf58c41be1 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> 
> 
>> @@ -233,6 +277,7 @@ static int read_from_mc_dir(char *mc_dir)
>>    */
>>   static int enumerate_mcs(void)
>>   {
>> +	int (*read_mc_dir)(char *mc_dir);
> 
> Could this be named read_from_mc_dir to avoid the unnecessary churn?
> 

Sure, will update.

>>   	char mc_dir[512], *temp;
>>   	struct dirent *ep;
>>   	char *sysfs_name;
>> @@ -241,6 +286,10 @@ static int enumerate_mcs(void)
>>   
>>   	if (get_vendor() == ARCH_INTEL) {
>>   		sysfs_name = UNCORE_IMC;
>> +		read_mc_dir = read_from_intel_mc_dir;
>> +	} else if (get_vendor() == ARCH_AMD) {
>> +		sysfs_name = AMD_UMC;
>> +		read_mc_dir = read_from_amd_mc_dir;
>>   	} else {
>>   		ksft_print_msg("Unsupported vendor\n");
>>   		return -1;
> 
> ...
> 
>    		}
>> @@ -335,15 +387,40 @@ static void perf_close_mc_read_mem_bw(void)
>>   static int perf_open_mc_read_mem_bw(int cpu_no)
>>   {
>>   	struct mc_counter_config *mc_counter;
>> -	int ret;
>> +	int ret, opened = 0;
>>   
>>   	list_for_each_entry(mc_counter, &mc_counters_list, entry)
>>   		mc_counter->fd = -1;
>>   
>>   	list_for_each_entry(mc_counter, &mc_counters_list, entry) {
>>   		ret = open_perf_read_event(cpu_no, mc_counter);
>> -		if (ret)
>> -			goto close_fds;
>> +		if (!ret) {
>> +			opened++;
>> +			continue;
>> +		}
>> +
>> +		/*
>> +		 * On AMD each memory controller (UMC) has its own PMU that is
>> +		 * tied to a specific NUMA node, unlike Intel where opening an
>> +		 * iMC event on the benchmark CPU always resolves to the local
>> +		 * memory controllers. A UMC PMU can only be opened from a CPU
>> +		 * that belongs to the same node and returns -ENODEV otherwise.
>> +		 * The benchmark runs on a single CPU, so skip the UMCs that
>> +		 * belong to remote nodes and measure only the memory
>> +		 * controllers local to the benchmark. This matches what
>> +		 * mbm_local_bytes accounts for on the resctrl side.
>> +		 */
>> +		if (ret == -ENODEV && get_vendor() == ARCH_AMD)
>> +			continue;
>> +
>> +		ksft_print_msg("Error opening memory bandwidth performance counter: %s\n",
>> +			       strerror(-ret));
>> +		goto close_fds;
>> +	}
>> +
>> +	if (!opened) {
>> +		ksft_print_msg("Could not open any memory controller counters\n");
>> +		goto close_fds;
>>   	}
> 
> As the comment above highlights, the test runs on a single CPU. It is thus not
> possible for the test to change which MC it needs to obtain data from.
> perf_open_mc_read_mem_bw() is called several times during the test run and
> repeated attempts at accessing MC that are known to fail is unnecessary.
> Considering that -ENODEV can be relied on to indicate that the MC is not
> valid for the test run, could it instead be used to drop that MC from
> mc_counters_list? This will avoid the subsequent unnecessary attempts at
> accessing these MC and also simplify the users that walk mc_counters_list
> during the test run since they can continue to rely on all MC being valid
> without needing to keep checking if the fd is valid.
> 

This is good approach which reduces the number of times we are iterating 
full mc_counters_list. Thanks for the suggestion.

> I suggest switching the list_for_each_entry() to list_for_each_entry_safe()
> and drop the MC's from the list on ENODEV. This also eliminates the need for the
> "opened" tracking that can be replaced with a list_empty(&mc_counters_list).
> 

I will update accordingly.

>>   
>>   	return 0;
>> @@ -363,14 +440,20 @@ static void do_mc_read_mem_bw_test(void)
>>   {
>>   	struct mc_counter_config *mc_counter;
>>   
>> -	list_for_each_entry(mc_counter, &mc_counters_list, entry)
>> +	list_for_each_entry(mc_counter, &mc_counters_list, entry) {
>> +		if (mc_counter->fd == -1)
>> +			continue;
>>   		read_mem_bw_ioctl_perf_event_ioc_reset_enable(mc_counter);
>> +	}
>>   
>>   	sleep(1);
>>   
>>   	/* Stop counters after a second to get results. */
>> -	list_for_each_entry(mc_counter, &mc_counters_list, entry)
>> +	list_for_each_entry(mc_counter, &mc_counters_list, entry) {
>> +		if (mc_counter->fd == -1)
>> +			continue;
>>   		read_mem_bw_ioctl_perf_event_ioc_disable(mc_counter);
>> +	}
>>   }
>>   
>>   /*
>> @@ -395,6 +478,10 @@ static int get_read_mem_bw_mc(float *bw_mc)
>>   	list_for_each_entry(r, &mc_counters_list, entry) {
>>   		struct membw_read_format measurement;
>>   
>> +		/* Skip memory controllers that were not opened (remote node). */
>> +		if (r->fd == -1)
>> +			continue;
>> +
>>   		if (read(r->fd, &measurement, sizeof(measurement)) == -1) {
>>   			ksft_perror("Couldn't get read bandwidth through MC");
>>   			return -1;
> 
> With unusable MC dropped from mc_counters_list all the above fd checks are
> unnecessary.
> 

Ack.

> Reinette
--
Thanks and Regards,
Swapnil