Message ID | 20230418114506.46788-16-ilpo.jarvinen@linux.intel.com |
---|---|
State | New |
Headers | show |
Series | selftests/resctrl: Fixes, cleanups, and rewritten CAT test | expand |
Hi Ilpo, On 4/18/2023 4:44 AM, Ilpo Järvinen wrote: > Callers of get_cbm_mask() are required to pass a string into which > the CBM bit mask is read into. Neither CAT nor CMT tests need the There is a double "into" above. Perhaps the second can be dropped? > mask as string but just convert it into an unsigned long value. > > The bit mask reader can only read .../cbm_mask files. > > Generalize the bit mask reading function into get_bit_mask() such that > it can be used to handle other files besides the .../cbm_mask and > handle the unsigned long conversion within within get_bit_mask() using > fscanf(). Alter get_cbm_mask() to construct the filename for > get_bit_mask(). > > 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> > --- > tools/testing/selftests/resctrl/cat_test.c | 5 +-- > tools/testing/selftests/resctrl/cmt_test.c | 5 +-- > tools/testing/selftests/resctrl/resctrl.h | 2 +- > tools/testing/selftests/resctrl/resctrlfs.c | 50 +++++++++++++++------ > 4 files changed, 40 insertions(+), 22 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c > index a998e6397518..9bf5d05d9e74 100644 > --- a/tools/testing/selftests/resctrl/cat_test.c > +++ b/tools/testing/selftests/resctrl/cat_test.c > @@ -18,7 +18,6 @@ > #define MAX_DIFF 1000000 > > static int count_of_bits; > -static char cbm_mask[256]; > static unsigned long long_mask; > static unsigned long cache_size; > > @@ -101,12 +100,10 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) > cache_size = 0; > > /* Get default cbm mask for L3/L2 cache */ > - ret = get_cbm_mask(cache_type, cbm_mask); > + ret = get_cbm_mask(cache_type, &long_mask); > if (ret) > return ret; > > - long_mask = strtoul(cbm_mask, NULL, 16); > - > /* Get L3/L2 cache size */ > ret = get_cache_size(cpu_no, cache_type, &cache_size); > if (ret) > diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c > index 2d434c03cbba..ae54bbabbd91 100644 > --- a/tools/testing/selftests/resctrl/cmt_test.c > +++ b/tools/testing/selftests/resctrl/cmt_test.c > @@ -17,7 +17,6 @@ > #define MAX_DIFF_PERCENT 15 > > static int count_of_bits; > -static char cbm_mask[256]; > static unsigned long long_mask; > static unsigned long cache_size; > > @@ -82,12 +81,10 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) > if (!validate_resctrl_feature_request(CMT_STR)) > return -1; > > - ret = get_cbm_mask("L3", cbm_mask); > + ret = get_cbm_mask("L3", &long_mask); > if (ret) > return ret; I think this is a good change. It does raise the question why long_mask is a global variable so I think it may make things go smoother if the patch making long_mask local is moved to be before this patch. Reinette
Hi Ilpo, > Callers of get_cbm_mask() are required to pass a string into which the CBM bit > mask is read into. Neither CAT nor CMT tests need the mask as string but just > convert it into an unsigned long value. > > The bit mask reader can only read .../cbm_mask files. > > Generalize the bit mask reading function into get_bit_mask() such that it can > be used to handle other files besides the .../cbm_mask and handle the > unsigned long conversion within within get_bit_mask() using fscanf(). Alter > get_cbm_mask() to construct the filename for get_bit_mask(). > > 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> > --- > tools/testing/selftests/resctrl/cat_test.c | 5 +-- > tools/testing/selftests/resctrl/cmt_test.c | 5 +-- > tools/testing/selftests/resctrl/resctrl.h | 2 +- > tools/testing/selftests/resctrl/resctrlfs.c | 50 +++++++++++++++------ > 4 files changed, 40 insertions(+), 22 deletions(-) > > diff --git a/tools/testing/selftests/resctrl/cat_test.c > b/tools/testing/selftests/resctrl/cat_test.c > index a998e6397518..9bf5d05d9e74 100644 > --- a/tools/testing/selftests/resctrl/cat_test.c > +++ b/tools/testing/selftests/resctrl/cat_test.c > @@ -18,7 +18,6 @@ > #define MAX_DIFF 1000000 > > static int count_of_bits; > -static char cbm_mask[256]; > static unsigned long long_mask; > static unsigned long cache_size; > > @@ -101,12 +100,10 @@ int cat_perf_miss_val(int cpu_no, int n, char > *cache_type) > cache_size = 0; > > /* Get default cbm mask for L3/L2 cache */ > - ret = get_cbm_mask(cache_type, cbm_mask); > + ret = get_cbm_mask(cache_type, &long_mask); > if (ret) > return ret; > > - long_mask = strtoul(cbm_mask, NULL, 16); > - > /* Get L3/L2 cache size */ > ret = get_cache_size(cpu_no, cache_type, &cache_size); > if (ret) > diff --git a/tools/testing/selftests/resctrl/cmt_test.c > b/tools/testing/selftests/resctrl/cmt_test.c > index 2d434c03cbba..ae54bbabbd91 100644 > --- a/tools/testing/selftests/resctrl/cmt_test.c > +++ b/tools/testing/selftests/resctrl/cmt_test.c > @@ -17,7 +17,6 @@ > #define MAX_DIFF_PERCENT 15 > > static int count_of_bits; > -static char cbm_mask[256]; > static unsigned long long_mask; > static unsigned long cache_size; > > @@ -82,12 +81,10 @@ int cmt_resctrl_val(int cpu_no, int n, char > **benchmark_cmd) > if (!validate_resctrl_feature_request(CMT_STR)) > return -1; > > - ret = get_cbm_mask("L3", cbm_mask); > + ret = get_cbm_mask("L3", &long_mask); > if (ret) > return ret; > > - long_mask = strtoul(cbm_mask, NULL, 16); > - > ret = get_cache_size(cpu_no, "L3", &cache_size); > if (ret) > return ret; > diff --git a/tools/testing/selftests/resctrl/resctrl.h > b/tools/testing/selftests/resctrl/resctrl.h > index ba36eb5fdf0d..bcc95faa5b4e 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -106,7 +106,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, char *cbm_mask); > +int get_cbm_mask(char *cache_type, unsigned long *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 > signal_handler_register(void); diff --git > a/tools/testing/selftests/resctrl/resctrlfs.c > b/tools/testing/selftests/resctrl/resctrlfs.c > index 7fef9068d7fd..f01ecfa64063 100644 > --- a/tools/testing/selftests/resctrl/resctrlfs.c > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > @@ -186,30 +186,29 @@ int get_cache_size(int cpu_no, char *cache_type, > unsigned long *cache_size) > #define CORE_SIBLINGS_PATH "/sys/bus/cpu/devices/cpu" > > /* > - * get_cbm_mask - Get cbm mask for given cache > - * @cache_type: Cache level L2/L3 > - * @cbm_mask: cbm_mask returned as a string > + * get_bit_mask - Get bit mask from given file > + * @filename: File containing the mask > + * @mask: The bit mask returned as unsigned long > * > * Return: = 0 on success, < 0 on failure. > */ > -int get_cbm_mask(char *cache_type, char *cbm_mask) > +static int get_bit_mask(char *filename, unsigned long *mask) > { > - char cbm_mask_path[1024]; > FILE *fp; > > - if (!cbm_mask) > + if (!filename || !mask) > return -1; > > - sprintf(cbm_mask_path, "%s/%s/cbm_mask", INFO_PATH, > cache_type); > - > - fp = fopen(cbm_mask_path, "r"); > + fp = fopen(filename, "r"); > if (!fp) { > - perror("Failed to open cache level"); > - > + fprintf(stderr, "Failed to open bit mask file '%s': %s\n", > + filename, strerror(errno)); > return -1; > } > - if (fscanf(fp, "%s", cbm_mask) <= 0) { > - perror("Could not get max cbm_mask"); > + > + if (fscanf(fp, "%lx", mask) <= 0) { > + fprintf(stderr, "Could not read bit mask file '%s': %s\n", > + filename, strerror(errno)); > fclose(fp); > > return -1; > @@ -219,6 +218,31 @@ int get_cbm_mask(char *cache_type, char > *cbm_mask) > return 0; > } > > +/* > + * get_cbm_bits - Get number of bits in cbm mask Is it get_cbm_mask? Best regards, Shaopeng TAN > + * @cache_type: Cache level L2/L3 > + * @mask: cbm_mask returned as unsigned long > + * > + * Return: = 0 on success, < 0 on failure. > + */ > +int get_cbm_mask(char *cache_type, unsigned long *mask) { > + char cbm_mask_path[1024]; > + int ret; > + > + if (!cache_type) > + return -1; > + > + snprintf(cbm_mask_path, sizeof(cbm_mask_path), > "%s/%s/cbm_mask", > + INFO_PATH, cache_type); > + > + ret = get_bit_mask(cbm_mask_path, mask); > + if (ret) > + return -1; > + > + return 0; > +} > + > /* > * get_core_sibling - Get sibling core id from the same socket for given CPU > * @cpu_no: CPU number > -- > 2.30.2
On Wed, 31 May 2023, Shaopeng Tan (Fujitsu) wrote: > Hi Ilpo, > > > Callers of get_cbm_mask() are required to pass a string into which the CBM bit > > mask is read into. Neither CAT nor CMT tests need the mask as string but just > > convert it into an unsigned long value. > > > > The bit mask reader can only read .../cbm_mask files. > > > > Generalize the bit mask reading function into get_bit_mask() such that it can > > be used to handle other files besides the .../cbm_mask and handle the > > unsigned long conversion within within get_bit_mask() using fscanf(). Alter > > get_cbm_mask() to construct the filename for get_bit_mask(). > > > > 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> > > --- > > tools/testing/selftests/resctrl/cat_test.c | 5 +-- > > tools/testing/selftests/resctrl/cmt_test.c | 5 +-- > > tools/testing/selftests/resctrl/resctrl.h | 2 +- > > tools/testing/selftests/resctrl/resctrlfs.c | 50 +++++++++++++++------ > > 4 files changed, 40 insertions(+), 22 deletions(-) > > > > diff --git a/tools/testing/selftests/resctrl/cat_test.c > > b/tools/testing/selftests/resctrl/cat_test.c > > index a998e6397518..9bf5d05d9e74 100644 > > --- a/tools/testing/selftests/resctrl/cat_test.c > > +++ b/tools/testing/selftests/resctrl/cat_test.c > > @@ -18,7 +18,6 @@ > > #define MAX_DIFF 1000000 > > > > static int count_of_bits; > > -static char cbm_mask[256]; > > static unsigned long long_mask; > > static unsigned long cache_size; > > > > @@ -101,12 +100,10 @@ int cat_perf_miss_val(int cpu_no, int n, char > > *cache_type) > > cache_size = 0; > > > > /* Get default cbm mask for L3/L2 cache */ > > - ret = get_cbm_mask(cache_type, cbm_mask); > > + ret = get_cbm_mask(cache_type, &long_mask); > > if (ret) > > return ret; > > > > - long_mask = strtoul(cbm_mask, NULL, 16); > > - > > /* Get L3/L2 cache size */ > > ret = get_cache_size(cpu_no, cache_type, &cache_size); > > if (ret) > > diff --git a/tools/testing/selftests/resctrl/cmt_test.c > > b/tools/testing/selftests/resctrl/cmt_test.c > > index 2d434c03cbba..ae54bbabbd91 100644 > > --- a/tools/testing/selftests/resctrl/cmt_test.c > > +++ b/tools/testing/selftests/resctrl/cmt_test.c > > @@ -17,7 +17,6 @@ > > #define MAX_DIFF_PERCENT 15 > > > > static int count_of_bits; > > -static char cbm_mask[256]; > > static unsigned long long_mask; > > static unsigned long cache_size; > > > > @@ -82,12 +81,10 @@ int cmt_resctrl_val(int cpu_no, int n, char > > **benchmark_cmd) > > if (!validate_resctrl_feature_request(CMT_STR)) > > return -1; > > > > - ret = get_cbm_mask("L3", cbm_mask); > > + ret = get_cbm_mask("L3", &long_mask); > > if (ret) > > return ret; > > > > - long_mask = strtoul(cbm_mask, NULL, 16); > > - > > ret = get_cache_size(cpu_no, "L3", &cache_size); > > if (ret) > > return ret; > > diff --git a/tools/testing/selftests/resctrl/resctrl.h > > b/tools/testing/selftests/resctrl/resctrl.h > > index ba36eb5fdf0d..bcc95faa5b4e 100644 > > --- a/tools/testing/selftests/resctrl/resctrl.h > > +++ b/tools/testing/selftests/resctrl/resctrl.h > > @@ -106,7 +106,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, char *cbm_mask); > > +int get_cbm_mask(char *cache_type, unsigned long *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 > > signal_handler_register(void); diff --git > > a/tools/testing/selftests/resctrl/resctrlfs.c > > b/tools/testing/selftests/resctrl/resctrlfs.c > > index 7fef9068d7fd..f01ecfa64063 100644 > > --- a/tools/testing/selftests/resctrl/resctrlfs.c > > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > > @@ -186,30 +186,29 @@ int get_cache_size(int cpu_no, char *cache_type, > > unsigned long *cache_size) > > #define CORE_SIBLINGS_PATH "/sys/bus/cpu/devices/cpu" > > > > /* > > - * get_cbm_mask - Get cbm mask for given cache > > - * @cache_type: Cache level L2/L3 > > - * @cbm_mask: cbm_mask returned as a string > > + * get_bit_mask - Get bit mask from given file > > + * @filename: File containing the mask > > + * @mask: The bit mask returned as unsigned long > > * > > * Return: = 0 on success, < 0 on failure. > > */ > > -int get_cbm_mask(char *cache_type, char *cbm_mask) > > +static int get_bit_mask(char *filename, unsigned long *mask) > > { > > - char cbm_mask_path[1024]; > > FILE *fp; > > > > - if (!cbm_mask) > > + if (!filename || !mask) > > return -1; > > > > - sprintf(cbm_mask_path, "%s/%s/cbm_mask", INFO_PATH, > > cache_type); > > - > > - fp = fopen(cbm_mask_path, "r"); > > + fp = fopen(filename, "r"); > > if (!fp) { > > - perror("Failed to open cache level"); > > - > > + fprintf(stderr, "Failed to open bit mask file '%s': %s\n", > > + filename, strerror(errno)); > > return -1; > > } > > - if (fscanf(fp, "%s", cbm_mask) <= 0) { > > - perror("Could not get max cbm_mask"); > > + > > + if (fscanf(fp, "%lx", mask) <= 0) { > > + fprintf(stderr, "Could not read bit mask file '%s': %s\n", > > + filename, strerror(errno)); > > fclose(fp); > > > > return -1; > > @@ -219,6 +218,31 @@ int get_cbm_mask(char *cache_type, char > > *cbm_mask) > > return 0; > > } > > > > +/* > > + * get_cbm_bits - Get number of bits in cbm mask > > Is it get_cbm_mask? Sure, thanks for noticing this. While correcting it, I also improved the description to match what the function now does.
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index a998e6397518..9bf5d05d9e74 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -18,7 +18,6 @@ #define MAX_DIFF 1000000 static int count_of_bits; -static char cbm_mask[256]; static unsigned long long_mask; static unsigned long cache_size; @@ -101,12 +100,10 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) cache_size = 0; /* Get default cbm mask for L3/L2 cache */ - ret = get_cbm_mask(cache_type, cbm_mask); + ret = get_cbm_mask(cache_type, &long_mask); if (ret) return ret; - long_mask = strtoul(cbm_mask, NULL, 16); - /* Get L3/L2 cache size */ ret = get_cache_size(cpu_no, cache_type, &cache_size); if (ret) diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c index 2d434c03cbba..ae54bbabbd91 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -17,7 +17,6 @@ #define MAX_DIFF_PERCENT 15 static int count_of_bits; -static char cbm_mask[256]; static unsigned long long_mask; static unsigned long cache_size; @@ -82,12 +81,10 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd) if (!validate_resctrl_feature_request(CMT_STR)) return -1; - ret = get_cbm_mask("L3", cbm_mask); + ret = get_cbm_mask("L3", &long_mask); if (ret) return ret; - long_mask = strtoul(cbm_mask, NULL, 16); - ret = get_cache_size(cpu_no, "L3", &cache_size); if (ret) return ret; diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index ba36eb5fdf0d..bcc95faa5b4e 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -106,7 +106,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, char *cbm_mask); +int get_cbm_mask(char *cache_type, unsigned long *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 signal_handler_register(void); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 7fef9068d7fd..f01ecfa64063 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -186,30 +186,29 @@ int get_cache_size(int cpu_no, char *cache_type, unsigned long *cache_size) #define CORE_SIBLINGS_PATH "/sys/bus/cpu/devices/cpu" /* - * get_cbm_mask - Get cbm mask for given cache - * @cache_type: Cache level L2/L3 - * @cbm_mask: cbm_mask returned as a string + * get_bit_mask - Get bit mask from given file + * @filename: File containing the mask + * @mask: The bit mask returned as unsigned long * * Return: = 0 on success, < 0 on failure. */ -int get_cbm_mask(char *cache_type, char *cbm_mask) +static int get_bit_mask(char *filename, unsigned long *mask) { - char cbm_mask_path[1024]; FILE *fp; - if (!cbm_mask) + if (!filename || !mask) return -1; - sprintf(cbm_mask_path, "%s/%s/cbm_mask", INFO_PATH, cache_type); - - fp = fopen(cbm_mask_path, "r"); + fp = fopen(filename, "r"); if (!fp) { - perror("Failed to open cache level"); - + fprintf(stderr, "Failed to open bit mask file '%s': %s\n", + filename, strerror(errno)); return -1; } - if (fscanf(fp, "%s", cbm_mask) <= 0) { - perror("Could not get max cbm_mask"); + + if (fscanf(fp, "%lx", mask) <= 0) { + fprintf(stderr, "Could not read bit mask file '%s': %s\n", + filename, strerror(errno)); fclose(fp); return -1; @@ -219,6 +218,31 @@ int get_cbm_mask(char *cache_type, char *cbm_mask) return 0; } +/* + * get_cbm_bits - Get number of bits in cbm mask + * @cache_type: Cache level L2/L3 + * @mask: cbm_mask returned as unsigned long + * + * Return: = 0 on success, < 0 on failure. + */ +int get_cbm_mask(char *cache_type, unsigned long *mask) +{ + char cbm_mask_path[1024]; + int ret; + + if (!cache_type) + return -1; + + snprintf(cbm_mask_path, sizeof(cbm_mask_path), "%s/%s/cbm_mask", + INFO_PATH, cache_type); + + ret = get_bit_mask(cbm_mask_path, mask); + if (ret) + return -1; + + return 0; +} + /* * get_core_sibling - Get sibling core id from the same socket for given CPU * @cpu_no: CPU number