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: <afa849bf-e89b-0c48-6bf2-f7ca58940567@intel.com>
Date:   Mon, 7 Nov 2022 15:53:58 -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 <tan.shaopeng@jp.fujitsu.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        Shuah Khan <shuah@kernel.org>
CC:     <linux-kernel@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>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20221101094341.3383073-6-tan.shaopeng@jp.fujitsu.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?ZVh5Ump6VFpkWTZXNWxRZ0ptN29LRjdtNDAwMy9rU01pMFlHTVltS1BDanA3?=
 =?utf-8?B?S1QwQ1cySGZITnJBRmtObGdJNnUvNFE1a1JpNzVsYWJjTUU2WEhQV0g5c1BP?=
 =?utf-8?B?dFVDK3czWHZzWTNvdEpvMnZZUzU5RmtIOUtGZFMxOTk3WUI3WjFBZXRUL2g2?=
 =?utf-8?B?VXNWU0dvMHBLVUtCOU5xY216alhFVUdBQWt2NDd3OFBGYzlVQ1NJaTgvRzVy?=
 =?utf-8?B?TDEyV1h1bEZ6aEpSYmxuN1NYek1xYWtMZkJhUHJ6RDlIOXJvTzFyZjJTTnU4?=
 =?utf-8?B?by9sOG53dTFwSjE1eU9QZDBuZ0Nic0FDT3BJMWFMZXVsL0RFTGczRWxSRyt4?=
 =?utf-8?B?WDQybVZDSEdhRFFHNjA5SCtxN0dPLzl1dURlcEk4dWw4WEZheDBOYzV5RTg3?=
 =?utf-8?B?eU5GU3gvaEM4QVVrenA4NTJUZmVSMXl3TURhZTFsSTJDVmkxa01GWEdOK09U?=
 =?utf-8?B?Ukt1MlJqUVdHTmkxSTlqcFZDNU5nVDExSzUvaEw4U0habm54aFRsMkxobDhU?=
 =?utf-8?B?ZW5MMjY2VlJrMkh3TjI4bzNmSmMxSkdhZVUwL1NXZXU2aUlIc3cyUHErQ296?=
 =?utf-8?B?RWY5K2xOMG5xSGtrcVJIK1huYy9YdTl3ZGt3TGxkbFFYcGVFNDN6N09HeUVJ?=
 =?utf-8?B?Vm9VKzZvYmVLVloxQXA5WFFNRzh2dWVFSW5BVzFyNDB1WGI3MU5sbzdENGVH?=
 =?utf-8?B?OHQ0OEVBOHR1TEs3bVltVmxTdDdYWTM4N2NiNFdFb0tuUkZqaURjSDUvU2d1?=
 =?utf-8?B?Wm11Wi9XVDFRLytrTU96Zk5KT084QksvTDZOQUhnV1JLam85SWxKZWJXNzhW?=
 =?utf-8?B?Z3dtVy81MmhsMUFVY3NWM2o2aXdWUUNyT1BZUTJhYWF5cDA4bUE4VktGOUxl?=
 =?utf-8?B?Rk9HeUt6R3dYSnI1SEdndk04UGpiTStpMjNwQWxrZUtoWmFBODBOWjgwNW9F?=
 =?utf-8?B?RTR2Vms0S0tJNnZiWTg1MFp6YmVoZmcydGo3MW13YjRMUmU3VmpNSWNGb2Zk?=
 =?utf-8?B?ZmZkb3NLQXFYd1VDOUhidG9tOFcvU3RGU1R3YmdlTkJ1Q1g0eEp6bTNrZHJa?=
 =?utf-8?B?WXBZMGxkTFNnWEYzRlRjakViSUpLK3dodHhZUkJhaEV4RjQ2eWFIbTlrQ0RD?=
 =?utf-8?B?dlIyYlYwemJPMTI5SnAvU3lyRUYxd3A5bVNGSlBxSTVxYzlLYUM4MzRWSm9p?=
 =?utf-8?B?bUY0NCtlcTBQRHhLZ1dPdHRCUkpObkZDeGEraDJodmFXNzIxN3JCTExHZStr?=
 =?utf-8?B?bmtvRExTQjhxejY5OVVtZEFKNVEvL0k4ejNBd042anBJaERURHdheXdxR1oy?=
 =?utf-8?B?N3BnbWQ5R0x3RWw1WDBxeUJvNGRTL0hHeENXbjYySjNDNXlnRGhucDk2QSs4?=
 =?utf-8?B?TEovVUo3TlZKbFdiVU5QTmVTNkE4bERORnVHWU1teXdocDFFQ1VmWnlwZ0Jy?=
 =?utf-8?B?N0xQVU1ZZnVKZ1dwUk9XR0d5cEhqWFhXMlIwWGJtQXlTT2ZYM1l0b3l3TlRh?=
 =?utf-8?B?aVRxcXZ6dGMwdi9rdnoxYVo4Rm5qTk5CNlZYSE9hYzdsRVdYam1CNk96emNt?=
 =?utf-8?B?T21icVNieHRqOGl6QTFGUzZVaXdxaTQwd0NvRmNOcWwyQk5HT3NuTjYrakpD?=
 =?utf-8?B?UXdpNi9DUU9NUUYzb2JTdWNyOXh1ZmxoZW9idm16LzR6YkQ2cThzeU1LRE1i?=
 =?utf-8?B?NjJ6UzRoUUFIR2hhN3F2RGl5dStlL2dIcnlsREpMcHZoRW5YWEtyaWRiQkkx?=
 =?utf-8?B?V01NVW5IRjVrdERObTdncTRQVTJrUUNQSnJMc0hqTXdIN1RKUnV4QURGMDkz?=
 =?utf-8?B?akM0dUlKYzUyMnFEWnNlL2dabFVrL21XYko1akpNZlZBbUsrVUFzeW1JT25J?=
 =?utf-8?B?K3FtRW9aUG5hQ0h1UitYc2w4cnhQMndnU0VZUk1KNkc3bEoxYTRSdkxjMUQ5?=
 =?utf-8?B?bWZNeC9tUFo1cC9ieU0wQlBiRHN5MktLdmt0REYwWXcvRnprQmJMRnVJM1NK?=
 =?utf-8?B?VUpYZzRHOWVPRENyRXJTKzh2bjR3VWNoUWVGWElkWDBySXlHQmY2WmZrc2hN?=
 =?utf-8?B?OUxvTHdtYU5EZDVBVkZ4ZzdzcDlISDNXeG9VUkFBbGM4em9MOHJBTktvR09p?=
 =?utf-8?B?Z3o2WHlUQXN2TUpGdHRmeWJxYUkrM0kvRTluWm5HUUlLYStZbmJrRmswUmd2?=
 =?utf-8?B?Q1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6c019f4c-5894-4685-1a35-08dac11b5763
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Nov 2022 23:54:00.2611
 (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: ZNpZMtFV1micExx2k8Utiz/v63Z+Mwc7WVcyWulPmTusdHFJdXqNgWLvcqqNlwLBsq3BEIhV5sIrCvKCc4CAh6sGafyaCFukMBz+1zrmQP4=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CH0PR11MB5299
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,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/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.
> 
> Signed-off-by: Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index df0d8d8526fc..8732cf736528 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -88,7 +88,6 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
>  	ksft_test_result(!res, "MBM: bw change\n");
>  	if ((get_vendor() == ARCH_INTEL) && res)
>  		ksft_print_msg("Intel MBM may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
> -	mbm_test_cleanup();
>  }
>  

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;		
}


>  static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
> @@ -107,7 +106,6 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
>  		sprintf(benchmark_cmd[1], "%d", span);
>  	res = mba_schemata_change(cpu_no, bw_report, benchmark_cmd);
>  	ksft_test_result(!res, "MBA: schemata change\n");
> -	mba_test_cleanup();
>  }

mba_schemata_change() has the same pattern as mbm_bw_change() so test result
files may linger when test fails.

>  
>  static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
> @@ -126,7 +124,6 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
>  	ksft_test_result(!res, "CMT: test\n");
>  	if ((get_vendor() == ARCH_INTEL) && res)
>  		ksft_print_msg("Intel CMT may be inaccurate when Sub-NUMA Clustering is enabled. Check BIOS configuration.\n");
> -	cmt_test_cleanup();
>  }

Same pattern again.

>  
>  static void run_cat_test(int cpu_no, int no_of_bits)
> @@ -142,7 +139,6 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  
>  	res = cat_perf_miss_val(cpu_no, no_of_bits, "L3");
>  	ksft_test_result(!res, "CAT: test\n");
> -	cat_test_cleanup();
>  }

Patch 4 makes this work. Thanks for doing that.

Reinette