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@xxxxxxxxx>
> > Signed-off-by: Fenghua Yu <fenghua.yu@xxxxxxxxx>
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@xxxxxxxxxxxxxxx>
> > ---
> > 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.
--
i.
