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

[Reinette Chatre]

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?

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

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

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

Reinette