[PATCH v2 0/6] selftests/resctrl: Add dynamic linked list management for IMC counters

[PATCH v2 0/6] selftests/resctrl: Add dynamic linked list management for IMC counters

[Yifan Wu]

Hi all,

This patch series adds dynamic linked list management for the IMC
counters, which can work based on the actual number of counters instead of
an upper limit, without the need for array out-of-bounds access check.

This patch series is based on the Reinette's patch series aimed at fixing
the resctrl test and can be found at:
https://lore.kernel.org/lkml/cover.1775266384.git.reinette.chatre@intel.com/

changelog:
1. Fixed the code style and variable naming.
2. The initialization and cleanup of the linked list are integrated in patch 1.
3. The use of arrays is refactored to linked list split in the remaining patches.
4. The IMC count and global variable imcs are removed.
For more details, see the commit message.

v1 can be found at:
https://lore.kernel.org/all/20260324125034.1509177-1-wuyifan50@huawei.com/

Yifan Wu (6):
  selftests/resctrl: Introduced linked list management for IMC counters
  selftests/resctrl: Refactor the discovery of IMC counters using linked
    list
  selftests/resctrl: Refactor the initialization of IMC's perf_event_attr
    using linked list
  selftests/resctrl: Refactor perf event open/close using linked list
  selftests/resctrl: Refactor reading from IMC using linked list
  selftests/resctrl: Remove the definition of the IMC counter config
    array and imcs.

 tools/testing/selftests/resctrl/mba_test.c    |   1 +
 tools/testing/selftests/resctrl/mbm_test.c    |   1 +
 tools/testing/selftests/resctrl/resctrl.h     |   2 +
 tools/testing/selftests/resctrl/resctrl_val.c | 147 ++++++++++--------
 4 files changed, 82 insertions(+), 69 deletions(-)

-- 
2.43.0

[PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters

[Yifan Wu]

Added linked list based management for IMC counter configurations,
allowing the system to dynamically allocate and clean up resources based on
actual hardware capabilities.

Signed-off-by: Yifan Wu <wuyifan50@huawei.com>
---
 tools/testing/selftests/resctrl/mba_test.c    |  1 +
 tools/testing/selftests/resctrl/mbm_test.c    |  1 +
 tools/testing/selftests/resctrl/resctrl.h     |  2 ++
 tools/testing/selftests/resctrl/resctrl_val.c | 20 +++++++++++++++++++
 4 files changed, 24 insertions(+)

diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
index 39cee9898359..4bb1a82eb195 100644
--- a/tools/testing/selftests/resctrl/mba_test.c
+++ b/tools/testing/selftests/resctrl/mba_test.c
@@ -166,6 +166,7 @@ static int check_results(void)
 
 static void mba_test_cleanup(void)
 {
+	cleanup_read_mem_bw_imc();
 	remove(RESULT_FILE_NAME);
 }
 
diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
index 6dbbc3b76003..68c89f50a34a 100644
--- a/tools/testing/selftests/resctrl/mbm_test.c
+++ b/tools/testing/selftests/resctrl/mbm_test.c
@@ -125,6 +125,7 @@ static int mbm_measure(const struct user_params *uparams,
 
 static void mbm_test_cleanup(void)
 {
+	cleanup_read_mem_bw_imc();
 	remove(RESULT_FILE_NAME);
 }
 
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 175101022bf3..a7556cdae0de 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -24,6 +24,7 @@
 #include <linux/perf_event.h>
 #include <linux/compiler.h>
 #include <linux/bits.h>
+#include <linux/list.h>
 #include "kselftest.h"
 
 #define MB			(1024 * 1024)
@@ -183,6 +184,7 @@ void mem_flush(unsigned char *buf, size_t buf_size);
 void fill_cache_read(unsigned char *buf, size_t buf_size, bool once);
 ssize_t get_fill_buf_size(int cpu_no, const char *cache_type);
 int initialize_read_mem_bw_imc(void);
+void cleanup_read_mem_bw_imc(void);
 int measure_read_mem_bw(const struct user_params *uparams,
 			struct resctrl_val_param *param, pid_t bm_pid);
 void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index f20d2194c35f..d9ae24e9d971 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -28,6 +28,7 @@ struct membw_read_format {
 };
 
 struct imc_counter_config {
+	struct list_head entry;
 	__u32 type;
 	__u64 event;
 	__u64 umask;
@@ -38,6 +39,7 @@ struct imc_counter_config {
 static char mbm_total_path[1024];
 static int imcs;
 static struct imc_counter_config imc_counters_config[MAX_IMCS];
+LIST_HEAD(imc_counters_list);
 static const struct resctrl_test *current_test;
 
 static void read_mem_bw_initialize_perf_event_attr(int i)
@@ -113,6 +115,7 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
 				    unsigned int *count)
 {
 	char imc_events_dir[PATH_MAX], imc_counter_cfg[PATH_MAX];
+	struct imc_counter_config *imc_counter;
 	unsigned int orig_count = *count;
 	char cas_count_cfg[1024];
 	struct dirent *ep;
@@ -167,11 +170,17 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
 			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;
+		list_add(&imc_counter->entry, &imc_counters_list);
 	}
 	if (*count == orig_count) {
 		ksft_print_msg("Unable to find events in %s\n", imc_events_dir);
@@ -303,6 +312,17 @@ int initialize_read_mem_bw_imc(void)
 	return 0;
 }
 
+void cleanup_read_mem_bw_imc(void)
+{
+	struct imc_counter_config *imc_counter, *tmp;
+
+	list_for_each_entry_safe(imc_counter, tmp,
+				 &imc_counters_list, entry) {
+		list_del(&imc_counter->entry);
+		free(imc_counter);
+	}
+}
+
 static void perf_close_imc_read_mem_bw(void)
 {
 	int mc;
-- 
2.43.0

Re: [PATCH v2 1/6] selftests/resctrl: Introduced linked list management for IMC counters

[Reinette Chatre]

Hi Yifan,

On 4/10/26 2:33 AM, Yifan Wu wrote:
> Added linked list based management for IMC counter configurations,
> allowing the system to dynamically allocate and clean up resources based on
> actual hardware capabilities.

Above provides a motivation for this work but it does not help reviewer understand
what the patch does. Could you please expand with more detail about what the patch
does and since it is incomplete, provide insight into the context of this work to
help review it? For example, above just has "Added linked list based management
for IMC counter configurations" to describe what the patch does (rest is motivation)
but the patch does not actually do this ... it just adds a new and unused data structure
with empty elements in parallel to existing data structures.

Nit: Could you please write all changelogs with an imperative tone? For example,
"Introduced" -> "Introduce" in the subject and "Added" -> "Add" above?

> 
> Signed-off-by: Yifan Wu <wuyifan50@huawei.com>
> ---
>  tools/testing/selftests/resctrl/mba_test.c    |  1 +
>  tools/testing/selftests/resctrl/mbm_test.c    |  1 +
>  tools/testing/selftests/resctrl/resctrl.h     |  2 ++
>  tools/testing/selftests/resctrl/resctrl_val.c | 20 +++++++++++++++++++
>  4 files changed, 24 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
> index 39cee9898359..4bb1a82eb195 100644
> --- a/tools/testing/selftests/resctrl/mba_test.c
> +++ b/tools/testing/selftests/resctrl/mba_test.c
> @@ -166,6 +166,7 @@ static int check_results(void)
>  
>  static void mba_test_cleanup(void)
>  {
> +	cleanup_read_mem_bw_imc();
>  	remove(RESULT_FILE_NAME);
>  }
>  
> diff --git a/tools/testing/selftests/resctrl/mbm_test.c b/tools/testing/selftests/resctrl/mbm_test.c
> index 6dbbc3b76003..68c89f50a34a 100644
> --- a/tools/testing/selftests/resctrl/mbm_test.c
> +++ b/tools/testing/selftests/resctrl/mbm_test.c
> @@ -125,6 +125,7 @@ static int mbm_measure(const struct user_params *uparams,
>  
>  static void mbm_test_cleanup(void)
>  {
> +	cleanup_read_mem_bw_imc();
>  	remove(RESULT_FILE_NAME);
>  }
>  
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 175101022bf3..a7556cdae0de 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -24,6 +24,7 @@
>  #include <linux/perf_event.h>
>  #include <linux/compiler.h>
>  #include <linux/bits.h>
> +#include <linux/list.h>
>  #include "kselftest.h"
>  
>  #define MB			(1024 * 1024)
> @@ -183,6 +184,7 @@ void mem_flush(unsigned char *buf, size_t buf_size);
>  void fill_cache_read(unsigned char *buf, size_t buf_size, bool once);
>  ssize_t get_fill_buf_size(int cpu_no, const char *cache_type);
>  int initialize_read_mem_bw_imc(void);
> +void cleanup_read_mem_bw_imc(void);
>  int measure_read_mem_bw(const struct user_params *uparams,
>  			struct resctrl_val_param *param, pid_t bm_pid);
>  void initialize_mem_bw_resctrl(const struct resctrl_val_param *param,
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index f20d2194c35f..d9ae24e9d971 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -28,6 +28,7 @@ struct membw_read_format {
>  };
>  
>  struct imc_counter_config {
> +	struct list_head entry;
>  	__u32 type;
>  	__u64 event;
>  	__u64 umask;
> @@ -38,6 +39,7 @@ struct imc_counter_config {
>  static char mbm_total_path[1024];
>  static int imcs;
>  static struct imc_counter_config imc_counters_config[MAX_IMCS];
> +LIST_HEAD(imc_counters_list);
>  static const struct resctrl_test *current_test;
>  
>  static void read_mem_bw_initialize_perf_event_attr(int i)
> @@ -113,6 +115,7 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
>  				    unsigned int *count)
>  {
>  	char imc_events_dir[PATH_MAX], imc_counter_cfg[PATH_MAX];
> +	struct imc_counter_config *imc_counter;
>  	unsigned int orig_count = *count;
>  	char cas_count_cfg[1024];
>  	struct dirent *ep;
> @@ -167,11 +170,17 @@ static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
>  			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;
> +		list_add(&imc_counter->entry, &imc_counters_list);
>  	}
>  	if (*count == orig_count) {
>  		ksft_print_msg("Unable to find events in %s\n", imc_events_dir);

Should cleanup_read_mem_bw_imc() be called on error exit path?

> @@ -303,6 +312,17 @@ int initialize_read_mem_bw_imc(void)
>  	return 0;
>  }
>  
> +void cleanup_read_mem_bw_imc(void)
> +{
> +	struct imc_counter_config *imc_counter, *tmp;
> +
> +	list_for_each_entry_safe(imc_counter, tmp,
> +				 &imc_counters_list, entry) {

Looks like above can fit on one line.

> +		list_del(&imc_counter->entry);
> +		free(imc_counter);
> +	}
> +}
> +
>  static void perf_close_imc_read_mem_bw(void)
>  {
>  	int mc;

Reinette

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

[Yifan Wu]

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.

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
  */
-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);
 	}
 }
 
@@ -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;
 }
 
 int initialize_read_mem_bw_imc(void)
-- 
2.43.0

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

[Reinette Chatre]

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


 

[PATCH v2 3/6] selftests/resctrl: Refactor the initialization of IMC's perf_event_attr using linked list

[Yifan Wu]

The initialization of perf_event_attr in iMC is refactord using
linked list. Removed the initialization of the global variable imcs.

Signed-off-by: Yifan Wu <wuyifan50@huawei.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 38 +++++++++----------
 1 file changed, 19 insertions(+), 19 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 60cda2214c13..ce675d349a6e 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -42,20 +42,18 @@ static struct imc_counter_config imc_counters_config[MAX_IMCS];
 LIST_HEAD(imc_counters_list);
 static const struct resctrl_test *current_test;
 
-static void read_mem_bw_initialize_perf_event_attr(int i)
+static void read_mem_bw_initialize_perf_event_attr(struct imc_counter_config *imc_counter)
 {
-	memset(&imc_counters_config[i].pe, 0,
-	       sizeof(struct perf_event_attr));
-	imc_counters_config[i].pe.type = imc_counters_config[i].type;
-	imc_counters_config[i].pe.size = sizeof(struct perf_event_attr);
-	imc_counters_config[i].pe.disabled = 1;
-	imc_counters_config[i].pe.inherit = 1;
-	imc_counters_config[i].pe.exclude_guest = 0;
-	imc_counters_config[i].pe.config =
-		imc_counters_config[i].umask << 8 |
-		imc_counters_config[i].event;
-	imc_counters_config[i].pe.sample_type = PERF_SAMPLE_IDENTIFIER;
-	imc_counters_config[i].pe.read_format =
+	imc_counter->pe.type = imc_counter->type;
+	imc_counter->pe.size = sizeof(struct perf_event_attr);
+	imc_counter->pe.disabled = 1;
+	imc_counter->pe.inherit = 1;
+	imc_counter->pe.exclude_guest = 0;
+	imc_counter->pe.config =
+		imc_counter->umask << 8 |
+		imc_counter->event;
+	imc_counter->pe.sample_type = PERF_SAMPLE_IDENTIFIER;
+	imc_counter->pe.read_format =
 		PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING;
 }
 
@@ -292,15 +290,17 @@ static int num_of_imcs(void)
 
 int initialize_read_mem_bw_imc(void)
 {
-	int imc;
+	struct imc_counter_config *imc_counter;
+	int ret;
 
-	imcs = num_of_imcs();
-	if (imcs <= 0)
-		return imcs;
+	ret = num_of_imcs();
+	if (ret < 0)
+		return ret;
 
 	/* Initialize perf_event_attr structures for all iMC's */
-	for (imc = 0; imc < imcs; imc++)
-		read_mem_bw_initialize_perf_event_attr(imc);
+	list_for_each_entry(imc_counter, &imc_counters_list, entry) {
+		read_mem_bw_initialize_perf_event_attr(imc_counter);
+	}
 
 	return 0;
 }
-- 
2.43.0

Re: [PATCH v2 3/6] selftests/resctrl: Refactor the initialization of IMC's perf_event_attr using linked list

[Reinette Chatre]

Hi Yifan,

On 4/10/26 2:33 AM, Yifan Wu wrote:
> @@ -292,15 +290,17 @@ static int num_of_imcs(void)
>  
>  int initialize_read_mem_bw_imc(void)
>  {
> -	int imc;
> +	struct imc_counter_config *imc_counter;
> +	int ret;
>  
> -	imcs = num_of_imcs();
> -	if (imcs <= 0)
> -		return imcs;
> +	ret = num_of_imcs();
> +	if (ret < 0)
> +		return ret;

I see this change from "imcs" to "ret" as a consequence of the semantic change
to num_of_imcs() done in previous patch. Please move this change to be
located with the semantic change that will make that switch easier to understand.

Reinette

[PATCH v2 4/6] selftests/resctrl: Refactor perf event open/close using linked list

[Yifan Wu]

Using linked list when open/close perf event.

Signed-off-by: Yifan Wu <wuyifan50@huawei.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 29 ++++++++++---------
 1 file changed, 15 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index ce675d349a6e..ce5f96d5457c 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -93,15 +93,15 @@ static void get_read_event_and_umask(char *cas_count_cfg, struct imc_counter_con
 	}
 }
 
-static int open_perf_read_event(int i, int cpu_no)
+static int open_perf_read_event(int cpu_no, struct imc_counter_config *imc_counter)
 {
-	imc_counters_config[i].fd =
-		perf_event_open(&imc_counters_config[i].pe, -1, cpu_no, -1,
+	imc_counter->fd =
+		perf_event_open(&imc_counter->pe, -1, cpu_no, -1,
 				PERF_FLAG_FD_CLOEXEC);
 
-	if (imc_counters_config[i].fd == -1) {
+	if (imc_counter->fd == -1) {
 		fprintf(stderr, "Error opening leader %llx\n",
-			imc_counters_config[i].pe.config);
+			imc_counter->pe.config);
 
 		return -1;
 	}
@@ -318,11 +318,11 @@ void cleanup_read_mem_bw_imc(void)
 
 static void perf_close_imc_read_mem_bw(void)
 {
-	int mc;
+	struct imc_counter_config *imc_counter;
 
-	for (mc = 0; mc < imcs; mc++) {
-		if (imc_counters_config[mc].fd != -1)
-			close(imc_counters_config[mc].fd);
+	list_for_each_entry(imc_counter, &imc_counters_list, entry) {
+		if (imc_counter->fd != -1)
+			close(imc_counter->fd);
 	}
 }
 
@@ -334,13 +334,14 @@ static void perf_close_imc_read_mem_bw(void)
  */
 static int perf_open_imc_read_mem_bw(int cpu_no)
 {
-	int imc, ret;
+	struct imc_counter_config *imc_counter;
+	int ret;
 
-	for (imc = 0; imc < imcs; imc++)
-		imc_counters_config[imc].fd = -1;
+	list_for_each_entry(imc_counter, &imc_counters_list, entry)
+		imc_counter->fd = -1;
 
-	for (imc = 0; imc < imcs; imc++) {
-		ret = open_perf_read_event(imc, cpu_no);
+	list_for_each_entry(imc_counter, &imc_counters_list, entry) {
+		ret = open_perf_read_event(cpu_no, imc_counter);
 		if (ret)
 			goto close_fds;
 	}
-- 
2.43.0

Re: [PATCH v2 4/6] selftests/resctrl: Refactor perf event open/close using linked list

[Reinette Chatre]

Hi Yifan,

On 4/10/26 2:33 AM, Yifan Wu wrote:
> Using linked list when open/close perf event.
> 

To make this easier to review and avoid breaking the tests, please split this
patch with changes to open_perf_read_event() located in preparatory patch and
the rest located with all the other changes to switch tests to the list instead of the
array.
Please similarly split the similar patches that follow.

Reinette

[PATCH v2 5/6] selftests/resctrl: Refactor reading from IMC using linked list

[Yifan Wu]

Read the memory bandwidth from the IMC using linked list.

Signed-off-by: Yifan Wu <wuyifan50@huawei.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 26 +++++++++----------
 1 file changed, 12 insertions(+), 14 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index ce5f96d5457c..65ae93205b38 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -57,15 +57,15 @@ static void read_mem_bw_initialize_perf_event_attr(struct imc_counter_config *im
 		PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING;
 }
 
-static void read_mem_bw_ioctl_perf_event_ioc_reset_enable(int i)
+static void read_mem_bw_ioctl_perf_event_ioc_reset_enable(struct imc_counter_config *imc_counter)
 {
-	ioctl(imc_counters_config[i].fd, PERF_EVENT_IOC_RESET, 0);
-	ioctl(imc_counters_config[i].fd, PERF_EVENT_IOC_ENABLE, 0);
+	ioctl(imc_counter->fd, PERF_EVENT_IOC_RESET, 0);
+	ioctl(imc_counter->fd, PERF_EVENT_IOC_ENABLE, 0);
 }
 
-static void read_mem_bw_ioctl_perf_event_ioc_disable(int i)
+static void read_mem_bw_ioctl_perf_event_ioc_disable(struct imc_counter_config *imc_counter)
 {
-	ioctl(imc_counters_config[i].fd, PERF_EVENT_IOC_DISABLE, 0);
+	ioctl(imc_counter->fd, PERF_EVENT_IOC_DISABLE, 0);
 }
 
 /*
@@ -361,16 +361,16 @@ static int perf_open_imc_read_mem_bw(int cpu_no)
  */
 static void do_imc_read_mem_bw_test(void)
 {
-	int imc;
+	struct imc_counter_config *imc_counter;
 
-	for (imc = 0; imc < imcs; imc++)
-		read_mem_bw_ioctl_perf_event_ioc_reset_enable(imc);
+	list_for_each_entry(imc_counter, &imc_counters_list, entry)
+		read_mem_bw_ioctl_perf_event_ioc_reset_enable(imc_counter);
 
 	sleep(1);
 
 	/* Stop counters after a second to get results. */
-	for (imc = 0; imc < imcs; imc++)
-		read_mem_bw_ioctl_perf_event_ioc_disable(imc);
+	list_for_each_entry(imc_counter, &imc_counters_list, entry)
+		read_mem_bw_ioctl_perf_event_ioc_disable(imc_counter);
 }
 
 /*
@@ -385,17 +385,15 @@ static void do_imc_read_mem_bw_test(void)
 static int get_read_mem_bw_imc(float *bw_imc)
 {
 	float reads = 0, of_mul_read = 1;
-	int imc;
+	struct imc_counter_config *r;
 
 	/*
 	 * Log read event values from all iMC counters into
 	 * struct imc_counter_config.
 	 * Take overflow into consideration before calculating total bandwidth.
 	 */
-	for (imc = 0; imc < imcs; imc++) {
+	list_for_each_entry(r, &imc_counters_list, entry) {
 		struct membw_read_format measurement;
-		struct imc_counter_config *r =
-			&imc_counters_config[imc];
 
 		if (read(r->fd, &measurement, sizeof(measurement)) == -1) {
 			ksft_perror("Couldn't get read bandwidth through iMC");
-- 
2.43.0

[PATCH v2 6/6] selftests/resctrl: Remove the definition of the IMC counter config array and imcs.

[Yifan Wu]

The definitions of the imc counter configuration array, imcs, and MAX_IMCS
are removed.

Signed-off-by: Yifan Wu <wuyifan50@huawei.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index 65ae93205b38..acc2a4c19cf4 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -14,7 +14,6 @@
 #define READ_FILE_NAME		"cas_count_read"
 #define DYN_PMU_PATH		"/sys/bus/event_source/devices"
 #define SCALE			0.00006103515625
-#define MAX_IMCS		40
 #define MAX_TOKENS		5
 
 #define CON_MBM_LOCAL_BYTES_PATH		\
@@ -37,8 +36,6 @@ struct imc_counter_config {
 };
 
 static char mbm_total_path[1024];
-static int imcs;
-static struct imc_counter_config imc_counters_config[MAX_IMCS];
 LIST_HEAD(imc_counters_list);
 static const struct resctrl_test *current_test;
 
-- 
2.43.0

Re: [PATCH v2 6/6] selftests/resctrl: Remove the definition of the IMC counter config array and imcs.

[Reinette Chatre]

Hi Yifan,

On 4/10/26 2:33 AM, Yifan Wu wrote:
> The definitions of the imc counter configuration array, imcs, and MAX_IMCS
> are removed.

Please squash these removals with the patch that removes the last
usage.

Reinette