[v4,0/2] selftests/resctrl: Bug fix and optimization

Message

Maciej Wieczor-Retman Sept. 22, 2023, 8:09 a.m. UTC

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.

Patch series is based on [1] which is based on [2] which are based on
ksefltest next branch.

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)
- 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/[e-mail]/
[v2] https://lore.kernel.org/all/[e-mail]/
[v3] https://lore.kernel.org/all/[e-mail]/

[1] https://lore.kernel.org/all/[e-mail]/
[2] https://lore.kernel.org/all/[e-mail]/

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   | 82 ++++---------------
 3 files changed, 67 insertions(+), 66 deletions(-)


base-commit: 3b3e8a34b1d50c2c5c6b030dab7682b123162cb4

Comments

Reinette Chatre Sept. 27, 2023, 10:16 p.m. UTC | #1

Hi Maciej,

On 9/22/2023 1:09 AM, Maciej Wieczor-Retman wrote:
> 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.

nitpick ... if there are comments in one patch of the series please consider
if it applies to other places in the series.

> 
> 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
> ksefltest next branch.

ksefltest -> kselftest

Reinette

Maciej Wieczor-Retman Sept. 28, 2023, 6:06 a.m. UTC | #2

Thanks for catching these, I'll remember to do that the next time.

On 2023-09-27 at 15:16:18 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 9/22/2023 1:09 AM, Maciej Wieczor-Retman wrote:
>> 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.
>
>nitpick ... if there are comments in one patch of the series please consider
>if it applies to other places in the series.
>
>> 
>> 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
>> ksefltest next branch.
>
>ksefltest -> kselftest
>
>Reinette