| Message ID | 20231024092634.7122-4-ilpo.jarvinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: CAT test improvements & generalized test framework | expand |
On 2023-10-24 at 12:26:13 +0300, Ilpo Järvinen wrote: >Callers of get_cbm_mask() are required to pass a string into which the >CBM bit mask is read. Neither CAT nor CMT tests need the mask as string "CBM bit mask" -> "CBM" / "capacity bitmask (CBM)"? Generally isn't cbm_mask an odd name because of the repetition (capacity bitmask mask)? After looking at x86/resctrl files putting "mask" after "cbm" only happens there. Maybe a better naming scheme here would be get_cbm_bits()? >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 get_bit_mask() using >fscanf(). Alter get_cbm_mask() to construct the filename for >get_bit_mask(). > >Also mark cache_type const while at it. > >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> >---
Hi Ilpo, On 10/24/2023 2:26 AM, Ilpo Järvinen wrote: ... > @@ -229,6 +228,31 @@ int get_cbm_mask(char *cache_type, char *cbm_mask) > return 0; > } > > +/* > + * get_cbm_mask - Get cbm bit mask I know you just copied code here but please keep an eye out for acronyms to be written in caps. This needs not be named to reflect verbatim what the function does. Looking ahead I wonder if "get_full_mask()/get_max_mask()" may not be a clear indication of what this does? Something like: get_full_mask()/get_max_mask() Get maximum Cache Bit Mask (CBM) @cache_type: Cache level(? or should this be "type") as "L2" or L3". @mask: Full/Maximum portion of cache available for allocation represented by bit mask returned as unsigned long. > + * @cache_type: Cache level L2/L3 > + * @mask: cbm_mask returned as unsigned long > + * > + * Return: = 0 on success, < 0 on failure. > + */ > +int get_cbm_mask(const char *cache_type, unsigned long *mask) > +{ > + char cbm_mask_path[1024]; > + int ret; > + > + if (!cache_type) > + return -1; Just to confirm ... error checking on mask is intentionally deferred until get_bit_mask()? > + > + 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 Reinette
On Thu, 2 Nov 2023, Reinette Chatre wrote: > On 10/24/2023 2:26 AM, Ilpo Järvinen wrote: > > > @@ -229,6 +228,31 @@ int get_cbm_mask(char *cache_type, char *cbm_mask) > > return 0; > > } > > > > +/* > > + * get_cbm_mask - Get cbm bit mask > > I know you just copied code here but please keep an eye out for acronyms > to be written in caps. Yeah, Maciej also commented on this. I've already made some changes but I'll incorporate some of your suggestions too. > This needs not be named to reflect verbatim what the function does. > Looking ahead I wonder if "get_full_mask()/get_max_mask()" may not be a > clear indication of what this does? > > Something like: > get_full_mask()/get_max_mask() Get maximum Cache Bit Mask (CBM) Having max in the name sounds useful. Also related to this, the local variables called long_mask should be renamed but perhaps not in this series to not block Maciej's work with neverending stream of cleanups :-). > @cache_type: Cache level(? or should this be "type") as "L2" or L3". > @mask: Full/Maximum portion of cache available for > allocation represented by bit mask > returned as unsigned long. > > > > + * @cache_type: Cache level L2/L3 > > + * @mask: cbm_mask returned as unsigned long > > + * > > + * Return: = 0 on success, < 0 on failure. > > + */ > > +int get_cbm_mask(const char *cache_type, unsigned long *mask) > > +{ > > + char cbm_mask_path[1024]; > > + int ret; > > + > > + if (!cache_type) > > + return -1; > > Just to confirm ... error checking on mask is intentionally deferred > until get_bit_mask()? I tried to put as much as possible into get_bit_mask() since every caller will have to do the same things anyway. I cannot avoid checking cache_type here because snprintf() is using it. Once again, very superb review of the whole series, thank you very much for all the effort! It's really appreciated!
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 224ba8544d8a..4852bbda2e71 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -93,18 +93,15 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type) int ret, pipefd[2], sibling_cpu_no; unsigned long cache_size = 0; unsigned long long_mask; - char cbm_mask[256]; int count_of_bits; char pipe_message; size_t span; /* 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 50bdbce9fba9..a6c79edc33cd 100644 --- a/tools/testing/selftests/resctrl/cmt_test.c +++ b/tools/testing/selftests/resctrl/cmt_test.c @@ -75,17 +75,14 @@ int cmt_resctrl_val(int cpu_no, int n, const char * const *benchmark_cmd) unsigned long cache_size = 0; unsigned long long_mask; char *span_str = NULL; - char cbm_mask[256]; int count_of_bits; size_t span; int ret, i; - 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 08b95b5a4949..e95121a113f3 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -99,7 +99,7 @@ void tests_cleanup(void); void mbm_test_cleanup(void); int mba_schemata_change(int cpu_no, const char * const *benchmark_cmd); void mba_test_cleanup(void); -int get_cbm_mask(char *cache_type, char *cbm_mask); +int get_cbm_mask(const 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 5ebd43683876..220dc83748ca 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -196,30 +196,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(const 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; @@ -229,6 +228,31 @@ int get_cbm_mask(char *cache_type, char *cbm_mask) return 0; } +/* + * get_cbm_mask - Get cbm bit 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(const 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