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 file defines functions that interact with the resctrl FS while resctrl_val.c file defines functions that perform measurements on the cache. Run_benchmark() fits logically into the second file before resctrl_val() function 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.
Series is based on kselftest next branch.
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) - Added 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/cover.1692880423.git.maciej.wieczor-retman@intel... [v2] https://lore.kernel.org/all/cover.1693213468.git.maciej.wieczor-retman@intel...
Wieczor-Retman Maciej (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 | 78 ++++--------------- 3 files changed, 64 insertions(+), 65 deletions(-)
base-commit: 9b1db732866bee060b9bca9493e5ebf5e8874c48
Writing bitmasks to the schemata can fail when the bitmask doesn't adhere to constraints defined by what a particular CPU supports. Some example of constraints are max length or having contiguous bits. The driver should properly return errors when any rule concerning bitmask format is broken.
Resctrl FS returns error codes from fprintf() only when fclose() is called. Current error checking scheme allows invalid bitmasks to be written into schemata file and the selftest doesn't notice because the fclose() error code isn't checked.
Substitute fopen(), flose() and fprintf() with open(), close() and write() to avoid error code buffering between fprintf() and fclose().
Remove newline character from the schema string after writing it to the schemata file so it prints correctly before function return.
Pass the string generated with strerror() to the "reason" buffer so the error message is more verbose. Extend "reason" buffer so it can hold longer messages.
Signed-off-by: Wieczor-Retman Maciej maciej.wieczor-retman@intel.com --- Changelog v3: - Rename fp to fd (Ilpo) - Remove strlen, strcspn and just use the snprintf value instead (Ilpo)
Changelog v2: - Rewrite patch message. - Double "reason" buffer size to fit longer error explanation. - Redo file interactions with syscalls instead of stdio functions.
tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++---------- 1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index bd36ee206602..b0b14a5bcbf5 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -488,9 +488,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, */ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) { - char controlgroup[1024], schema[1024], reason[64]; - int resource_id, ret = 0; - FILE *fp; + char controlgroup[1024], schema[1024], reason[128]; + int resource_id, fd, schema_len = -1, ret = 0;
if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) && strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) && @@ -518,27 +517,30 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) || !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR))) - sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata); + schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n", + "L3:", resource_id, '=', schemata); if (!strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) || !strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR))) - sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata); + schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n", + "MB:", resource_id, '=', schemata);
- fp = fopen(controlgroup, "w"); - if (!fp) { + fd = open(controlgroup, O_WRONLY); + if (!fd) { sprintf(reason, "Failed to open control group"); ret = -1;
goto out; } - - if (fprintf(fp, "%s\n", schema) < 0) { - sprintf(reason, "Failed to write schemata in control group"); - fclose(fp); + if (write(fd, schema, schema_len) < 0) { + snprintf(reason, sizeof(reason), + "write() failed : %s", strerror(errno)); + close(fd); ret = -1;
goto out; } - fclose(fp); + close(fd); + schema[schema_len - 1] = 0;
out: ksft_print_msg("Write schema "%s" to resctrl FS%s%s\n",
Hi Maciej,
On 9/1/2023 6:42 AM, Wieczor-Retman Maciej wrote:
Writing bitmasks to the schemata can fail when the bitmask doesn't adhere to constraints defined by what a particular CPU supports. Some example of constraints are max length or having contiguous bits. The driver should properly return errors when any rule concerning bitmask format is broken.
Resctrl FS returns error codes from fprintf() only when fclose() is called. Current error checking scheme allows invalid bitmasks to be written into schemata file and the selftest doesn't notice because the fclose() error code isn't checked.
Substitute fopen(), flose() and fprintf() with open(), close() and write() to avoid error code buffering between fprintf() and fclose().
Remove newline character from the schema string after writing it to the schemata file so it prints correctly before function return.
Pass the string generated with strerror() to the "reason" buffer so the error message is more verbose. Extend "reason" buffer so it can hold longer messages.
Signed-off-by: Wieczor-Retman Maciej maciej.wieczor-retman@intel.com
When I build the tests with this applied I encounter the following:
resctrlfs.c: In function ‘write_schemata’: resctrlfs.c:475:14: warning: implicit declaration of function ‘open’; did you mean ‘popen’? [-Wimplicit-function-declaration] 475 | fd = open(controlgroup, O_WRONLY); | ^~~~ | popen resctrlfs.c:475:33: error: ‘O_WRONLY’ undeclared (first use in this function) 475 | fd = open(controlgroup, O_WRONLY); | ^~~~~~~~ resctrlfs.c:475:33: note: each undeclared identifier is reported only once for each function it appears in
Changelog v3:
- Rename fp to fd (Ilpo)
- Remove strlen, strcspn and just use the snprintf value instead (Ilpo)
Changelog v2:
- Rewrite patch message.
- Double "reason" buffer size to fit longer error explanation.
- Redo file interactions with syscalls instead of stdio functions.
tools/testing/selftests/resctrl/resctrlfs.c | 26 +++++++++++---------- 1 file changed, 14 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index bd36ee206602..b0b14a5bcbf5 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -488,9 +488,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, */ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) {
- char controlgroup[1024], schema[1024], reason[64];
- int resource_id, ret = 0;
- FILE *fp;
- char controlgroup[1024], schema[1024], reason[128];
- int resource_id, fd, schema_len = -1, ret = 0;
if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) && strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) && @@ -518,27 +517,30 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) || !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n","L3:", resource_id, '=', schemata);
sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n","MB:", resource_id, '=', schemata);
- fp = fopen(controlgroup, "w");
- if (!fp) {
- fd = open(controlgroup, O_WRONLY);
- if (!fd) { sprintf(reason, "Failed to open control group");
It makes code easier to understand and maintain if it is kept consistent. It is thus unexpected for open() error handling to be untouched while write() error handling is modified. I think the addition of errno in error handling of write() is helpful. Could you do the same for open()?
ret = -1;goto out; }
- if (fprintf(fp, "%s\n", schema) < 0) {
sprintf(reason, "Failed to write schemata in control group");fclose(fp);
- if (write(fd, schema, schema_len) < 0) {
snprintf(reason, sizeof(reason),"write() failed : %s", strerror(errno));close(fd);goto out; }
- fclose(fp);
- close(fd);
- schema[schema_len - 1] = 0;
out: ksft_print_msg("Write schema "%s" to resctrl FS%s%s\n",
Reinette
On 2023-09-11 at 09:59:06 -0700, Reinette Chatre wrote:
Hi Maciej, When I build the tests with this applied I encounter the following:
resctrlfs.c: In function ‘write_schemata’: resctrlfs.c:475:14: warning: implicit declaration of function ‘open’; did you mean ‘popen’? [-Wimplicit-function-declaration] 475 | fd = open(controlgroup, O_WRONLY); | ^~~~ | popen resctrlfs.c:475:33: error: ‘O_WRONLY’ undeclared (first use in this function) 475 | fd = open(controlgroup, O_WRONLY); | ^~~~~~~~ resctrlfs.c:475:33: note: each undeclared identifier is reported only once for each function it appears in
Hmm, that's odd. How do you build the tests?
I use "make -C tools/testing/selftests/resctrl" while in the root kernel source directory. I tried to get the same error you experienced by compiling some dummy test program with "open" and "O_WRONLY". From the experiment I found that the "resctrl.h" header provides the declarations that are causing your errors.
diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index bd36ee206602..b0b14a5bcbf5 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -488,9 +488,8 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp, */ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) {
- char controlgroup[1024], schema[1024], reason[64];
- int resource_id, ret = 0;
- FILE *fp;
- char controlgroup[1024], schema[1024], reason[128];
- int resource_id, fd, schema_len = -1, ret = 0;
if (strncmp(resctrl_val, MBA_STR, sizeof(MBA_STR)) && strncmp(resctrl_val, MBM_STR, sizeof(MBM_STR)) && @@ -518,27 +517,30 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val) if (!strncmp(resctrl_val, CAT_STR, sizeof(CAT_STR)) || !strncmp(resctrl_val, CMT_STR, sizeof(CMT_STR)))
sprintf(schema, "%s%d%c%s", "L3:", resource_id, '=', schemata);
schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n","L3:", resource_id, '=', schemata);
sprintf(schema, "%s%d%c%s", "MB:", resource_id, '=', schemata);
schema_len = snprintf(schema, sizeof(schema), "%s%d%c%s\n","MB:", resource_id, '=', schemata);
- fp = fopen(controlgroup, "w");
- if (!fp) {
- fd = open(controlgroup, O_WRONLY);
- if (!fd) { sprintf(reason, "Failed to open control group");
It makes code easier to understand and maintain if it is kept consistent. It is thus unexpected for open() error handling to be untouched while write() error handling is modified. I think the addition of errno in error handling of write() is helpful. Could you do the same for open()?
Okay, I'll add that, thanks.
Hi Maciej,
On 9/11/2023 11:32 PM, Maciej Wieczór-Retman wrote:
On 2023-09-11 at 09:59:06 -0700, Reinette Chatre wrote:
Hi Maciej, When I build the tests with this applied I encounter the following:
resctrlfs.c: In function ‘write_schemata’: resctrlfs.c:475:14: warning: implicit declaration of function ‘open’; did you mean ‘popen’? [-Wimplicit-function-declaration] 475 | fd = open(controlgroup, O_WRONLY); | ^~~~ | popen resctrlfs.c:475:33: error: ‘O_WRONLY’ undeclared (first use in this function) 475 | fd = open(controlgroup, O_WRONLY); | ^~~~~~~~ resctrlfs.c:475:33: note: each undeclared identifier is reported only once for each function it appears in
Hmm, that's odd. How do you build the tests?
I applied this series on top of kselftest repo's "next" branch.
I use a separate build directory and first ran "make headers". After that, $ make O=<build dir> -C tools/testing/selftests/resctrl
I use "make -C tools/testing/selftests/resctrl" while in the root kernel source directory. I tried to get the same error you experienced by compiling some dummy test program with "open" and "O_WRONLY". From the experiment I found that the "resctrl.h" header provides the declarations that are causing your errors.
From what I can tell resctrl.h does not include fcntl.h that provides what is needed.
Reinette
On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
Hi Maciej,
On 9/11/2023 11:32 PM, Maciej Wieczór-Retman wrote:
On 2023-09-11 at 09:59:06 -0700, Reinette Chatre wrote:
Hi Maciej, When I build the tests with this applied I encounter the following:
resctrlfs.c: In function ‘write_schemata’: resctrlfs.c:475:14: warning: implicit declaration of function ‘open’; did you mean ‘popen’? [-Wimplicit-function-declaration] 475 | fd = open(controlgroup, O_WRONLY); | ^~~~ | popen resctrlfs.c:475:33: error: ‘O_WRONLY’ undeclared (first use in this function) 475 | fd = open(controlgroup, O_WRONLY); | ^~~~~~~~ resctrlfs.c:475:33: note: each undeclared identifier is reported only once for each function it appears in
Hmm, that's odd. How do you build the tests?
I applied this series on top of kselftest repo's "next" branch.
I use a separate build directory and first ran "make headers". After that, $ make O=<build dir> -C tools/testing/selftests/resctrl
I do the same, just without the build directory, but that shouldn't matter here I guess.
I use "make -C tools/testing/selftests/resctrl" while in the root kernel source directory. I tried to get the same error you experienced by compiling some dummy test program with "open" and "O_WRONLY". From the experiment I found that the "resctrl.h" header provides the declarations that are causing your errors.
From what I can tell resctrl.h does not include fcntl.h that provides what is needed.
I found out you can run "gcc -M <file>" and it will recursively tell you what headers are including other headers.
Using this I found that "resctrl.h" includes <sys/mount.h> which in turn includes <fcntl.h> out of /usr/include/sys directory. Is that also the case on your system?
Hi Maciej,
On 9/12/2023 10:59 PM, Maciej Wieczór-Retman wrote:
On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
Hi Maciej,
On 9/11/2023 11:32 PM, Maciej Wieczór-Retman wrote:
On 2023-09-11 at 09:59:06 -0700, Reinette Chatre wrote:
Hi Maciej, When I build the tests with this applied I encounter the following:
resctrlfs.c: In function ‘write_schemata’: resctrlfs.c:475:14: warning: implicit declaration of function ‘open’; did you mean ‘popen’? [-Wimplicit-function-declaration] 475 | fd = open(controlgroup, O_WRONLY); | ^~~~ | popen resctrlfs.c:475:33: error: ‘O_WRONLY’ undeclared (first use in this function) 475 | fd = open(controlgroup, O_WRONLY); | ^~~~~~~~ resctrlfs.c:475:33: note: each undeclared identifier is reported only once for each function it appears in
Hmm, that's odd. How do you build the tests?
I applied this series on top of kselftest repo's "next" branch.
I use a separate build directory and first ran "make headers". After that, $ make O=<build dir> -C tools/testing/selftests/resctrl
I do the same, just without the build directory, but that shouldn't matter here I guess.
I use "make -C tools/testing/selftests/resctrl" while in the root kernel source directory. I tried to get the same error you experienced by compiling some dummy test program with "open" and "O_WRONLY". From the experiment I found that the "resctrl.h" header provides the declarations that are causing your errors.
From what I can tell resctrl.h does not include fcntl.h that provides what is needed.
I found out you can run "gcc -M <file>" and it will recursively tell you what headers are including other headers.
Using this I found that "resctrl.h" includes <sys/mount.h> which in turn includes <fcntl.h> out of /usr/include/sys directory. Is that also the case on your system?
No. The test system I used is running glibc 2.35 and it seems that including fcntl.h was added to sys/mount.h in 2.36. See glibc commit 78a408ee7ba0 ("linux: Add open_tree")
Generally we should avoid indirect inclusions and here I think certainly so since it cannot be guaranteed that fcntl.h would be available via sys/mount.h.
Reinette
Hi,
On 2023-09-13 at 11:49:19 -0700, Reinette Chatre wrote:
Hi Maciej,
On 9/12/2023 10:59 PM, Maciej Wieczór-Retman wrote:
On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
Hi Maciej,
On 9/11/2023 11:32 PM, Maciej Wieczór-Retman wrote:
On 2023-09-11 at 09:59:06 -0700, Reinette Chatre wrote:
Hi Maciej, When I build the tests with this applied I encounter the following:
resctrlfs.c: In function ‘write_schemata’: resctrlfs.c:475:14: warning: implicit declaration of function ‘open’; did you mean ‘popen’? [-Wimplicit-function-declaration] 475 | fd = open(controlgroup, O_WRONLY); | ^~~~ | popen resctrlfs.c:475:33: error: ‘O_WRONLY’ undeclared (first use in this function) 475 | fd = open(controlgroup, O_WRONLY); | ^~~~~~~~ resctrlfs.c:475:33: note: each undeclared identifier is reported only once for each function it appears in
Hmm, that's odd. How do you build the tests?
I applied this series on top of kselftest repo's "next" branch.
I use a separate build directory and first ran "make headers". After that, $ make O=<build dir> -C tools/testing/selftests/resctrl
I do the same, just without the build directory, but that shouldn't matter here I guess.
I use "make -C tools/testing/selftests/resctrl" while in the root kernel source directory. I tried to get the same error you experienced by compiling some dummy test program with "open" and "O_WRONLY". From the experiment I found that the "resctrl.h" header provides the declarations that are causing your errors.
From what I can tell resctrl.h does not include fcntl.h that provides what is needed.
I found out you can run "gcc -M <file>" and it will recursively tell you what headers are including other headers.
Using this I found that "resctrl.h" includes <sys/mount.h> which in turn includes <fcntl.h> out of /usr/include/sys directory. Is that also the case on your system?
No. The test system I used is running glibc 2.35 and it seems that including fcntl.h was added to sys/mount.h in 2.36. See glibc commit 78a408ee7ba0 ("linux: Add open_tree")
Generally we should avoid indirect inclusions and here I think certainly so since it cannot be guaranteed that fcntl.h would be available via sys/mount.h.
Okay, would including the fcntl.h header to resctrl.h be okay in this case? Or is there some other more sophisticated way of doing that (some include guard or checking glibc version for example)?
Hi Maciej,
On 9/13/2023 11:01 PM, Maciej Wieczór-Retman wrote:
On 2023-09-13 at 11:49:19 -0700, Reinette Chatre wrote:
On 9/12/2023 10:59 PM, Maciej Wieczór-Retman wrote:
On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
On 9/11/2023 11:32 PM, Maciej Wieczór-Retman wrote:
On 2023-09-11 at 09:59:06 -0700, Reinette Chatre wrote:
Hi Maciej, When I build the tests with this applied I encounter the following:
resctrlfs.c: In function ‘write_schemata’: resctrlfs.c:475:14: warning: implicit declaration of function ‘open’; did you mean ‘popen’? [-Wimplicit-function-declaration] 475 | fd = open(controlgroup, O_WRONLY); | ^~~~ | popen resctrlfs.c:475:33: error: ‘O_WRONLY’ undeclared (first use in this function) 475 | fd = open(controlgroup, O_WRONLY); | ^~~~~~~~ resctrlfs.c:475:33: note: each undeclared identifier is reported only once for each function it appears in
Hmm, that's odd. How do you build the tests?
I applied this series on top of kselftest repo's "next" branch.
I use a separate build directory and first ran "make headers". After that, $ make O=<build dir> -C tools/testing/selftests/resctrl
I do the same, just without the build directory, but that shouldn't matter here I guess.
I use "make -C tools/testing/selftests/resctrl" while in the root kernel source directory. I tried to get the same error you experienced by compiling some dummy test program with "open" and "O_WRONLY". From the experiment I found that the "resctrl.h" header provides the declarations that are causing your errors.
From what I can tell resctrl.h does not include fcntl.h that provides what is needed.
I found out you can run "gcc -M <file>" and it will recursively tell you what headers are including other headers.
Using this I found that "resctrl.h" includes <sys/mount.h> which in turn includes <fcntl.h> out of /usr/include/sys directory. Is that also the case on your system?
No. The test system I used is running glibc 2.35 and it seems that including fcntl.h was added to sys/mount.h in 2.36. See glibc commit 78a408ee7ba0 ("linux: Add open_tree")
Generally we should avoid indirect inclusions and here I think certainly so since it cannot be guaranteed that fcntl.h would be available via sys/mount.h.
Okay, would including the fcntl.h header to resctrl.h be okay in this case? Or is there some other more sophisticated way of doing that (some include guard or checking glibc version for example)?
Ideally fcntl.h would be included in the file it is used. Doing so you may encounter the same problems as Ilpo in [1]. If that is the case and that fix works for you then you may want to have this series depend on Ilpo's work.
Reinette
[1] https://lore.kernel.org/lkml/dfc53e-3f92-82e4-6af-d1a28e8c199a@linux.intel.c...
On 2023-09-14 at 08:14:25 -0700, Reinette Chatre wrote:
Hi Maciej,
On 9/13/2023 11:01 PM, Maciej Wieczór-Retman wrote:
On 2023-09-13 at 11:49:19 -0700, Reinette Chatre wrote:
On 9/12/2023 10:59 PM, Maciej Wieczór-Retman wrote:
On 2023-09-12 at 09:00:28 -0700, Reinette Chatre wrote:
On 9/11/2023 11:32 PM, Maciej Wieczór-Retman wrote:
On 2023-09-11 at 09:59:06 -0700, Reinette Chatre wrote: > Hi Maciej, > When I build the tests with this applied I encounter the following: > > resctrlfs.c: In function ‘write_schemata’: > resctrlfs.c:475:14: warning: implicit declaration of function ‘open’; did you mean ‘popen’? [-Wimplicit-function-declaration] > 475 | fd = open(controlgroup, O_WRONLY); > | ^~~~ > | popen > resctrlfs.c:475:33: error: ‘O_WRONLY’ undeclared (first use in this function) > 475 | fd = open(controlgroup, O_WRONLY); > | ^~~~~~~~ > resctrlfs.c:475:33: note: each undeclared identifier is reported only once for each function it appears in
Hmm, that's odd. How do you build the tests?
I applied this series on top of kselftest repo's "next" branch.
I use a separate build directory and first ran "make headers". After that, $ make O=<build dir> -C tools/testing/selftests/resctrl
I do the same, just without the build directory, but that shouldn't matter here I guess.
I use "make -C tools/testing/selftests/resctrl" while in the root kernel source directory. I tried to get the same error you experienced by compiling some dummy test program with "open" and "O_WRONLY". From the experiment I found that the "resctrl.h" header provides the declarations that are causing your errors.
From what I can tell resctrl.h does not include fcntl.h that provides what is needed.
I found out you can run "gcc -M <file>" and it will recursively tell you what headers are including other headers.
Using this I found that "resctrl.h" includes <sys/mount.h> which in turn includes <fcntl.h> out of /usr/include/sys directory. Is that also the case on your system?
No. The test system I used is running glibc 2.35 and it seems that including fcntl.h was added to sys/mount.h in 2.36. See glibc commit 78a408ee7ba0 ("linux: Add open_tree")
Generally we should avoid indirect inclusions and here I think certainly so since it cannot be guaranteed that fcntl.h would be available via sys/mount.h.
Okay, would including the fcntl.h header to resctrl.h be okay in this case? Or is there some other more sophisticated way of doing that (some include guard or checking glibc version for example)?
Ideally fcntl.h would be included in the file it is used. Doing so you may encounter the same problems as Ilpo in [1]. If that is the case and that fix works for you then you may want to have this series depend on Ilpo's work.
Thanks a lot for finding this, and yes, I get the same errors by adding the header. I'll send the next version of this series with the added header rebased on top of Ilpo's series you mentioned.
Reinette
[1] https://lore.kernel.org/lkml/dfc53e-3f92-82e4-6af-d1a28e8c199a@linux.intel.c...
resctrlfs.c file 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 file 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.
Signed-off-by: Wieczor-Retman Maciej maciej.wieczor-retman@intel.com Reviewed-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com --- 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 838d1a438f33..c6cca28810cd 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -89,7 +89,6 @@ int validate_bw_report_request(char *bw_report); bool validate_resctrl_feature_request(const char *resctrl_val); 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 f0f6c5f6e98b..764acf5efa14 100644 --- a/tools/testing/selftests/resctrl/resctrl_val.c +++ b/tools/testing/selftests/resctrl/resctrl_val.c @@ -621,6 +621,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 b0b14a5bcbf5..2f1ca184ac40 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -291,58 +291,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
Hi Maciej,
On 9/1/2023 6:42 AM, Wieczor-Retman Maciej wrote:
resctrlfs.c file contains mostly functions that interact in some way
This can just be "resctrlfs.c contains ..." (no need for "file")
with resctrl FS entries while functions inside resctrl_val.c deal with measurements and benchmarking.
run_benchmark() is located in resctrlfs.c file even though it's
same here
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.
Signed-off-by: Wieczor-Retman Maciej maciej.wieczor-retman@intel.com Reviewed-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
Hi,
On 2023-09-11 at 09:59:31 -0700, Reinette Chatre wrote:
Hi Maciej,
On 9/1/2023 6:42 AM, Wieczor-Retman Maciej wrote:
resctrlfs.c file contains mostly functions that interact in some way
This can just be "resctrlfs.c contains ..." (no need for "file")
Right, sorry, I'll correct this and the one below.
with resctrl FS entries while functions inside resctrl_val.c deal with measurements and benchmarking.
run_benchmark() is located in resctrlfs.c file even though it's
same here
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.
Signed-off-by: Wieczor-Retman Maciej maciej.wieczor-retman@intel.com Reviewed-by: Ilpo Järvinen ilpo.jarvinen@linux.intel.com
Reviewed-by: Reinette Chatre reinette.chatre@intel.com
Reinette
Thank you for the review!
linux-kselftest-mirror@lists.linaro.org
-
Maciej Wieczór-Retman -
Reinette Chatre -
Wieczor-Retman Maciej