Re: [PATCH] selftests/resctrl: Fix noncont_cat_run_test for AMD

Moger, Babu
2024-06-07T22:35:23Z

Hi Reinette,

On 6/7/2024 4:47 PM, Reinette Chatre wrote:
> Hi Babu,
>
> On 6/7/24 11:16 AM, Moger, Babu wrote:
>> On 6/6/2024 6:58 PM, Reinette Chatre wrote:
>>> On 6/6/24 4:09 PM, Moger, Babu wrote:
>
>>>> @@ -301,15 +320,8 @@ static int noncont_cat_run_test(const struct
>>>> resctrl_test *test,
>>>>          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");
>>>> +       if (!(arch_supports_noncont_cat(test) && sparse_masks)) {
>>>> +               ksft_print_msg("Hardware does not support
>>>> non-contiguous CBM!\n");
>>>
>>> Please fix the test as well as the message. It is not an error if
>>> hardware does
>>> not support non-contiguous CBM. It is an error if the hardware and
>>> kernel disagrees whether
>>> non-contiguous CBM is supported.
>>
>> Not sure about this comment.
>>
>> Did you mean?
>>
>>   if (!arch_supports_noncont_cat(test)) {
>>                  ksft_print_msg("Hardware does not support
>> non-contiguous CBM!\n");
>>                  return 0;
>
> The above changes whether support for non-contiguous CBM is treated as
> an error but the
> test should still proceed since the test goes on to write different CBM
> to the system
> and verifies results are as expected based on what hardware supports.
>
>>           } else if (arch_supports_noncont_cat(test) && !sparse_masks)) {
>>                  ksft_print_msg("Hardware and kernel support for
>> non-contiguous CBM does not match!\n");
>>                  return 1;
>
> I can see how this will work for AMD for the scenario being checked but
> not for
> the different Intel variants.
>
> I think this can all be simplified with something like:
>     if (arch_supports_noncont_cat(test) != sparse_masks)) {
>         ksft_print_msg("Hardware and kernel differ on non-contiguous
> CBM support!\n");
>         return 1;
>     }
>
> I modified the message slightly since non-contiguous CBM does not
> actually require kernel
> support.
>
> What do you think?

Yes. That is fine.
Thank you
- Babu Moger