Hello,
On 2024-07-08 at 09:39:02 -0700, Reinette Chatre wrote:
>Hi Maciej,
>
>On 7/4/24 12:23 AM, Maciej Wieczor-Retman wrote:
>> On 2024-07-03 at 13:51:03 -0700, Reinette Chatre wrote:
>> > On 7/3/24 12:43 AM, Maciej Wieczór-Retman wrote:
>> > > On 3.07.2024 00:21, Reinette Chatre wrote:
>> > > > On 7/1/24 7:18 AM, Maciej Wieczor-Retman wrote:
>
>...
>
>> > > SNC might not be enabled at all so there would be no reason to send the user
>> > > to check their BIOS - instead they can learn they have offline CPUs and they can
>> > > work on fixing that. In my opinion it could be beneficial to have more specialized
>> > > messages in the selftests to help users diagnose problems quicker.
>> >
>> > My goal is indeed to have specialized messages. There cannot be a specialized message
>> > if snc_reliable == 1. In this case it needs to be generic since SNC may or may not be
>> > enabled and it is up to the user to investigate further.
>>
>> How about this in cmt_run_test() for example:
>>
>> if (snc_unreliable)
>> ksft_print_msg("Intel CMT may be inaccurate or inefficient when Sub-NUMA Clustering is enabled and not properly detected.\n");
>
>It is ok with me if you want to keep the message even if the test succeeds. Without seeing
>the new implementation it is unclear to me why the SNC check below is guarded by an ARCH_INTEL
>check while the one above is not. Ideally this should be consistent to not confuse how
>the architectures need to be treated here.
Right, I'll add the get_vendor() check to this too.
>
>The message does sound a bit vague to me about being able to detect SNC. How about something
>like:
> Sub-NUMA Clustering could not be detected properly (see earlier messages for details).
> Intel CMT may be inaccurate.
It sounds good, I'll change the message to this.
>> else if (ret && (get_vendor() == ARCH_INTEL) && !snc_kernel_support())
>> ksft_print_msg("Kernel doesn't support Sub-NUMA Clustering but it is enabled. Check BIOS configuration.\n");
>
>The "Check BIOS configuration" guidance is not clear to me. If the kernel does not
>support SNC then the user could also be guided to upgrade their kernel? Perhaps that
>snippet can just be dropped? To be more specific that SNC enabling is not a kernel
>configuration but a system configuration this can read (please feel free to improve):
> Kernel doesn't support Sub-NUMA Clustering but it is enabled on the system.
I suppose you're right, this does look better. Thanks!
>
>
>Reinette
--
Kind regards
Maciej Wieczór-Retman
