[03/24] selftests/resctrl: Refactor get_cbm_mask()

Message ID 20231024092634.7122-4-ilpo.jarvinen@linux.intel.com
State New
Headers
Series selftests/resctrl: CAT test improvements & generalized test framework |

Commit Message

Ilpo Järvinen Oct. 24, 2023, 9:26 a.m. UTC
  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
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>
---
 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(-)
  

Comments

Maciej Wieczor-Retman Oct. 27, 2023, 11:39 a.m. UTC | #1
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>
>---
  
Reinette Chatre Nov. 2, 2023, 5:47 p.m. UTC | #2
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
  
Ilpo Järvinen Nov. 3, 2023, 12:09 p.m. UTC | #3
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!
  

Patch

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