Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <7bba4db8-eaac-238d-b764-89d0ae2a9bba@linuxfoundation.org>
Date:   Tue, 25 Jul 2023 08:49:39 -0600
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Subject: Re: [PATCH v5 12/19] selftests/resctrl: Remove
 "malloc_and_init_memory" param from run_fill_buf()
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        linux-kselftest@vger.kernel.org,
        Reinette Chatre <reinette.chatre@intel.com>,
        Shuah Khan <shuah@kernel.org>,
        Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>,
        linux-kernel@vger.kernel.org,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20230717131507.32420-1-ilpo.jarvinen@linux.intel.com>
 <20230717131507.32420-13-ilpo.jarvinen@linux.intel.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
In-Reply-To: <20230717131507.32420-13-ilpo.jarvinen@linux.intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 7/17/23 07:15, Ilpo Järvinen wrote:
> run_fill_buf()'s malloc_and_init_memory parameter is always 1. There's
> also duplicated memory init code for malloc_and_init_memory == 0 case
> in fill_buf() which is unused.
> 
> Remove the malloc_and_init_memory parameter and the duplicated mem init
> code.
> 
> While at it, fix also a typo in run_fill_buf() prototype's argument.
> 
> Co-developed-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Reviewed-by: Reinette Chatre <reinette.chatre@intel.com>
> ---
>   tools/testing/selftests/resctrl/cache.c       |  6 ++--
>   tools/testing/selftests/resctrl/fill_buf.c    | 28 +++----------------
>   tools/testing/selftests/resctrl/resctrl.h     |  3 +-
>   .../testing/selftests/resctrl/resctrl_tests.c | 13 ++++-----
>   tools/testing/selftests/resctrl/resctrlfs.c   | 12 ++++----
>   5 files changed, 19 insertions(+), 43 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cache.c b/tools/testing/selftests/resctrl/cache.c
> index 53df66814cd6..63b551c44f1d 100644
> --- a/tools/testing/selftests/resctrl/cache.c
> +++ b/tools/testing/selftests/resctrl/cache.c
> @@ -210,7 +210,7 @@ int measure_cache_vals(struct resctrl_val_param *param, int bm_pid)
>    */
>   int cat_val(struct resctrl_val_param *param)
>   {
> -	int malloc_and_init_memory = 1, memflush = 1, operation = 0, ret = 0;
> +	int memflush = 1, operation = 0, ret = 0;
>   	char *resctrl_val = param->resctrl_val;
>   	pid_t bm_pid;
>   
> @@ -247,8 +247,8 @@ int cat_val(struct resctrl_val_param *param)
>   			if (ret)
>   				break;
>   
> -			if (run_fill_buf(param->span, malloc_and_init_memory,
> -					 memflush, operation, resctrl_val)) {
> +			if (run_fill_buf(param->span, memflush, operation,
> +					 resctrl_val)) {
>   				fprintf(stderr, "Error-running fill buffer\n");
>   				ret = -1;
>   				goto pe_close;
> diff --git a/tools/testing/selftests/resctrl/fill_buf.c b/tools/testing/selftests/resctrl/fill_buf.c
> index 785cbd8d0148..d8f5505eb9e6 100644
> --- a/tools/testing/selftests/resctrl/fill_buf.c
> +++ b/tools/testing/selftests/resctrl/fill_buf.c
> @@ -138,36 +138,18 @@ static int fill_cache_write(unsigned char *start_ptr, unsigned char *end_ptr,
>   	return 0;
>   }
>   
> -static int
> -fill_cache(size_t buf_size, int malloc_and_init, int memflush,
> -	   int op, char *resctrl_val)
> +static int fill_cache(size_t buf_size, int memflush, int op, char *resctrl_val)
>   {
>   	unsigned char *start_ptr, *end_ptr;
> -	unsigned long long i;
>   	int ret;
>   
> -	if (malloc_and_init)
> -		start_ptr = malloc_and_init_memory(buf_size);
> -	else
> -		start_ptr = malloc(buf_size);
> -
> +	start_ptr = malloc_and_init_memory(buf_size);
>   	if (!start_ptr)
>   		return -1;
>   
>   	startptr = start_ptr;
>   	end_ptr = start_ptr + buf_size;
>   
> -	/*
> -	 * It's better to touch the memory once to avoid any compiler
> -	 * optimizations
> -	 */
> -	if (!malloc_and_init) {
> -		for (i = 0; i < buf_size; i++)
> -			*start_ptr++ = (unsigned char)rand();
> -	}
> -
> -	start_ptr = startptr;
> -
>   	/* Flush the memory before using to avoid "cache hot pages" effect */
>   	if (memflush)
>   		mem_flush(start_ptr, buf_size);
> @@ -188,14 +170,12 @@ fill_cache(size_t buf_size, int malloc_and_init, int memflush,
>   	return 0;
>   }
>   
> -int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
> -		 char *resctrl_val)
> +int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val)
>   {
>   	size_t cache_size = span;
>   	int ret;
>   
> -	ret = fill_cache(cache_size, malloc_and_init_memory, memflush, op,
> -			 resctrl_val);
> +	ret = fill_cache(cache_size, memflush, op, resctrl_val);
>   	if (ret) {
>   		printf("\n Error in fill cache\n");
>   		return -1;
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index 52068ceea956..3054cc4ef0e3 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -97,8 +97,7 @@ int write_bm_pid_to_resctrl(pid_t bm_pid, char *ctrlgrp, char *mongrp,
>   			    char *resctrl_val);
>   int perf_event_open(struct perf_event_attr *hw_event, pid_t pid, int cpu,
>   		    int group_fd, unsigned long flags);
> -int run_fill_buf(size_t span, int malloc_and_init_memory, int memflush, int op,
> -		 char *resctrl_va);
> +int run_fill_buf(size_t span, int memflush, int op, char *resctrl_val);
>   int resctrl_val(char **benchmark_cmd, struct resctrl_val_param *param);
>   int mbm_bw_change(size_t span, int cpu_no, char *bw_report, char **benchmark_cmd);
>   void tests_cleanup(void);
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index bf0cadab36b0..125ed0ca11e3 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -89,7 +89,7 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, size_t span,
>   	}
>   
>   	if (!has_ben)
> -		sprintf(benchmark_cmd[5], "%s", MBA_STR);
> +		sprintf(benchmark_cmd[4], "%s", MBA_STR);
>   	res = mbm_bw_change(span, cpu_no, bw_report, benchmark_cmd);
>   	ksft_test_result(!res, "MBM: bw change\n");
>   	if ((get_vendor() == ARCH_INTEL) && res)
> @@ -141,7 +141,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
>   	}
>   
>   	if (!has_ben)
> -		sprintf(benchmark_cmd[5], "%s", CMT_STR);
> +		sprintf(benchmark_cmd[4], "%s", CMT_STR);
>   	res = cmt_resctrl_val(cpu_no, 5, benchmark_cmd);
>   	ksft_test_result(!res, "CMT: test\n");
>   	if ((get_vendor() == ARCH_INTEL) && res)
> @@ -267,16 +267,15 @@ int main(int argc, char **argv)
>   		benchmark_cmd[ben_count] = NULL;
>   	} else {
>   		/* If no benchmark is given by "-b" argument, use fill_buf. */
> -		for (i = 0; i < 6; i++)
> +		for (i = 0; i < 5; i++)
>   			benchmark_cmd[i] = benchmark_cmd_area[i];
>   
>   		strcpy(benchmark_cmd[0], "fill_buf");
>   		sprintf(benchmark_cmd[1], "%zu", span);
>   		strcpy(benchmark_cmd[2], "1");
> -		strcpy(benchmark_cmd[3], "1");
> -		strcpy(benchmark_cmd[4], "0");
> -		strcpy(benchmark_cmd[5], "");
> -		benchmark_cmd[6] = NULL;
> +		strcpy(benchmark_cmd[3], "0");
> +		strcpy(benchmark_cmd[4], "");
> +		benchmark_cmd[5] = NULL;
>   	}

Lots of copying - can the above be simplified. Also seeing this
warning:

WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
#233: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:276:
+		strcpy(benchmark_cmd[3], "0");

WARNING: Prefer strscpy over strcpy - see: https://github.com/KSPP/linux/issues/88
#234: FILE: tools/testing/selftests/resctrl/resctrl_tests.c:277:
+		strcpy(benchmark_cmd[4], "");

total: 0 errors, 2 warnings, 142 lines checked

I am applying the patch set to linux-kselftest next - please fix
the aboe and send a patch on top of linux-kselftest next

thanks,
-- Shuah