Re: [PATCH v2 2/2] selftests/resctrl: Add L3_BIT_USAGE to check allocation reporting

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Richard,

On 8/10/26 2:51 AM, Richard Cheng wrote:
> Add L3_BIT_USAGE to verify that info/L3/bit_usage follows the root
> group's CBM.
> 
> Write full, low, and high valid masks, then check the selected domain.
> Require X or S for portions used by SW and H or 0 for portions outside
> the allocation.
> 
> Signed-off-by: Richard Cheng <icheng@nvidia.com>
> ---

...

> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index a06c57954740..9205f93fd669 100644
> --- a/tools/testing/selftests/resctrl/cat_test.c
> +++ b/tools/testing/selftests/resctrl/cat_test.c
> @@ -440,6 +440,192 @@ static int cbm_validate_run_test(const struct resctrl_test *test,
>  	return 0;
>  }
>  
> +/*
> + * L3_BIT_USAGE - Verify info/L3/bit_usage reflects the allocation.
> + *

Similar to the previous patch I do not see anything unique to L3 in this test. Here too
it looks like it can be expanded to provide coverage for L2 with just a new
struct resctrl_test definition.

> + * bit_usage annotates each cache portion. 'X' and 'S' mean software uses the
> + * portion, while 'H' and '0' mean it does not. With only the root group
> + * present, bit_usage must track the root CBM bit-for-bit.
> + */
> +static bool bit_used_by_sw(char c)
> +{
> +	return c == 'X' || c == 'S';
> +}
> +
> +static bool bit_unused_by_sw(char c)
> +{
> +	return c == 'H' || c == '0';
> +}
> +
> +static bool bit_usage_valid(char c)
> +{
> +	return bit_used_by_sw(c) || bit_unused_by_sw(c);
> +}
> +
> +static int bit_usage_for_domain(const char *resource, int domain_id,
> +				char *out, size_t out_len)
> +{
> +	char path[PATH_MAX], *line = NULL, *saveptr;
> +	char *usage, *endptr, *id;
> +	size_t line_len = 0;
> +	long parsed_id;
> +	ssize_t bytes;
> +	FILE *fp;
> +	int ret;
> +
> +	snprintf(path, sizeof(path), "%s/%s/bit_usage", INFO_PATH, resource);
> +	fp = fopen(path, "r");
> +	if (!fp) {
> +		ksft_perror("Error opening bit_usage");
> +		return -errno;
> +	}
> +
> +	bytes = getline(&line, &line_len, fp);
> +	fclose(fp);
> +	if (bytes < 0) {
> +		ksft_print_msg("Error reading bit_usage\n");
> +		free(line);
> +		return -EIO;
> +	}
> +
> +	ret = -ENOENT;
> +	for (id = strtok_r(line, "=;\n", &saveptr); id;
> +	     id = strtok_r(NULL, "=;\n", &saveptr)) {

Could the pattern be '=' instead of the comprehensive '=;\n' to make this more robust?

> +		usage = strtok_r(NULL, "=;\n", &saveptr);

Could this pattern just be ';\n'?

> +		if (!usage)
> +			break;
> +
> +		errno = 0;
> +		parsed_id = strtol(id, &endptr, 10);
> +		if (errno || *endptr || parsed_id != domain_id)
> +			continue;
> +
> +		ret = snprintf(out, out_len, "%s", usage);
> +		if (ret < 0 || (size_t)ret >= out_len)
> +			ret = -ENOSPC;
> +		else
> +			ret = 0;
> +		goto out;
> +	}
> +
> +	ksft_print_msg("No bit_usage entry for domain %d\n", domain_id);
> +out:
> +	free(line);
> +	return ret;
> +}
> +
> +static int bit_usage_check_mask(const struct resctrl_test *test, int cpu,
> +				unsigned long mask, unsigned int count_of_bits)
> +{
> +	char usage[sizeof(unsigned long) * 8 + 1];
> +	char schemata[64];
> +	unsigned int i;
> +	int domain_id;
> +	int ret;
> +
> +	snprintf(schemata, sizeof(schemata), "%lx", mask);
> +	ret = write_schemata("", schemata, cpu, test->resource);
> +	if (ret) {
> +		ksft_print_msg("Failed to set CBM 0x%lx\n", mask);
> +		return ret;
> +	}
> +
> +	ret = get_domain_id(test->resource, cpu, &domain_id);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = bit_usage_for_domain(test->resource, domain_id, usage,
> +				   sizeof(usage));
> +	if (ret)
> +		return ret;
> +
> +	if (strlen(usage) != count_of_bits) {
> +		ksft_print_msg("bit_usage \"%s\" has %zu chars, expected %u\n",
> +			       usage, strlen(usage), count_of_bits);
> +		return KSFT_FAIL;
> +	}
> +
> +	for (i = 0; i < count_of_bits; i++) {
> +		bool in_cbm;
> +		int bit;
> +		char c;
> +
> +		bit = count_of_bits - 1 - i;
> +		in_cbm = (mask >> bit) & 1;
> +		c = usage[i];
> +
> +		if (!bit_usage_valid(c)) {
> +			ksft_print_msg("Invalid bit_usage character '%c' for CBM 0x%lx\n",
> +				       c, mask);
> +			return KSFT_FAIL;
> +		}
> +		if (in_cbm != bit_used_by_sw(c)) {
> +			ksft_print_msg("CBM 0x%lx portion %d shows '%c', %s allocation\n",

portion -> bit?

> +				       mask, bit, c, in_cbm ? "in" : "not in");
> +			return KSFT_FAIL;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int bit_usage_run_test(const struct resctrl_test *test,
> +			      const struct user_params *uparams)
> +{
> +	unsigned int count_of_bits, min_cbm_bits, partial_bits;
> +	unsigned long full_mask, high_mask, masks[3];
> +	unsigned int nr_masks = 1;
> +	unsigned int i;
> +	int ret;
> +
> +	ret = get_full_cbm(test->resource, &full_mask);
> +	if (ret)
> +		return ret;
> +
> +	ret = resource_info_unsigned_get(test->resource, "min_cbm_bits",
> +					 &min_cbm_bits);
> +	if (ret)
> +		return ret;
> +
> +	count_of_bits = count_bits(full_mask);
> +
> +	/* Every cache portion. */
> +	masks[0] = full_mask;
> +
> +	if (count_of_bits > min_cbm_bits) {

I would argue if count_of_bits is less than min_cbm_bits then it is a bug because
the hardware claims to require more bits than it claims to support. This test can
just fail right away if this is encountered.


> +		partial_bits = count_of_bits / 2;
> +		partial_bits = max(partial_bits, min_cbm_bits);

These patches do not compile for me. I needed to add
#include <linux/kernel.h> to cat_test.c to obtain definition of max().


> +
> +		/* Lowest valid partial allocation. */
> +		masks[nr_masks++] = create_bit_mask(0, partial_bits);
> +
> +		if (partial_bits) {
> +			/* Highest valid partial allocation. */
> +			high_mask = create_bit_mask(count_of_bits - partial_bits,
> +						    partial_bits);
> +			if (high_mask != masks[nr_masks - 1])
> +				masks[nr_masks++] = high_mask;
> +		}
> +	}
> +
> +	for (i = 0; i < nr_masks; i++) {
> +		ret = bit_usage_check_mask(test, uparams->cpu, masks[i],
> +					   count_of_bits);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	ksft_print_msg("Pass: bit_usage reflects the allocation\n");
> +
> +	return 0;
> +}
> +
> +static bool bit_usage_feature_check(const struct resctrl_test *test)
> +{
> +	return test_resource_feature_check(test) &&
> +	       resource_info_file_exists(test->resource, "bit_usage");
> +}
> +
>  struct resctrl_test l3_cat_test = {
>  	.name = "L3_CAT",
>  	.group = "CAT",
> @@ -457,6 +643,14 @@ struct resctrl_test l3_cbm_validate_test = {
>  	.run_test = cbm_validate_run_test,
>  };
>  
> +struct resctrl_test l3_bit_usage_test = {
> +	.name = "L3_BIT_USAGE",
> +	.group = "CAT",
> +	.resource = "L3",
> +	.feature_check = bit_usage_feature_check,
> +	.run_test = bit_usage_run_test,
> +};

Could this be added?

struct resctrl_test l2_bit_usage_test = {
	.name = "L2_BIT_USAGE",
	.group = "CAT",
	.resource = "L2",
	.feature_check = bit_usage_feature_check,
	.run_test = bit_usage_run_test,
};


> +
>  struct resctrl_test l3_noncont_cat_test = {
>  	.name = "L3_NONCONT_CAT",
>  	.group = "CAT",
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index a6fc688ed997..765aa5d5814d 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -241,6 +241,7 @@ extern struct resctrl_test mba_test;
>  extern struct resctrl_test cmt_test;
>  extern struct resctrl_test l3_cat_test;
>  extern struct resctrl_test l3_cbm_validate_test;
> +extern struct resctrl_test l3_bit_usage_test;
>  extern struct resctrl_test l3_noncont_cat_test;
>  extern struct resctrl_test l2_noncont_cat_test;
>  
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 57a4d0815b67..e6e948d55f38 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -20,6 +20,7 @@ static struct resctrl_test *resctrl_tests[] = {
>  	&cmt_test,
>  	&l3_cat_test,
>  	&l3_cbm_validate_test,
> +	&l3_bit_usage_test,
>  	&l3_noncont_cat_test,
>  	&l2_noncont_cat_test,
>  };

Reinette