| Message ID | 2c1011e7630605365b67caa6ddfe4e8ee2ba5bff.1707487039.git.maciej.wieczor-retman@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | selftests/resctrl: Add non-contiguous CBMs in Intel CAT selftest | expand |
On Fri, 9 Feb 2024, Maciej Wieczor-Retman wrote: > Add tests for both L2 and L3 CAT to verify the return values > generated by writing non-contiguous CBMs don't contradict the > reported non-contiguous support information. > > Use a logical XOR to confirm return value of write_schemata() and > non-contiguous CBMs support information match. > > Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com> > Reviewed-by: Reinette Chatre <reinette.chatre@intel.com> Reviewed-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
Hi Maciej, On 2/9/2024 6:02 AM, Maciej Wieczor-Retman wrote: ... > diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c > index 39fc9303b8e8..d4b7bf8a6780 100644 > --- a/tools/testing/selftests/resctrl/cat_test.c > +++ b/tools/testing/selftests/resctrl/cat_test.c > @@ -294,6 +294,71 @@ 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; I missed that "ret" is "unsigned int" while the test expects it to be signed ("if (ret < 0)") and it is used to have return value of functions that return < 0 on error. > + char schemata[64]; > + int bit_center; > + > + /* Check to compare sparse_masks content to CPUID output. */ > + ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks); > + if (ret) > + return ret; > + > + if (!strcmp(test->resource, "L3")) > + __cpuid_count(0x10, 1, eax, ebx, ecx, edx); > + else if (!strcmp(test->resource, "L2")) > + __cpuid_count(0x10, 2, eax, ebx, ecx, edx); > + else > + return -EINVAL; > + > + if (sparse_masks != ((ecx >> 3) & 1)) { > + ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n"); > + return 1; > + } > + > + /* Write checks initialization. */ > + ret = get_full_cbm(test->resource, &full_cache_mask); > + if (ret < 0) > + return ret; > + bit_center = count_bits(full_cache_mask) / 2; It would be nice if no new static check issues are introduced into the resctrl selftests. I did a quick check and this is a problematic portion. We know that the full_cache_mask cannot have zero bits but it is not obvious to the checkers, thus thinking that bit_center may be zero resulting in a bit shift of "-2" bits attempt later on. Could you please add some error checking to ensure expected values to avoid extra noise from checkers when this code lands upstream? Thank you Reinette
Hello! On 2024-02-09 at 09:21:16 -0800, Reinette Chatre wrote: >Hi Maciej, > >On 2/9/2024 6:02 AM, Maciej Wieczor-Retman wrote: > >... > >> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c >> index 39fc9303b8e8..d4b7bf8a6780 100644 >> --- a/tools/testing/selftests/resctrl/cat_test.c >> +++ b/tools/testing/selftests/resctrl/cat_test.c >> @@ -294,6 +294,71 @@ 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; > >I missed that "ret" is "unsigned int" while the test expects it to >be signed ("if (ret < 0)") and it is used to have return value of functions >that return < 0 on error. > Oh, sorry, I'll fix that. > >> + char schemata[64]; >> + int bit_center; >> + >> + /* Check to compare sparse_masks content to CPUID output. */ >> + ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks); >> + if (ret) >> + return ret; >> + >> + if (!strcmp(test->resource, "L3")) >> + __cpuid_count(0x10, 1, eax, ebx, ecx, edx); >> + else if (!strcmp(test->resource, "L2")) >> + __cpuid_count(0x10, 2, eax, ebx, ecx, edx); >> + else >> + return -EINVAL; >> + >> + if (sparse_masks != ((ecx >> 3) & 1)) { >> + ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n"); >> + return 1; >> + } >> + >> + /* Write checks initialization. */ >> + ret = get_full_cbm(test->resource, &full_cache_mask); >> + if (ret < 0) >> + return ret; >> + bit_center = count_bits(full_cache_mask) / 2; > >It would be nice if no new static check issues are introduced into the >resctrl selftests. I did a quick check and this is a problematic portion. >We know that the full_cache_mask cannot have zero bits but it is not >obvious to the checkers, thus thinking that bit_center may be zero >resulting in a bit shift of "-2" bits attempt later on. Could you please >add some error checking to ensure expected values to avoid extra noise from >checkers when this code lands upstream? > >Thank you Sure, I guess I could make the check 'if (bit_center < 3)' to also check if the full_cache_mask isn't too short for some reason (since later 2 is substracted from bit_center for the 'hole' bit shift). Or would this need some comment as well (why exactly the '3' is there, maybe write something about about the minimal full_cache_mask length for this test)? > >Reinette >
Hi Maciej, On 2/11/2024 11:38 PM, Maciej Wieczor-Retman wrote: > Sure, I guess I could make the check 'if (bit_center < 3)' to also check if the > full_cache_mask isn't too short for some reason (since later 2 is substracted > from bit_center for the 'hole' bit shift). Thank you. > Or would this need some comment as well (why exactly the '3' is there, maybe > write something about about the minimal full_cache_mask length for this test)? The use of "Or" and "as well" makes it unclear what you propose. I do think the check in addition to a comment will be helpful. Thank you. Reinette
diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c index 39fc9303b8e8..d4b7bf8a6780 100644 --- a/tools/testing/selftests/resctrl/cat_test.c +++ b/tools/testing/selftests/resctrl/cat_test.c @@ -294,6 +294,71 @@ 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 schemata[64]; + int bit_center; + + /* Check to compare sparse_masks content to CPUID output. */ + ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks); + if (ret) + return ret; + + if (!strcmp(test->resource, "L3")) + __cpuid_count(0x10, 1, eax, ebx, ecx, edx); + else if (!strcmp(test->resource, "L2")) + __cpuid_count(0x10, 2, eax, ebx, ecx, edx); + else + return -EINVAL; + + if (sparse_masks != ((ecx >> 3) & 1)) { + ksft_print_msg("CPUID output doesn't match 'sparse_masks' file content!\n"); + return 1; + } + + /* Write checks initialization. */ + ret = get_full_cbm(test->resource, &full_cache_mask); + if (ret < 0) + return ret; + bit_center = count_bits(full_cache_mask) / 2; + cont_mask = full_cache_mask >> bit_center; + + /* Contiguous mask write check. */ + snprintf(schemata, sizeof(schemata), "%lx", cont_mask); + ret = write_schemata("", schemata, uparams->cpu, test->resource); + if (ret) { + ksft_print_msg("Write of contiguous CBM failed\n"); + return 1; + } + + /* + * Non-contiguous mask write check. CBM has a 0xf hole approximately in the middle. + * Output is compared with support information to catch any edge case errors. + */ + noncont_mask = ~(0xfUL << (bit_center - 2)) & full_cache_mask; + snprintf(schemata, sizeof(schemata), "%lx", noncont_mask); + ret = write_schemata("", schemata, uparams->cpu, test->resource); + if (ret && sparse_masks) + ksft_print_msg("Non-contiguous CBMs supported but write of non-contiguous CBM failed\n"); + else if (ret && !sparse_masks) + ksft_print_msg("Non-contiguous CBMs not supported and write of non-contiguous CBM failed as expected\n"); + else if (!ret && !sparse_masks) + ksft_print_msg("Non-contiguous CBMs not supported but write of non-contiguous CBM succeeded\n"); + + return !ret == !sparse_masks; +} + +static bool noncont_cat_feature_check(const struct resctrl_test *test) +{ + if (!resctrl_resource_exists(test->resource)) + return false; + + return resource_info_file_exists(test->resource, "sparse_masks"); +} + struct resctrl_test l3_cat_test = { .name = "L3_CAT", .group = "CAT", @@ -301,3 +366,19 @@ struct resctrl_test l3_cat_test = { .feature_check = test_resource_feature_check, .run_test = cat_run_test, }; + +struct resctrl_test l3_noncont_cat_test = { + .name = "L3_NONCONT_CAT", + .group = "CAT", + .resource = "L3", + .feature_check = noncont_cat_feature_check, + .run_test = noncont_cat_run_test, +}; + +struct resctrl_test l2_noncont_cat_test = { + .name = "L2_NONCONT_CAT", + .group = "CAT", + .resource = "L2", + .feature_check = noncont_cat_feature_check, + .run_test = noncont_cat_run_test, +}; diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h index f434a6543b4f..2051bd135e0d 100644 --- a/tools/testing/selftests/resctrl/resctrl.h +++ b/tools/testing/selftests/resctrl/resctrl.h @@ -209,5 +209,7 @@ extern struct resctrl_test mbm_test; extern struct resctrl_test mba_test; extern struct resctrl_test cmt_test; extern struct resctrl_test l3_cat_test; +extern struct resctrl_test l3_noncont_cat_test; +extern struct resctrl_test l2_noncont_cat_test; #endif /* RESCTRL_H */ diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c index 3044179ee6e9..f3dc1b9696e7 100644 --- a/tools/testing/selftests/resctrl/resctrl_tests.c +++ b/tools/testing/selftests/resctrl/resctrl_tests.c @@ -19,6 +19,8 @@ static struct resctrl_test *resctrl_tests[] = { &mba_test, &cmt_test, &l3_cat_test, + &l3_noncont_cat_test, + &l2_noncont_cat_test, }; static int detect_vendor(void)