Received-SPF: pass (google.com: domain of linux-kernel+bounces-33702-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <227e43a1-a11a-4d49-8dde-c8b65527e926@intel.com>
Date: Mon, 22 Jan 2024 08:32:36 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2 4/4] selftests/resctrl: Add non-contiguous CBMs CAT
 test
To: =?UTF-8?Q?Maciej_Wiecz=C3=B3r-Retman?= <maciej.wieczor-retman@intel.com>
CC: Fenghua Yu <fenghua.yu@intel.com>, Shuah Khan <shuah@kernel.org>,
	<ilpo.jarvinen@linux.intel.com>, <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>
 <6v4hgra7bd5nwubcgtbsidmoohrgxdw7no4faa4a54g4vrxo63@c6qnujvwn5ej>
 <73cea732-3757-4aec-a39c-4f0e50f6fb70@intel.com>
 <su2d6i22icynusoykmowfnzery6hlqugovaw4xcrhb7r3ymbx2@aeimvrxsc5xw>
 <05db3bdc-9d54-44d2-8aae-07d66053892e@intel.com>
 <wvehk3j2dcf3lkuantb7shefymxj2mnzv4pgbiqbk4jkmmzeid@lurk3dam5tb6>
 <92bccaea-b73f-42e0-a386-1a73eb6f88de@intel.com>
 <m3bhu3j3jzdo67mmuezjmwu4elyqjdwmoixcxm7qthls5tueqk@h7bvwxmy3l4y>
Content-Language: en-US
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <m3bhu3j3jzdo67mmuezjmwu4elyqjdwmoixcxm7qthls5tueqk@h7bvwxmy3l4y>
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?bVdUVDBETjdqNmNUc3pjVWpJblRSZWNtZEFwUUozQ01PWG9GN2k4bnNZWXZx?=
 =?utf-8?B?cGppSHJuRzBsMTNCbWx2OGVCYnJzZHRXUTl5YzBtcVpzSWRUb2o2ZWV5bWRG?=
 =?utf-8?B?SjFtY1VEVkF3ZmlYYUJzVWNDNVhqWGFQejVJSXpZYnVpR1FIaElEWUV4T3JU?=
 =?utf-8?B?aHY4SXFuVEFrRzhrK3RURWNJT2w1cFZOejBEbkkvL2ZpdVp6NGNxc1ZZSDVn?=
 =?utf-8?B?NUkxVEpyRzNub2REcjhmNldmV2dka2VNdkNlR25YMGlNQ3NXeWhoUUNXODUy?=
 =?utf-8?B?STVPb3BZTTV2MzNPUGUzRU9ZWFArK3NlR2VCdG0vRTllZ0Q0ZWxZcFpXWDdT?=
 =?utf-8?B?dTN2M1dVSzI2NjluRFZGVlhsYnlTdHJDa2RXQWtEaktsdjNRaDBSdkllSXRa?=
 =?utf-8?B?NmU0eEVydXpocjRJdEZMcjhFOXdxdjQwSy9qV3hCVEViVFY0YVdldDFuOEpr?=
 =?utf-8?B?UEE5a3hVR1NySDZNSUlEcnB6cnozVHM3WWZjV3dzUXRzbkp1Q01ETUFYckN3?=
 =?utf-8?B?UHNjaHlrZzFia3BhZm5Ua01JR2RXb2Y4RmtoRzlvKzJPajc2blpTSHg4UnFX?=
 =?utf-8?B?ejNRQ3h3dm4zT05hT0VNVjNzYlNMYXEwQ01OazNzaDRoR2ZNYUhjaHkwaWZU?=
 =?utf-8?B?V3hKTE9tY2RCdEhNWGlWcFZhMFBBWmlpUTMxeG4rVjlmL3p5bEF5cXdrZjJk?=
 =?utf-8?B?RllwMlhnUTh3dWNHMWpqd1NxaXV6VTRLWU5rRXhIcEdYZWJtYzkzbThHMzdT?=
 =?utf-8?B?OUVlamg1SERZM3FMMHd4YTZET0tKTGZRQUE3WnJQN2loR0l2L2Z4U2NXNWJt?=
 =?utf-8?B?YWJHYzV4QUl6VjY2ZXgzRThnU2VjNm5WK1NWZ3NTM1dPdExPbVNMdXNWcEZE?=
 =?utf-8?B?R3NHM3YrMVZEOWI3N3Bxc2xpUDE4YkRTb1AwZ0pmaHFQbWY5enUzSGhzVDNJ?=
 =?utf-8?B?Z1htbHhPRGF6ak5qS0xwY0p2bitVY3dPUlcvcHZuMjBWWldOYmZtRGxQSlRE?=
 =?utf-8?B?eldmVU1qUWFEUStMM2ZHZGllN0prN2FraDF6eUdBZGVJSm1aUUUzT0hsL2xz?=
 =?utf-8?B?RHNUdUllN0JBeEI1VUZrczUxUkVLbFpWWHBMNVg3bzVQL3BET1hGeGtZRHdu?=
 =?utf-8?B?MkF6VVB0MW5uQlNBeU9PaG0wdkNkdlQ5SmdHRDFOa2p5WFBJYWFDZDFwai9j?=
 =?utf-8?B?REdkZUdId01wN0djS1hCcDBSa284NEJVbDRSUDNMb2xINXdyckN2c0hIb0dL?=
 =?utf-8?B?YUY3QUE1WjR6Z3J4OFFXNVVPRzFWaUFvcGVxNk9ieVBVcWROdkQzOW5EcDBt?=
 =?utf-8?B?WTNWaVVrUWFiYnI1SWNiUlg0NW4zd2JKeW01YmEwVXBqT1lvU3RRdG9tVXdv?=
 =?utf-8?B?VDRDT3hJZ3ZBWis5aDQ1TkxodkhpQndHQjRiYlRQeTBxYUVRek5PMFA4ZjdC?=
 =?utf-8?B?L1NHazNKTnAwcFRNQkpRa0taaTBJLys5ekMyNVpUU0MvVDc3YUtwWnVUb3ZW?=
 =?utf-8?B?RHBtZWNObXFiVUpMMEZhRDNSUVlMbVNadGtHL3VGL1p6a3c0NTdlekhSZXo4?=
 =?utf-8?B?ZFhnZDlsSUJYVnpLWWtCa1Z2OVg4V0VxaGpsSVpMVjJkbVZQVlFLQjNGK1Ey?=
 =?utf-8?B?U0R4MmpsSGhRVTd4NTQyUDhpTjdCNGZsNkQ2bXJNYWVnZ0tFOUVzL3UxQkpF?=
 =?utf-8?B?UXJhZ3EvakMxNWlFVEowS0I0T3JRUGlOUkZkUjZjSG5hbFNwbjFNQkdSSEF2?=
 =?utf-8?B?NkZ1SXBvbzlVeWxRcC9rNDBXNzhjOWpGR1FFNnFzZFBobjhxSDIzSnJCcTk2?=
 =?utf-8?B?M21sc3lyWEhOYkZtbDNkWUl1dVVhQityaEU5RGFIcjYxcmFjdWtoUG1VY2ZC?=
 =?utf-8?B?Ull4NjNPWGxlaE9qN0hYZG5GYjFUZ1BDaUNWMHJLRUNiWjZ0L0N1VjZQZzhn?=
 =?utf-8?B?VVBWTVVVeGJOTmY3cTU2dzd0Q1FWMGVZV2I4NDgxeVU5SXA1VUhKQnB0QjRZ?=
 =?utf-8?B?Sk1pcURlcC9KMm41Qk40L2FjbEVqUjV2b3B3bUFwcXUzVUZkTkttSEROVS8x?=
 =?utf-8?B?dnBRRnFHR2FCV1E0eWVYeGMxK01xZkp5OXRGWkhmSGRZWEM5MVh2Rk9YalQz?=
 =?utf-8?B?VTdOWDh4ZU93R2NSb3kzZ1JkRFhrMmFrUWZLdlB5MmZZZ3ZVTFZibS8zbTJt?=
 =?utf-8?B?VEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8ae46a96-0f4c-4b43-0984-08dc1b67bf28
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jan 2024 16:32:38.4462
 (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: 2ln2xD4NhmoR9pj8EU2wR81T73eEk7nPZkoYeWd9mghv+CwIbOjRIBZQN7ixNdORMnLfQk/ZxHtXtFaIUuRBV00Fj2dg6KpqsxVBMvcMdS4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7382
X-OriginatorOrg: intel.com

Hi Maciej,

On 1/21/2024 11:56 PM, Maciej Wieczór-Retman wrote:
> Hi!
> 
> On 2024-01-19 at 08:39:31 -0800, Reinette Chatre wrote:
>> Hi Maciej,
>>
>> On 1/18/2024 11:37 PM, Maciej Wieczór-Retman wrote:
>>> On 2024-01-18 at 09:15:46 -0800, Reinette Chatre wrote:
>>>> On 1/18/2024 4:02 AM, Maciej Wieczór-Retman wrote:
>>>>> On 2024-01-17 at 10:49:06 -0800, Reinette Chatre wrote:
>>>>>> On 1/17/2024 12:26 AM, Maciej Wieczór-Retman wrote:
>>>>>>> On 2024-01-08 at 14:42:11 -0800, Reinette Chatre wrote:
>>>>>>>> On 12/12/2023 6:52 AM, Maciej Wieczor-Retman wrote:
>>>>
>>>>>>>>> +	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)
>>>>>>>>> +		return ret;
>>>>>>>>
>>>>>>>> How will user know what failed? I am seeing this single test exercise a few scenarios
>>>>>>>> and it is not obvious to me if the issue will be clear if this test,
>>>>>>>> noncont_cat_run_test(), fails.
>>>>>>>
>>>>>>> write_schemata() either succeeds with '0' or errors out with a negative value. If
>>>>>>> the contiguous mask write fails, write_schemata should print out what was wrong
>>>>>>> and I believe that the test will report an error rather than failure.
>>>>>>
>>>>>> Right. I am trying to understand whether the user will be able to decipher what failed
>>>>>> in case there is an error. Seems like in this case the user is expected to look at the
>>>>>> source code of the test to understand what the test was trying to do at the time it
>>>>>> encountered the failure. In this case user may be "lucky" that this test only has
>>>>>> one write_schemata() call _not_ followed by a ksft_print_msg() so user can use that
>>>>>> reasoning to figure out which write_schemata() failed to further dig what test was
>>>>>> trying to do. 
>>>>>
>>>>> When a write_schemata() is executed the string that is being written gets
>>>>> printed. If there are multiple calls in a single tests and one fails I'd imagine
>>>>> it would be easy for the user to figure out which one failed.
>>>>
>>>> It would be easy for the user the figure out if (a) it is obvious to the user
>>>> what schema a particular write_schema() call attempted to write and (b) all the
>>>> write_schema() calls attempt to write different schema.
>>>
>>> Okay, your comment made me wonder if on error the schemata still is printed. I
>>> double checked in the code and whether write_schemata() fails or not it has a
>>> goto path where before returning it will print out the schema. So I believe that
>>> satisfies your (a) condition.
>>
>> Let me try with an example.
>> Scenario 1:
>> The test has the following code:
>> 	...
>> 	write_schemata(..., "0xfff", ...);
>> 	...
>> 	write_schemata(..., "0xf0f", ...);
>> 	...
>>
>> Scenario 2:
>> The test has the following code:
>> 	...
>> 	write_schemata(..., schemata, ...);
>> 	...
>> 	write_schemata(..., schemata, ...);
>> 	...
>>
>> Any failure of write_schemata() in scenario 1 will be easy to trace. As you
>> state, write_schemata() prints the schemata attempted and it will thus be
>> easy to look at the code to see which write_schemata() call failed since it
>> is obvious from the code which schemata was attempted.
>> A failure of one of the write_schemata() in scenario 2 will not be as easy
>> to trace since the user first needs to determine what the value of "schemata"
>> is at each call and that may depend on the platform, bit shifting done in test,
>> and state of system state at time of test.
> 
> Doing things similar to scenario 1 would be great from a debugging perspective
> but since the masks can have different sizes putting literals there seems
> impossible.
> 
> Maybe the code could be improved by putting an example CBM in the comment above
> a write_schemata() call? "For a 12 bit maximum CBM value, the contiguous
> schemata will look like '0x3f'" and "For a 12 bit maximum CBM value, the
> non-contiguous schemata will look like '0xf0f'"
> 
> This seems like the closest I could get to what you're
> showing in scenario 1 (which I assume would be the best).

I am not asking you to use literals. I am trying to demonstrate that the only way
it would be obvious to the user where a failure is is when the test uses literals.
I continue to try to motivate for clear indication to user/developer what failed
when this test failed ... this could just be a ksft_print_msg() when the
write_schemata() call we are talking about fails.

> 
>>> As for (b) depends on what you meant. Other tests that run more than one
>>> write_schemata() use different ones every time (CAT, MBM, MBA). Do you suggest
>>> that the non-contiguous test should attempt more schematas? For example shift
>>> the bit hole from one side to the other? I assumed one CBM with a centered bit
>>> hole would be enough to check if non-contiguous CBM feature works properly and
>>> more CBMs would be redundant.
>>
>> Let me try with an example.
>> Scenario 1:
>> The test has the following code:
>> 	...
>> 	write_schemata(..., "0xfff", ...);
>> 	...
>> 	write_schemata(..., "0xf0f", ...);
>> 	...
>>
>> Scenario 2:
>> The test has the following code:
>> 	...
>> 	write_schemata(..., "0xfff", ...);
>> 	...
>> 	write_schemata(..., "0xfff", ...);
>> 	...
>>
>> A failure of either write_schemata() in scenario 1 will be easy to trace since 
>> the schemata attempted is different in each case. The schemata printed by the
>> write_schemata() error message can thus easily be connected to the specific
>> write_schemata() call.
>> A failure of either write_schemata() in scenario 2 is not so obvious since they
>> both attempted the same schemata so the error message printed by write_schemata()
>> could belong to either. 
> 
> I believe my code follows the first scenario example (since one schemata is half
> the full CBM, and the other one is the full CBM with a hole in the middle).

I know. This thread digressed into discussion about when it would be ok to
omit error message from caller of write_schemata().

> I'm sorry to drag this thread out but I want to be sure if I'm right or are you
> suggesting something and I missed it?

Please just add a ksft_print_msg() to noncont_cat_run_test() when this
write_schemata() fails.

Reinette