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: <406820f8-1d28-cdde-9494-20231fec0468@intel.com>
Date:   Thu, 13 Jul 2023 15:53:22 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.13.0
Subject: Re: [PATCH v4 05/19] selftests/resctrl: Unmount resctrl FS before
 starting the first test
Content-Language: en-US
To:     =?UTF-8?Q?Ilpo_J=c3=a4rvinen?= <ilpo.jarvinen@linux.intel.com>,
        <linux-kselftest@vger.kernel.org>, Shuah Khan <shuah@kernel.org>,
        "Shaopeng Tan" <tan.shaopeng@jp.fujitsu.com>,
        Fenghua Yu <fenghua.yu@intel.com>,
        <linux-kernel@vger.kernel.org>
References: <20230713131932.133258-1-ilpo.jarvinen@linux.intel.com>
 <20230713131932.133258-6-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230713131932.133258-6-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?SDRXcWl6OUU0dUQ2WlRXRUc2RCtuMmxPM0Z2UlAveXdoRE9SM3p4NTEvNC9n?=
 =?utf-8?B?alY2bks1ajZoekNMOGt6MUNseGVpTkdPQTVaZGZDc201ZXRvSS8zdjJjVWRP?=
 =?utf-8?B?VmxQMlJOV05OaGQ4T3JyRXk4U25YNTRQTmVQMW9JNnpIZFlSWS9rMktiOWJ1?=
 =?utf-8?B?NDh2YjB6RUtUMzJtMURFRUdkb1VRdjkwdnNDNEF0VVhRUTQvTnB1NU5qbEs5?=
 =?utf-8?B?R3l4cnRHKzNUK21sQzhhbWRGeC9RbU10UVpCbFcvK2tWdlBlK0ZFa29WZGV0?=
 =?utf-8?B?ckNxNDFtTVpSMHorTEJRL2p4SG1TR1hjZHBHYm9zRmE3c1VjR3loNG81V1FC?=
 =?utf-8?B?ZFNaVmFuT0VaR0VCclRGUVdWMWxQRkV4S2tOUy9ITmZCVjVRSVA4ZC8wYysy?=
 =?utf-8?B?RWFHbkRjd0Q5K3pnUmZDbjk3QTRFQ0JQZnRFeHpwMEN0SnlTRWx0RzFtbnlG?=
 =?utf-8?B?MlBSM3pmVmc0dnh5K2JYVmQ3ZDNycFplRUxTVjR3dXRpV2lBREIwQ2VLZW9K?=
 =?utf-8?B?S1IwMmFsVHRwUk5DWi84ZW5LdjhzeGxEbUhDcUZaZWNJSzBwOEJnWEt6ajE4?=
 =?utf-8?B?aGYrOTBxeUJRNGVZQmZPU290TWtNcUFXZCs5anhDbVdGbFFINmdaVlA4eGpk?=
 =?utf-8?B?U2k5aktyNXlzc1VZVEtqRFIvVitJOTNydnAyYUh3WnNIa0RUOWpwYTExajhh?=
 =?utf-8?B?Q1RhRWIxdGtGZjExL2xpTVlMNE1KZldWdlFIQjJ3WXVaeXBMa3F3a1M3OVMy?=
 =?utf-8?B?cHI5WTZGUWtBa2ZTTkpkZGxCVFJVWDgzV0xxdy9EWCtGblFwQks4ZjFoZUdW?=
 =?utf-8?B?Q21CQWR5dlNSb0Uvb21rOTBOTlo3RTRabWJWV2ZOU2tNcVozQmUyVUZ4WWNl?=
 =?utf-8?B?cGg0aG0xaTR0REdwcG9yTUhQK0FKbmR0aFdxdXlNREdCdHQyRjE4cmVsZHg4?=
 =?utf-8?B?Zml3VEpkNE1VNWVKQkJiZS9idDdmSUJxQzh4UXlOZFZVd0tsdkM3RXBvY2dx?=
 =?utf-8?B?eTZyTVkzYXZ1NXl4RElvblVZUFZ1RkFETFhDMWpxR0cva2dUWkh3dE9salFa?=
 =?utf-8?B?TDI5U0R0Q2NpU3pOa0luNTg3dFNoZ2xrWmh0OVlQaTQwVmxvOXpFbThUS2sy?=
 =?utf-8?B?di95S0cvWnI1WjFUTUxaYW9FalZNbEwwM2Z0ZWdCeENQYm1NeStBQXZXVVZJ?=
 =?utf-8?B?TCt4VmZyRUF3UVVSeENsMDd0dUtSMzlWSVM4TzMwTzFCMzV5NUtxOS84c2M3?=
 =?utf-8?B?RklxektHbUtld1B5T0NNM09idVZYSE1TVlN5emx5VHNEdmZiYzhEcVFsYmR3?=
 =?utf-8?B?cU8wbWxtdVBaSjFBZDFVdmt1aGYyVnhuUDNyY2tDelRQU0I3cGV4WDZMdU4w?=
 =?utf-8?B?UmRFNm1yaVBLb2hTSWFnUm9hcHhQQXREV012bXlyK1lrMkhNeUkxajFlaENW?=
 =?utf-8?B?bFZheUhERGZtaDIxck5xU0JGd1c1ckVyaHhFZHFLdTIzWWwwNmd6Y3IzS0wx?=
 =?utf-8?B?Y0FkQU9JZi9Vak1pZmxWS2lFWDB6UE0yM3R4V0E2SHlyV3hxYnRTeE5TVDRG?=
 =?utf-8?B?V3VCNG1KeHE3cHJndWg1dy9rWTRncE10c2xmcVNwVmNselFFbnFTd0ZwSk81?=
 =?utf-8?B?U212RVdDMjJOaEtrVDBHQTQyNTByRGdWQnlidzhJYXNzalNkSktsR3NQS3pD?=
 =?utf-8?B?aFJ1ZE54aThvYWhkcXV2OHY5V1hzV3JBWlAwckgvbVJaa3UzUFR2YlNSbjlm?=
 =?utf-8?B?MDEyMTI2NWx1SDc4TVlxdSthSm9NTTZKNWRMSGFlVW9sLzNZV0NVTEU1N21K?=
 =?utf-8?B?TkpqU0NPZnpsUDN0ZTdUc1FCR2VBbERSTVYza2lGWTdyMjk2czNKd1FId3Bn?=
 =?utf-8?B?RTZReFJZT09KaUxOTHdyMHJGMWQyTWRQaUpwUnRtVlpMR3lCalpTaVhaQS90?=
 =?utf-8?B?Rk9lcGhUMzZKTzRSdEt1VzJybGlSNm13SVFadUdKaFYxaVU3cDBsSitPZWRw?=
 =?utf-8?B?aE1ZU2pacHg2SVZyRDc1QStMMDVNOWhBV295dmlGSHFxU2JKeXdNMTQrRlZL?=
 =?utf-8?B?Wk42Wk82STZ3RDhtM1JDbmJ3S3VZU0FyZzlUbEVDRkZhNVVrRVVyb21rRGRv?=
 =?utf-8?B?eWFKSFVXS2RIY2RvOUF5TjlqbjgrR1NwQVduYVF6cG1Peks3MXB0U2lFRGpC?=
 =?utf-8?B?YVE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: c6ef748b-1167-474e-ee49-08db83f3f70e
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Jul 2023 22:53:24.9690
 (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: ksfNG9ulhZkNckp7bzdMP6PvdCNTNbKkK5EgSoFmX+2Es3pUWE/EbpcVtkIgEXvLXSThjBWJWpznqTAlz7rp8NoOrcDfZQL1uJyoQrfnvAY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR11MB4689
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 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,T_SCC_BODY_TEXT_LINE
        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 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> Resctrl FS mount/remount/umount code is hard to track. Better approach
> is to use mount/umount pair for each test but that assumes resctrl FS
> is not mounted beforehand.
> 
> Change umount_resctrlfs() so that it can unmount resctrl FS from any
> path, and enable further simplifications into mount/remount/umount
> logic by unmounting resctrl FS at the start if a pre-existing
> mountpoint is found.
> 
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl_tests.c | 2 ++
>  tools/testing/selftests/resctrl/resctrlfs.c     | 6 ++++--
>  2 files changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 9b9751206e1c..b1b2d28b52f7 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -250,6 +250,8 @@ int main(int argc, char **argv)
>  	if (!check_resctrlfs_support())
>  		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
>  
> +	umount_resctrlfs();
> +

umount_resctrlfs() can fail. I think there should be error checking
here and no tests should be run if resctrl cannot be unmounted since
the hardware and resctrl state is not known (without more effort).

>  	filter_dmesg();
>  
>  	ksft_set_plan(tests ? : 4);
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index fb00245dee92..23f75aeaa198 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -82,10 +82,12 @@ int remount_resctrlfs(bool mum_resctrlfs)
>  
>  int umount_resctrlfs(void)
>  {
> -	if (find_resctrl_mount(NULL))
> +	char mountpoint[256];
> +
> +	if (find_resctrl_mount(mountpoint))
>  		return 0;

It looks like the intent is to return 0 if find_resctrl_mount()
returns -ENOENT. It is not clear to me that it should also
return 0 if find_resctrl_mount() returns -ENXIO.

>  
> -	if (umount(RESCTRL_PATH)) {
> +	if (umount(mountpoint)) {
>  		perror("# Unable to umount resctrl");
>  
>  		return errno;

Reinette