| Message ID | c7b66a4682829894ec72d8a1f78e324233ef0535.1702392177.git.maciej.wieczor-retman@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Add non-contiguous CBMs in Intel CAT selftest | expand |
Hi Maciej, On 12/12/2023 6:52 AM, Maciej Wieczor-Retman wrote: > The CAT non-contiguous selftests have to read the file responsible for > reporting support of non-contiguous CBMs in Intel CAT. Then the test "in Intel CAT" -> "in kernel (resctrl)" > compares if that information matches what is reported by CPUID output. > > Add a generic helper function to read a chosen functionality support > information. Since this is a generic function that just reads a value from a file it cannot be assumed that the value represents functionality support. > > Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> > --- > Changelog v2: > - Added this patch. > > tools/testing/selftests/resctrl/resctrl.h | 1 + > tools/testing/selftests/resctrl/resctrlfs.c | 25 +++++++++++++++++++++ > 2 files changed, 26 insertions(+) > > diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h > index 739e16d08a7b..8f72d94b9cbe 100644 > --- a/tools/testing/selftests/resctrl/resctrl.h > +++ b/tools/testing/selftests/resctrl/resctrl.h > @@ -161,6 +161,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start); > int get_full_cbm(const char *cache_type, unsigned long *mask); > int get_mask_no_shareable(const char *cache_type, unsigned long *mask); > int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); > +int read_info_res_file(const char *resource, const char *filename); > void ctrlc_handler(int signum, siginfo_t *info, void *ptr); > int signal_handler_register(void); > void signal_handler_unregister(void); > diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c > index 0e97036a64b8..70333440ff2f 100644 > --- a/tools/testing/selftests/resctrl/resctrlfs.c > +++ b/tools/testing/selftests/resctrl/resctrlfs.c > @@ -249,6 +249,31 @@ static int get_bit_mask(const char *filename, unsigned long *mask) > return 0; > } > > +int read_info_res_file(const char *resource, const char *filename) Considering that this is intended to be a new generic utility, could you please add some function documentation? > +{ > + char file_path[PATH_MAX]; > + FILE *fp; > + int ret; > + > + snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource, > + filename); > + > + fp = fopen(file_path, "r"); > + if (!fp) { > + perror("Error in opening sparse_masks file\n"); The error messages do not match the goal of this function to be generic. Also, please note the recent cleanup done by Ilpo to replace the perror() by ksft_perror(). > + return -1; > + } > + > + if (fscanf(fp, "%u", &ret) <= 0) { I find this to be potentially confusing. The function claims to be a generic utility to read a value from a resctrl file ... but hidden within is that the value is required to be unsigned, which is then cast into an int. This could be made more specific and robust with something like below: int resource_info_unsigned_get(const char *resource, const char *filename, unsigned int *val) The return value will be the result of the request. If resource_info_unsigned_get() returns 0 then @val will contain the value read. > + perror("Could not get sparse_masks contents\n"); > + fclose(fp); > + return -1; > + } > + > + fclose(fp); > + return ret; > +} > + > /* > * create_bit_mask- Create bit mask from start, len pair > * @start: LSB of the mask Reinette
Hi! On 2024-01-08 at 14:36:36 -0800, Reinette Chatre wrote: >Hi Maciej, > >On 12/12/2023 6:52 AM, Maciej Wieczor-Retman wrote: >> The CAT non-contiguous selftests have to read the file responsible for >> reporting support of non-contiguous CBMs in Intel CAT. Then the test > >"in Intel CAT" -> "in kernel (resctrl)" Sure, will fix. >> compares if that information matches what is reported by CPUID output. >> >> Add a generic helper function to read a chosen functionality support >> information. > >Since this is a generic function that just reads a value from a file it >cannot be assumed that the value represents functionality support. Right, I'll rewrite this. >> >> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> >> --- >> Changelog v2: >> - Added this patch. >> >> tools/testing/selftests/resctrl/resctrl.h | 1 + >> tools/testing/selftests/resctrl/resctrlfs.c | 25 +++++++++++++++++++++ >> 2 files changed, 26 insertions(+) >> >> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h >> index 739e16d08a7b..8f72d94b9cbe 100644 >> --- a/tools/testing/selftests/resctrl/resctrl.h >> +++ b/tools/testing/selftests/resctrl/resctrl.h >> @@ -161,6 +161,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start); >> int get_full_cbm(const char *cache_type, unsigned long *mask); >> int get_mask_no_shareable(const char *cache_type, unsigned long *mask); >> int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); >> +int read_info_res_file(const char *resource, const char *filename); >> void ctrlc_handler(int signum, siginfo_t *info, void *ptr); >> int signal_handler_register(void); >> void signal_handler_unregister(void); >> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c >> index 0e97036a64b8..70333440ff2f 100644 >> --- a/tools/testing/selftests/resctrl/resctrlfs.c >> +++ b/tools/testing/selftests/resctrl/resctrlfs.c >> @@ -249,6 +249,31 @@ static int get_bit_mask(const char *filename, unsigned long *mask) >> return 0; >> } >> >> +int read_info_res_file(const char *resource, const char *filename) > >Considering that this is intended to be a new generic utility, could you >please add some function documentation? Sure >> +{ >> + char file_path[PATH_MAX]; >> + FILE *fp; >> + int ret; >> + >> + snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource, >> + filename); >> + >> + fp = fopen(file_path, "r"); >> + if (!fp) { >> + perror("Error in opening sparse_masks file\n"); > >The error messages do not match the goal of this function to be generic. >Also, please note the recent cleanup done by Ilpo to replace the perror() >by ksft_perror(). Thanks for catching this, will fix. >> + return -1; >> + } >> + >> + if (fscanf(fp, "%u", &ret) <= 0) { > >I find this to be potentially confusing. The function claims to be a generic >utility to read a value from a resctrl file ... but hidden within is that the >value is required to be unsigned, which is then cast into an int. This could be >made more specific and robust with something like below: > int resource_info_unsigned_get(const char *resource, const char *filename, > unsigned int *val) > >The return value will be the result of the request. If resource_info_unsigned_get() >returns 0 then @val will contain the value read. Right, that might be confusing. Will fix according to your comment, thanks! >> + perror("Could not get sparse_masks contents\n"); >> + fclose(fp); >> + return -1; >> + } >> + >> + fclose(fp); >> + return ret; >> +} >> + >> /* >> * create_bit_mask- Create bit mask from start, len pair >> * @start: LSB of the mask > >Reinette
diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index 739e16d08a7b..8f72d94b9cbe 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -161,6 +161,7 @@ unsigned int count_contiguous_bits(unsigned long val, unsigned int *start); int get_full_cbm(const char *cache_type, unsigned long *mask); int get_mask_no_shareable(const char *cache_type, unsigned long *mask); int get_cache_size(int cpu_no, const char *cache_type, unsigned long *cache_size); +int read_info_res_file(const char *resource, const char *filename); void ctrlc_handler(int signum, siginfo_t *info, void *ptr); int signal_handler_register(void); void signal_handler_unregister(void); diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c index 0e97036a64b8..70333440ff2f 100644 --- a/tools/testing/selftests/resctrl/resctrlfs.c +++ b/tools/testing/selftests/resctrl/resctrlfs.c @@ -249,6 +249,31 @@ static int get_bit_mask(const char *filename, unsigned long *mask) return 0; } +int read_info_res_file(const char *resource, const char *filename) +{ + char file_path[PATH_MAX]; + FILE *fp; + int ret; + + snprintf(file_path, sizeof(file_path), "%s/%s/%s", INFO_PATH, resource, + filename); + + fp = fopen(file_path, "r"); + if (!fp) { + perror("Error in opening sparse_masks file\n"); + return -1; + } + + if (fscanf(fp, "%u", &ret) <= 0) { + perror("Could not get sparse_masks contents\n"); + fclose(fp); + return -1; + } + + fclose(fp); + return ret; +} + /* * create_bit_mask- Create bit mask from start, len pair * @start: LSB of the mask
The CAT non-contiguous selftests have to read the file responsible for reporting support of non-contiguous CBMs in Intel CAT. Then the test compares if that information matches what is reported by CPUID output. Add a generic helper function to read a chosen functionality support information. Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> --- Changelog v2: - Added this patch. tools/testing/selftests/resctrl/resctrl.h | 1 + tools/testing/selftests/resctrl/resctrlfs.c | 25 +++++++++++++++++++++ 2 files changed, 26 insertions(+)