| Message ID | 20210307145502.2916364-3-fenghua.yu@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Miscellaneous fixes for resctrl selftests | expand |
> -----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
Hi, Babu, On Fri, Mar 12, 2021 at 01:08:31PM -0600, Babu Moger wrote: > > From: Fenghua Yu <fenghua.yu@intel.com> > > 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
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; + sprintf(cbm_mask_path, "%s/%s/cbm_mask", CBM_MASK_PATH, cache_type); fp = fopen(cbm_mask_path, "r");
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() (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(-)