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: <3df815f8-345e-7089-1c05-f46592ffa393@intel.com>
Date:   Fri, 21 Apr 2023 17:11:28 -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 04/24] selftests/resctrl: Remove mum_resctrlfs
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>, <linux-kernel@vger.kernel.org>
CC:     Shaopeng Tan <tan.shaopeng@jp.fujitsu.com>
References: <20230418114506.46788-1-ilpo.jarvinen@linux.intel.com>
 <20230418114506.46788-5-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230418114506.46788-5-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?QUhDYTlZMUNSOUJFelUxd0d5SjB4Nkl4QUkvcFl6MG5FL25oMWpSZXZ3N3pz?=
 =?utf-8?B?VWhqa0cwQWljeDNPaWM5OVBNOXZLVkJFZTRFMHV6eXQ3Q2NuY1pvYm1CVFhq?=
 =?utf-8?B?NmVZWUhpOTFoZDFoalR6aGRFNGRlZFl2SnNEeTNmUE5MWlhEczBIczJxejV4?=
 =?utf-8?B?YjBrQlBaV0hFV3ZxbjIxQ1JjVUw3Wmc0Qm41b3hWQ3VZNHZMdlp6WlhlSW5E?=
 =?utf-8?B?azRjeGhtZitCamM2TDcvNnc2bklIQmxQZ1RJWk5ncEt4OFo3eVNxYmp4OEE5?=
 =?utf-8?B?MkxWVjdQUFovYm1senhwZ3FsUkRWSHNCanY1ekk0OWhCejdLZk1YMS9KZ0Ru?=
 =?utf-8?B?UTgyWk1KSkpBNEJSdnJYQitJWlYxYng1OStiUW1iYjFlRWVSZnA5OVBkV0g4?=
 =?utf-8?B?UTM2ZG1HQWRSalFXRnVkUWJRSHlzamJaQjRiNlJOWWhwNUFxdE5pQzg2ZC9E?=
 =?utf-8?B?WElwRGsvcUtpQ2FyRjdIWk1ZWFhuY1JOZWFTaVhBZS9uU2dLTlZ4MzVEMElq?=
 =?utf-8?B?Mm8rYndTaWlibkV1bHQ1MDBqNFJiY2ZJb0c3dW9OQVZNQ1MyWGQya1JZdmk1?=
 =?utf-8?B?Vk1lWEtFZDI5Y1R3VWd0d3dBdDFwWUtRZURuSWNPWEtlSkppRVJldFBKaEdN?=
 =?utf-8?B?U2lzUmdhQmtCS2hudjdGYlFYNTExQXpYd3p4cCtJbXh2cm9oTFptWks2cDNX?=
 =?utf-8?B?ME9zYUd4VW5xV09CZ2VVeUFhQWdDUStPY055YW82QkVvb2VvbFZQbnFOR2NQ?=
 =?utf-8?B?ak1UOGpydnMwSjNvbXByM3o3d3QvWDRIV09raVZHSVFKSWgwUEJwbVpvTVVy?=
 =?utf-8?B?cVQrTHVaN2cxeEVmM05aeEQ2Y1BSOTc3a0JXb1AzaXNzS1dmTmp4cW5wQVp5?=
 =?utf-8?B?Z1FZckRoelJwVUl5aDVYL3JpWk9heXl5ZjBIRmJBd0p2WHJ4RnRseGo1VEo1?=
 =?utf-8?B?dEdsNGt6R1JmZGxhb29mS2J5Y1dONkpVdVVpTXRjSXhVbCtZVk9XZzVLaDY4?=
 =?utf-8?B?dzZvK2V0L3lIQTZyV0t6RVcrYWNiMy9SaThpY2U1eW4rbTBueC9uTnZteW0z?=
 =?utf-8?B?OFJwalo2bXc4cGhqS1djQjlkNUFOeUVsV1BTd3pvWGppczFKVDNDODBtanp6?=
 =?utf-8?B?a25KOE41eXUySmgwMkZ6R0hXeGxxZFA1MXNMbVZUK1ZUR0lRV0p6cm1HVVJG?=
 =?utf-8?B?cTBkcTBOaURIYnZLR1doZlcwVyttdjMwZGdmeVk3MEUwdHdzNlFkUHh5bmhu?=
 =?utf-8?B?c3lQSGx0SHVpZWMvNWpSZHVCM1c4NG5jck5GZktlZ1ZrQ2xYa0pIQ1l0clJu?=
 =?utf-8?B?M2FxMnE2MVJpN0dzUWNZVjNhNmwwNm45YXNkR2NZTmx2a3l3aEdDUTNJTHRW?=
 =?utf-8?B?YUQxVFVic1M5cDM1MUFwM0pISS9qUDUwdFlyelhUSFdOTHBIUWdJMmpiUFBq?=
 =?utf-8?B?RzkxSjFYOGY5OFBJUk5nS3d6c01ISldYNjJrbWkxWGhHTldIMFIyM0QrWnhM?=
 =?utf-8?B?WUFHdUxhWDFwT1lFUmZKUys3d3RoaGtkelo4Ylk3SHZjNmxwMWR3TGRzMk1C?=
 =?utf-8?B?czh1MXFSNlIrVk5ickFFY2U4bWsyNU1CRXIrZ055a1Y0SW4raEl5SzZxalVy?=
 =?utf-8?B?NUxub2tLaHErQW5aOEJ0Y1U3YmhtbTdxSjZSNFRxL2JzU2tkYmQ4dzRMN0dG?=
 =?utf-8?B?NC9lS0p4YkRoNm1XcmtnR3lML3pjVm1icS80MDkyc3d0aDlDcEZmY1Fvamwz?=
 =?utf-8?B?UWdUK3loNzA4aW04WWNOek9NajBmRXlONTRUZW5RY25ENkRVY1c3WXZDNHgz?=
 =?utf-8?B?eEFYay80d3U0dUtQTXBTcDU1OWs5YXRadis3ZHFOMEFqM3ltWXRpT3dVK2E1?=
 =?utf-8?B?VDhmY1NpaGZqVzliR1JkNWM4dmVuQUtvTnc0Y1haZ0NEb1d4d0RacjZhMVJT?=
 =?utf-8?B?cXlKUDI0U2IweldDNjhpSGw5ZEx0cjhOZHlROUlSQmVzblh0ZGh4SVlsRzNn?=
 =?utf-8?B?V1BTb3JQQ1IyMGY3K2lpbk9sRnNDV3Y3Mi9zZFZYbEljbmNBTld0d1U1dGNj?=
 =?utf-8?B?L05RNm5aQ1VRakY1RzhHZTRPbTN1eWFNSzB0SmZVczlPTlJxVkR3OVk5Mll3?=
 =?utf-8?B?aW5ZcUg5RlU1bjdCSkZHNEdMT1JxT2w1YnhPSXlyVExFRTRIMUhENUxKdzlN?=
 =?utf-8?B?T0E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: d1ba7943-b69d-485d-5f63-08db42c6203a
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:11:31.7310
 (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: gFnl24zu365se5VJyXUT5BFSqxlXZ70zFgGUGb+mwl+phNhULTvgTFa/z7jcKyJZ6Sb7xyWxJRbQu9cD3SmYeg8JJj1HO/6towX9CRimopY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN9PR11MB5227
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,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:
> Resctrl FS mount/umount are now cleanly paired.
> 
> Removed mum_resctrlfs that is what is left of the earlier remount
Removed -> Remove?
I am not sure what is meant with "that is what is left" ... 


> trickery to make the code cleaner. Rename remount_resctrlfs() to
> mount_resctrlfs() to match the reduced functionality.

These two logical changes would be easier to review if done
in two patches.

...


> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index 5c9ed52b69f2..f3303459136d 100644
> --- a/tools/testing/selftests/resctrl/resctrl_tests.c
> +++ b/tools/testing/selftests/resctrl/resctrl_tests.c
> @@ -77,7 +77,7 @@ 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);
> +	res = mount_resctrlfs();

As mentioned in previous patch the way I see it remount is still needed.

>  	if (res) {
>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
>  		return;
> @@ -106,7 +106,7 @@ static void run_mba_test(bool has_ben, char **benchmark_cmd, int span,
>  
>  	ksft_print_msg("Starting MBA Schemata change ...\n");
>  
> -	res = remount_resctrlfs(false);
> +	res = mount_resctrlfs();
>  	if (res) {
>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
>  		return;
> @@ -132,7 +132,7 @@ static void run_cmt_test(bool has_ben, char **benchmark_cmd, int cpu_no)
>  
>  	ksft_print_msg("Starting CMT test ...\n");
>  
> -	res = remount_resctrlfs(false);
> +	res = mount_resctrlfs();
>  	if (res) {
>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
>  		return;
> @@ -160,7 +160,7 @@ static void run_cat_test(int cpu_no, int no_of_bits)
>  
>  	ksft_print_msg("Starting CAT test ...\n");
>  
> -	res = remount_resctrlfs(false);
> +	res = mount_resctrlfs();
>  	if (res) {
>  		ksft_exit_fail_msg("Failed to mount resctrl FS\n");
>  		return;
> diff --git a/tools/testing/selftests/resctrl/resctrlfs.c b/tools/testing/selftests/resctrl/resctrlfs.c
> index 42f547a81e25..45f785213143 100644
> --- a/tools/testing/selftests/resctrl/resctrlfs.c
> +++ b/tools/testing/selftests/resctrl/resctrlfs.c
> @@ -48,29 +48,19 @@ static int find_resctrl_mount(char *buffer)
>  }
>  
>  /*
> - * remount_resctrlfs - Remount resctrl FS at /sys/fs/resctrl
> - * @mum_resctrlfs:	Should the resctrl FS be remounted?
> + * mount_resctrlfs - Mount resctrl FS at /sys/fs/resctrl
>   *
>   * If not mounted, mount it.
> - * If mounted and mum_resctrlfs then remount resctrl FS.
> - * If mounted and !mum_resctrlfs then noop
>   *
>   * Return: 0 on success, non-zero on failure
>   */
> -int remount_resctrlfs(bool mum_resctrlfs)
> +int mount_resctrlfs(void)
>  {
> -	char mountpoint[256];
>  	int ret;
>  
> -	ret = find_resctrl_mount(mountpoint);
> -	if (ret)
> -		strcpy(mountpoint, RESCTRL_PATH);
> -
> -	if (!ret && mum_resctrlfs && umount(mountpoint))
> -		ksft_print_msg("Fail: unmounting \"%s\"\n", mountpoint);
> -
> -	if (!ret && !mum_resctrlfs)
> -		return 0;
> +	ret = find_resctrl_mount(NULL);
> +	if (!ret)
> +		return -1;

This seems to assume that resctrl is always unmounted. Should the main program
thus start by unmounting resctrl before it runs any test in case it is mounted
when user space starts the tests?

Reinette