Received-SPF: pass (google.com: domain of linux-kernel+bounces-46444-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Wed, 31 Jan 2024 13:55:16 +0100
From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
CC: <fenghua.yu@intel.com>, <shuah@kernel.org>,
	<linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>,
	<ilpo.jarvinen@linux.intel.com>
Subject: Re: [PATCH v3 5/5] selftests/resctrl: Add non-contiguous CBMs CAT
 test
Message-ID: <jbgkiwfkotntcdzhwf27dceit4w3j37sumkayixmyuee6zyaqg@un22a2iljgmr>
References: <cover.1706180726.git.maciej.wieczor-retman@intel.com>
 <647fbfd449f8b0e0ad6cfe58bb280ff44ee162b8.1706180726.git.maciej.wieczor-retman@intel.com>
 <db08ba56-ae73-4c70-87fb-aae59e524238@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <db08ba56-ae73-4c70-87fb-aae59e524238@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?iso-8859-1?Q?A1BlZJ1xn4XW1ZRCw90ugKit3SsXVSKac5+MfNK/twlxDlo00Fh69yvQK8?=
 =?iso-8859-1?Q?NZW8O1Sw6m9J9SPsHUjJ6EhkeBsZC+GG6aGJZhr91fovTO/E7g1Fb6Y0gV?=
 =?iso-8859-1?Q?+Oe+3vQ6oGBxnKQ7cWuMnprLZm1gdkBQM52Z8GdIYl28liZowApRExwB1M?=
 =?iso-8859-1?Q?jHuGniBq8U185b26t2SS834oLrIxacTqKEc30LVTOZQlBYoZ+ClwGMUPI6?=
 =?iso-8859-1?Q?vJwP/k4ijNE+oBZRlkq4mN5tk6b9as7M10/xb6aDfYLFOnzX4bAKmQsq7c?=
 =?iso-8859-1?Q?p214au/cEm/uyI5KxU/4evLkMzBwa+VrSi1wkYgCtLPNYAHPsiJNVizMn1?=
 =?iso-8859-1?Q?8mmMDOXbNTjcHuQk9twUYs9a5UUf6GA6oNMvGz4Be0cTgucTnha2Y1x+LU?=
 =?iso-8859-1?Q?/rJTj0PhP4HbjDA7reWghvDsCSRrQANB1cBBKx2TnSos8RqS1sgjNK2Rqn?=
 =?iso-8859-1?Q?SweU0cWQLLDTUvccs86XnG5lbTOhc33sXZzAqgSDqKbtsIUd2rffsKasZ3?=
 =?iso-8859-1?Q?QUTLP2GHFxfV1bpUo6T2m6Opfa2Eig0M8XOWP8WsrycoD0WrF4/nZcVPVo?=
 =?iso-8859-1?Q?4N5hM/I0o0hK0l/fNEFXrs/jAmau5BnKm6nnOIxrqeeIWUFpCKdxrKqwLx?=
 =?iso-8859-1?Q?nURMyg0WLIWvzkM6zPfyM1hXc878mAQ5M+HAPnIZTeijJjTPsw7oZVN2fT?=
 =?iso-8859-1?Q?PSySIpNv2olqFfiu2Xjz7yNsYZ2+o8Qs7Hx+BCC4RQTHMemCr2XqVQJhV8?=
 =?iso-8859-1?Q?NRnY49rLgpTsU+zkzsCJoa2g+JvY0WVBWAc1TP61X1WHG0tAgdhVwTa4Ev?=
 =?iso-8859-1?Q?8V2iQxtHbM6RuW37HSSBqfq6MtaEB7PFTmpRRtm26U9PTXqxqaGLojkh91?=
 =?iso-8859-1?Q?pxAppd3jUKrPrR/k9FMq07GLvj+oiFDFoOGGFmsry598GzOCcpgpkssyZG?=
 =?iso-8859-1?Q?a9DVWfifVsDbw3s8O8bH0FxzRCzNUbkp/TDXEyRXsH+T1Pn5vQ4fsdejgM?=
 =?iso-8859-1?Q?UwEu3jOcR86xOiCj8kRqHueDDd2daHW81CVCDF2YaKye8xeAaqSLd/2gBV?=
 =?iso-8859-1?Q?uf0gx+yEeL/Bgc3yCTfWk5cnEK/uQ55347TrdD/Yu/recz8GNiDjRkeNUp?=
 =?iso-8859-1?Q?mg9vDrsbWY1oM/697RQBftDL+x/N6tcNRm+MP1dRMJNtW7KrfVRTFh7lsP?=
 =?iso-8859-1?Q?pe2PWZL7JxEUo055gxKc3scZAJHl/TaPthFFY7ve1lscB2eewcxVK6zjXQ?=
 =?iso-8859-1?Q?IGlphoDGqVcQeDbQGQQ71VCefglXntM/tdO9Yd366LsxyesN99nSf6Eg3F?=
 =?iso-8859-1?Q?XSOha8WSu/VdqMyiLofYxAtBl8uABTfnXzN/UrnvVK567RJhkjwbwFnfgu?=
 =?iso-8859-1?Q?xf35scOnvy7hJeT4rrIqzCb840u1JbyljDZl5cXF6GVIHWdJWgMH/7C+pB?=
 =?iso-8859-1?Q?XcNXpBdZQcOcv7YziISM0yceeIMEq7MlePG6IOjnBp9tAzE2hxBh64y6H/?=
 =?iso-8859-1?Q?obPCcV5zRw0e+C2WbcqwOWLnu/drgI7xF1FIa+hVvOsuw4ATnB3lNkJksb?=
 =?iso-8859-1?Q?it1b72s0d1U/QpN7G4szmVXo9aqP2gCn9XPx3ExIckj2qnRXfMORrYQrAd?=
 =?iso-8859-1?Q?+m8typzn8vWbbFvDOzf6M1byL+n/HRpWWvY6b6eJ/BkPm6oaN6f15vhuz3?=
 =?iso-8859-1?Q?kRc4xQ+DFEKEcUuPxMs=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 70a3897c-c6c1-4d69-5ca8-08dc225be5bb
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6239.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 31 Jan 2024 12:55:28.3854
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: qM6CU8ksZRT+uqtxjYOg/tbajxceyQLawAOACzIbVaDeWZ4ExUKh8JFMjQP/Jc5eHGb7BtApoEk8A+LJnLw07sCZs3E+QtZjcABEmjGos84=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ2PR11MB7428
X-OriginatorOrg: intel.com

Hi!

On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
>Hi Maciej,
>
>On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
>> Add tests for both L2 and L3 CAT to verify the return values
>> generated by writing non-contiguous CBMs don't contradict the
>> reported non-contiguous support information.
>> 
>> Use a logical XOR to confirm return value of write_schemata() and
>> non-contiguous CBMs support information match.
>> 
>> Signed-off-by: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
>> ---
>> Changelog v3:
>> - Roll back __cpuid_count part. (Reinette)
>> - Update function name to read sparse_masks file.
>> - Roll back get_cache_level() changes.
>> - Add ksft_print_msg() to contiguous schemata write error handling
>>   (Reinette).
>> 
>> Changelog v2:
>> - Redo the patch message. (Ilpo)
>> - Tidy up __cpuid_count calls. (Ilpo)
>> - Remove redundant AND in noncont_mask calculations (Ilpo)
>> - Fix bit_center offset.
>> - Add newline before function return. (Ilpo)
>> - Group non-contiguous tests with CAT tests. (Ilpo)
>> - Use a helper for reading sparse_masks file. (Ilpo)
>> - Make get_cache_level() available in other source files. (Ilpo)
>> 
>>  tools/testing/selftests/resctrl/cat_test.c    | 81 +++++++++++++++++++
>>  tools/testing/selftests/resctrl/resctrl.h     |  2 +
>>  .../testing/selftests/resctrl/resctrl_tests.c |  2 +
>>  3 files changed, 85 insertions(+)
>> 
>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>> index 39fc9303b8e8..9086bf359072 100644
>> --- a/tools/testing/selftests/resctrl/cat_test.c
>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>> @@ -294,6 +294,71 @@ static int cat_run_test(const struct resctrl_test *test, const struct user_param
>>  	return ret;
>>  }
>>  
>> +static int noncont_cat_run_test(const struct resctrl_test *test,
>> +				const struct user_params *uparams)
>> +{
>> +	unsigned long full_cache_mask, cont_mask, noncont_mask;
>> +	unsigned int eax, ebx, ecx, edx, ret, sparse_masks;
>> +	char schemata[64];
>> +	int bit_center;
>> +
>> +	/* Check to compare sparse_masks content to CPUID output. */
>> +	ret = resource_info_unsigned_get(test->resource, "sparse_masks", &sparse_masks);
>> +	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");
>> +		return -1;
>
>If I understand correctly this falls into the "test failure" [1] category
>and should return 1? ...
>
>> +	}
>> +
>> +	/* Write checks initialization. */
>> +	ret = get_full_cbm(test->resource, &full_cache_mask);
>> +	if (ret < 0)
>> +		return ret;
>> +	bit_center = count_bits(full_cache_mask) / 2;
>> +	cont_mask = full_cache_mask >> bit_center;
>> +
>> +	/* Contiguous mask write check. */
>> +	snprintf(schemata, sizeof(schemata), "%lx", cont_mask);
>> +	ret = write_schemata("", schemata, uparams->cpu, test->resource);
>> +	if (ret) {
>> +		ksft_print_msg("Write of contiguous CBM failed\n");
>> +		return ret;
>
>... although here I think the goal to distinguish between test error and test failure
>falls apart since it is not possible to tell within the test if the failure is
>because of error in the test or if test failed.

Is there even a distinction between test error and failure in resctrl selftest?
I've been looking at it for a while and can't find any instances where
ksft_test_result_error() would be used. Everywhere I look it's either pass or
fail. By grep-ing over all selftests I found only five tests that use
ksft_test_result_error().

Furthermore there is this one "TODO" in kselftests.h:

	/* TODO: how does "error" differ from "fail" or "skip"? */

If you meant the distintion less literally then I'd say the sparse_masks
comparison to CPUID would be a failure. What I had in mind is that it tries to
validate a resctrl interface relevant to non-contiguous CBMs. If it fails
there is probably something wrong with the code concerning non-contiguous CBMs.
On the other hand writing contiguous CBMs shouldn't fail as far as the
non-contiguous CBMs in CAT test is concerned. So if that fails there might be
something wrong on a higher level and I'd say that can be more of an error than
a failure.

But I'm just saying how I undestood it so far. If there is some clear
distinction between error and failure definitions I could try to separate it
more explicitly.

>
>Reinette
>
>[1] https://lore.kernel.org/all/33787043-5823-6de4-4e5c-a24a136ba541@linux.intel.com/
>

-- 
Kind regards
Maciej Wiecz?r-Retman