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).