Received-SPF: pass (google.com: domain of linux-kernel+bounces-32473-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 22 Jan 2024 08:56:12 +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: <m3bhu3j3jzdo67mmuezjmwu4elyqjdwmoixcxm7qthls5tueqk@h7bvwxmy3l4y>
References: <cover.1702392177.git.maciej.wieczor-retman@intel.com>
 <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>
Content-Type: text/plain; charset="iso-8859-1"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <92bccaea-b73f-42e0-a386-1a73eb6f88de@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?SU3+/aHHGoc/eaxdkZJkWPiVJWbIBsGuj9amTp6RjyFkW/UEfCbfUmtyuE?=
 =?iso-8859-1?Q?CRfGPZtkwveAFHKnzqFyV0kp485ov7Src5zIcevJFK647mw6H/ZWlTv4sJ?=
 =?iso-8859-1?Q?ckF/k58cO3fCanlSo01PtpOjCFzhKDhG27H2h/sdC0l7ONpqPSZiYgvbTU?=
 =?iso-8859-1?Q?1UVrgoPOXPog0/l/alHLXKn6gU/+3Vt+UanYYXpkJgTNmF6MNfZZaFPWDs?=
 =?iso-8859-1?Q?EHKDBAsLnOikmfk3bTZcea83ZxfBnrGCf3z+ibjI7+2NxRk3rV0W/EW9hj?=
 =?iso-8859-1?Q?77PcR+xqRep0FXVBbDsNsHdFS6AdPYPSVocZvpQAJ6dLCk7zTR2IPPtk/p?=
 =?iso-8859-1?Q?eYLme3XMe8l0yPLydMMVGDEYhJ+F7DgbttuexbQRh/R0AsFm7iKx00As0u?=
 =?iso-8859-1?Q?OWCGlOjswRlbsIpYbROgfPj5pLjoqJrZCv53QQx+lrMHZWStuAG4GQbBoc?=
 =?iso-8859-1?Q?Y9FxerxgfRN4c4guQMa99YxqYqenp2iVZ8M58QZrgtII2pyZcdwlf66qHI?=
 =?iso-8859-1?Q?Umf/UETQPrMMIAquKm5ChtgYWU5PdKeoMM8SwaRRu1tHo1pYjEOrWkMM3G?=
 =?iso-8859-1?Q?JShgyIfjb3e498ZM4icvWWl7tLC/q6n52eF4oplvN9IVfknR3uEf+dtcYf?=
 =?iso-8859-1?Q?GLBiaN0WvHA5A/fx/MMZxFOdNml7QKEjOZI5+Om+T7GyHW/frIEJGrf87E?=
 =?iso-8859-1?Q?X8KKoETZMwYvgi7boIkyF+DfF2pby7f5W/yK1/50UvLG98eH4b4WSor/Ek?=
 =?iso-8859-1?Q?l9j67Lhm+W4KIl/g+v2uSIYPQfVGugQPZPhc9ekp0iTTYHKOaXYykqieUO?=
 =?iso-8859-1?Q?FfWTJK0MJ109dWoKEcoU4ldOzxiStHCDLEFUWTmyuwrtiV0dMzaM096ndO?=
 =?iso-8859-1?Q?fdpwDVbpWbKET24wfXCZVZUM/9+uWfWqSRMjvDuEL8E35RE0ZAe+xKwy0M?=
 =?iso-8859-1?Q?jyyQcTePwMFQxwGiZ6DNwCIM/6GdjvrbUf0WA0A/b60XonbSXMEsHRiU0f?=
 =?iso-8859-1?Q?CL1d467GP3YQjwoKbqXg4ybsn7kyTup6ceX2jyDDZws68bC/pGRpPvmUE2?=
 =?iso-8859-1?Q?hGUmO6P6Fr3nfRE5aBh72vhHrjJL73hnfYSjH+xDkiggaB3fUKtgUxCt0J?=
 =?iso-8859-1?Q?BbKZpBse5aovLqpckx34YGV8LIejr49/oI1LLr5FpYzzbARUr8Ham001gU?=
 =?iso-8859-1?Q?n3WZJf590QBOmA/cu7CfvRjk0u5Nq5IgCGujGHdJkWhHawKxFPNyy89jgK?=
 =?iso-8859-1?Q?zabxS9+D6V3TUkMEonFNDRUYn6AT+cWVVHe5RsZQke2tjv4AdeFJlW5m+F?=
 =?iso-8859-1?Q?iYNXiUbamfXa/oVQBl9fjNmsF+0tqS6jK8nZhMWsBjh8xgzHsEL1mEKAmr?=
 =?iso-8859-1?Q?pwFmEnOTDXKi487f0HNA6fDLetaY731y9cjJQ9Nhj6LAcnJOw0F4WTCr6N?=
 =?iso-8859-1?Q?lC+MFmEJek0Y2bWCF4daGHfO+SwQ5jwNZNg/kO/j+9SI0SVWyowSfzYWSs?=
 =?iso-8859-1?Q?9BgbXNbHahKNQPuynYR4d2BOSHm361SJ656UWh80ADES5kLCL3ghRIIES6?=
 =?iso-8859-1?Q?QrPBRmXn2NDd547OKin8jnIfE5Bad7X5F13tqvGetpAS3gNrxHbsUhorwK?=
 =?iso-8859-1?Q?HmSuS2JsDFuh4xfSBbWn8ubUzHJOJKKgkfYKI3fFmfw0WnRD1liiKEJn9i?=
 =?iso-8859-1?Q?H53cw8kSwsDmQjGKj18=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 7d177d02-ed78-4254-76af-08dc1b1f9ecc
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6231.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Jan 2024 07:56:20.4426
 (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: 8OXcAA94uUra9T8oZwETc93mBf3855IP3brD8xhWT7/+OTV9Wr1/FFJjAaz1lcFpigtKiGigvSJDuM5RAx4cY04uK23ChlJpg8nHO+vIwHQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL3PR11MB6385
X-OriginatorOrg: intel.com

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.
>> 
>> Okay, your comment made me wonder if on error the schemata still is printed. I
>> double checked in the code and whether write_schemata() fails or not it has a
>> goto path where before returning it will print out the schema. So I believe that
>> satisfies your (a) condition.
>
>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(..., schemata, ...);
>	...
>	write_schemata(..., schemata, ...);
>	...
>
>Any failure of write_schemata() in scenario 1 will be easy to trace. As you
>state, write_schemata() prints the schemata attempted and it will thus be
>easy to look at the code to see which write_schemata() call failed since it
>is obvious from the code which schemata was attempted.
>A failure of one of the write_schemata() in scenario 2 will not be as easy
>to trace since the user first needs to determine what the value of "schemata"
>is at each call and that may depend on the platform, bit shifting done in test,
>and state of system state at time of test.

Doing things similar to scenario 1 would be great from a debugging perspective
but since the masks can have different sizes putting literals there seems
impossible.

Maybe the code could be improved by putting an example CBM in the comment above
a write_schemata() call? "For a 12 bit maximum CBM value, the contiguous
schemata will look like '0x3f'" and "For a 12 bit maximum CBM value, the
non-contiguous schemata will look like '0xf0f'"

This seems like the closest I could get to what you're
showing in scenario 1 (which I assume would be the best).

>> 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 believe my code follows the first scenario example (since one schemata is half
the full CBM, and the other one is the full CBM with a hole in the middle).

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?

>
>Reinette

-- 
Kind regards
Maciej Wiecz?r-Retman