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

["Moger, Babu" <bmoger@amd.com>]

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