Message-ID: <h7cbw6x7bxx5o7vyamhacgarx5fs5rgmyttawxkrl6fgtjjqmk@naisceux4lns>
Date: Tue, 23 Jan 2024 08:58:04 +0100
From: Maciej Wieczór-Retman <maciej.wieczor-retman@...el.com>
To: Reinette Chatre <reinette.chatre@...el.com>
CC: Fenghua Yu <fenghua.yu@...el.com>, Shuah Khan <shuah@...nel.org>,
	<ilpo.jarvinen@...ux.intel.com>, <linux-kernel@...r.kernel.org>,
	<linux-kselftest@...r.kernel.org>
Subject: Re: [PATCH v2 4/4] selftests/resctrl: Add non-contiguous CBMs CAT
 test

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@...1 ~]# ./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

Powered by blists - more mailing lists