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