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

Ilpo Järvinen
2026-03-06T09:52:08Z

[-- Attachment #1: Type: text/plain, Size: 2847 bytes --]

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@intel.com>
> ---
> 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@linux.intel.com>

--
i.