Received-SPF: pass (google.com: domain of linux-kernel+bounces-59724-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <b63354ab-458a-46a5-a80a-971845f1de9d@intel.com>
Date: Fri, 9 Feb 2024 09:21:16 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 5/5] selftests/resctrl: Add non-contiguous CBMs CAT
 test
Content-Language: en-US
To: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>,
	<shuah@kernel.org>, <fenghua.yu@intel.com>
CC: <linux-kselftest@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<ilpo.jarvinen@linux.intel.com>
References: <cover.1707487039.git.maciej.wieczor-retman@intel.com>
 <2c1011e7630605365b67caa6ddfe4e8ee2ba5bff.1707487039.git.maciej.wieczor-retman@intel.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <2c1011e7630605365b67caa6ddfe4e8ee2ba5bff.1707487039.git.maciej.wieczor-retman@intel.com>
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?dVk2RXNzM3hlMzJSZm51UjFzRnJNV3hDVzJRbXFmaUxBVDl6dWxjSVN1UzM1?=
 =?utf-8?B?ZldGaU43bHNKR24yeFEvZFNXcDVISW5ZVEtrT1MxZFh1U2l3L0RETDBBeU9M?=
 =?utf-8?B?QVZSSVhaMGtYbWF5cEFIakRVRDVQK1VNSFQvWmloRXp6WjNBWGFucVNsTDNk?=
 =?utf-8?B?WkJ5OEZYU2xrNWZ3bDErcXNJdFhKTmZJbXFDRlRzaXlWODhkSXZvbWo0Mm9t?=
 =?utf-8?B?ekxyNGdFVks2TDJIUGw2V2hYanhrcXVHeEpLTGdMcTg5b0tWbkVla2hDSWoz?=
 =?utf-8?B?QS9OVm0waXplalhzMmRlbXZ3bVhadkgzTlpqb1J3cm5tcjk1Uk5rY1B2dGV0?=
 =?utf-8?B?Z2g0dy9aM1FkTVRiV05uaTBtZE8zSTRjWXRFazc3OC9WaUhweUhhOW1jNHVW?=
 =?utf-8?B?TWtRRDlYczJ2NVNUOWEvRTdnaGs1MzFuaXlla1J2RHpiMnNiQXZtSzdGNVlt?=
 =?utf-8?B?bTE0Vkh3UjRRaW15dlQxZUFXVkYyeXZ1NklBZldhY0F4eDZlUk1jZ2x3UWNl?=
 =?utf-8?B?alJMUGhxenRWNitMMVJ3c0tHcUFTYjFmYzFMdkRpL2RzNmZ3RWxSYmxHYisz?=
 =?utf-8?B?MDY3Nk5JLzBrTnVVbUxiWElnSWJaeGlqVjJwbHl0K2VlelhoWmQ1cU54MEky?=
 =?utf-8?B?TDlRc1VRKzc1Rko5aW8wUDNVMDdjT0FKckZYTTk1bUl2MGwxU2xwbGZsV3BL?=
 =?utf-8?B?cWd0LzFXNHI0eWtBdUZBbzh1OFpzak9ldDNtc1I1dHgzVkh0ZzZtaXBZWEZp?=
 =?utf-8?B?MUczYzdxSlNzTUpPeDIyTzJWckF1YWNISzd0UHYxNzA2WFE0c2tRM1RjcFFj?=
 =?utf-8?B?Z3JvM3FsYW5vcTZjU1R0WWNVZnlRSHYyQWk0cHpSNTBDcDZTVlRCbGlJU3Nl?=
 =?utf-8?B?U2VhMitBaHFwTU1FTjFpT3pLdVFOcStSdnhaeWFTTmxNNmJ0UVdnYzNqSDZa?=
 =?utf-8?B?SXpZSTRIZnRTSE5vbjdyYmlUYzdMNUpuOHd2eW1ORk1iTFc4aHo1bXhERUZM?=
 =?utf-8?B?SU9VVFU1NDdWN0h3dWlxZ285UDNsRWFiMDBnNElSeXlxa05uaXk4QURWUlg0?=
 =?utf-8?B?UCtDY2EwVVQ3NjIwSldtYXp0bmlFUE8zSlJQcEJDdW1qUVpOd2ZndzI5ZVZv?=
 =?utf-8?B?cGFyR2w2L1pkVjRnN0ZxMlpmckpva0h3NXpKV3pJaTZ2MzdkbXRMU0dwUVRJ?=
 =?utf-8?B?b1MyMkNzK2prRnp3YWJkb0EwQ1EzY1ozOGZ5L2lUVTZqTWNhUHhCUW95ZEtz?=
 =?utf-8?B?WUFVSDhERk1ac0VsdEFaMEFXK3VZOEtGa3Y0QmNqMzNiWS8rbVRDTlE2UUxP?=
 =?utf-8?B?WEdKalVoVHI2Uk52ZTFjL0tWNnlZTmFwbDU0aE1SbEg5LzQrZ0Faa3FpNW5m?=
 =?utf-8?B?bE9WdjZYWGoxWHNlL2FnSFY0MGZuQVhhZFdKQ25PZ2pQWllNMzNUOXRla2to?=
 =?utf-8?B?MmIxN2Q1dm00TTd5cFZZOU95ZjdUSXhLVWVCeTZCNEhmOFF1Y2FvLzRqVW9r?=
 =?utf-8?B?R1VSbHlaT016VTcxRDk5NDRXV29kS2JGSlpvMFBYcDAwSngyRFhKNVc4dTE4?=
 =?utf-8?B?ZG9XNmtLa2VXTUxNSWVsMk1zQzlJYWJSWkx0aTBOajdHNGZSWkxvSDQyREFH?=
 =?utf-8?B?ZnJaZytEeVI2dVpFaG1HZHI3WkZEcE1DUFlkMjlvYmxRRkRKVTNQRHNuTUht?=
 =?utf-8?B?TWowT3o5SElhWlNxYVBtM2UxRURzaDg5Q01NNC9od3E2UGpzZzA2SzJOK01r?=
 =?utf-8?B?YWVSZE8xb1JMQlRYY2JqRGpJMTVDVUdLSVBQN0hIa212Nk05U0d3YnVXazRZ?=
 =?utf-8?B?Q2pMMGVtYnRXL1hvMnRpc0REQUpRd3FiTDI5bzRUd0ZieUkrd0wzYU9LSnc4?=
 =?utf-8?B?b1Yxb2d4WWdGZEwzVXE2ZVhRWi9QTjMySENEOUp5b1pTVjhIcEMzWlkwZERj?=
 =?utf-8?B?Umh0dTJYWHZiTUZta2NxSmFiaFdkcDQ3UklDY3l6QStlcGlNRUp4eTM2bVhS?=
 =?utf-8?B?YS96b3BLYzBQR1hYUVNuZDZUNCt5RXJNWEM4UkJkMWpZcjJ6QmhudTYrZVZp?=
 =?utf-8?B?dEZhWTFZSkdteklLREFVTGE0L0xBVEdaSGNYZVlhTCs4ZjN4NHBXVEM3cDd5?=
 =?utf-8?B?djdnc1R5REE1MmY3SzJaNUIrTWRxeHljMVl0MHVqbGcxTFM0Skd2N3pKNWd1?=
 =?utf-8?B?WHc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: b79a2b69-bcf2-4793-bba3-08dc29938632
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Feb 2024 17:21:16.9316
 (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: K9UdLU/ayDUZElFTa9VvfpCaISIrNXNIxWEsk4GmyrIhTBqC3jOR4XwYVXWGhSRk0Bktb9c8HVHr/pmNP+SLBs7jeFJ2ff/IDV9VRdzsNcc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MN0PR11MB6157
X-OriginatorOrg: intel.com

Hi Maciej,

On 2/9/2024 6:02 AM, Maciej Wieczor-Retman wrote:

..

> diff --git a/tools/testing/selftests/resctrl/cat_test.c b/tools/testing/selftests/resctrl/cat_test.c
> index 39fc9303b8e8..d4b7bf8a6780 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;

I missed that "ret" is "unsigned int" while the test expects it to
be signed ("if (ret < 0)") and it is used to have return value of functions
that return < 0 on error.


> +	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;
> +	}
> +
> +	/* 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;

It would be nice if no new static check issues are introduced into the
resctrl selftests. I did a quick check and this is a problematic portion.
We know that the full_cache_mask cannot have zero bits but it is not
obvious to the checkers, thus thinking that bit_center may be zero
resulting in a bit shift of "-2" bits attempt later on. Could you please
add some error checking to ensure expected values to avoid extra noise from
checkers when this code lands upstream?

Thank you

Reinette