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

[PATCH 0/3] 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.1773432891.git.reinette.chatre@intel.com/

Yifan Wu (3):
  selftests/resctrl: Introduced linked list management for IMC counters
  selftests/resctrl: Replace array-based IMC counter management with
    linked lists
  selftests/resctrl: Add cleanup for MBM/MBA test

 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 | 159 ++++++++++--------
 4 files changed, 95 insertions(+), 68 deletions(-)

-- 
2.33.0

[PATCH 1/3] 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/resctrl.h     |  1 +
 tools/testing/selftests/resctrl/resctrl_val.c | 25 +++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 175101022bf3..29c9f76132f0 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)
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index f20d2194c35f..ac58d3862281 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 imc_list;
 	__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_configs);
 static const struct resctrl_test *current_test;
 
 static void read_mem_bw_initialize_perf_event_attr(int i)
@@ -235,6 +237,7 @@ static int read_from_imc_dir(char *imc_dir, unsigned int *count)
  */
 static int num_of_imcs(void)
 {
+	struct imc_counter_config *imc_counters_config;
 	char imc_dir[512], *temp;
 	unsigned int count = 0;
 	struct dirent *ep;
@@ -263,14 +266,23 @@ static int num_of_imcs(void)
 			 * first character is a numerical digit or not.
 			 */
 			if (temp[0] >= '0' && temp[0] <= '9') {
+				imc_counters_config = malloc(sizeof(struct imc_counter_config));
+				if (!imc_counters_config) {
+					ksft_print_msg("Unable to allocate memory for iMC counters\n");
+
+					return -1;
+				}
+				memset(imc_counters_config, 0, sizeof(struct imc_counter_config));
 				sprintf(imc_dir, "%s/%s/", DYN_PMU_PATH,
 					ep->d_name);
 				ret = read_from_imc_dir(imc_dir, &count);
 				if (ret) {
+					free(imc_counters_config);
 					closedir(dp);
 
 					return ret;
 				}
+				list_add(&imc_counters_config->imc_list, &imc_counters_configs);
 			}
 		}
 		closedir(dp);
@@ -303,6 +315,19 @@ int initialize_read_mem_bw_imc(void)
 	return 0;
 }
 
+void cleanup_read_mem_bw_imc(void)
+{
+	struct imc_counter_config *next_imc_counters_config;
+	struct imc_counter_config *imc_counters_config;
+
+	list_for_each_entry_safe(imc_counters_config, next_imc_counters_config,
+				 &imc_counters_configs, imc_list) {
+		list_del(&imc_counters_config->imc_list);
+		free(imc_counters_config);
+	}
+	INIT_LIST_HEAD(&imc_counters_configs);
+}
+
 static void perf_close_imc_read_mem_bw(void)
 {
 	int mc;
-- 
2.33.0

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

[Reinette Chatre]

Hi Yifan,

On 3/24/26 5:50 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.

Thank you very much for taking on this change, this is a good addition.

One note on the customs, even though this is user space code it is the Linux kernel
developers that work with it mostly and to support this the style is expected to
(as much as possible) match kernel coding style. Specifically related to this, please
follow coding and changelog guidance in Documentation/process/coding-style.rst and 
Documentation/process/maintainer-tip.rst.

> 
> Signed-off-by: Yifan Wu <wuyifan50@huawei.com>
> ---
>  tools/testing/selftests/resctrl/resctrl.h     |  1 +
>  tools/testing/selftests/resctrl/resctrl_val.c | 25 +++++++++++++++++++
>  2 files changed, 26 insertions(+)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 175101022bf3..29c9f76132f0 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>

(extra space)

>  #include "kselftest.h"
>  
>  #define MB			(1024 * 1024)
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index f20d2194c35f..ac58d3862281 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 imc_list;

To make it obvious that this is an entry/node of a list as opposed to a list
header, please use a name like "entry" or "node" or similar.

>  	__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_configs);
>  static const struct resctrl_test *current_test;
>  
>  static void read_mem_bw_initialize_perf_event_attr(int i)
> @@ -235,6 +237,7 @@ static int read_from_imc_dir(char *imc_dir, unsigned int *count)
>   */
>  static int num_of_imcs(void)
>  {
> +	struct imc_counter_config *imc_counters_config;

Please avoid variable shadowing.

>  	char imc_dir[512], *temp;
>  	unsigned int count = 0;
>  	struct dirent *ep;
> @@ -263,14 +266,23 @@ static int num_of_imcs(void)
>  			 * first character is a numerical digit or not.
>  			 */
>  			if (temp[0] >= '0' && temp[0] <= '9') {
> +				imc_counters_config = malloc(sizeof(struct imc_counter_config));

One example of the kernel coding style applied here (see "Allocating memory" in
Documentation/process/coding-style.rst) is to use pointer variable and
not typing out the struct.

Even so, this does not look to be the right place to do this allocation ... (more below)

> +				if (!imc_counters_config) {
> +					ksft_print_msg("Unable to allocate memory for iMC counters\n");
> +
> +					return -1;
> +				}
> +				memset(imc_counters_config, 0, sizeof(struct imc_counter_config));

Please use calloc() instead of the malloc() followed by memset().

>  				sprintf(imc_dir, "%s/%s/", DYN_PMU_PATH,
>  					ep->d_name);
>  				ret = read_from_imc_dir(imc_dir, &count);

read_from_imc_dir() obtains "count", which is used as index to imc_counters_config[],
as reference because it increments this counter. Doing so enables read_from_imc_dir()
to initialize more than one array element.

Only allocating one element for the list thus does not look right and I would actually
expect this allocation to be closer to the initialization where it is known whether
a new element is needed or not. Having new list element allocation within
parse_imc_read_bw_events() looks more appropriate?

>  				if (ret) {
> +					free(imc_counters_config);
>  					closedir(dp);
>  
>  					return ret;
>  				}
> +				list_add(&imc_counters_config->imc_list, &imc_counters_configs);
>  			}
>  		}
>  		closedir(dp);
> @@ -303,6 +315,19 @@ int initialize_read_mem_bw_imc(void)
>  	return 0;
>  }
>  
> +void cleanup_read_mem_bw_imc(void)
> +{
> +	struct imc_counter_config *next_imc_counters_config;

This could just be "*tmp" to make its usage clear.

> +	struct imc_counter_config *imc_counters_config;

I find the "imc_counters_configs" and "imc_counters_config" names too similar
for comfort and makes the code harder to follow. A short name that is obviously
different from the global list name will make the code much easier to read.
How about "imc_counter"?

> +
> +	list_for_each_entry_safe(imc_counters_config, next_imc_counters_config,
> +				 &imc_counters_configs, imc_list) {
> +		list_del(&imc_counters_config->imc_list);
> +		free(imc_counters_config);
> +	}
> +	INIT_LIST_HEAD(&imc_counters_configs);

Why is INIT_LIST_HEAD() needed?

> +}
> +
>  static void perf_close_imc_read_mem_bw(void)
>  {
>  	int mc;

This patch allocates memory but never frees is. This is done later in patch #3
as a fix but that change would be better as part of this to make it easier to
consider the memory management.

Reinette

[PATCH 2/3] selftests/resctrl: Replace array-based IMC counter management with linked lists

[Yifan Wu]

Convert IMC counter management from static array to dynamic
linked list allocation.

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

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index ac58d3862281..417d87ba368a 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		\
@@ -38,36 +37,37 @@ 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_configs);
 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_counters_config)
 {
-	memset(&imc_counters_config[i].pe, 0,
+	memset(&imc_counters_config->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_counters_config->pe.type = imc_counters_config->type;
+	imc_counters_config->pe.size = sizeof(struct perf_event_attr);
+	imc_counters_config->pe.disabled = 1;
+	imc_counters_config->pe.inherit = 1;
+	imc_counters_config->pe.exclude_guest = 0;
+	imc_counters_config->pe.config =
+		imc_counters_config->umask << 8 |
+		imc_counters_config->event;
+	imc_counters_config->pe.sample_type = PERF_SAMPLE_IDENTIFIER;
+	imc_counters_config->pe.read_format =
 		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_counters_config)
 {
-	ioctl(imc_counters_config[i].fd, PERF_EVENT_IOC_RESET, 0);
-	ioctl(imc_counters_config[i].fd, PERF_EVENT_IOC_ENABLE, 0);
+	ioctl(imc_counters_config->fd, PERF_EVENT_IOC_RESET, 0);
+	ioctl(imc_counters_config->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_counters_config)
 {
-	ioctl(imc_counters_config[i].fd, PERF_EVENT_IOC_DISABLE, 0);
+	ioctl(imc_counters_config->fd, PERF_EVENT_IOC_DISABLE, 0);
 }
 
 /*
@@ -75,7 +75,8 @@ 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_counters_config)
 {
 	char *token[MAX_TOKENS];
 	int i = 0;
@@ -89,21 +90,21 @@ 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_counters_config->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_counters_config->umask = strtol(token[i + 1], NULL, 16);
 	}
 }
 
-static int open_perf_read_event(int i, int cpu_no)
+static int open_perf_read_event(struct imc_counter_config *imc_counters_config, int cpu_no)
 {
-	imc_counters_config[i].fd =
-		perf_event_open(&imc_counters_config[i].pe, -1, cpu_no, -1,
+	imc_counters_config->fd =
+		perf_event_open(&imc_counters_config->pe, -1, cpu_no, -1,
 				PERF_FLAG_FD_CLOEXEC);
 
-	if (imc_counters_config[i].fd == -1) {
+	if (imc_counters_config->fd == -1) {
 		fprintf(stderr, "Error opening leader %llx\n",
-			imc_counters_config[i].pe.config);
+			imc_counters_config->pe.config);
 
 		return -1;
 	}
@@ -112,10 +113,10 @@ static int open_perf_read_event(int i, int cpu_no)
 }
 
 static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
-				    unsigned int *count)
+				    struct imc_counter_config *imc_counters_config)
 {
 	char imc_events_dir[PATH_MAX], imc_counter_cfg[PATH_MAX];
-	unsigned int orig_count = *count;
+	unsigned int orig_count = imcs;
 	char cas_count_cfg[1024];
 	struct dirent *ep;
 	int path_len;
@@ -165,17 +166,13 @@ 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_counters_config[*count].type = type;
-		get_read_event_and_umask(cas_count_cfg, *count);
-		/* Do not fail after incrementing *count. */
-		*count += 1;
+		imc_counters_config->type = type;
+		get_read_event_and_umask(cas_count_cfg, imc_counters_config);
+		/* Do not fail after incrementing count. */
+		imcs++;
 	}
-	if (*count == orig_count) {
+	if (imcs == orig_count) {
 		ksft_print_msg("Unable to find events in %s\n", imc_events_dir);
 		goto out_close;
 	}
@@ -186,7 +183,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, struct imc_counter_config *imc_counters_config)
 {
 	char imc_counter_type[PATH_MAX];
 	unsigned int type;
@@ -214,7 +211,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, imc_counters_config);
 	if (ret) {
 		ksft_print_msg("Unable to parse bandwidth event and umask\n");
 		return ret;
@@ -239,7 +236,7 @@ static int num_of_imcs(void)
 {
 	struct imc_counter_config *imc_counters_config;
 	char imc_dir[512], *temp;
-	unsigned int count = 0;
+	imcs = 0;
 	struct dirent *ep;
 	int ret;
 	DIR *dp;
@@ -275,7 +272,7 @@ static int num_of_imcs(void)
 				memset(imc_counters_config, 0, sizeof(struct imc_counter_config));
 				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, imc_counters_config);
 				if (ret) {
 					free(imc_counters_config);
 					closedir(dp);
@@ -286,7 +283,7 @@ static int num_of_imcs(void)
 			}
 		}
 		closedir(dp);
-		if (count == 0) {
+		if (imcs == 0) {
 			ksft_print_msg("Unable to find iMC counters\n");
 
 			return -1;
@@ -297,20 +294,22 @@ static int num_of_imcs(void)
 		return -1;
 	}
 
-	return count;
+	return imcs;
 }
 
 int initialize_read_mem_bw_imc(void)
 {
-	int imc;
+	int ret;
+	struct imc_counter_config *imc_counters_config;
 
-	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_counters_config, &imc_counters_configs, imc_list) {
+		read_mem_bw_initialize_perf_event_attr(imc_counters_config);
+	}
 
 	return 0;
 }
@@ -330,11 +329,11 @@ void cleanup_read_mem_bw_imc(void)
 
 static void perf_close_imc_read_mem_bw(void)
 {
-	int mc;
+	struct imc_counter_config *imc_counters_config;
 
-	for (mc = 0; mc < imcs; mc++) {
-		if (imc_counters_config[mc].fd != -1)
-			close(imc_counters_config[mc].fd);
+	list_for_each_entry(imc_counters_config, &imc_counters_configs, imc_list) {
+		if (imc_counters_config->fd != -1)
+			close(imc_counters_config->fd);
 	}
 }
 
@@ -346,13 +345,14 @@ static void perf_close_imc_read_mem_bw(void)
  */
 static int perf_open_imc_read_mem_bw(int cpu_no)
 {
-	int imc, ret;
+	int ret;
+	struct imc_counter_config *imc_counters_config;
 
-	for (imc = 0; imc < imcs; imc++)
-		imc_counters_config[imc].fd = -1;
+	list_for_each_entry(imc_counters_config, &imc_counters_configs, imc_list)
+		imc_counters_config->fd = -1;
 
-	for (imc = 0; imc < imcs; imc++) {
-		ret = open_perf_read_event(imc, cpu_no);
+	list_for_each_entry(imc_counters_config, &imc_counters_configs, imc_list) {
+		ret = open_perf_read_event(imc_counters_config, cpu_no);
 		if (ret)
 			goto close_fds;
 	}
@@ -372,16 +372,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_counters_config;
 
-	for (imc = 0; imc < imcs; imc++)
-		read_mem_bw_ioctl_perf_event_ioc_reset_enable(imc);
+	list_for_each_entry(imc_counters_config, &imc_counters_configs, imc_list)
+		read_mem_bw_ioctl_perf_event_ioc_reset_enable(imc_counters_config);
 
 	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_counters_config, &imc_counters_configs, imc_list)
+		read_mem_bw_ioctl_perf_event_ioc_disable(imc_counters_config);
 }
 
 /*
@@ -396,17 +396,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_configs, imc_list) {
 		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.33.0

Re: [PATCH 2/3] selftests/resctrl: Replace array-based IMC counter management with linked lists

[Reinette Chatre]

Hi Yifan,

On 3/24/26 5:50 AM, Yifan Wu wrote:
> Convert IMC counter management from static array to dynamic
> linked list allocation.

Could you please split this patch into two? 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?

> 
> Signed-off-by: Yifan Wu <wuyifan50@huawei.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 134 +++++++++---------
>  1 file changed, 66 insertions(+), 68 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index ac58d3862281..417d87ba368a 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		\
> @@ -38,36 +37,37 @@ 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_configs);
>  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_counters_config)

In parameters also, please use a variable name used for element that is further
away from list header name. How about just "imc_counter" for the function parameter? 

...

> @@ -112,10 +113,10 @@ static int open_perf_read_event(int i, int cpu_no)
>  }
>  
>  static int parse_imc_read_bw_events(char *imc_dir, unsigned int type,
> -				    unsigned int *count)
> +				    struct imc_counter_config *imc_counters_config)
>  {
>  	char imc_events_dir[PATH_MAX], imc_counter_cfg[PATH_MAX];
> -	unsigned int orig_count = *count;
> +	unsigned int orig_count = imcs;

Why is global imcs used/needed here? The intention behind orig_count is just to
check if any iMC counters were added by this function. Original code checked 
by comparing the "before" and "after" array index but with a switch to a list this
can just be done locally, for example, with a boolean.

>  	char cas_count_cfg[1024];
>  	struct dirent *ep;
>  	int path_len;
> @@ -165,17 +166,13 @@ 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_counters_config[*count].type = type;
> -		get_read_event_and_umask(cas_count_cfg, *count);
> -		/* Do not fail after incrementing *count. */
> -		*count += 1;
> +		imc_counters_config->type = type;
> +		get_read_event_and_umask(cas_count_cfg, imc_counters_config);
> +		/* Do not fail after incrementing count. */
> +		imcs++;

Note that this is a loop that may initialize more than one counter and since it
uses the single element provided as function parameter each new counter will just
overwrite the previous's settings.

As mentioned in patch #1 it looks more appropriate to allocate and initialize
new list entry here within parse_imc_read_bw_events().

> @@ -239,7 +236,7 @@ static int num_of_imcs(void)
>  {
>  	struct imc_counter_config *imc_counters_config;
>  	char imc_dir[512], *temp;
> -	unsigned int count = 0;
> +	imcs = 0;
>  	struct dirent *ep;
>  	int ret;
>  	DIR *dp;
> @@ -275,7 +272,7 @@ static int num_of_imcs(void)
>  				memset(imc_counters_config, 0, sizeof(struct imc_counter_config));
>  				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, imc_counters_config);
>  				if (ret) {
>  					free(imc_counters_config);
>  					closedir(dp);
> @@ -286,7 +283,7 @@ static int num_of_imcs(void)
>  			}
>  		}
>  		closedir(dp);
> -		if (count == 0) {
> +		if (imcs == 0) {

Is this global necessary? How about list_empty() instead?

>  			ksft_print_msg("Unable to find iMC counters\n");
>  
>  			return -1;
> @@ -297,20 +294,22 @@ static int num_of_imcs(void)
>  		return -1;
>  	}
>  
> -	return count;
> +	return imcs;

Looking at how the caller, initialize_read_mem_bw_imc() below, uses the return
value it does not seem necessary to track the number of entries anymore. Could the
global imcs just be dropped?

>  }
>  
>  int initialize_read_mem_bw_imc(void)
>  {
> -	int imc;
> +	int ret;
> +	struct imc_counter_config *imc_counters_config;
>  
> -	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_counters_config, &imc_counters_configs, imc_list) {
> +		read_mem_bw_initialize_perf_event_attr(imc_counters_config);
> +	}
>  
>  	return 0;
>  }
Reinette

[PATCH 3/3] selftests/resctrl: Add cleanup for MBM/MBA test

[Yifan Wu]

Added cleanup calls in MBA and MBM tests to prevent resource leaks.

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  | 1 +
 3 files changed, 3 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 29c9f76132f0..27dca1e7cf8e 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -184,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,
-- 
2.33.0