Re: [PATCH v2 2/9] selftests/resctrl: Do not store iMC counter value in counter config structure

[Ilpo Järvinen]

On Tue, 3 Mar 2026, Reinette Chatre wrote:

> The MBM and MBA tests compare MBM memory bandwidth measurements against
> the memory bandwidth event values obtained from each memory controller's
> PMU. The memory bandwidth event settings are discovered from the memory
> controller details found in /sys/bus/event_source/devices/uncore_imc_N and
> stored in struct imc_counter_config.
> 
> In addition to event settings struct imc_counter_config contains
> imc_counter_config::return_value in which the associated event value is
> stored on every read.
> 
> The event value is consumed and immediately recorded at regular intervals.
> The stored value is never consumed afterwards, making its storage as part
> of event configuration unnecessary.
> 
> Remove the return_value member from struct imc_counter_config. Instead
> just use a local variable for use during event reading.
> 
> Signed-off-by: Reinette Chatre <reinette.chatre@xxxxxxxxx>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 11 +++++------
>  1 file changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index a5a8badb83d4..2cc22f61a1f8 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -32,7 +32,6 @@ struct imc_counter_config {
>  	__u64 event;
>  	__u64 umask;
>  	struct perf_event_attr pe;
> -	struct membw_read_format return_value;
>  	int fd;
>  };
>  
> @@ -312,23 +311,23 @@ static int get_read_mem_bw_imc(float *bw_imc)
>  	 * Take overflow into consideration before calculating total bandwidth.
>  	 */
>  	for (imc = 0; imc < imcs; imc++) {
> +		struct membw_read_format return_value;
>  		struct imc_counter_config *r =
>  			&imc_counters_config[imc];
>  
> -		if (read(r->fd, &r->return_value,
> -			 sizeof(struct membw_read_format)) == -1) {
> +		if (read(r->fd, &return_value, sizeof(return_value)) == -1) {
>  			ksft_perror("Couldn't get read bandwidth through iMC");
>  			return -1;
>  		}
>  
> -		__u64 r_time_enabled = r->return_value.time_enabled;
> -		__u64 r_time_running = r->return_value.time_running;
> +		__u64 r_time_enabled = return_value.time_enabled;
> +		__u64 r_time_running = return_value.time_running;
>  
>  		if (r_time_enabled != r_time_running)
>  			of_mul_read = (float)r_time_enabled /
>  					(float)r_time_running;
>  
> -		reads += r->return_value.value * of_mul_read * SCALE;
> +		reads += return_value.value * of_mul_read * SCALE;
>  	}

This looks mostly okay though here too I don't like the variable name. 
Something like "measurement" would tell what it is much better than overly 
vague "return_value".

Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>

-- 
 i.