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

[Maciej Wieczor-Retman]

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
>

-- 
Kind regards
Maciej Wieczór-Retman