Received-SPF: pass (google.com: domain of linux-kernel+bounces-50217-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Message-ID: <bb3ec879-074a-4b5c-bc54-5ca4398f0c0b@intel.com>
Date: Fri, 2 Feb 2024 09:10:25 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v3 5/5] selftests/resctrl: Add non-contiguous CBMs CAT
 test
Content-Language: en-US
To: Maciej Wieczor-Retman <maciej.wieczor-retman@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>
References: <cover.1706180726.git.maciej.wieczor-retman@intel.com>
 <647fbfd449f8b0e0ad6cfe58bb280ff44ee162b8.1706180726.git.maciej.wieczor-retman@intel.com>
 <db08ba56-ae73-4c70-87fb-aae59e524238@intel.com>
 <jbgkiwfkotntcdzhwf27dceit4w3j37sumkayixmyuee6zyaqg@un22a2iljgmr>
 <6a7b14a7-966d-4953-9b9b-0c847c83ddbb@intel.com>
 <icdfnbyc7kvt7llxc5qq5i7icmuev5us3i4pqapyreqfmkkztk@iqsthvhf2uem>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <icdfnbyc7kvt7llxc5qq5i7icmuev5us3i4pqapyreqfmkkztk@iqsthvhf2uem>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?TXhBc0twWG92K3RrbCttUTNiL25RRlpmYXZKekJUb09pZUQ2dEVvcTJzMUVW?=
 =?utf-8?B?YWxrZTZocHpheE5QNzR1cjFMRjZ3bnRQUjQ1R3FqbkpSdG92d2dvelZINHVI?=
 =?utf-8?B?c0hpWHVwNWZJNVgzTk1IYUpQaWxxRWVEWVFSdmtIaUdya292dm5Vc2MyUUJ3?=
 =?utf-8?B?S0pmNTZXWDQ2SWs1bTJ0Qm1hMHBnMlRGTUwrVmpsMUVoY0hWZ2dTWWNFRDJv?=
 =?utf-8?B?VzVGaEN6a29kVCt5RzZnOWhxQUVxUzRnZHVxempnYldQUW9ncjQ0OHUrdDNR?=
 =?utf-8?B?K0dlbm4xYndHdnFwdFBxdzJkdFJUdHQya1ZyWWMxemRnNy9GYjhlQzgzdlgz?=
 =?utf-8?B?dkM0U094c0pzL3J4Zkc2N3JUeTU4TVFaZ3Z6Z0RGZ25FR2lZcHpJVlhwYkk0?=
 =?utf-8?B?UDZhRWlMcnBTSVd4cnpmL3gzQ3JSWHpCNHY3Tk5KY2xuZHBlVUpHRis1MkE4?=
 =?utf-8?B?YW1lVnFOWm0yMkdHMnJIUDhiV3BxL05QMXFGQzZwUFZxbXMxTklnQjVDMDU2?=
 =?utf-8?B?U3VucFd6UXlKVVlnenhSVkhGbXJTS2Vvank0c2l3TXZiZEMyZzdjbUVkUmQv?=
 =?utf-8?B?ekNKT2pEaS9GOHJ2VDBIcERPeXpsemNBTmNQS1IxZzdRek9PWG15LzR5ZmtG?=
 =?utf-8?B?ZnFMejJ0WEViUjJLYlU5K2dMam1ESWN6QzYra0FkN1JxMXZaemhPK2VDMEFQ?=
 =?utf-8?B?SEx3STVwcTB5cDR3Q21JZ3U2bDlxVktSdFVZUVFDTDR5S0pkbUZZcWVzeTRZ?=
 =?utf-8?B?eURTUjBCdFBKcFhiUmowSlhxcFd3Q1BHSWNEYmMybUh5QTl6M2VJbnlWUHZm?=
 =?utf-8?B?djArVXROSU41NzBuQW91RzJZRTFUU1ZGUW0wY3dVYWNQeFVBM1NwK2JpMVo0?=
 =?utf-8?B?L3ZJR3ZkYmh5YUhKOVhqS1RwUGNTaFlkUzdVQXRIMnFiek5wUlIrZWE4bXJ6?=
 =?utf-8?B?Mk9BZHdadTR0S2NicVp6Y3RNZ1ZaUFdOa2pKYUthUExOaGx2eEJFbzc3enlY?=
 =?utf-8?B?N0kzS0tFb01pVVBrQlFSUjJ0RmEwdC9yajEvSVhGaWlLQVR1cmM1ZWlKZnNK?=
 =?utf-8?B?WXlXVjlzOU43OGgzV0hZaVdabkZIZzZBNTExYXNidVFDTmMrNWg2T2kxUmpY?=
 =?utf-8?B?ZmZNR21XSmZTbkkwZXdkbkpSYjJqOWl0UlV4UjlCbGUrd0lCUlAwR1hzYktN?=
 =?utf-8?B?eWpkZ2ppbC82WEY5N3ZCSlJCMFcrcG5ReElKdysyb3U5R2NkemVvMXk0ZGhl?=
 =?utf-8?B?MExabFZkR084SjFPWlk1b1FvU0dNS09kM3N3M2ZkVXU0ZkdiUVc0eHhSdWVG?=
 =?utf-8?B?SC9iVG1vY29tR2JXcGg3T3NOYnVyc1IxTEtFczZUdWQzWThXd3ZPL0hZSXJC?=
 =?utf-8?B?YTVsMUJVaE9mMDdWeCtsOG9pOTdIZ2JmMHBYZXdxNEV2TGRsS1hoaE82b2ow?=
 =?utf-8?B?V3JwQzgrenhOTEE0ZEJIZzdRUUdobkRCSmdxN3NXTFArcjFsQ2tlejFFTXEy?=
 =?utf-8?B?Vys4RlIxNmV6dGtBYVNlKy93MFgvL0FZT2lsbVJMQkdKS0FHSVdwQVBVWVZz?=
 =?utf-8?B?R0xNd2pVYmVUa2VONlowc2duY1c2anZTbC80d3FuaHFqMElkaG5NOFBnZDFu?=
 =?utf-8?B?bExQNlBES0ZydjdSMFVTc1U3dVJsamdEQjZwenFWSzlsY0c3UTZFQnVuL1o4?=
 =?utf-8?B?VytjczNjRldVUVZnWlFHTnBrcU9jQmR4OVkyVXFRRXFMT282cWlVY1hOT1Z1?=
 =?utf-8?B?Nk1QejExSnRFVTEzbzBWYTJMOU1aM0dFMnBwTTcvUFpDeTZ6OGV5M1NaOUFr?=
 =?utf-8?B?MzhWRWI2Y2tXa1RXRWQ1NFRNTkZZOExUL2djclhJaGZWYVR0Z05xaDBESzZS?=
 =?utf-8?B?ZnFQdEE1Sk9uaU0razRWMEdvZE5WK09BczFlTWlWTVZyR3FVellEMm13eWYr?=
 =?utf-8?B?TnpUc0N0N05WVFV1VzZvcUw3eDltZllxSldJMWIzYTREM0huK3laS1hPREg1?=
 =?utf-8?B?OWFPNXdJaS9FUHlvcU1XU3lleTV5ZEdBVXZnc1JsVVJhNnNYazI4U2wzMldJ?=
 =?utf-8?B?OENFbktCMjdHWWE5czUzc29rSjlZM3ppbGJOWnNwZXdCb3Fjckttdk1kVjRL?=
 =?utf-8?B?TTAwcGxtSGdPdWpudUFIRmppdlJoQnNVM1NiZnMvelBoWHJqdmpiTTlEZ3l2?=
 =?utf-8?B?SVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f446dbfe-b876-4fcd-aff6-08dc2411da28
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Feb 2024 17:10:27.4229
 (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: B5Q6jemsd+5ntzygC+As7FfYAPsPLYwN+9aN+fH2xzQc3crBupvJkiIiW2utMnWFQXZ9T98USyhWWsSly40x27Bo1Glgg2NHQE3yB5HpjO4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7551
X-OriginatorOrg: intel.com

Hi Maciej,

On 2/2/2024 2:17 AM, Maciej Wieczor-Retman wrote:
> On 2024-02-01 at 11:47:44 -0800, Reinette Chatre wrote:
>> Hi Maciej,
>>
>> On 1/31/2024 4:55 AM, Maciej Wieczor-Retman wrote:
>>> On 2024-01-26 at 13:10:18 -0800, Reinette Chatre wrote:
>>>> On 1/25/2024 3:13 AM, Maciej Wieczor-Retman wrote:
>>>>> +	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?
>>
>> There is such a distinction in the current tests (and from what I understand the reason
>> behind the logical XOR used in this test) . In existing tests the running of
>> the test precedes and is clearly separate from determining of the test pass/fail.
>> All the current tests have a clear "run the test" phase where data is collected to
>> a file, followed by an analysis (aka "check results") phase that looks at collected
>> data to determine if the test passes or fails.
>> Note how all the "check results" return either 0 or 1 to indicate test pass
>> or fail respectively. Specifically, you can refer to:
>> mbm_test.c->check_results()
>> mba_test.c->check_results()
>> cmt_test.c->check_results()
>> cat_test.c->check_results()
>>
>>> 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().
>>
>> Yes, from the user perspective there is no such distinction. This seems to
>> be entirely internal to the resctrl selftests (but I do not think that this
>> should or can be a hard requirement).
> 
> Okay, thank you, that's what I wanted to know.
> 
>>
>>>
>>> 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.
>>
>> Wrong with which code? As I understand this particular check compares the
>> resctrl view of the world to the hardware realities. If this check fails
>> then I do not think this is an issue with the test code (which would make it a test
>> error) but instead a resctrl bug and thus a test failure.
> 
> I also meant a resctrl bug. I was thinking about the kernel resctrl code that
> handles taking the CPUID information about non-contiguous CBMs and putting it in
> the sparse_masks file.
> 
> If there was a hardware problem and CPUID returned wrong information, then the
> check wouldn't fail as sparse_masks relies on CPUID too and both values would
> match. So in view of this I thought that this check could make sure that the
> resctrl kernel code handles CPUID returned information properly.
> 
> So should this check be moved from the "run the test" phase to the end of the
> function ("check results" phase) to signify that it's not an error but a
> failure?

I do not think this test matches the "run" and "check" phases of previous tests,
unless you create a new test for every scenario checked within this test.

Just returning 1 when the check (if (sparse_masks != ((ecx >> 3) & 1))) fails
should be ok, no?

>>> 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.
>>
>> I think that the write_schemata() can fail for a variety of reasons, some may
>> indicate an issue with the test while some may indicate an issue with resctrl.
>> It is not possible for the caller of write_schemata() to distinguish.
>>
>>> 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.
>>
>> I do not think it is possible to clearly distinguish between error and failure.
>> These are already lumped together as a ksft_test_result_fail() anyway so no
>> risk of confusion to folks just running the tests.
>> I think the final test result may be confusing to folks parsing the
>> resctrl selftest internals:
>>
>> 	run_single_test()
>> 	{
>> 		...
>> 		ret = test->run_test(test, uparams);
>> 		ksft_test_result(!ret, "%s: test\n", test->name);
>> 		...
>> 	}
>>
>> above means that a test returning negative or greater than zero value is
>> considered a test failure and resctrl tests may return either in the case of
>> an actual test failure ... but from user perspective there is no difference
>> so I do not think it is an issue, just lack of consistency in the resctrl
>> test internals in cases like write_schemata() failure where a possible
>> test fail is captured as a test error. 
>>
>> I do not think it is required to be strict here. Keeping "test returns
>> negative or greater than zero on test failure" seems reasonable to me.
> 
> Okay, so the approach I applied in noncont_cat_run_test() with write_schemata()
> is acceptable?

In general I'd say a write_schemata() failure's return code will be acceptable,
but you should be consistent in this test. There are two write_schemata()
calls in this test, one treats an error return as a failure and the other treats
an error return as an error. Considering this inconsistency I would thus rather
suggest that you always treat write_schemata() error return as a test failure.

Reinette