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: <74cab34d-767c-aa10-807d-3cbab7907ca9@intel.com>
Date:   Fri, 21 Apr 2023 17:09:10 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.10.0
Subject: Re: [PATCH v2 03/24] selftests/resctrl: Move resctrl FS mount/umount
 to higher level
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        <linux-kselftest@vger.kernel.org>,
        Fenghua Yu <fenghua.yu@intel.com>,
        "Shuah Khan" <shuah@kernel.org>,
        Sai Praneeth Prakhya <sai.praneeth.prakhya@intel.com>,
        Babu Moger <babu.moger@amd.com>, <linux-kernel@vger.kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
References: <20230418114506.46788-1-ilpo.jarvinen@linux.intel.com>
 <20230418114506.46788-4-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230418114506.46788-4-ilpo.jarvinen@linux.intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?dVdBSzdBQ1JVbE9PQkY1Y0hTbGRxZGVGVXdaby80OGlMTEdUY1pwWXRhbUtk?=
 =?utf-8?B?aEl4V2NkTmtOR3ZqNEJzL1hxbXZZTWRwYUF1NGNMNXFQYkEyMTZCd2hhbUps?=
 =?utf-8?B?Z2dha0RROTM4WDB2Wm9WeDhQU1JpODF2ZFlJWkpHbW1TeU1jczhaYzZMQ2FP?=
 =?utf-8?B?ZVhjZDFPekpFcGIrSWxOc3BnWDhxMHpESFJMYnc2UXNtSlBNNTgyZGxpdHVq?=
 =?utf-8?B?Z2M2S0ZCUmlPaDEwNm4vYjBLWEFMc1p1TW52MENWWXRXRTMxM0taam1YaWxE?=
 =?utf-8?B?Vk5JYTNaOG5ITXVzeVRjdEVoblR6RkVnUXdHeFJkeGFXTFIvelVRNnlwKzBG?=
 =?utf-8?B?QTJ5MUpqR2ttYzk0RVVVRTJDTkJzd1FkcHlSWDhUU2JHK0IzZmQ3UEljQnRo?=
 =?utf-8?B?VGpLQmFSeCt4TitkdURkWHUydWplUWhTajhCbDh4THdFZk1XWHFIQzR4M0hC?=
 =?utf-8?B?THJibS9TVXZGUE82TVdtRlN2ajJPdkRsb1drYTZRVFJCRlRwSlhlMUczcWlm?=
 =?utf-8?B?a3hBRytTQ09oZmJaSUg3dmViUzVjbHEyQjQxYUpsQ1J6K3d6N0o5NHNCVEV2?=
 =?utf-8?B?ckRVaU9HcUE4dG0zUmxiSWlwU1VqUXdLakgrSnRGY2IxZzZHTTJoeEFmelJX?=
 =?utf-8?B?ck9PcE5BMmZjRzRqQmpHZmo0cDlseDFPQWNQKzI3RlJCQXNyaTlnRTBNT2Rn?=
 =?utf-8?B?aC9tWTZFSE1yMUdsM0s4azVkcTh6dE4zbU1QdkR0L256Uy8xU2h3c0wxNnlr?=
 =?utf-8?B?Q0FZQTRQc2d0eWlVdmZkMGZaVzV5TWtYMllkbVArVGhxQVBjZTBsWis0NUFM?=
 =?utf-8?B?SFZsMlFxNmNXM3d3NGpLWDVxblJ0cEdjWEdRaHdubzRtRlgybTRTRXUzYUhG?=
 =?utf-8?B?eW5TRzZpM21zZ3VWbFVSS2h6dUd0VDBOZmhHR1dLQzkvNFBGdW5RVkdOZU1u?=
 =?utf-8?B?RGx1VDZkeFhRT2MxenVTbVpjRGpuNXY1b1hjT3lwNnhkMlJyL3pNeHcvd3J4?=
 =?utf-8?B?eXpQdzhURVgzTDl5RzBLL1lBMHBHNDlMR0daOHVRQW1WbXhtcEQvQTVIYi8v?=
 =?utf-8?B?VmNnZHN0SDBwdm44cU41ZHVNbC95T2FFZG9MT0ZLcC9QM0VkZEw1QWZBeCtp?=
 =?utf-8?B?M2FtaU92eksvSDkzckErZ2NaMzVFcmFsR013NXRTTXkwb2RIa0VVUHFjUmRS?=
 =?utf-8?B?Yk1KdzgyVmdUQ0V1RmxhMDJPYjlZSFVteENOdlQ2aDlXN2REYS9zaEl3TWgx?=
 =?utf-8?B?MjNrMVJoSVVSbm5yOXd6U3BpWjlySmRoYk9mVm5KMWZKTEY0QjNYU2VraXhy?=
 =?utf-8?B?YkVnaWpzYlQrcW0yK2lDY0k3cmhNcjN3ODBmUEVOM0J3UHhHclVPSUlRRisx?=
 =?utf-8?B?bVB1UlZISFQyVFBRMVA3ZUVvdFo4Q2J1YWpEVEJqdCtVT0FuRVZGTFlZVnZw?=
 =?utf-8?B?OUpoMytPcjVTdEk5bnBPOG9HUm1LRmxIOHZtNkVFRUQvR3Y4dTJUb21Xc1Fx?=
 =?utf-8?B?M0ZSSm1RRWswcURkcSs0cDEzNVNNMWY2Zi9aWHgrNytiS1dMMFF1TWRLZHFi?=
 =?utf-8?B?WVU3RkorUTB0UTNiUXdmRmdLRVV3T253RlNNOFIyVW1rSFBXdWQrTTk2T2JZ?=
 =?utf-8?B?bnZBQVZUK3JyN2l1cGYvK3RVVVlYSWNFOUQ0bWVUREN5TzBWZlIyaUh0YW1K?=
 =?utf-8?B?M1ZqVjdOdGNKUFhKM1l2NU11WXRrL3hQaEk2N2RWdFBEdThKbzV6a21VTGFJ?=
 =?utf-8?B?aFdHK21YaHVtV3hNYzNZSWUvUktvVzVFeU1vellpcURSSHIxZkVHRzdUVm9v?=
 =?utf-8?B?L2M5cG5DNC9pQ1FuSS9TdUtsOFBZNnVSREdObWdhcnh5UURzeFBDRzcyYy9i?=
 =?utf-8?B?WDFNVkh4cm9XZDlncGNOdHpVZW5BSzJLaU10U0xtVUpWMHdKa25LWlZUR2gz?=
 =?utf-8?B?cGVUUWZ4c1RHNHNsbXlTY3VzTW0yWkdoVlZ6T0lVVEhaUWJNVk9ZMGo3T2V3?=
 =?utf-8?B?R1NDNGx3TmJuUjhsK2t5V2dNN2dXM3h6bXZBRFpjMjVyRUx3bmJEU1NLUVFj?=
 =?utf-8?B?b2gvdnJaYWJrakptWjFaQmxXckVXZU9jM0xDZ25SbTEwVGtVMmRzVG1iRk1W?=
 =?utf-8?B?MzJoZExRWENyOHJUSDhmSDZWdjIvbzNxT3BhQ05QemI0RmppanR3ME9mdXJR?=
 =?utf-8?B?aUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fd2b219c-a6c1-49b0-ac53-08db42c5cd3e
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 22 Apr 2023 00:09:12.4340
 (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: 9pZ6R/ntvwzme9TduB/bU2qFxmUJC9di/XVgprGZlh3mrif0anLAueusNQa3ace4VoJUyPV3c9Toah7X1wvvLWlgzKm4zRPJW2CCz3GiBWY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5937
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.7 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_H2,SPF_HELO_NONE,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED 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 Ilpo,

On 4/18/2023 4:44 AM, Ilpo Järvinen wrote:
> A few places currently lack umounting resctrl FS on error paths.

You mention "A few places" (plural). In the patch I do see that
cmt_resctrl_val() is missing an unmount. Where are the other places?

> Each and every test does require resctrl FS to be present already for
> feature check. Thus, it makes sense to just mount it on higher level in
> resctrl_tests.c.
> 
> Move resctrl FS mount/unmount into each test function in
> resctrl_tests.c. Make feature validation to simply check that resctrl
> FS is mounted.
> 

...

> diff --git a/tools/testing/selftests/resctrl/cmt_test.c b/tools/testing/selftests/resctrl/cmt_test.c
> index af71b2141271..426d11189a99 100644
> --- a/tools/testing/selftests/resctrl/cmt_test.c
> +++ b/tools/testing/selftests/resctrl/cmt_test.c
> @@ -86,10 +86,6 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
>  
>  	cache_size = 0;
>  
> -	ret = remount_resctrlfs(true);
> -	if (ret)
> -		return ret;
> -
>  	if (!validate_resctrl_feature_request(CMT_STR))
>  		return -1;
>  
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 9b9751206e1c..5c9ed52b69f2 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -77,9 +77,15 @@ static void run_mbm_test(bool has_ben, char **benchmark_cmd, int span,
>  
>  	ksft_print_msg("Starting MBM BW change ...\n");
>  
> +	res = remount_resctrlfs(false);

I think that should be remount_resctrlfs(true). Please note that any of the tests could be
run separately from the command line and thus each test need to ensure a clean
environment, it cannot assume that (a) user space provided it with a clean and/or
unmounted resctrl or (b) that any test was run before it.

> +	if (res) {
> +		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
> +		return;
> +	}
> +
>  	if (!validate_resctrl_feature_request(MBM_STR) || (get_vendor() != ARCH_INTEL)) {
>  		ksft_test_result_skip("Hardware does not support MBM or MBM is disabled\n");
> -		return;
> +		goto umount;
>  	}
>  

Reinette