Received-SPF: pass (google.com: domain of linux-kernel+bounces-21155-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: <8f8056a0-b5db-459b-a479-39f6967c4e29@intel.com>
Date: Tue, 9 Jan 2024 09:17:42 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 4/4] selftests/resctrl: Add non-contiguous CBMs CAT
 test
Content-Language: en-US
To: =?UTF-8?Q?Ilpo_J=C3=A4rvinen?= <ilpo.jarvinen@linux.intel.com>
CC: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>, Fenghua Yu
	<fenghua.yu@intel.com>, Shuah Khan <shuah@kernel.org>, LKML
	<linux-kernel@vger.kernel.org>, <linux-kselftest@vger.kernel.org>
References: <cover.1702392177.git.maciej.wieczor-retman@intel.com>
 <10c3afd7f62c63db31a3d4af86529144a5d7bbf9.1702392177.git.maciej.wieczor-retman@intel.com>
 <fd6acf6a-2610-406b-b363-220121a45aee@intel.com>
 <33787043-5823-6de4-4e5c-a24a136ba541@linux.intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <33787043-5823-6de4-4e5c-a24a136ba541@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?MmpLcmR0VTRXZDBaaTlFUklDRE1zSnlncDhKcVZmRzdVQ1JqWEUvTUQwRjdD?=
 =?utf-8?B?WGRJOXU3ZFNPM2R2S0o5M1FESEJOa0F0TmVoNHdXQWhDcEdVL2N3aFRna2RI?=
 =?utf-8?B?S0tXeWpHbGZVNHdCaTFSYXE2MU4xMHZST2w5WnEvdGVqcjRyYzNreXFnTGg3?=
 =?utf-8?B?WXhTTmxvRUxiUERVK2xlT3pEakRHVjVjZ21JZmZObHVQdXplU1l4STMxTEQy?=
 =?utf-8?B?ZTE5UlltU0RJNzc2MnJDMXFtUWRkRXc3MFhNT05qNVlYajVxcExhY1orTWxJ?=
 =?utf-8?B?ci9hQ01PVDUwWlVoZys4eWpaVGprOHp5eDBOc1J0V2ZxTkt4OWpxcUVvTkJ2?=
 =?utf-8?B?MHp1bldWUnFob1dqK2hXa3owQjd2UFRTM1o1b0IrckYrY1NDVC9wbSszWVp5?=
 =?utf-8?B?NExnSzlPZXcrOWhhaHZHZG81ZGtldG9NbC90dHk2MW1GVENLK1BUajYwUGlL?=
 =?utf-8?B?R3FkV2h3cW9NZjhDaWoxb3dFTWZINk5LOHZuU1VFWHplcVh6TFFiSEtRZ01x?=
 =?utf-8?B?R1JqZDU2NXBobnllZDlBNEpaNXk2VW5uOEVIODRKblI5OVBTM0JUNmlRbi9l?=
 =?utf-8?B?bjlHb3NoN1NlS1V5MVJkd2RLdGwwamhaRmhMUkZXYnM3UWdqYzRiWS9nWDg3?=
 =?utf-8?B?RGFiK1luU1d0NTV3V0Q2UTBxQnNrZ3pNSnV6NzVJWUhhR0dpREc5SkI5THRX?=
 =?utf-8?B?MnFpN2ZLUVNacGdCc1owM3hab2ZCcDNqcXlLa0RRb2dKbjBudU81d3pwOSt4?=
 =?utf-8?B?OXlEUFpKeWhvNStnSWhLaEt6a2t2SWdsQWdCOXFzbzUzWmZvMm80MHhscG1J?=
 =?utf-8?B?ZFlhbk9iU1VpT3lMMVM2S0N1QzUrNGt3VWcxRzA3QlV1RGw4LzFKcXhFOWdu?=
 =?utf-8?B?MEZiQ3kvWmNadHhIN3E0eUpYSVBTY0tlYmNOeEp6RFVpSVpPYTFGOHA3VS82?=
 =?utf-8?B?cXlSMVJCSXYreGw0UlZnNXR3amtqTnpPWk13dmdhS1NlQXd0MnlhNDJWM09a?=
 =?utf-8?B?bWZsVWR5ZHAyRjhDamNqRXYxNnpLZjNrN1Q2ZnIxNDR1dmk5NGFBZ3poRkZJ?=
 =?utf-8?B?VTlPdTJKM2JKYlB2RDhtcDc4dzhmVXl5RTUzQnZsU0wwSWRBNlF0ZTQzQlFx?=
 =?utf-8?B?WFl2YWREYVc5REdUalJuQ2NaRkhQSDh4Z2JkTCs2cVhVY2p5R0c2SkhVeW9x?=
 =?utf-8?B?UE95cXQvUjVjSDB2RFFmeGpJQ1BlQUIxckt2MkFtd3pEa2x4MlczZmg2a0to?=
 =?utf-8?B?U2Z6a0NwNTQ3OVFtUnlHZ0Z0VGRkNUlLdlorbXNpdFM3MGJhYmNtazhHMnVv?=
 =?utf-8?B?RnJMNnVqNHJiTFdJMTVlbXNDb0VSa0s5eGJ5QlR2ekwwYTNSWUVWQm50RjJ2?=
 =?utf-8?B?QUM2ejN2Z1lrZmtyY1J4UXlOL1R3aVhzN2ljcUVDb1RJb1VNUkluNE9QRHVv?=
 =?utf-8?B?NWNPR1NadzFDbEs3d0MzMHRkbG9nUE5TODlQQjN4WDhnTFF0emxCQkpHcjls?=
 =?utf-8?B?djNuT2RWcUxxUTVDZm53cUt1dnJ0dlRpaSs4a2hKZjRtd2h4Q3lNbERWUEha?=
 =?utf-8?B?WTBsUnpXMzA0UE5YTmgzbWZKOFp3cW1aMmhsaHZzMGZVajlmcThzd1hlWjBj?=
 =?utf-8?B?QUs5UlRSalJjd0QzOTBKeG5FbVN3T01QTWNIUG43N2o4c0M1V2huNktLVnRp?=
 =?utf-8?B?bHJOTXVVMm0wdHdsclNjTERBcWd4dW1Cd21iY3YyQ0RRU1pmY0VPWTh3d3J0?=
 =?utf-8?B?eHhuZmprRlFTemVDQkZ4cjdBa1dVRE40OVYrOHdFekxXZzFzYS9kU29kL1Ew?=
 =?utf-8?B?eEExaVYzd3pMMnJrUkR0L0FQNFpyUDBpUVhMell1RHljNERXUW1MRjBYTEJs?=
 =?utf-8?B?Z2g4RzRkYXI1Z1Q4a0VVREpEWmlYOU15V3MzemdBZHRJWUFaSFZoQXhXWndv?=
 =?utf-8?B?TXF3bFpucUtNaHRnNXNuSDVQZ29LOGJEeTBVMWZuVTJnV2x1a1ZZNFRlZ1Bv?=
 =?utf-8?B?ZUNqaGUrb29wazJjRURWc2lIOXpPK2hHM1haRFRKcTF4dnpmcXFaSnVVejRR?=
 =?utf-8?B?MjhzU2xsVFQyK0VWbHpDRVVMVzVqNnA2K1FrbEhMaVFjUXhLVW1EbE05K1Ns?=
 =?utf-8?B?a0ZkWi9rZXZ2anAxMUxtejNKSnFDOUxwVnVTc2htV0Nhbk5Jb0Joa3UwbDVt?=
 =?utf-8?B?MUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6f9a0cbd-5300-4140-0293-08dc1136e46e
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Jan 2024 17:17:44.0118
 (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: XuXMU5G8puCCXWmZ1pu6FLn8eBFVZbQdhAoCq8JDW1rB9tU0irxrCyp2NLWyM+zhXT0CMXypgU3OyQcdCzpMyCiN6qshhrEmlA0HqQFDvss=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR11MB5881
X-OriginatorOrg: intel.com

Hi Ilpo,

On 1/9/2024 1:13 AM, Ilpo Järvinen wrote:
> On Mon, 8 Jan 2024, Reinette Chatre wrote:
> 
>> Hi Maciej,
>>
>> On 12/12/2023 6:52 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 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    | 75 +++++++++++++++++++
>>>  tools/testing/selftests/resctrl/resctrl.h     |  3 +
>>>  .../testing/selftests/resctrl/resctrl_tests.c |  2 +
>>>  tools/testing/selftests/resctrl/resctrlfs.c   |  2 +-
>>>  4 files changed, 81 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
>>> index 7dc7206b3b99..ecf553a89aae 100644
>>> --- a/tools/testing/selftests/resctrl/cat_test.c
>>> +++ b/tools/testing/selftests/resctrl/cat_test.c
>>> @@ -292,6 +292,65 @@ 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;
>>> +	int level, bit_center, sparse_masks;
>>> +	char schemata[64];
>>> +
>>> +	/* Check to compare sparse_masks content to cpuid output. */
>>
>> "cpuid" -> "CPUID" (to note it is an instruction)
>>
>>> +	sparse_masks = read_info_res_file(test->resource, "sparse_masks");
>>> +	if (sparse_masks < 0)
>>> +		return sparse_masks;
>>> +
>>> +	level = get_cache_level(test->resource);
>>> +	if (level < 0)
>>> +		return -EINVAL;
>>> +	__cpuid_count(0x10, 4 - level, eax, ebx, ecx, edx);
>>
>> Please do not invent relationships. Please replace the "4 - level" with
>> specific index used that depends on particular cache. The cache level
>> may not even be needed, just use the resource to determine the correct
>> index.
> 
> This is actually my fault, I suggested Maciej could use arithmetics there.

No problem. The math works for the current values but there is no such
relationship. If hypothetically a new cache level needs to be supported
then this computation cannot be relied upon to continue to be correct.

>>> +	return !ret == !sparse_masks;
>>
>> Please return negative on error. Ilpo just did a big cleanup to address this.
> 
> Test failure is not same as an error. So tests should return negative for 
> errors which prevent even running test at all, and 0/1 for test 
> success/fail.
> 

Thanks for catching this. I missed this subtlety in the framework.

Reinette