[v1] selftests/resctrl: Bug fix and optimizations

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

Posted by Wieczor-Retman, Maciej 2 years, 3 months ago

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

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

Posted by Ilpo Järvinen 2 years, 3 months ago

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 2/3] selftests/resctrl: Move run_benchmark() to a more fitting file

Posted by Maciej Wieczór-Retman 2 years, 3 months ago

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

[PATCH 1/3] selftests/resctrl: Fix schemata write error check
[PATCH 2/3] selftests/resctrl: Move run_benchmark() to a more fitting file
[PATCH 3/3] selftests: Add printf attribute to ksefltest prints