Received-SPF: pass (google.com: domain of linux-kernel+bounces-35793-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: <1dd1c812-ecbb-4972-88a8-4ec7dd82ba8a@intel.com>
Date: Tue, 23 Jan 2024 09:42:07 -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?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: <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>
 <227e43a1-a11a-4d49-8dde-c8b65527e926@intel.com>
 <h7cbw6x7bxx5o7vyamhacgarx5fs5rgmyttawxkrl6fgtjjqmk@naisceux4lns>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <h7cbw6x7bxx5o7vyamhacgarx5fs5rgmyttawxkrl6fgtjjqmk@naisceux4lns>
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?T0dBSFdGaEVlcjBtMlpYSlZSMVZmblkySGZIa2prQlZvRjFKVVVvSlYzbHZZ?=
 =?utf-8?B?MTA0R0lhbEtDakxDbHRmL1JvT0J6cmEvekxxdFBabkJPSGpmZWhVRHk0MnAw?=
 =?utf-8?B?M3YzL3dXUE13OGNBMXB1UmZPeS9RRkVBbDU0cWVQbFMyMFF1RXZCWm5qSE9p?=
 =?utf-8?B?dHRvOFE1VGs2ZVpBbEx1VGgzRGNjamhXU0VIWWMzL1l4RGRicUxEQnFiK3Ri?=
 =?utf-8?B?dWZpL2VhOEpFNW5WbFUvMm1PVlJIR2xRcHNEY0U1VDhURWlxNmFGbVZSaitM?=
 =?utf-8?B?S3cyUzgzbVEyWEhSMmM0clhQNXA3ajlJd3F6SUV6RC9xYmx4d0MvV05RT3VN?=
 =?utf-8?B?Y3ZINnlRcTJiM3k2bTdpOGZYVFI3ck50Z0pUM3lEbUJaOGpmaXRxY1pDOEEz?=
 =?utf-8?B?dGU1bDdhNXprYUdXRmprakEzelJSTXQ0TUV3WFY1SmpPV25oNGg1VUFueVY1?=
 =?utf-8?B?MG40b3ZMb2JuUS95cnoxN0cvdUZSblFiamprZ1R0UVdsVFBMcmVqVUJTbUJl?=
 =?utf-8?B?WnBTbDBhK0tPV1d6eVpoSGtpUytkR3d2dmFBcU5hbDNvQ0RMR1hYOGdVRmZB?=
 =?utf-8?B?WGgwTkJva0tEcllMQmxUMEsrbmhRc0RiSkZGUERnOUNWeFpjcGU0YzRpWUcw?=
 =?utf-8?B?RmlneVF3TW5xTWYxYytLTlNxemEzRlc2TjhPMUdVdDlHemYrNVczMU9vZ0pG?=
 =?utf-8?B?Q0ZHOU1yRGlrOXBwcmZtUXFPWlZJdUJsRXpPQWpaekM0cytKejI2QW84STEw?=
 =?utf-8?B?azR3Q1hMWkhnM0lZcXRUUjJNbHkzeDcza0E2UzRCZFNSQkRtQk1mT3pjQzA2?=
 =?utf-8?B?ZDh6emR4TUZzcTByMkg2eGU2QURCS3lxNzA5Mk1ZY2NaU0crdWR5VG1iKzVw?=
 =?utf-8?B?RmkwNHFTQ0pLa1A0TWx3SUtSY3VhTnhFa1FoaXdkOEswN2dGMGxHVC9ObU9l?=
 =?utf-8?B?VXBvK0ZTVFp6Q3pHS3FsTk9wZ3BMSEFuZCtjZjRBUDEwNnpobVAwWXRiQU82?=
 =?utf-8?B?cG1KdVhpdmhqS1dwTExiWU1FMDN0UWw0Tld2TUs5ZlI3dWNid25PMEJpeUFY?=
 =?utf-8?B?QkxZR09XcGFLWS9iWFpSdS9YTmoycHJPczJQSDZKUmp3OTY1QVpaVzBxWVZ2?=
 =?utf-8?B?RjFFZm9POWd2UXJZYkxWRDNQMjdCZlAyZG9lMEJVTW1KeSs3eEZOaXV0K1Nt?=
 =?utf-8?B?YUJOR21QdjVjVlpBUGZ2SExZeC92TUNWb0tYbm85WXBNbHFUaCtvS05oQnFD?=
 =?utf-8?B?cUFmQzJpT3VxN3h3TjlRaC9rUzJIVGxZWXNrL1VFei9BTW1icVoxTzl2ZzhP?=
 =?utf-8?B?RGtSd2hmZ0c2Tm9obHZhV1NXeFNDR2xtZ1ZmcFdvdUtOb3R1dGJHY25kVHBL?=
 =?utf-8?B?bXlGWmtrZW9qUE16bHprb0RqNzRyVlBhVXN6TkdxM2xScmJvZFVQU1h6Z2Z3?=
 =?utf-8?B?ODV3UjZQSCtjN1FaYklBUlorQVlwUmRyL0dPTkpDUFQrZUxCMHMrY1l5UFp4?=
 =?utf-8?B?NlU0Ui9kbXpDb3JyakFJK20xT0VDL3M5WDUwaUUzUk5QUTcxQjNwc25NNFFB?=
 =?utf-8?B?OFlEZWJPczVWWkhKNHE1cEFjSWJHck5LTkp5N2RiM2hQaFJTRTJVdUt1dklE?=
 =?utf-8?B?YncyTWh1Y3hVVFBDeU0wSU5oR0pFcEVLNDlRMkFRNFoxeUlnR1V0ZFFzQjVh?=
 =?utf-8?B?MnUwT0FwRnJlSm9tZHFnZEtIcStldVdKNG5vMC9GVkg2eTFwVklKMVR0YllK?=
 =?utf-8?B?YXFzR09TQ3U0V0RHNDBHWWswbXd6RlZ0UFY2VThlSHBxdWYvNkMwVTdkdVdi?=
 =?utf-8?B?bDlwMTdkZzhsaWduWUZYd3VHUHZORjB3TkJjRUROUXNhZ2RWNWpyN25UK09z?=
 =?utf-8?B?dXo4Ky81Mmk1SVJzeko2SFNrZkkzY3l1RnZIRDR0dGtQTXVWb3hLcE9xMDFx?=
 =?utf-8?B?cGxQUVhNTlFTRStUWkNvbG1nU2VtamdUSTdvcnlQVURDM3h1L0ZyYWpWVU9p?=
 =?utf-8?B?Nm5hc2JQVXRJY2IxcGMrQTBjdkRFd0xYQWV2dks4ZTBPRzk3Z21WcEQ4MVND?=
 =?utf-8?B?d29ZdzBlSzJwZ1hQN05ZN0c4YVFVa3NTdmlQZDdHb3hnTmVDOW92cjYwUG9P?=
 =?utf-8?B?cEZJVW5wRXdUUmdaSk02cC9DY0NvQ1RlTURITVlybEhaYmxJSWY5bU9PWFZ3?=
 =?utf-8?B?Q2c9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3db3cf7b-9c23-4504-2555-08dc1c3a9ffb
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jan 2024 17:42:09.8995
 (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: 5VBb1WzNyyKd4i+cv7bdQ9+KNIRItehERqmS6idSCEXvoWRS7vtA8TcYXZ2NsZQZ9PbLSqg16nYO14vvNthGWW3cEQYOLabBmr2uTHUlDF0=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5244
X-OriginatorOrg: intel.com

Hi Maciej,

On 1/22/2024 11:58 PM, Maciej Wieczór-Retman wrote:
> On 2024-01-22 at 08:32:36 -0800, Reinette Chatre wrote:
>> 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.
>>>
>>>>> 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'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.
> 
> My point all along was that if write_schemata() fails it already prints out all
> the necessary information. I'd like to avoid adding redundant messages so please
> take a look at how it looks now:

Please consider that there may be different perspectives of "necessary information".

> I injected write_schemata() with an error so it will take a path as if write()
> failed with 'Permission denied' as a reason. Here is the output for L3
> non-contiguous CAT test:
> 
> 	[root@spr1 ~]# ./resctrl_tests -t L3_NONCONT_CAT
> 	TAP version 13
> 	# Pass: Check kernel supports resctrl filesystem
> 	# Pass: Check resctrl mountpoint "/sys/fs/resctrl" exists
> 	# resctrl filesystem not mounted
> 	# dmesg: [   18.579861] resctrl: L3 allocation detected
> 	# dmesg: [   18.590395] resctrl: L2 allocation detected
> 	# dmesg: [   18.595181] resctrl: MB allocation detected
> 	# dmesg: [   18.599963] resctrl: L3 monitoring detected
> 	1..1
> 	# Starting L3_NONCONT_CAT test ...
> 	# Mounting resctrl to "/sys/fs/resctrl"
> 	# Write schema "L3:0=ff" to resctrl FS # write() failed : Permission denied
> 	not ok 1 L3_NONCONT_CAT: test
> 	# Totals: pass:0 fail:1 xfail:0 xpass:0 skip:0 error:0

Understood.

> Of course if you still think adding a ksft_print_msg() there would be meaningful
> I'll try to write a sensible message. But I hope you can see what I meant when I
> wrote that the user could already easily see what failed.

I do still believe that it will be helpful if there is a ksft_print_msg() with
something like "Unable to write contiguous CBM" or "Write of contiguous CBM failed"
or ... ? 

Reinette