linux-kernel - Re: [PATCH v4 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <7ccac5b3-cfba-4f31-804b-45da60b0c08f@intel.com>
Date: Tue, 17 Sep 2024 08:32:11 -0700
From: Reinette Chatre <reinette.chatre@...el.com>
To: Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>
CC: <fenghua.yu@...el.com>, <shuah@...nel.org>,
	<linux-kselftest@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<ilpo.jarvinen@...ux.intel.com>, <tony.luck@...el.com>
Subject: Re: [PATCH v4 2/2] selftests/resctrl: Adjust SNC support messages

Hi Maciej,

On 8/27/24 1:15 AM, Maciej Wieczor-Retman wrote:
> On 2024-08-12 at 16:40:10 -0700, Reinette Chatre wrote:
>> On 7/12/24 2:04 AM, Maciej Wieczor-Retman wrote:

>>> +
>>> +	if ((get_vendor() == ARCH_INTEL) && snc_unreliable) {
>>> +		ksft_print_msg("Sub-NUMA Clustering could not be detected properly (see earlier messages for details).\n");
>>> +		ksft_print_msg("Intel CAT may be inaccurate.\n");
>>> +	}
>>
>> This is still relevant but unclear why previous message checked "ret" but above does not.
> 
> The above check tries to explain why a failure happened.
> 
> This check is a reminder about a false positive - the test passes but
> "snc_unreliable" was set. I guess we could make this check to test "!ret"?

Thinking about this more ... if the test results cannot be trusted at all (whether tests pass or
fail) when snc_reliable is true then it seems more appropriate to just skip these tests when
SNC detection is unreliable.

> 
>>
>>> +
>>>    	return ret;
>>>    }
>>> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
>>> index 0c045080d808..471e134face0 100644
>>> --- a/tools/testing/selftests/resctrl/cmt_test.c
>>> +++ b/tools/testing/selftests/resctrl/cmt_test.c
>>> @@ -175,8 +175,14 @@ static int cmt_run_test(const struct resctrl_test *test, const struct user_param
>>>    		goto out;
>>>    	ret = check_results(&param, span, n);
>>> -	if (ret && (get_vendor() == ARCH_INTEL))
>>> -		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
>>> +	if (ret && (get_vendor() == ARCH_INTEL) && !snc_kernel_support())
>>> +		ksft_print_msg("Kernel doesn't support Sub-NUMA Clustering but it is enabled on the system.\n");
>>> +
>>> +	if ((get_vendor() == ARCH_INTEL) && snc_unreliable) {
>>> +		ksft_print_msg("Sub-NUMA Clustering could not be detected properly (see earlier messages for details).\n");
>>> +		ksft_print_msg("Intel CMT may be inaccurate.\n");
>>> +	}
>>> +
>>
>> CMT may be inaccurate in both scenarios (no kernel support or unreliable detection). Why only
>> check "ret" in case there is no kernel support?
> 
> I guess the same thing from above can apply here? Test "!ret"? Perhaps then make
> this check into "else if ()" instead of just "if" since they will be exclusive?
> 
>>
>>>    out:
>>>    	free(span_str);
>>> diff --git a/tools/testing/selftests/resctrl/mba_test.c b/tools/testing/selftests/resctrl/mba_test.c
>>> index ab8496a4925b..a805c14fe04b 100644
>>> --- a/tools/testing/selftests/resctrl/mba_test.c
>>> +++ b/tools/testing/selftests/resctrl/mba_test.c
>>> @@ -179,6 +179,13 @@ static int mba_run_test(const struct resctrl_test *test, const struct user_param
>>>    		return ret;
>>>    	ret = check_results();
>>> +	if (ret && (get_vendor() == ARCH_INTEL) && !snc_kernel_support())
>>> +		ksft_print_msg("Kernel doesn't support Sub-NUMA Clustering but it is enabled on the system.\n");
>>> +
>>> +	if ((get_vendor() == ARCH_INTEL) && snc_unreliable) {
>>> +		ksft_print_msg("Sub-NUMA Clustering could not be detected properly (see earlier messages for details).\n");
>>> +		ksft_print_msg("Intel MBA may be inaccurate.\n");
>>> +	}
>>
>> As I understand there is no change to MBA when SNC is enabled. These additions thus seem unnecessary.
> 
> I just rechecked by installing 6.9 kernel (no SNC kernel support) and using this
> series selftest. MBA seems to fail in these conditions. I think it is because
> MBA pulls values from resctrl and from iMC and then compares them. My guess is
> that iMC works on the halved cache while resctrl (without new SNC support) uses
> the whole cache. Here is the log from the MBA test I did:

Apologies, yes, while MBA is not impacted by SNC the MBA selftest relies on
MBM that is impacted by SNC.

Reinette