Write_schemata() uses fprintf() to write a bitmask into a schemata file
inside resctrl FS. It checks fprintf() return value but it doesn't check
fclose() return value. Error codes from fprintf() such as write errors,
are buffered and flushed back to the user only after fclose() is executed
which means any invalid bitmask can be written into the schemata file.
Rewrite write_schemata() to use syscalls instead of stdio file
operations to avoid the buffering.
The resctrlfs.c defines functions that interact with the resctrl FS
while resctrl_val.c defines functions that perform measurements on
the cache. Run_benchmark() fits logically into the second file before
resctrl_val() that uses it.
Move run_benchmark() from resctrlfs.c to resctrl_val.c and remove
redundant part of the kernel-doc comment. Make run_benchmark() static
and remove it from the header file.
Patch series is based on [1] which is based on [2] which are based on
kselftest next branch.
Changelog v5:
- Add Ilpo's reviewed-by tag to Patch 1/2.
- Reword patch messages slightly.
- Add error check to schema_len variable.
Changelog v4:
- Change git signature from Wieczor-Retman Maciej to Maciej
Wieczor-Retman.
- Rebase onto [1] which is based on [2]. (Reinette)
- Add fcntl.h explicitly to provide glibc backward compatibility.
(Reinette)
Changelog v3:
- Use snprintf() return value instead of strlen() in write_schemata().
(Ilpo)
- Make run_benchmark() static and remove it from the header file.
(Reinette)
- Add Ilpo's reviewed-by tag to Patch 2/2.
- Patch messages and cover letter rewording.
Changelog v2:
- Change sprintf() to snprintf() in write_schemata().
- Redo write_schemata() with syscalls instead of stdio functions.
- Fix typos and missing dots in patch messages.
- Branch printf attribute patch to a separate series.
[v1] https://lore.kernel.org/all/[email protected]/
[v2] https://lore.kernel.org/all/[email protected]/
[v3] https://lore.kernel.org/all/[email protected]/
[v4] https://lore.kernel.org/all/[email protected]/
[1] https://lore.kernel.org/all/[email protected]/
[2] https://lore.kernel.org/all/[email protected]
Maciej Wieczor-Retman (2):
selftests/resctrl: Fix schemata write error check
selftests/resctrl: Move run_benchmark() to a more fitting file
tools/testing/selftests/resctrl/resctrl.h | 1 -
tools/testing/selftests/resctrl/resctrl_val.c | 50 +++++++++++
tools/testing/selftests/resctrl/resctrlfs.c | 88 +++++--------------
3 files changed, 73 insertions(+), 66 deletions(-)
base-commit: 3b3e8a34b1d50c2c5c6b030dab7682b123162cb4
--
2.42.0
resctrlfs.c contains mostly functions that interact in some way with
resctrl FS entries while functions inside resctrl_val.c deal with
measurements and benchmarking.
run_benchmark() is located in resctrlfs.c even though it's purpose
is not interacting with the resctrl FS but to execute cache checking
logic.
Move run_benchmark() to resctrl_val.c just before resctrl_val() that
makes use of run_benchmark(). Make run_benchmark() static since it's
not used between multiple files anymore.
Remove return comment from kernel-doc since the function is type void.
Reviewed-by: Ilpo Järvinen <[email protected]>
Reviewed-by: Reinette Chatre <[email protected]>
Signed-off-by: Maciej Wieczor-Retman <[email protected]>
---
Changelog v4:
- Reword patch message very slightly. (Reinette)
Changelog v3:
- Make run_benchmark() static and remove it from the header. (Reinette)
- Remove return void kernel-doc comment. (Ilpo)
- Added Ilpo's reviewed-by tag.
tools/testing/selftests/resctrl/resctrl.h | 1 -
tools/testing/selftests/resctrl/resctrl_val.c | 50 ++++++++++++++++++
tools/testing/selftests/resctrl/resctrlfs.c | 52 -------------------
3 files changed, 50 insertions(+), 53 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
index 8578a8b4e145..a33f414f6019 100644
--- a/tools/testing/selftests/resctrl/resctrl.h
+++ b/tools/testing/selftests/resctrl/resctrl.h
@@ -86,7 +86,6 @@ int validate_bw_report_request(char *bw_report);
bool validate_resctrl_feature_request(const char *resource, const char *feature);
char *fgrep(FILE *inf, const char *str);
int taskset_benchmark(pid_t bm_pid, int cpu_no);
-void run_benchmark(int signum, siginfo_t *info, void *ucontext);
int write_schemata(char *ctrlgrp, char *schemata, int cpu_no,
char *resctrl_val);
int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index a9fe61133119..0577e983067a 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -625,6 +625,56 @@ measure_vals(struct resctrl_val_param *param, unsigned long *bw_resc_start)
return 0;
}
+/*
+ * run_benchmark - Run a specified benchmark or fill_buf (default benchmark)
+ * in specified signal. Direct benchmark stdio to /dev/null.
+ * @signum: signal number
+ * @info: signal info
+ * @ucontext: user context in signal handling
+ */
+static void run_benchmark(int signum, siginfo_t *info, void *ucontext)
+{
+ int operation, ret, memflush;
+ char **benchmark_cmd;
+ size_t span;
+ bool once;
+ FILE *fp;
+
+ benchmark_cmd = info->si_ptr;
+
+ /*
+ * Direct stdio of child to /dev/null, so that only parent writes to
+ * stdio (console)
+ */
+ fp = freopen("/dev/null", "w", stdout);
+ if (!fp)
+ PARENT_EXIT("Unable to direct benchmark status to /dev/null");
+
+ if (strcmp(benchmark_cmd[0], "fill_buf") == 0) {
+ /* Execute default fill_buf benchmark */
+ span = strtoul(benchmark_cmd[1], NULL, 10);
+ memflush = atoi(benchmark_cmd[2]);
+ operation = atoi(benchmark_cmd[3]);
+ if (!strcmp(benchmark_cmd[4], "true"))
+ once = true;
+ else if (!strcmp(benchmark_cmd[4], "false"))
+ once = false;
+ else
+ PARENT_EXIT("Invalid once parameter");
+
+ if (run_fill_buf(span, memflush, operation, once))
+ fprintf(stderr, "Error in running fill buffer\n");
+ } else {
+ /* Execute specified benchmark */
+ ret = execvp(benchmark_cmd[0], benchmark_cmd);
+ if (ret)
+ perror("wrong\n");
+ }
+
+ fclose(stdout);
+ PARENT_EXIT("Unable to run specified benchmark");
+}
+
/*
* resctrl_val: execute benchmark and measure memory bandwidth on
* the benchmark
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index 342a3dbcdbb6..385983b5c9ce 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -294,58 +294,6 @@ int taskset_benchmark(pid_t bm_pid, int cpu_no)
return 0;
}
-/*
- * run_benchmark - Run a specified benchmark or fill_buf (default benchmark)
- * in specified signal. Direct benchmark stdio to /dev/null.
- * @signum: signal number
- * @info: signal info
- * @ucontext: user context in signal handling
- *
- * Return: void
- */
-void run_benchmark(int signum, siginfo_t *info, void *ucontext)
-{
- int operation, ret, memflush;
- char **benchmark_cmd;
- size_t span;
- bool once;
- FILE *fp;
-
- benchmark_cmd = info->si_ptr;
-
- /*
- * Direct stdio of child to /dev/null, so that only parent writes to
- * stdio (console)
- */
- fp = freopen("/dev/null", "w", stdout);
- if (!fp)
- PARENT_EXIT("Unable to direct benchmark status to /dev/null");
-
- if (strcmp(benchmark_cmd[0], "fill_buf") == 0) {
- /* Execute default fill_buf benchmark */
- span = strtoul(benchmark_cmd[1], NULL, 10);
- memflush = atoi(benchmark_cmd[2]);
- operation = atoi(benchmark_cmd[3]);
- if (!strcmp(benchmark_cmd[4], "true"))
- once = true;
- else if (!strcmp(benchmark_cmd[4], "false"))
- once = false;
- else
- PARENT_EXIT("Invalid once parameter");
-
- if (run_fill_buf(span, memflush, operation, once))
- fprintf(stderr, "Error in running fill buffer\n");
- } else {
- /* Execute specified benchmark */
- ret = execvp(benchmark_cmd[0], benchmark_cmd);
- if (ret)
- perror("wrong\n");
- }
-
- fclose(stdout);
- PARENT_EXIT("Unable to run specified benchmark");
-}
-
/*
* create_grp - Create a group only if one doesn't exist
* @grp_name: Name of the group
--
2.42.0