Message-ID: <jkpv2zrgtudixtah3d7byy7lk5gcngdprae764xxregkbx3lil@xximqpsphax3>
Date: Thu, 25 Jan 2024 08:12:52 +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

Hi Reinette!

On 2024-01-23 at 09:42:07 -0800, Reinette Chatre wrote:
>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".

Oh of course. By that I meant the failed schemata which I assumed is what you
were looking for in this error handling here.

>
>> 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
>
>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 ... ? 

Sure, I can see how that can be helpful, I'll add "Write of contiguous CBM
failed", thanks!

>
>Reinette
>

-- 
Kind regards
Maciej Wieczór-Retman

Powered by blists - more mailing lists