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 <[email protected]> > --- > 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 <[email protected]> -- i.
