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

[Swapnil Sapkal]

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