[PATCH 0/3] selftests/resctrl: Bug fix and optimizations

[PATCH 0/3] selftests/resctrl: Bug fix and optimizations

[Wieczor-Retman, Maciej]

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 flushed back to the user only after fclose() is executed which means
any invalid bitmask can be written into the schemata file.

Save fclose() return value so it can be returned at the end of the
function.

Add a perror() call after fprintf() so if any error occurs the error
message is more verbose

Kselftest.h declares many variadic functions that can print some
formatted message while also executing selftest logic. These
declarations don't have any compiler mechanism to verify if passed
arguments are valid in comparison with format specifiers used in
printf() calls.

Add a __printf() macro similiar to other tools in the kernel.

Add __printf() attributes to function definitions inside kselftest.h that
use printing

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 just before
resctrl_val() function definition.

Series is based on kselftest next branch

Wieczor-Retman, Maciej (3):
  selftests/resctrl: Fix schemata write error check
  selftests/resctrl: Move run_benchmark() to a more fitting file
  selftests: Add printf attribute to ksefltest prints

 tools/testing/selftests/kselftest.h           | 18 +++---
 tools/testing/selftests/resctrl/resctrl_val.c | 52 ++++++++++++++++
 tools/testing/selftests/resctrl/resctrlfs.c   | 60 ++-----------------
 3 files changed, 68 insertions(+), 62 deletions(-)


base-commit: 13eb52f6293dbda02890698d92f3d9913d8d5aeb
-- 
2.42.0

[PATCH 1/3] selftests/resctrl: Fix schemata write error check

[Wieczor-Retman, Maciej]

Writing bitmasks to the schemata can fail when the bitmask doesn't
adhere to some constraints defined by what a particular CPU supports.
Some example of constraints are max length or being 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.

Add error check to the fclose() call.

Add perror() just after fprintf so a proper error message can be seen.

Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
---
 tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
index bd36ee206602..a6d0b632cbc6 100644
--- a/tools/testing/selftests/resctrl/resctrlfs.c
+++ b/tools/testing/selftests/resctrl/resctrlfs.c
@@ -532,13 +532,17 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
 	}
 
 	if (fprintf(fp, "%s\n", schema) < 0) {
-		sprintf(reason, "Failed to write schemata in control group");
+		sprintf(reason, "fprintf() failed with error : %s",
+			strerror(errno));
 		fclose(fp);
 		ret = -1;
 
 		goto out;
 	}
-	fclose(fp);
+	ret = fclose(fp);
+	if (ret)
+		sprintf(reason, "Failed to write schemata in control group : %s",
+			strerror(errno));
 
 out:
 	ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n",
-- 
2.42.0

[PATCH 2/3] selftests/resctrl: Move run_benchmark() to a more fitting file

[Wieczor-Retman, Maciej]

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() function 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() function
that makes use of run_benchmark()

Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
---
 tools/testing/selftests/resctrl/resctrl_val.c | 52 +++++++++++++++++++
 tools/testing/selftests/resctrl/resctrlfs.c   | 52 -------------------
 2 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
index f0f6c5f6e98b..667542c084eb 100644
--- a/tools/testing/selftests/resctrl/resctrl_val.c
+++ b/tools/testing/selftests/resctrl/resctrl_val.c
@@ -621,6 +621,58 @@ 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
+ *
+ * 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");
+}
+
 /*
  * 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 a6d0b632cbc6..e3c94614c086 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
-- 
2.42.0

[PATCH 3/3] selftests: Add printf attribute to ksefltest prints

[Wieczor-Retman, Maciej]

Kselftest header defines multiple variadic function that use printf
along with other logic

There is no format checking for the variadic functions that use
printing inside kselftest.h. Because of this the compiler won't
be able to catch instances of mismatched print formats and debugging
tests might be more difficult

Add the common __printf attribute macro to kselftest.h

Add __printf attribute to every function using formatted printing with
variadic arguments

Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
---
 tools/testing/selftests/kselftest.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
index 829be379545a..ff47ed711879 100644
--- a/tools/testing/selftests/kselftest.h
+++ b/tools/testing/selftests/kselftest.h
@@ -77,6 +77,8 @@
 #define KSFT_XPASS 3
 #define KSFT_SKIP  4
 
+#define __printf(a, b)   __attribute__((format(printf, a, b)))
+
 /* counters */
 struct ksft_count {
 	unsigned int ksft_pass;
@@ -134,7 +136,7 @@ static inline void ksft_print_cnts(void)
 		ksft_cnt.ksft_xskip, ksft_cnt.ksft_error);
 }
 
-static inline void ksft_print_msg(const char *msg, ...)
+static inline __printf(1, 2) void ksft_print_msg(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -146,7 +148,7 @@ static inline void ksft_print_msg(const char *msg, ...)
 	va_end(args);
 }
 
-static inline void ksft_test_result_pass(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_pass(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -160,7 +162,7 @@ static inline void ksft_test_result_pass(const char *msg, ...)
 	va_end(args);
 }
 
-static inline void ksft_test_result_fail(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_fail(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -186,7 +188,7 @@ static inline void ksft_test_result_fail(const char *msg, ...)
 		ksft_test_result_fail(fmt, ##__VA_ARGS__);\
 	} while (0)
 
-static inline void ksft_test_result_xfail(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_xfail(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -200,7 +202,7 @@ static inline void ksft_test_result_xfail(const char *msg, ...)
 	va_end(args);
 }
 
-static inline void ksft_test_result_skip(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_skip(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -215,7 +217,7 @@ static inline void ksft_test_result_skip(const char *msg, ...)
 }
 
 /* TODO: how does "error" differ from "fail" or "skip"? */
-static inline void ksft_test_result_error(const char *msg, ...)
+static inline __printf(1, 2) void ksft_test_result_error(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -262,7 +264,7 @@ static inline int ksft_exit_fail(void)
 		  ksft_cnt.ksft_xfail +	\
 		  ksft_cnt.ksft_xskip)
 
-static inline int ksft_exit_fail_msg(const char *msg, ...)
+static inline __printf(1, 2) int ksft_exit_fail_msg(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
@@ -289,7 +291,7 @@ static inline int ksft_exit_xpass(void)
 	exit(KSFT_XPASS);
 }
 
-static inline int ksft_exit_skip(const char *msg, ...)
+static inline __printf(1, 2) int ksft_exit_skip(const char *msg, ...)
 {
 	int saved_errno = errno;
 	va_list args;
-- 
2.42.0

Re: [PATCH 1/3] selftests/resctrl: Fix schemata write error check

[Ilpo Järvinen]

Ki,

You're lacking a few people from the To/Cc list. Please see KERNEL 
SELFTEST FRAMEWORK entry in MAINTAINERS.

On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:

> Writing bitmasks to the schemata can fail when the bitmask doesn't
> adhere to some constraints defined by what a particular CPU supports.
> Some example of constraints are max length or being having contiguous

"being having" is not good English.

> 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.

I wonder if this is actually related to libc doing buffering between 
fprintf() and the actual write() syscall.

> 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.
> 
> Add error check to the fclose() call.
> 
> Add perror() just after fprintf so a proper error message can be seen.
> 
> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index bd36ee206602..a6d0b632cbc6 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -532,13 +532,17 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>  	}
>  
>  	if (fprintf(fp, "%s\n", schema) < 0) {
> -		sprintf(reason, "Failed to write schemata in control group");
> +		sprintf(reason, "fprintf() failed with error : %s",
> +			strerror(errno));

These should use snprintf() to make sure the buffer does not overflow. 

>  		fclose(fp);
>  		ret = -1;
>  
>  		goto out;
>  	}
> -	fclose(fp);
> +	ret = fclose(fp);
> +	if (ret)
> +		sprintf(reason, "Failed to write schemata in control group : %s",
> +			strerror(errno));
>  
>  out:
>  	ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n",
> 

-- 
 i.

Re: [PATCH 2/3] selftests/resctrl: Move run_benchmark() to a more fitting file

[Ilpo Järvinen]

On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:

Thanks for this patch, the new location is much more appropriate and 
logical (more than once I've tried to look for this from the wrong file).

> 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() function 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() function
> that makes use of run_benchmark()

Please terminate your sentences in changelog with . like in normal 
writing.

> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_val.c | 52 +++++++++++++++++++
>  tools/testing/selftests/resctrl/resctrlfs.c   | 52 -------------------
>  2 files changed, 52 insertions(+), 52 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
> index f0f6c5f6e98b..667542c084eb 100644
> --- a/tools/testing/selftests/resctrl/resctrl_val.c
> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
> @@ -621,6 +621,58 @@ 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
> + *
> + * Return: void

This Return: void feels waste of screen space as if it wouldn't be 
obvious from the function signature.

> + */
> +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 a6d0b632cbc6..e3c94614c086 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
> 

-- 
 i.

Re: [PATCH 3/3] selftests: Add printf attribute to ksefltest prints

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 3904 bytes --]

On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:

> Kselftest header defines multiple variadic function that use printf
> along with other logic
> 
> There is no format checking for the variadic functions that use
> printing inside kselftest.h. Because of this the compiler won't
> be able to catch instances of mismatched print formats and debugging
> tests might be more difficult
> 
> Add the common __printf attribute macro to kselftest.h
> 
> Add __printf attribute to every function using formatted printing with
> variadic arguments

Please add . to terminate the sentences.

The patch looks fine:
Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>

...However, there are formatting errors it found yet to fix.

-- 
 i.

> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
> ---
>  tools/testing/selftests/kselftest.h | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/tools/testing/selftests/kselftest.h b/tools/testing/selftests/kselftest.h
> index 829be379545a..ff47ed711879 100644
> --- a/tools/testing/selftests/kselftest.h
> +++ b/tools/testing/selftests/kselftest.h
> @@ -77,6 +77,8 @@
>  #define KSFT_XPASS 3
>  #define KSFT_SKIP  4
>  
> +#define __printf(a, b)   __attribute__((format(printf, a, b)))
> +
>  /* counters */
>  struct ksft_count {
>  	unsigned int ksft_pass;
> @@ -134,7 +136,7 @@ static inline void ksft_print_cnts(void)
>  		ksft_cnt.ksft_xskip, ksft_cnt.ksft_error);
>  }
>  
> -static inline void ksft_print_msg(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_print_msg(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -146,7 +148,7 @@ static inline void ksft_print_msg(const char *msg, ...)
>  	va_end(args);
>  }
>  
> -static inline void ksft_test_result_pass(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_pass(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -160,7 +162,7 @@ static inline void ksft_test_result_pass(const char *msg, ...)
>  	va_end(args);
>  }
>  
> -static inline void ksft_test_result_fail(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_fail(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -186,7 +188,7 @@ static inline void ksft_test_result_fail(const char *msg, ...)
>  		ksft_test_result_fail(fmt, ##__VA_ARGS__);\
>  	} while (0)
>  
> -static inline void ksft_test_result_xfail(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_xfail(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -200,7 +202,7 @@ static inline void ksft_test_result_xfail(const char *msg, ...)
>  	va_end(args);
>  }
>  
> -static inline void ksft_test_result_skip(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_skip(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -215,7 +217,7 @@ static inline void ksft_test_result_skip(const char *msg, ...)
>  }
>  
>  /* TODO: how does "error" differ from "fail" or "skip"? */
> -static inline void ksft_test_result_error(const char *msg, ...)
> +static inline __printf(1, 2) void ksft_test_result_error(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -262,7 +264,7 @@ static inline int ksft_exit_fail(void)
>  		  ksft_cnt.ksft_xfail +	\
>  		  ksft_cnt.ksft_xskip)
>  
> -static inline int ksft_exit_fail_msg(const char *msg, ...)
> +static inline __printf(1, 2) int ksft_exit_fail_msg(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> @@ -289,7 +291,7 @@ static inline int ksft_exit_xpass(void)
>  	exit(KSFT_XPASS);
>  }
>  
> -static inline int ksft_exit_skip(const char *msg, ...)
> +static inline __printf(1, 2) int ksft_exit_skip(const char *msg, ...)
>  {
>  	int saved_errno = errno;
>  	va_list args;
> 

Re: [PATCH 1/3] selftests/resctrl: Fix schemata write error check

[Maciej Wieczór-Retman]

Hi,

On 2023-08-24 at 15:52:05 +0300, Ilpo Järvinen wrote:
>Ki,
>
>You're lacking a few people from the To/Cc list. Please see KERNEL 
>SELFTEST FRAMEWORK entry in MAINTAINERS.

Thank you, I thought I checked the MAINTAINERS file well enough. I'll
add them in the next version

>On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>
>> Writing bitmasks to the schemata can fail when the bitmask doesn't
>> adhere to some constraints defined by what a particular CPU supports.
>> Some example of constraints are max length or being having contiguous
>
>"being having" is not good English.

Thanks, I'll change it

>> 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.
>
>I wonder if this is actually related to libc doing buffering between 
>fprintf() and the actual write() syscall.

I started looking and apparently in the manpages for fclose [1] it says
it uses fflush() to flush any buffered data in the stream. So that would
probably confirm that it does buffering there.

In this case is there a situation when the fprintf() before fclose()
would report an error? I'm thinking if there is a point to keep error
checking after both function calls or just fclose(). 

Or would putting additional fflush() after fprintf() make some sense?
To have separate error checks for both function calls.

>> 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.
>> 
>> Add error check to the fclose() call.
>> 
>> Add perror() just after fprintf so a proper error message can be seen.
>> 
>> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
>> ---
>>  tools/testing/selftests/resctrl/resctrlfs.c | 8 ++++++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
>> index bd36ee206602..a6d0b632cbc6 100644
>> --- a/tools/testing/selftests/resctrl/resctrlfs.c
>> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
>> @@ -532,13 +532,17 @@ int write_schemata(char *ctrlgrp, char *schemata, int cpu_no, char *resctrl_val)
>>  	}
>>  
>>  	if (fprintf(fp, "%s\n", schema) < 0) {
>> -		sprintf(reason, "Failed to write schemata in control group");
>> +		sprintf(reason, "fprintf() failed with error : %s",
>> +			strerror(errno));
>
>These should use snprintf() to make sure the buffer does not overflow. 

Sure, I'll change it.

>>  		fclose(fp);
>>  		ret = -1;
>>  
>>  		goto out;
>>  	}
>> -	fclose(fp);
>> +	ret = fclose(fp);
>> +	if (ret)
>> +		sprintf(reason, "Failed to write schemata in control group : %s",
>> +			strerror(errno));
>>  
>>  out:
>>  	ksft_print_msg("Write schema \"%s\" to resctrl FS%s%s\n",
>> 
>
>-- 
> i.
>

[1] https://man7.org/linux/man-pages/man3/fclose.3.html

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH 2/3] selftests/resctrl: Move run_benchmark() to a more fitting file

[Maciej Wieczór-Retman]

Hello,

On 2023-08-24 at 15:56:25 +0300, Ilpo Järvinen wrote:
>On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>
>Thanks for this patch, the new location is much more appropriate and 
>logical (more than once I've tried to look for this from the wrong file).
>
>> 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() function 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() function
>> that makes use of run_benchmark()
>
>Please terminate your sentences in changelog with . like in normal 
>writing.

Sure, I'll change it for the next version

>> Signed-off-by: Wieczor-Retman, Maciej <maciej.wieczor-retman@intel.com>
>> ---
>>  tools/testing/selftests/resctrl/resctrl_val.c | 52 +++++++++++++++++++
>>  tools/testing/selftests/resctrl/resctrlfs.c   | 52 -------------------
>>  2 files changed, 52 insertions(+), 52 deletions(-)
>> 
>> diff --git a/tools/testing/selftests/resctrl/resctrl_val.c b/tools/testing/selftests/resctrl/resctrl_val.c
>> index f0f6c5f6e98b..667542c084eb 100644
>> --- a/tools/testing/selftests/resctrl/resctrl_val.c
>> +++ b/tools/testing/selftests/resctrl/resctrl_val.c
>> @@ -621,6 +621,58 @@ 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
>> + *
>> + * Return: void
>
>This Return: void feels waste of screen space as if it wouldn't be 
>obvious from the function signature.

I'll remove it

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH 3/3] selftests: Add printf attribute to ksefltest prints

[Maciej Wieczór-Retman]

Hi,

On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
>On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>
>> Kselftest header defines multiple variadic function that use printf
>> along with other logic
>> 
>> There is no format checking for the variadic functions that use
>> printing inside kselftest.h. Because of this the compiler won't
>> be able to catch instances of mismatched print formats and debugging
>> tests might be more difficult
>> 
>> Add the common __printf attribute macro to kselftest.h
>> 
>> Add __printf attribute to every function using formatted printing with
>> variadic arguments
>
>Please add . to terminate the sentences.

Thanks, I'll fix it in the next version

>The patch looks fine:
>Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>
>...However, there are formatting errors it found yet to fix.

I believe you mean cache.c#L297.

I think I saw you're preparing some patches that remove the line that
reports the formatting error so I chose to not correct here.

Please let me know if I still should change it or would that be
redundant.

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH 3/3] selftests: Add printf attribute to ksefltest prints

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1560 bytes --]

On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
> On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
> >
> >> Kselftest header defines multiple variadic function that use printf
> >> along with other logic
> >> 
> >> There is no format checking for the variadic functions that use
> >> printing inside kselftest.h. Because of this the compiler won't
> >> be able to catch instances of mismatched print formats and debugging
> >> tests might be more difficult
> >> 
> >> Add the common __printf attribute macro to kselftest.h
> >> 
> >> Add __printf attribute to every function using formatted printing with
> >> variadic arguments
> >
> >Please add . to terminate the sentences.
> 
> Thanks, I'll fix it in the next version
> 
> >The patch looks fine:
> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >
> >...However, there are formatting errors it found yet to fix.
> 
> I believe you mean cache.c#L297.
> 
> I think I saw you're preparing some patches that remove the line that
> reports the formatting error so I chose to not correct here.
> 
> Please let me know if I still should change it or would that be
> redundant.

There are other selftests besides resctrl which had a few warnings.

Making the selftests to rebuild though might be a bit tricky (you won't 
see the warnings otherwise), I don't know the command needed to clean 
selftests but I guess one can always force their timestamps to force 
recompile with:
  git ls-files tools/testing/selftests | xargs touch

-- 
 i.

Re: [PATCH 1/3] selftests/resctrl: Fix schemata write error check

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1792 bytes --]

On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
> On 2023-08-24 at 15:52:05 +0300, Ilpo Järvinen wrote:
> >Ki,
> >
> >You're lacking a few people from the To/Cc list. Please see KERNEL 
> >SELFTEST FRAMEWORK entry in MAINTAINERS.
> 
> Thank you, I thought I checked the MAINTAINERS file well enough. I'll
> add them in the next version
> 
> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
> >
> >> Writing bitmasks to the schemata can fail when the bitmask doesn't
> >> adhere to some constraints defined by what a particular CPU supports.
> >> Some example of constraints are max length or being having contiguous
> >
> >"being having" is not good English.
> 
> Thanks, I'll change it
> 
> >> 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.
> >
> >I wonder if this is actually related to libc doing buffering between 
> >fprintf() and the actual write() syscall.
> 
> I started looking and apparently in the manpages for fclose [1] it says
> it uses fflush() to flush any buffered data in the stream. So that would
> probably confirm that it does buffering there.
> 
> In this case is there a situation when the fprintf() before fclose()
> would report an error? I'm thinking if there is a point to keep error
> checking after both function calls or just fclose(). 
>
> Or would putting additional fflush() after fprintf() make some sense?
> To have separate error checks for both function calls.

Another approach would be to use syscalls directly (open, write, and 
close to eliminate the buffering entirely. Given schema is already 
written into local variable first, it would be quite straightforward to do 
that conversion.


-- 
 i.

Re: [PATCH 1/3] selftests/resctrl: Fix schemata write error check

[Maciej Wieczór-Retman]

On 2023-08-25 at 11:43:22 +0300, Ilpo Järvinen wrote:
>On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
>> On 2023-08-24 at 15:52:05 +0300, Ilpo Järvinen wrote:
>> >Ki,
>> >
>> >You're lacking a few people from the To/Cc list. Please see KERNEL 
>> >SELFTEST FRAMEWORK entry in MAINTAINERS.
>> 
>> Thank you, I thought I checked the MAINTAINERS file well enough. I'll
>> add them in the next version
>> 
>> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>> >
>> >> Writing bitmasks to the schemata can fail when the bitmask doesn't
>> >> adhere to some constraints defined by what a particular CPU supports.
>> >> Some example of constraints are max length or being having contiguous
>> >
>> >"being having" is not good English.
>> 
>> Thanks, I'll change it
>> 
>> >> 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.
>> >
>> >I wonder if this is actually related to libc doing buffering between 
>> >fprintf() and the actual write() syscall.
>> 
>> I started looking and apparently in the manpages for fclose [1] it says
>> it uses fflush() to flush any buffered data in the stream. So that would
>> probably confirm that it does buffering there.
>> 
>> In this case is there a situation when the fprintf() before fclose()
>> would report an error? I'm thinking if there is a point to keep error
>> checking after both function calls or just fclose(). 
>>
>> Or would putting additional fflush() after fprintf() make some sense?
>> To have separate error checks for both function calls.
>
>Another approach would be to use syscalls directly (open, write, and 
>close to eliminate the buffering entirely. Given schema is already 
>written into local variable first, it would be quite straightforward to do 
>that conversion.

Okay, I'll try that then, thanks

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH 3/3] selftests: Add printf attribute to ksefltest prints

[Maciej Wieczór-Retman]

On 2023-08-25 at 11:28:17 +0300, Ilpo Järvinen wrote:
>On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
>> On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
>> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>> >
>> >> Kselftest header defines multiple variadic function that use printf
>> >> along with other logic
>> >> 
>> >> There is no format checking for the variadic functions that use
>> >> printing inside kselftest.h. Because of this the compiler won't
>> >> be able to catch instances of mismatched print formats and debugging
>> >> tests might be more difficult
>> >> 
>> >> Add the common __printf attribute macro to kselftest.h
>> >> 
>> >> Add __printf attribute to every function using formatted printing with
>> >> variadic arguments
>> >
>> >Please add . to terminate the sentences.
>> 
>> Thanks, I'll fix it in the next version
>> 
>> >The patch looks fine:
>> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> >
>> >...However, there are formatting errors it found yet to fix.
>> 
>> I believe you mean cache.c#L297.
>> 
>> I think I saw you're preparing some patches that remove the line that
>> reports the formatting error so I chose to not correct here.
>> 
>> Please let me know if I still should change it or would that be
>> redundant.
>
>There are other selftests besides resctrl which had a few warnings.
>
>Making the selftests to rebuild though might be a bit tricky (you won't 
>see the warnings otherwise), I don't know the command needed to clean 
>selftests but I guess one can always force their timestamps to force 
>recompile with:
>  git ls-files tools/testing/selftests | xargs touch

Okay, I think I counted 13 more caused by the __printf().
I just ran:
	$ make -C tools/testing/selftests
and to rerun it later:
	$ make -C tools/testing/selftests clean

But do you think all these fixes fit into this series?
Is so, should I put them in a separate patch or just append to this one?

-- 
Kind regards
Maciej Wieczór-Retman

Re: [PATCH 3/3] selftests: Add printf attribute to ksefltest prints

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 2313 bytes --]

On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:

> On 2023-08-25 at 11:28:17 +0300, Ilpo Järvinen wrote:
> >On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
> >> On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
> >> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
> >> >
> >> >> Kselftest header defines multiple variadic function that use printf
> >> >> along with other logic
> >> >> 
> >> >> There is no format checking for the variadic functions that use
> >> >> printing inside kselftest.h. Because of this the compiler won't
> >> >> be able to catch instances of mismatched print formats and debugging
> >> >> tests might be more difficult
> >> >> 
> >> >> Add the common __printf attribute macro to kselftest.h
> >> >> 
> >> >> Add __printf attribute to every function using formatted printing with
> >> >> variadic arguments
> >> >
> >> >Please add . to terminate the sentences.
> >> 
> >> Thanks, I'll fix it in the next version
> >> 
> >> >The patch looks fine:
> >> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> >> >
> >> >...However, there are formatting errors it found yet to fix.
> >> 
> >> I believe you mean cache.c#L297.
> >> 
> >> I think I saw you're preparing some patches that remove the line that
> >> reports the formatting error so I chose to not correct here.
> >> 
> >> Please let me know if I still should change it or would that be
> >> redundant.
> >
> >There are other selftests besides resctrl which had a few warnings.
> >
> >Making the selftests to rebuild though might be a bit tricky (you won't 
> >see the warnings otherwise), I don't know the command needed to clean 
> >selftests but I guess one can always force their timestamps to force 
> >recompile with:
> >  git ls-files tools/testing/selftests | xargs touch
> 
> Okay, I think I counted 13 more caused by the __printf().
> I just ran:
> 	$ make -C tools/testing/selftests
> and to rerun it later:
> 	$ make -C tools/testing/selftests clean
> 
> But do you think all these fixes fit into this series?
> Is so, should I put them in a separate patch or just append to this one?

Please fix them in separate patches.

IMO, the most logical approach is to make own series out of these printf 
attribute fixes and change, and another series for anything that is purely 
resctrl related.

-- 
 i.

Re: [PATCH 3/3] selftests: Add printf attribute to ksefltest prints

[Maciej Wieczór-Retman]

On 2023-08-25 at 12:14:12 +0300, Ilpo Järvinen wrote:
>On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
>
>> On 2023-08-25 at 11:28:17 +0300, Ilpo Järvinen wrote:
>> >On Fri, 25 Aug 2023, Maciej Wieczór-Retman wrote:
>> >> On 2023-08-24 at 16:10:12 +0300, Ilpo Järvinen wrote:
>> >> >On Thu, 24 Aug 2023, Wieczor-Retman, Maciej wrote:
>> >> >
>> >> >> Kselftest header defines multiple variadic function that use printf
>> >> >> along with other logic
>> >> >> 
>> >> >> There is no format checking for the variadic functions that use
>> >> >> printing inside kselftest.h. Because of this the compiler won't
>> >> >> be able to catch instances of mismatched print formats and debugging
>> >> >> tests might be more difficult
>> >> >> 
>> >> >> Add the common __printf attribute macro to kselftest.h
>> >> >> 
>> >> >> Add __printf attribute to every function using formatted printing with
>> >> >> variadic arguments
>> >> >
>> >> >Please add . to terminate the sentences.
>> >> 
>> >> Thanks, I'll fix it in the next version
>> >> 
>> >> >The patch looks fine:
>> >> >Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
>> >> >
>> >> >...However, there are formatting errors it found yet to fix.
>> >> 
>> >> I believe you mean cache.c#L297.
>> >> 
>> >> I think I saw you're preparing some patches that remove the line that
>> >> reports the formatting error so I chose to not correct here.
>> >> 
>> >> Please let me know if I still should change it or would that be
>> >> redundant.
>> >
>> >There are other selftests besides resctrl which had a few warnings.
>> >
>> >Making the selftests to rebuild though might be a bit tricky (you won't 
>> >see the warnings otherwise), I don't know the command needed to clean 
>> >selftests but I guess one can always force their timestamps to force 
>> >recompile with:
>> >  git ls-files tools/testing/selftests | xargs touch
>> 
>> Okay, I think I counted 13 more caused by the __printf().
>> I just ran:
>> 	$ make -C tools/testing/selftests
>> and to rerun it later:
>> 	$ make -C tools/testing/selftests clean
>> 
>> But do you think all these fixes fit into this series?
>> Is so, should I put them in a separate patch or just append to this one?
>
>Please fix them in separate patches.
>
>IMO, the most logical approach is to make own series out of these printf 
>attribute fixes and change, and another series for anything that is purely 
>resctrl related.

Okay, in that case I'll put this patch along with other fixes into a
separate series. Thanks for the help

-- 
Kind regards
Maciej Wieczór-Retman