Re: [PATCH v3 2/3] selftests/resctrl: Replace counter index references with pointers

[Reinette Chatre]

Hi Yifan,

Thank you. I just have a few style fixup comments ...

On 5/22/26 2:05 AM, Yifan Wu wrote:
> Replace direct counter number references with pointers to remove the
> dependency on fixed array indexing and enable the use of different
> data structures for counter management.
> 
> Signed-off-by: Yifan Wu <wuyifan50@xxxxxxxxxx>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 62 +++++++++----------
>  1 file changed, 31 insertions(+), 31 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index a72dc4ae61fe..3d2b6919717a 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -42,40 +42,40 @@ 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,
> +	memset(&imc_counter->pe, 0,
>  	       sizeof(struct perf_event_attr));

nit: above can fit on a single line

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

nit: above can fit on a single line

> +	imc_counter->pe.sample_type = PERF_SAMPLE_IDENTIFIER;
> +	imc_counter->pe.read_format =
>  		PERF_FORMAT_TOTAL_TIME_ENABLED | PERF_FORMAT_TOTAL_TIME_RUNNING;
>  }
>  

...

> @@ -89,21 +89,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_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);
>  	}
>  }
>  
> -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);

Please improve readibility here by moving the above two lines up while ensuring alignment with
open parenthesis. Specifically:
	imc_counter->fd = perf_event_open(&imc_counter->pe, -1, cpu_no, -1,
					  PERF_FLAG_FD_CLOEXEC);
...

With the style fixups:
| Reviewed-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>

Reinette