RE: [PATCH v5 02/21] selftests/resctrl: Fix compilation issues for global variables

[Babu Moger <babu.moger@amd.com>]

> -----Original Message-----
> From: Fenghua Yu <fenghua.yu@intel.com>
> Sent: Sunday, March 7, 2021 8:55 AM
> To: Shuah Khan <shuah@kernel.org>; Tony Luck <tony.luck@intel.com>;
> Reinette Chatre <reinette.chatre@intel.com>; Moger, Babu
> <Babu.Moger@amd.com>
> Cc: linux-kselftest <linux-kselftest@vger.kernel.org>; linux-kernel <linux-
> kernel@vger.kernel.org>; Fenghua Yu <fenghua.yu@intel.com>
> Subject: [PATCH v5 02/21] selftests/resctrl: Fix compilation issues for global
> variables
> 
> Reinette reported following compilation issue on Fedora 32, gcc version
> 10.1.1
> 
> /usr/bin/ld: cqm_test.o:<src_dir>/cqm_test.c:22: multiple definition of
> `cache_size'; cat_test.o:<src_dir>/cat_test.c:23: first defined here
> 
> The same issue is reported for long_mask, cbm_mask, count_of_bits etc
> variables as well. Compiler isn't happy because these variables are
> defined globally in two .c files namely cqm_test.c and cat_test.c and
> the compiler during compilation finds that the variable is already
> defined (multiple definition error).
> 
> Taking a closer look at the usage of these variables reveals that these
> variables are used only locally to functions such as cqm_resctrl_val()

%s/ locally to functions/locally in two functions

> (defined in cqm_test.c) and cat_perf_miss_val() (defined in cat_test.c).
> These variables are not shared between those functions. So, there is no
> need for these variables to be global. Hence, fix this issue by making
> them static variables.
> 
> Reported-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Fenghua Yu <fenghua.yu@intel.com>
> ---
> Change Log:
> v5:
> - Define long_mask, cbm_mask, count_of_bits etc as static variables
>   (Shuah).
> - Split this patch into patch 2 and 3 (Shuah).
> 
>  tools/testing/selftests/resctrl/cat_test.c  | 10 +++++-----
>  tools/testing/selftests/resctrl/cqm_test.c  | 10 +++++-----
>  tools/testing/selftests/resctrl/resctrl.h   |  2 +-
>  tools/testing/selftests/resctrl/resctrlfs.c | 10 +++++-----
>  4 files changed, 16 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/cat_test.c
> b/tools/testing/selftests/resctrl/cat_test.c
> index 5da43767b973..bdeeb5772592 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -17,10 +17,10 @@
>  #define MAX_DIFF_PERCENT	4
>  #define MAX_DIFF		1000000
> 
> -int count_of_bits;
> -char cbm_mask[256];
> -unsigned long long_mask;
> -unsigned long cache_size;
> +static int count_of_bits;
> +static char cbm_mask[256];
> +static unsigned long long_mask;
> +static unsigned long cache_size;
> 
>  /*
>   * Change schemata. Write schemata to specified
> @@ -136,7 +136,7 @@ int cat_perf_miss_val(int cpu_no, int n, char
> *cache_type)
>  		return -1;
> 
>  	/* Get default cbm mask for L3/L2 cache */
> -	ret = get_cbm_mask(cache_type);
> +	ret = get_cbm_mask(cache_type, cbm_mask);
>  	if (ret)
>  		return ret;
> 
> diff --git a/tools/testing/selftests/resctrl/cqm_test.c
> b/tools/testing/selftests/resctrl/cqm_test.c
> index 5e7308ac63be..de33d1c0466e 100644
> --- a/tools/testing/selftests/resctrl/cqm_test.c
> +++ b/tools/testing/selftests/resctrl/cqm_test.c
> @@ -16,10 +16,10 @@
>  #define MAX_DIFF		2000000
>  #define MAX_DIFF_PERCENT	15
> 
> -int count_of_bits;
> -char cbm_mask[256];
> -unsigned long long_mask;
> -unsigned long cache_size;
> +static int count_of_bits;
> +static char cbm_mask[256];
> +static unsigned long long_mask;
> +static unsigned long cache_size;
> 
>  static int cqm_setup(int num, ...)
>  {
> @@ -125,7 +125,7 @@ int cqm_resctrl_val(int cpu_no, int n, char
> **benchmark_cmd)
>  	if (!validate_resctrl_feature_request("cqm"))
>  		return -1;
> 
> -	ret = get_cbm_mask("L3");
> +	ret = get_cbm_mask("L3", cbm_mask);
>  	if (ret)
>  		return ret;
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h
> b/tools/testing/selftests/resctrl/resctrl.h
> index 39bf59c6b9c5..959c71e39bdc 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -92,7 +92,7 @@ void tests_cleanup(void);
>  void mbm_test_cleanup(void);
>  int mba_schemata_change(int cpu_no, char *bw_report, char
> **benchmark_cmd);
>  void mba_test_cleanup(void);
> -int get_cbm_mask(char *cache_type);
> +int get_cbm_mask(char *cache_type, char *cbm_mask);
>  int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size);
>  void ctrlc_handler(int signum, siginfo_t *info, void *ptr);
>  int cat_val(struct resctrl_val_param *param);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c
> b/tools/testing/selftests/resctrl/resctrlfs.c
> index 19c0ec4045a4..2a16100c9c3f 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -49,8 +49,6 @@ static int find_resctrl_mount(char *buffer)
>  	return -ENOENT;
>  }
> 
> -char cbm_mask[256];
> -
>  /*
>   * remount_resctrlfs - Remount resctrl FS at /sys/fs/resctrl
>   * @mum_resctrlfs:	Should the resctrl FS be remounted?
> @@ -205,16 +203,18 @@ int get_cache_size(int cpu_no, char *cache_type,
> unsigned long *cache_size)
>  /*
>   * get_cbm_mask - Get cbm mask for given cache
>   * @cache_type:	Cache level L2/L3
> - *
> - * Mask is stored in cbm_mask which is global variable.
> + * @cbm_mask:	cbm_mask returned as a string
>   *
>   * Return: = 0 on success, < 0 on failure.
>   */
> -int get_cbm_mask(char *cache_type)
> +int get_cbm_mask(char *cache_type, char *cbm_mask)
>  {
>  	char cbm_mask_path[1024];
>  	FILE *fp;
> 
> +	if (!cbm_mask)
> +		return -1;

Can cbm_mask be NULL? I see it is statically allocated.
Or should this be if (!(*cbm_mask))? Or did I miss something.

> +
>  	sprintf(cbm_mask_path, "%s/%s/cbm_mask", CBM_MASK_PATH,
> cache_type);
> 
>  	fp = fopen(cbm_mask_path, "r");
> --
> 2.30.1