| Message ID | 20240408163247.3224-2-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: resctrl_val() related cleanups & improvements | expand |
Hi Ilpo, On 4/8/2024 9:32 AM, Ilpo Järvinen wrote: > get_mem_bw_imc() handles fds in a for loop but close() is based on two > fixed indexes READ and WRITE. > > Open code all for loops to READ+WRITE entries for clarity. > > Suggested-by: Reinette Chatre <reinette.chatre@intel.com> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> > --- > > v3: > - Rework entirely, use open coding instead of for loops for clarity > --- > tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++++--------- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c > index 445f306d4c2f..456cf0d0b8ca 100644 > --- a/tools/testing/selftests/resctrl/resctrl_val.c > +++ b/tools/testing/selftests/resctrl/resctrl_val.c > @@ -306,26 +306,28 @@ static int initialize_mem_bw_imc(void) > static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc) > { > float reads, writes, of_mul_read, of_mul_write; > - int imc, j, ret; > + int imc, ret; > > /* Start all iMC counters to log values (both read and write) */ > reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1; > for (imc = 0; imc < imcs; imc++) { > - for (j = 0; j < 2; j++) { > - ret = open_perf_event(imc, cpu_no, j); > - if (ret) > - return -1; > - } > - for (j = 0; j < 2; j++) > - membw_ioctl_perf_event_ioc_reset_enable(imc, j); > + ret = open_perf_event(imc, cpu_no, READ); > + if (ret) > + return -1; > + ret = open_perf_event(imc, cpu_no, WRITE); > + if (ret) > + return -1; > + > + membw_ioctl_perf_event_ioc_reset_enable(imc, READ); > + membw_ioctl_perf_event_ioc_reset_enable(imc, WRITE); > } The above highlights how the error checking is broken here and leaving this code like this until the later fix arrives is confusing (to me). Could you please squash "selftests/resctrl: Fix closing IMC fds on error" into this patch? Even so, I do not think that this addresses all the issues with error handling surrounding these fds. If I understand correctly these fds are currently (and remains to be after this series) closed within get_mem_bw_imc(). In this series there seems to be many occasions where open_perf_event() succeeds but later failures encountered before get_mem_bw_imc() results in these fds not being cleaned up. What do you think of having a perf_close_imc_mem_bw() to match with perf_open_imc_mem_bw() that closes the fds and can be called from within get_mem_bw_imc() as well as earlier if a failure is encountered between perf_open_imc_mem_bw() and get_mem_bw_imc()? Reinette
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c index 445f306d4c2f..456cf0d0b8ca 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -306,26 +306,28 @@ static int initialize_mem_bw_imc(void) static int get_mem_bw_imc(int cpu_no, char *bw_report, float *bw_imc) { float reads, writes, of_mul_read, of_mul_write; - int imc, j, ret; + int imc, ret; /* Start all iMC counters to log values (both read and write) */ reads = 0, writes = 0, of_mul_read = 1, of_mul_write = 1; for (imc = 0; imc < imcs; imc++) { - for (j = 0; j < 2; j++) { - ret = open_perf_event(imc, cpu_no, j); - if (ret) - return -1; - } - for (j = 0; j < 2; j++) - membw_ioctl_perf_event_ioc_reset_enable(imc, j); + ret = open_perf_event(imc, cpu_no, READ); + if (ret) + return -1; + ret = open_perf_event(imc, cpu_no, WRITE); + if (ret) + return -1; + + membw_ioctl_perf_event_ioc_reset_enable(imc, READ); + membw_ioctl_perf_event_ioc_reset_enable(imc, WRITE); } sleep(1); /* Stop counters after a second to get results (both read and write) */ for (imc = 0; imc < imcs; imc++) { - for (j = 0; j < 2; j++) - membw_ioctl_perf_event_ioc_disable(imc, j); + membw_ioctl_perf_event_ioc_disable(imc, READ); + membw_ioctl_perf_event_ioc_disable(imc, WRITE); } /*
get_mem_bw_imc() handles fds in a for loop but close() is based on two fixed indexes READ and WRITE. Open code all for loops to READ+WRITE entries for clarity. Suggested-by: Reinette Chatre <reinette.chatre@intel.com> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com> --- v3: - Rework entirely, use open coding instead of for loops for clarity --- tools/testing/selftests/resctrl/resctrl_val.c | 22 ++++++++++--------- 1 file changed, 12 insertions(+), 10 deletions(-)