Received-SPF: pass (google.com: domain of linux-kernel+bounces-34857-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Tue, 23 Jan 2024 08:58:04 +0100
From: Maciej =?utf-8?Q?Wiecz=C3=B3r-Retman?= <maciej.wieczor-retman@intel.com>
To: Reinette Chatre <reinette.chatre@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>
Subject: Re: [PATCH v2 4/4] selftests/resctrl: Add non-contiguous CBMs CAT
 test
Message-ID: <h7cbw6x7bxx5o7vyamhacgarx5fs5rgmyttawxkrl6fgtjjqmk@naisceux4lns>
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>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <227e43a1-a11a-4d49-8dde-c8b65527e926@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?hXQ5pRcYplH7pL5mZ/tkaTkDNvFyT5z2JaWj2WZpys2Z4qgxtTAonlU7oc?=
 =?iso-8859-1?Q?N/Jkr/Ieu5J+3XZoOTqdqoOBhDa/zddlFKTItvwSFZbqiLIC1jYpsgeYLq?=
 =?iso-8859-1?Q?C7wyAktXNw9Ed7c435g/MCp0TodSUGpMOTeP5K2/PREczT2skPBJqBMUMv?=
 =?iso-8859-1?Q?RKvNMsggRZI6Pa3FdDH1W6Ny5pbzqI/udJJHel6lNA94tOFbR+R6KdGCGW?=
 =?iso-8859-1?Q?TEHj7pgclNLsclpGfE/bKOrFF2V9YBYpzecX1VNdR9cjE2lFOnsPAVva2E?=
 =?iso-8859-1?Q?clKNlg1UPWpM7j2ftfYv6NQD7e4IfGe4eMSk/j5zBN3F0yjLxCAq/+IOai?=
 =?iso-8859-1?Q?MY0Xymz4AGx71/cfbeNmtWCRVrVYnI1TfriHjMCiIdp2z7MP5rfgaXa/l9?=
 =?iso-8859-1?Q?kxnNOb4G3oho45RESlInqrqLgyGRi4tmn0nJiM27BgDZkho45wKYjNDfFC?=
 =?iso-8859-1?Q?3ksm6leysqwXxglOQWPGfNrq5/J69j5hm8Zh5CsjkEif/knm6Ywko+tMvH?=
 =?iso-8859-1?Q?lgaWcjOTSePI1LKfeiy8uNzLHavtXdpY4dgDOLPRtYumyNjvRzeZhTjYom?=
 =?iso-8859-1?Q?fvX0IsUPfq95qNfbhfmB/UtJEfVIrOBJACh/pP/EhAvBkUwcg0WCwZMK4e?=
 =?iso-8859-1?Q?PX9mA+YI8frd0L1U1USiziF2H29adaPSPJEIz4Qb0oONzHl5DuUnWGth3q?=
 =?iso-8859-1?Q?1E6ZNSzk/1I5/xesFHqLC9qaHQOI14+OQl1FqunU2vToIkRXMHXROzek2N?=
 =?iso-8859-1?Q?71aVJ+OkhQo6DMPl7BClH3hBTF4G6MMZn7x+sdUOx4vOV+PN17IQPaIK01?=
 =?iso-8859-1?Q?WjIiflWvx7c/TeVph1m0Ipholdb8vDcGY7g4aYFiLLx+qVRbijxwxVWAiF?=
 =?iso-8859-1?Q?Er0ADm9eG1pYql2waL3pYKHco9xzGdAEyCVPR1y0/LtHX9M/Nmp/RNr3Ka?=
 =?iso-8859-1?Q?nTkLLGAS9ugs0USR5gp907/SrsrEdEZ5VNYPA4DK5RcK4IsoAfJWEFgOoP?=
 =?iso-8859-1?Q?Ldtkyk+Mw+CZls2SBrgk2uM0asUyv9E2tpfuZL7VB7hIdPb2QQHtW1vSJl?=
 =?iso-8859-1?Q?ZQU2LxPK6eyrrlCaYSxW1BAyTmnqKAL0Or5a7e01Ey2k9+/mufxkYWB6up?=
 =?iso-8859-1?Q?ih7A2Rl4wB8PTq0qEQVsVjU7CZtMd0p8L5ZI+rsqv+dhkjmOoBDbwQ84ua?=
 =?iso-8859-1?Q?C7zg5rkepYfO97HES0lZXL4rBtnqC1AhV5OGj09xqFXCT6dMtho+uR1pd8?=
 =?iso-8859-1?Q?FHlaP0uH0AaPCSezJYV0qLIz87wBXjm41VkL8qtGGyHLQ9eQaEpMt3Ca8u?=
 =?iso-8859-1?Q?gWi0IC/agO8/kcGojBZIsHqSVWCD5TWdHQPo5cJJ1ZIINviLhu/T7Gr0QW?=
 =?iso-8859-1?Q?Ryft6GN6tcUmYw4EWuHwgZu0fktuFA4TMWY7rvXpmrKbn3slEt6rwcROJi?=
 =?iso-8859-1?Q?GV8JxYWRlD0NY3unjnjDoB5lKZApjsZKledNyNOL3bMC+ymTS9qkWHEtWZ?=
 =?iso-8859-1?Q?+t/MePhTZhbWqM6qibXx3Vk+0w7OdohUT5kgLQwuRP2ME05oD88kHYWRMg?=
 =?iso-8859-1?Q?azplF1aenMxLAKVJYPgYCr/gIS35LgaVHAo4LX03k5GAt6bsgYccgQ62pF?=
 =?iso-8859-1?Q?XAUEGetfxbsNTfT/04wrc8QS4T/LmA0Gl/HzQaD22v2Fy8NtlAuGmobHvX?=
 =?iso-8859-1?Q?SiW9kD+z667EVOoBfV8=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 78a6838a-44c5-4957-89c7-08dc1be90bc7
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Jan 2024 07:58:12.0653
 (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: pWstjzKrn5ZeliESkJg/Hg4jcULjVo5XHoPOlQAoPzKmCpJP/8jbPQj0hPFc2UHNzNInFPdTwO4MG1G5X92Nv/icKzjmhfWKwthtJbvsWXE=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7629
X-OriginatorOrg: intel.com

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:

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

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.

>
>Reinette
>

-- 
Kind regards
Maciej Wiecz?r-Retman