Re: [PATCH v2 2/6] selftests/resctrl: Refactor the discovery of IMC counters using linked list

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Yifan,

On 4/10/26 2:33 AM, Yifan Wu wrote:
> Use linked list to refactor the discovery of IMC counters. The counting
> during the discovery and the check on the upper limit of the number
> of IMC counters are removed.

Please apply the changelog comment of patch #1 to all patches in this
series.

> 
> Signed-off-by: Yifan Wu <wuyifan50@huawei.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 35 ++++++++-----------
>  1 file changed, 14 insertions(+), 21 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index d9ae24e9d971..60cda2214c13 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -75,7 +75,7 @@ static void read_mem_bw_ioctl_perf_event_ioc_disable(int i)
>   * @cas_count_cfg:	Config
>   * @count:		iMC number

Note function description above containing description of @count parameter

>   */
> -static void get_read_event_and_umask(char *cas_count_cfg, unsigned int count)
> +static void get_read_event_and_umask(char *cas_count_cfg, struct imc_counter_config *imc_counter)

Since above replaces @count with @imc_counter, please update function description to match.

>  {
>  	char *token[MAX_TOKENS];
>  	int i = 0;
> @@ -89,9 +89,9 @@ static void get_read_event_and_umask(char *cas_count_cfg, unsigned int count)
>  		if (!token[i])
>  			break;
>  		if (strcmp(token[i], "event") == 0)
> -			imc_counters_config[count].event = strtol(token[i + 1], NULL, 16);
> +			imc_counter->event = strtol(token[i + 1], NULL, 16);
>  		if (strcmp(token[i], "umask") == 0)
> -			imc_counters_config[count].umask = strtol(token[i + 1], NULL, 16);
> +			imc_counter->umask = strtol(token[i + 1], NULL, 16);
>  	}
>  }
>  
> @@ -111,12 +111,11 @@ static int open_perf_read_event(int i, int cpu_no)
>  	return 0;
>  }
>  
> -static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
> -				    unsigned int *count)
> +static int parse_imc_read_bw_events(char *imc_dir, unsigned int type)
>  {
>  	char imc_events_dir[PATH_MAX], imc_counter_cfg[PATH_MAX];
>  	struct imc_counter_config *imc_counter;
> -	unsigned int orig_count = *count;
> +	bool found_event = false;
>  	char cas_count_cfg[1024];
>  	struct dirent *ep;
>  	int path_len;
> @@ -166,23 +165,18 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
>  			ksft_perror("Could not get iMC cas count read");
>  			goto out_close;
>  		}
> -		if (*count >= MAX_IMCS) {
> -			ksft_print_msg("Maximum iMC count exceeded\n");
> -			goto out_close;
> -		}
>  		imc_counter = calloc(1, sizeof(*imc_counter));
>  		if (!imc_counter) {
>  			ksft_perror("Unable to allocate memory for iMC counters\n");
>  			goto out_close;
>  		}
>  
> -		imc_counters_config[*count].type = type;
> -		get_read_event_and_umask(cas_count_cfg, *count);
> -		/* Do not fail after incrementing *count. */
> -		*count += 1;
> +		imc_counter->type = type;
> +		get_read_event_and_umask(cas_count_cfg, imc_counter);
>  		list_add(&imc_counter->entry, &imc_counters_list);
> +		found_event = true;
>  	}
> -	if (*count == orig_count) {
> +	if (!found_event) {
>  		ksft_print_msg("Unable to find events in %s\n", imc_events_dir);
>  		goto out_close;
>  	}
> @@ -193,7 +187,7 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
>  }
>  
>  /* Get type and config of an iMC counter's read event. */
> -static int read_from_imc_dir(char *imc_dir, unsigned int *count)
> +static int read_from_imc_dir(char *imc_dir)
>  {
>  	char imc_counter_type[PATH_MAX];
>  	unsigned int type;
> @@ -221,7 +215,7 @@ static int read_from_imc_dir(char *imc_dir, unsigned int *count)
>  		ksft_perror("Could not get iMC type");
>  		return -1;
>  	}
> -	ret = parse_imc_read_bw_events(imc_dir, type, count);
> +	ret = parse_imc_read_bw_events(imc_dir, type);
>  	if (ret) {
>  		ksft_print_msg("Unable to parse bandwidth event and umask\n");
>  		return ret;
> @@ -245,7 +239,6 @@ static int read_from_imc_dir(char *imc_dir, unsigned int *count)
>  static int num_of_imcs(void)
>  {
>  	char imc_dir[512], *temp;
> -	unsigned int count = 0;
>  	struct dirent *ep;
>  	int ret;
>  	DIR *dp;
> @@ -274,7 +267,7 @@ static int num_of_imcs(void)
>  			if (temp[0] >= '0' && temp[0] <= '9') {
>  				sprintf(imc_dir, "%s/%s/", DYN_PMU_PATH,
>  					ep->d_name);
> -				ret = read_from_imc_dir(imc_dir, &count);
> +				ret = read_from_imc_dir(imc_dir);
>  				if (ret) {
>  					closedir(dp);
>  
> @@ -283,7 +276,7 @@ static int num_of_imcs(void)
>  			}
>  		}
>  		closedir(dp);
> -		if (count == 0) {
> +		if (list_empty(&imc_counters_list)) {
>  			ksft_print_msg("Unable to find iMC counters\n");
>  
>  			return -1;
> @@ -294,7 +287,7 @@ static int num_of_imcs(void)
>  		return -1;
>  	}
>  
> -	return count;
> +	return 0;

Since num_of_imcs() now returns a code instead of the number of iMCs found it seems
appropriate to change its name to match, something like "enumerate_imcs()"? Also
please note that num_of_imcs() still has function comments that needs to be updated
to match the new return code. Please check all patches to ensure when a function
signature is changed its description is considered also.

>  }
>  
>  int initialize_read_mem_bw_imc(void)

hmm ... this patch changes how iMCs are enumerated, now placing the data in the new
linked list, but the rest of the code still refers to the (now uninitialized) array.
After this patch the tests are thus broken and if somebody ever needs to do a bisect
and happens to land on this patch is will cause inconvenience.

Please split patches to be incremental changes where tests continue working after
every patch. You can do so with one patch where utilities receive pointer to
array element instead of index as parameter and another patch that switches the
code to use a list. (If this sounds familiar I just copied&pasted the previous
sentence from the v1 review.)

To provide more detail on what this would look like, consider the changes to
get_read_event_and_umask() in this patch:

@@ -75,7 +75,7 @@ static void read_mem_bw_ioctl_perf_event_ioc_disable(int i)
  * @cas_count_cfg:	Config
  * @count:		iMC number
  */
-static void get_read_event_and_umask(char *cas_count_cfg, unsigned int count)
+static void get_read_event_and_umask(char *cas_count_cfg, struct imc_counter_config *imc_counter)
 {
 	char *token[MAX_TOKENS];
 	int i = 0;
@@ -89,9 +89,9 @@ static void get_read_event_and_umask(char *cas_count_cfg, unsigned int count)
 		if (!token[i])
 			break;
 		if (strcmp(token[i], "event") == 0)
-			imc_counters_config[count].event = strtol(token[i + 1], NULL, 16);
+			imc_counter->event = strtol(token[i + 1], NULL, 16);
 		if (strcmp(token[i], "umask") == 0)
-			imc_counters_config[count].umask = strtol(token[i + 1], NULL, 16);
+			imc_counter->umask = strtol(token[i + 1], NULL, 16);
 	}
 }

A change like above can be made as first patch in the series in the code that still supports the
array with the callers providing a pointer to array element instead, for example:

	get_read_event_and_umask(cas_count_cfg, &imc_counters_config[*count]);

All the changes to utilities similar to above that simply replaces the index with a pointer to
struct imc_counter_config can be grouped into that first patch of the series. For example, the changes to
read_mem_bw_initialize_perf_event_attr(), open_perf_read_event(), read_mem_bw_ioctl_perf_event_ioc_reset_enable(), etc.

Grouping such changes into a preparatory patch is simple to review and reduces the churn when
switching to the list.

Reinette