Re: [PATCH v2 4/4] selftests/resctrl: Add non-contiguous CBMs CAT test

[Reinette Chatre]

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