Re: [PATCH v5 02/21] selftests/resctrl: Fix compilation issues for global variables
Hi, Babu,
On Fri, Mar 12, 2021 at 01:08:31PM -0600, Babu Moger wrote:
> > From: Fenghua Yu
> > 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
OK. Will change it.
> > -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.
This is a sanity checking. Although current callers do pass statically
allocated cbm_mask to the parameter, future callers may incorrectly pass
un-allocated cbm_mask to the parameter and may cause segmentation fault
without the sanity checking. To debug this kind of issue, the sanity
checking will be very helpful.
So I would keep this sanity checking.
Thanks.
-Fenghua
RE: [PATCH v5 02/21] selftests/resctrl: Fix compilation issues for global variables
> -Original Message-
> From: Fenghua Yu
> Sent: Sunday, March 7, 2021 8:55 AM
> To: Shuah Khan ; Tony Luck ;
> Reinette Chatre ; Moger, Babu
>
> Cc: linux-kselftest ; linux-kernel ker...@vger.kernel.org>; Fenghua Yu
> 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:/cqm_test.c:22: multiple definition of
> `cache_size'; cat_test.o:/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
> Signed-off-by: Fenghua Yu
> ---
> 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 100
>
> -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 200
> #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/
[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:/cqm_test.c:22: multiple definition of
`cache_size'; cat_test.o:/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()
(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
Signed-off-by: Fenghua Yu
---
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 100
-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 200
#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;
+
sprintf(cbm_mask_path, "%s/%s/cbm_mask", CBM_MASK_PATH, cache_type);
fp =
