Re: [PATCH] selftests/resctrl: Add non-contiguous CBMs CAT test

[Ilpo Järvinen]

On Mon, 20 Nov 2023, Maciej Wieczór-Retman wrote:
> On 2023-11-17 at 14:55:54 +0200, Ilpo Järvinen wrote:
> >On Thu, 9 Nov 2023, Maciej Wieczor-Retman wrote:
> >
> >> Non-contiguous CBM support for Intel CAT has been merged into the kernel
> >> with Commit 0e3cd31f6e90 ("x86/resctrl: Enable non-contiguous CBMs in
> >> Intel CAT") but there is no selftest that would validate if this feature
> >> works correctly.
> >> 
> >> The selftest needs to verify if writing non-contiguous CBMs to the
> >> schemata file behaves as expected in comparison to the information about
> >> non-contiguous CBMs support.
> >> 
> >> Add tests for both L2 and L3 CAT to verify if the return values
> >> generated by writing non-contiguous CBMs don't contradict the
> >> reported non-contiguous support information.

> >> @@ -342,6 +342,87 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
> >>  	return ret;
> >>  }
> >>  
> >> +static int noncont_cat_run_test(const struct resctrl_test *test,
> >> +				const struct user_params *uparams)
> >> +{
> >> +	unsigned long full_cache_mask, cont_mask, noncont_mask;
> >> +	unsigned int eax, ebx, ecx, edx, ret, sparse_masks;
> >> +	char res_path[PATH_MAX];
> >> +	char schemata[64];
> >> +	int bit_center;
> >> +	FILE *fp;
> >> +
> >> +	/* Check to compare sparse_masks content to cpuid output. */
> >> +	snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH,
> >> +		 test->resource, "sparse_masks");
> >> +
> >> +	fp = fopen(res_path, "r");
> >> +	if (!fp) {
> >> +		perror("# Error in opening file\n");
> >> +		return errno;
> >> +	}
> >> +
> >> +	if (fscanf(fp, "%u", &sparse_masks) <= 0) {
> >> +		perror("Could not get sparse_masks contents\n");
> >> +		fclose(fp);
> >> +		return -1;
> >> +	}
> >> +
> >> +	fclose(fp);
> >
> >Add a function to do this conversion into resctrlfs.c.
> 
> By conversion do you mean the above calls to fopen, fscanf and fclose? Or did
> you mean the below __cpuid_count? Since you mention making get_cache_level
> non-static I assume the first is the case but just wanted to confirm.

You convert the 0/1 read from a resctrl related file into (unsigned) int 
here. Create a helper function for that into resctrlfs.c that returns int 
(to be able to return also errors) and just call it from here with the 
feature string you're interested in, the helper should deal with the rest.

> >> +	return !ret == !sparse_masks;
> >> +}
> >> +
> >> +static bool noncont_cat_feature_check(const struct resctrl_test *test)
> >> +{
> >> +	char res_path[PATH_MAX];
> >> +	struct stat statbuf;
> >> +
> >> +	snprintf(res_path, sizeof(res_path), "%s/%s/%s", INFO_PATH,
> >> +		 test->resource, "sparse_masks");
> >> +
> >> +	if (stat(res_path, &statbuf))
> >> +		return false;
> >
> >This looks generic enough that validate_resctrl_feature_request() should 
> >be somehow adapted to cover also these cases. Perhaps it would be best to 
> >just split validate_resctrl_feature_request() into multiple functions.
> 
> As in conditionally call a function inside validate_resctrl_feature_request()
> that would check for the existance of a particular file that would indicate if a
> feature is supported or not?

I meant that validate_resctrl_feature_request() should probably be split 
into 2 or 3 functions, each taking different arguments and one them 
checks mon_features, another presence of sparse_masks file (any file on 
the level actually).

> Does implementing it as a new entry in resctrl_test struct that would hold the
> desired filename seem reasonable?

I'm not convinced it's useful to put it into the test structure. A simple 
function that calls into one or more of the functions provided 
in resctrlfs.c seems enough for me.

> Or would it be better to pass it as a new function argument (similiar to 
> how "const char *feature" is used now)?

I'd create a separate function in resctrlfs.c instead (IMO, a new function 
should be also done for those callers which currently use const 
char *feature).


-- 
 i.