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!
@@ -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)
@@ -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;
@@ -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);
@@ -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