Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <0e8d382f-7cf9-7987-548e-2ff8e2f10e92@intel.com>
Date:   Tue, 8 Nov 2022 09:07:03 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.4.1
Subject: Re: [PATCH v3 5/5] selftests/resctrl: Remove duplicate codes that
 clear each test result file
Content-Language: en-US
To:     "Shaopeng Tan (Fujitsu)" <tan.shaopeng@fujitsu.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>
CC:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-kselftest@vger.kernel.org" <linux-kselftest@vger.kernel.org>
References: <20221101094341.3383073-1-tan.shaopeng@jp.fujitsu.com>
 <20221101094341.3383073-6-tan.shaopeng@jp.fujitsu.com>
 <afa849bf-e89b-0c48-6bf2-f7ca58940567@intel.com>
 <TYAPR01MB6330156C8B4F7A35290F44788B3F9@TYAPR01MB6330.jpnprd01.prod.outlook.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <TYAPR01MB6330156C8B4F7A35290F44788B3F9@TYAPR01MB6330.jpnprd01.prod.outlook.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aDErL1RaSHlacFdpbjRFSStTUEQya3p0c0xXRlFOK21YMEc4N3REbkhOY2VX?=
 =?utf-8?B?WG5TWHRvQkpac01HSWdIT2phNWx3UExieXk3UEJxUG1NWUYyaGdNdHhiTkxW?=
 =?utf-8?B?YWhCSy8xcXdnb3lvT3VYK0lST2RaTHlja2xZc09PVU12bVFFVnhNVlNKL1By?=
 =?utf-8?B?QU13N2dieW9iUmx4bkozM1RUQ3VSSk1ZaUtFV3c5Y2JOZEp0M3Fldi9HY2I1?=
 =?utf-8?B?dTJrYXY4T0pWN1VsbWJxdkZreXVxakNRTC9OOEJaYWdneUVIYk9ieURmdHpq?=
 =?utf-8?B?K1E5NHhsd0ZjczRiUS95Q2VZMmZlT25WZkxmTFlHZW1MVmlhakw3dE1BQzJr?=
 =?utf-8?B?MkM0YVAxYjJaWHZFTXF3ZktnSTNhcnVmR0M1NTcrbjVJTmNCMHlIWFMyK0hF?=
 =?utf-8?B?UFpMY016R0JqUkNyd1VJdGM0MzNHc29QeVdHd2pvbXp3T2pJblUvbXVHcE13?=
 =?utf-8?B?ZUxGczVqYUp3Z090dS9ZUHlicGp6NzNnLysvbVAzQkM5amFZOWpMMmdoZTVQ?=
 =?utf-8?B?VUhMQ2d1UVpxNlhXMlM1YVEyWXo1VFljSXZnV1gzSk5WbUYrVkVFZFRUSTNT?=
 =?utf-8?B?d1luUWtFbHdOSXBDMWcvOHJ2R2R2eUQ0WHkzNEp4aUNHbkRzUGJGdG5WNE5v?=
 =?utf-8?B?Tit3SGNmTzlpZSt1V0pZekVkYWlzTnAvZWdRQnM3cVdHVjUyMlFVcEY1TU41?=
 =?utf-8?B?NHdwZkU0SGVWK3NnM240dFBybGdxOVNieU9ha0FibkFLZlIwS1BFYU5jUUQw?=
 =?utf-8?B?aC80UjBQN3p3c3YybHQyOEdjZVE3SlZuWUpaamVKSFRGVUpwNVR1QmJLZnli?=
 =?utf-8?B?d04zN2VEbEl2bGYyTEsreUtkRElxenl2WXpMS05WTFVQVVBSQVhmVXRYSWcx?=
 =?utf-8?B?aFZOakEvRlFSZnBKbG5nME9BWGdvZDF3bVNra2tKeGMvQUxYckVpb0wrNko2?=
 =?utf-8?B?WnBYdDV6L0F0V2RYYWVJalRybGtlNlJDTmZGdlJqRGVDMWhMcjNESnVBYW9z?=
 =?utf-8?B?R0ZYcUdpRzQ4VFdhTC93VkZJcmFGQ0c1bWJFY05QcmE4aUdYbUkxZUR0eUx3?=
 =?utf-8?B?ZmU2NXJVTGVwMTViMGZ2UW9ST3plc1dpZzB3VWtwVVZCVk8wN1pGWTFHNUhr?=
 =?utf-8?B?UzdqUGI2RFg3NHBSY3V2eStqalY4b3RmK2NtOHQwV0lTVEtha3ViQzlhSmZq?=
 =?utf-8?B?WnNKYWpkMW9xcGFMUkM2LzJ5eWh1Z1lQcURZa1BNa2VrcTAyVTRMeUhsdC9h?=
 =?utf-8?B?eVdvZWY5b1FkN1Bvblo4VEhOS052K0dZUStOc1Vud3VhTEQ5M01GcTBWMmVH?=
 =?utf-8?B?U1E1SWtCSGNYb2hwdTZnZlFoZWVCdTBqOFBBMVQzTlduRmJzUzJQdHR5bDF3?=
 =?utf-8?B?UEwvYUVYNHVSeGNGaTc5a2ZnajBQRVk5MUM1OUFHWEdmaVpETU9EVGF5MkFQ?=
 =?utf-8?B?SWtHOWFJNGpyOGJpTXJ0RDdmbHRmS1hDbjRrLzRMV3ZRd3YrM3JSRUVwcVAx?=
 =?utf-8?B?VFk2ZGNteWZ5RHBhYWhLWjZUWlpST09Yb081VklYNjZUY2FPM05Wa2xrcGxD?=
 =?utf-8?B?eUhVejJMWEFFZE5xYng2aTF3Tk1aTFd3eHh6S0VVUVpDV0dNMHhxZ05WTHFM?=
 =?utf-8?B?RXdCU1hLbkNlQkIzb2tmTjMwdVdIWXI3NmM2aEQ1YWFhbzYyS0lRN2VuYU5a?=
 =?utf-8?B?ak9yRko0K2tEYzZRS2pQTTNPRXFVNVNmTDRQdCt2WUJvWFNjcG1Mc2pMSit3?=
 =?utf-8?B?MTB6S25wMTdiMURjM3RQc1V4SVZpZ3NQSjFCWTRMYjduNEtpblVPRkdsNmtQ?=
 =?utf-8?B?cjRlZkNNazJhNGMweHB2dVVxOXdaa1lWeWNsZ0VhUmQ3YjNpMUZsV0Z4VmZs?=
 =?utf-8?B?NVlCdHVZY0hEMVRCaXdJQUt1YmlEQURmb0ZucUpGLzNhUTNIclY4UzhVTFp1?=
 =?utf-8?B?NnQ4THdTNy9kMklvNndIMlVjWVpHQ2x5V3ZJTTFPdGxnOUMweXlncEEzVGRJ?=
 =?utf-8?B?SEVPUUdWRzk5azVmZjVYTW4wczlCSlpFTVF0bzFoTmdkS0FQa3RtSXJIcXFa?=
 =?utf-8?B?L3hta0U2MThGYS9uOGl6c3pRcm9VdzZtMW43L3dXM3lxS3lkZVY1TEZUNEJQ?=
 =?utf-8?B?RjNYdHNxNjNWcGFpOUo2ODdGd0FsdlRJR2s5bVhVeWFjNDJuWCtSa0FVMHBC?=
 =?utf-8?B?VVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 8844b219-16f8-4e01-235a-08dac1abaa2e
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Nov 2022 17:07:06.6940
 (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: mhuWR496ci7JqoMXJVzpg8oHih7VeisItsytVHpLUamMVvsAVzvASZ11iLxPPXW7+Z3a1lEYAqNXge/uzXgnTm6Z1OWgo5gIbpXPr5iPiy4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ1PR11MB6275
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Shaopeng,

On 11/8/2022 12:32 AM, Shaopeng Tan (Fujitsu) wrote:
> Hi Shuah and Reinette,
> 
>> On 11/1/2022 2:43 AM, Shaopeng Tan wrote:
>>> Before exiting each test function(run_cmt/cat/mbm/mba_test()),
>>> test results("ok","not ok") are printed by ksft_test_result() and then
>>> temporary result files are cleaned by function
>>> cmt/cat/mbm/mba_test_cleanup().
>>> However, before running ksft_test_result(), function
>>> cmt/cat/mbm/mba_test_cleanup() has been run in each test function as
>>> follows:
>>>    cmt_resctrl_val()
>>>    cat_perf_miss_val()
>>>    mba_schemata_change()
>>>    mbm_bw_change()
>>>
>>> Remove duplicate codes that clear each test result file.
>>
>> This isn't making much sense to me. Please include test report before and after
>> this change in the change log.
> 
> With or without this patch, there is no effect on the result message.
> These functions were executed twice, in brief, it runs as follows:
>  - cmt/cat/mbm/mba_test_cleanup()
>  - ksft_test_result()
>  - cmt/cat/mbm/mba_test_cleanup()
> So, I deleted once. 
> 
>> From what I can tell this still seem to suffer from the problem where the test
>> files may not be cleaned. With the removal of mbm_test_cleanup() the cleanup
>> is now expected to be done in mbm_bw_change().
>>
>> Note that:
>>
>> mbm_bw_change()
>> {
>> 	...
>>
>> 	ret = resctrl_val(benchmark_cmd, &param);
>> 	if (ret)
>> 		return ret;
>>
>> 	/* Test results stored in file */
>>
>> 	ret = check_results(span);
>> 	if (ret)
>> 		return ret; <== Return without cleaning test result file
>>
>> 	mbm_test_cleanup(); <== Test result file cleaned only when test
>> passed.
>>
>> 	return 0;
>> }
> 
> I intend to avoid this problem through the following codes.
> 
> mbm_bw_change()
> {
>         ret = resctrl_val(benchmark_cmd, &param);
>         if (ret)
> -               return ret;
> +               goto out;
> 
>         ret = check_results(span);
>         if (ret)
> -               return ret;
> +               goto out;
> 
> +out:
>         mbm_test_cleanup();
> 
> -       return 0;
> +       return ret;
> }
> 

Yes, even though file removal may now encounter ENOENT this
does seem the most robust route and the possible error is ok
since mbm_test_cleanup() does not check the return code.
Could you please replicate this pattern to the other functions
(mba_schemata_change() and cmt_resctrl_val()) also?

Reinette