linux-kernel - Re: [PATCH v3 2/2] selftests/resctrl: Adjust SNC support messages

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <3e6qgkiefkytuq2vvrkownaisyzolc4d5s57ju5zg7e4uoiuvv@fjuem3gj2dtm>
Date: Tue, 9 Jul 2024 09:49:49 +0200
From: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
CC: <shuah@...nel.org>, <fenghua.yu@...el.com>,
	<linux-kernel@...r.kernel.org>, <linux-kselftest@...r.kernel.org>,
	<ilpo.jarvinen@...ux.intel.com>, <tony.luck@...el.com>
Subject: Re: [PATCH v3 2/2] selftests/resctrl: Adjust SNC support messages

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