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

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

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