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 <[email protected]>
Signed-off-by: Ilpo Järvinen <[email protected]>
---
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);
}
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);
}
/*
--
2.39.2
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 <[email protected]>
> Signed-off-by: Ilpo Järvinen <[email protected]>
> ---
>
> 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