Received-SPF: pass (google.com: domain of linux-kernel+bounces-61154-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Mon, 12 Feb 2024 08:38:10 +0100
From: Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>
To: Reinette Chatre <reinette.chatre@intel.com>
CC: <shuah@kernel.org>, <fenghua.yu@intel.com>,
	<linux-kselftest@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
	<ilpo.jarvinen@linux.intel.com>
Subject: Re: [PATCH v5 5/5] selftests/resctrl: Add non-contiguous CBMs CAT
 test
Message-ID: <gybtxxilnj3wychwz5cdzltwzhhazx2dvzkyk7r3afqnkilja3@e3btzsz6odc3>
References: <cover.1707487039.git.maciej.wieczor-retman@intel.com>
 <2c1011e7630605365b67caa6ddfe4e8ee2ba5bff.1707487039.git.maciej.wieczor-retman@intel.com>
 <b63354ab-458a-46a5-a80a-971845f1de9d@intel.com>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <b63354ab-458a-46a5-a80a-971845f1de9d@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?fvI1AOJWsaJ5vAt8uQpYVKeXghCuexq9fo8nKYwaEjCpu/sFqVLhtnIkJ6?=
 =?iso-8859-1?Q?FThDqeUGHSaUC5e6jLRzq4viZk9H61FcDsUwroL4x7XEB6VsD+Z7eZY4OR?=
 =?iso-8859-1?Q?+gIqF9Ec5D2s3VgDsHWPF2TvONqCoJsE9+Uiqsp5WBKPgDM0DYq3uykGZZ?=
 =?iso-8859-1?Q?8H0jBbKzpOv//EG39AwCMPFbnonYd04QuFE8MortfsCxWjZOpi2EFAmHER?=
 =?iso-8859-1?Q?a0b/L+js1DvVTRkVR6Lp0jierQmfoyaMcfbilFTg5i9vPsduO+EoApxzU+?=
 =?iso-8859-1?Q?WqdjAQwd9e+3cyixQrN5ziYmwhEEJ4aDdajjgObWiVZCRhcCOQYZb4yKUs?=
 =?iso-8859-1?Q?+QNtsIkA5580EyG+OJq7AYGpCrvH2i4CnPjR2frMi3SBK8NwzqAV3JFGF7?=
 =?iso-8859-1?Q?YbxaoxevoB0AjcqWXJ3IxSHPQYOS80Fe7DGFmQAnhFUcAolA8eUhEM5wbz?=
 =?iso-8859-1?Q?zBsWXAvv5VUMSyaNsJg0y3vrpdKlM9osrMgnOH7pRBUIugDokQ2I1IpYUK?=
 =?iso-8859-1?Q?5TinEap5dKBGlb1ZqfVph5MuXU6uMOUaSuvUqeSbt2gVZ0NV+R9AJz5VOW?=
 =?iso-8859-1?Q?VBUT0gQeLAcipsnj7JQ85Rq1Vz3j2EeiIXZdkEsFGj/f8ydaGPJlQAQ+SH?=
 =?iso-8859-1?Q?Fxdqk8zweNF/JWLEF7hlK27cwgBmVGueJ4IjhODg3QqyLazBrHNRkAGDB3?=
 =?iso-8859-1?Q?kwNQM03msjUKnKQKq5jkeBSz5L0R4nRdxD1rzICr+jM4dTwmtQnfX68ze0?=
 =?iso-8859-1?Q?yqHuxh8cAxqt94L7mzhflBv9Or/KRIlku2+/Uz/r6dHjyxCzHNqJ9clmc4?=
 =?iso-8859-1?Q?EADX/SdAKtyZJvbn5i67+BunpDlNKEL3Ic5qeFtQ/jLHu6sDxoGNPI6Vkq?=
 =?iso-8859-1?Q?CI5yRoQ94XpZjqvZZyCdiA/lMzA6PWf+F6rAHGajfgKsjbCfwdvmU+FG55?=
 =?iso-8859-1?Q?jDzZ9Q5kkeccUoo6+Tpd7waeeJvB+Do2xIFWhPo6SowhlGYU9l5wDPevuW?=
 =?iso-8859-1?Q?ufoDA6IPYHiM7tEBKDL8vGGKZgeJHyTwqGzemRa9ye53Gj/ulN7I9pDnVv?=
 =?iso-8859-1?Q?BgRNU3SIctBQwwbnMkOemCS/74qDlT8DXtxt3kse6iU0Jlqvk8vxqI6YUH?=
 =?iso-8859-1?Q?NdLMVJFneHHhD3Csx5wH5HsObJl1TR777s5xuOGFXdqAaV58bbQXcniYbb?=
 =?iso-8859-1?Q?5ov0nJXvoaXhFV0XcVO5B8BOosPPhiy63hEtvsm/vlysKbRcYW9uMrDh7w?=
 =?iso-8859-1?Q?rnNX1UYUTj+jcbkzrxg3za0pkpu8xFMEEPA9l/tAx03OVZusACr149J+Zt?=
 =?iso-8859-1?Q?xV96YUzZNLAcl21vEfqPgCJkSZuBuTGlGerzMq9ouX0mf33cm4CgC815SE?=
 =?iso-8859-1?Q?7x7/gKQyrDl0z8z6Il3pnwsgG9Hu/wi9c9bKHkcKFSuoE+yRt8vAzsG+/r?=
 =?iso-8859-1?Q?lRseyQCNhBz2d8anv5pK9xxdxsl9Sunp0M7Ink+luHEnf8dVoADQktckmf?=
 =?iso-8859-1?Q?w4+234D+HY50obG28ZHYJ9BYv4pZcHH+/heC4E3AarHRUGgkwkmeTGGjbn?=
 =?iso-8859-1?Q?KJnnOZol6hi8pXBaTUZfo6oIPj6rRkl0yz5MJtNA7QT/iMCdin/r/ex0I8?=
 =?iso-8859-1?Q?HmRjdAIcbIxeY4B+g+PmD4REAgGZHckVqusTIEoayQCxTgwT8Fh0FdNh5m?=
 =?iso-8859-1?Q?suHLo8AwtNyfrDoukmE=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4046db4a-d8b5-44ab-681f-08dc2b9d959e
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 12 Feb 2024 07:38:20.1786
 (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: 2/zINT5/fg0T/u28b1Sqc0IHK9rV5Dk8EN8+yP3FRwojt6v35SU5bPlJa0JRyhPq6sCxaGeAyre8XaAu5Y+3q1pa2UqiStdC+oyYm3MYCGI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7557
X-OriginatorOrg: intel.com

Hello!

On 2024-02-09 at 09:21:16 -0800, Reinette Chatre wrote:
>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.
>

Oh, sorry, I'll fix that.

>
>> +	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

Sure, I guess I could make the check 'if (bit_center < 3)' to also check if the
full_cache_mask isn't too short for some reason (since later 2 is substracted
from bit_center for the 'hole' bit shift).

Or would this need some comment as well (why exactly the '3' is there, maybe
write something about about the minimal full_cache_mask length for this test)?

>
>Reinette
>

-- 
Kind regards
Maciej Wiecz?r-Retman