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: <09605219-19db-ba2d-aaad-9e279543f461@intel.com>
Date:   Thu, 13 Jul 2023 15:57:05 -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 07/19] selftests/resctrl: Refactor remount_resctrl(bool
 mum_resctrlfs) to mount_resctrl()
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-8-ilpo.jarvinen@linux.intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230713131932.133258-8-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?bEJpMmlYUlh5Z3o1djYxUDc3YTFzaFNST1lHODk5QzdQcDcyUnhJQmNyTXJy?=
 =?utf-8?B?d3lNYzhHWmJkSk9JancxODdtS3ovRWczbHc1UFh5NFA4cndoNEg1clMxNnNY?=
 =?utf-8?B?Ry9nZFVldTRmcWc2eU5vY3hmWTFsUEFudmkwaEJsUlVTSklidFcvbENEMENz?=
 =?utf-8?B?R2RYekhoUDVyTVJHK29HWmIwaUxRVFl6VFBndXdneW50QTZwakZhY2t6QzMv?=
 =?utf-8?B?MVVkWURHdnBrSUFDcHNzSFdUVnc1a09PN1NkOVZOUHNJUUNsdUdwSVljSXoy?=
 =?utf-8?B?bmxjaDd1SXN0cWdNSkRlN2tGOFN4MU9PZ1NPWUsrNi9tNXlNN2FyN1libVR4?=
 =?utf-8?B?d3djNHZiVU1aY1VPTDJBOW03aUJmYVVEUFhic0I1N1NhWnpqWVBHRFpoRjZD?=
 =?utf-8?B?ODdmQ0VSM0crSHdNR3VDNGNXMEcxd1BXd25ZbVRjME5nSk8wNmI4YngzWUpy?=
 =?utf-8?B?SEY1dnRvV1VuTXdEd3d2em1JKzRrTTJoYmlsVXhjQ0RWKzJMeWcwc1NERmgr?=
 =?utf-8?B?UTY1andkRU9LU29Fc2lKMCtBQXdOdkd1NEV6RzNzeEVYWm90QTNOeFliZllF?=
 =?utf-8?B?SXU0S2t6eVpQSHNvZURPMThVMmREd3FpMCsvMHMvM0RURjJuaThwMm5melo5?=
 =?utf-8?B?dFJsQVVKTzM0aWJuQkhuZW5BQmFXYWp5OXVMUXpDWTRrenFRS1l2Q2dFdDZy?=
 =?utf-8?B?Umo2TUFhaWp4dE1LZGorQVBkSkdNUnAvVXBJSWZ1RkpVQWhNaEJmREF3cXpk?=
 =?utf-8?B?MGsrTGtZUlo0MVVSRGhwMGxlYjE5cXBNaVJQYXBOc2FvdTNubTBxZkJEY2hz?=
 =?utf-8?B?NFllQklqZlZYTVNOWUpsSWlLd0g5WTVIUGVYWVZpVWZWNTMvMUNlTEViNnJ5?=
 =?utf-8?B?TGFCRVdLbGhWaWw1TmJ3SXlCeWxaWUZxS3lOcTdwSmNZaFN1R2ZGL0hTcmFG?=
 =?utf-8?B?WmNPbjBySXV6Q0J3ejlLZG16ekwxUEwyZmZKSXRMYzhGek1tandWRVdrSEtm?=
 =?utf-8?B?WWlwbnRNR2NQc3AwdTVXaUFUUUtlbVlQay9tQ1g1KzJJWXVsY0d4cDAxL2ht?=
 =?utf-8?B?N2Fqd0tHZldyWTVLMDFyWkk0VlJHUUptR2FrRFdYekpEaVl6REE3UzRtNlJG?=
 =?utf-8?B?bjFRV1VrQ21LSHNpSytEZS9nQ1RHZ2VEcEJya3V5TmJoYXRvYStKdE1iOXBo?=
 =?utf-8?B?UkJmOVZ0MGdMTGpvcXd6V3c0YTVYdVNaam5RcnRDMXV0a1VNUmlFUnhTbS85?=
 =?utf-8?B?Ni9FRGRkR0xZMUFtZnYzb2F1a3YvajQ4Njd2N0I1TFZ6L1kxdDRpR05JZEJ5?=
 =?utf-8?B?U1N2OUFyOE9pMHo5cFphOWNIQmhzYzBWeEFxQndWVmQ5VURSRWtlSWlDUWtJ?=
 =?utf-8?B?REx1TUJ6K0Jqc2Q2clNBdnlrcHdUMDVESEhmYk5CNElHeEJHMHhrelpNUERD?=
 =?utf-8?B?VUQySytOc3Z2OTdLNzcxdHhuRWpPaE1vT2w4YW5hYWNYM2tzTEgwQmZkODBR?=
 =?utf-8?B?NGZKOHRQWlFyemRiSkJQSWk2OHQvUFh0b29YNE9veTR6anpxTmovVVFDZm1s?=
 =?utf-8?B?MVhWdFNXN0VwL28vZjhTRnVuOVhMcHhoYmRFd1ZQcTZESTJFSGI0dmhZRkha?=
 =?utf-8?B?MTNuWlgvdnYrcVpLaGhNMnRBMkQ1Q0ZxdTd4d1BrNDNmVTlBdUEvekZwZWNi?=
 =?utf-8?B?TElSOVpyU1ljQVhtd1pyaFQxajZkNXpxb3FJelBqREEwa016dnY5V1MwK3NF?=
 =?utf-8?B?NGFPUFNHTlNPUlVBaDdVVEw5VmlVRGRER29EUktOMUlUMGJtN0Q1OUlCL0pL?=
 =?utf-8?B?L08vc2x5eW9PcldSYk1rSlp1cmNhOGtVVHhaSGtiYW1Pb0FwZVJQZGVubTkr?=
 =?utf-8?B?aUhISTgwM3RnbEZnVDJ4TFVkcDA2WG5wQUUwSnN3c3dtTCtFYTJHMElLeStL?=
 =?utf-8?B?cXlWNEZVQUJueDFXN0NLRWF3TUpObjl5WjJPL09aVldZZkpxaDhsY3FaWXVt?=
 =?utf-8?B?M3J1TmVVKzdjQjAzYnVScW03OXpXam5SNnZ4ekhFUTVYL1UrL3RQS1cxMmZl?=
 =?utf-8?B?K3BPUnVEa0pFTkh0dW96MDFnVkxmeC96WHlMdTRwWnh6aXdGRE9WS1VmV0gz?=
 =?utf-8?B?L2gwbndoOS9hUjV5N1VhMDRKZlBjTWt3ZzhCYkhWdjhkVURRMXYrVkl4SFZs?=
 =?utf-8?B?aUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f6037516-aa6f-4512-0e90-08db83f47c15
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:57:08.1281
 (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: jQX6X5qKvvsYgxwDXIE+SfVVtIKsGsglijoP4X5tqo0fBW2nBSA7syT8XqrozifWtt+bebf0rpwR0a0ZFVziwfpiK7F/sTTgErvAb84rPjU=
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_PASS,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:
> Mount/umount of the resctrl FS is now paired nicely per test.
> 
> Rename remount_resctrl(bool mum_resctrlfs) to mount_resctrl(). Make
> it unconditionally try to mount the resctrl FS and return error if
> resctrl FS was mounted already.
> 
> While at it, group the mount/umount prototypes in the header.
> 
> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  tools/testing/selftests/resctrl/resctrl.h     |  2 +-
>  .../testing/selftests/resctrl/resctrl_tests.c |  8 ++++----
>  tools/testing/selftests/resctrl/resctrlfs.c   | 20 +++++--------------
>  3 files changed, 10 insertions(+), 20 deletions(-)
> 
> diff --git a/tools/testing/selftests/resctrl/resctrl.h b/tools/testing/selftests/resctrl/resctrl.h
> index f455f0b7e314..23af3fb73cb4 100644
> --- a/tools/testing/selftests/resctrl/resctrl.h
> +++ b/tools/testing/selftests/resctrl/resctrl.h
> @@ -85,8 +85,8 @@ extern char llc_occup_path[1024];
>  int get_vendor(void);
>  bool check_resctrlfs_support(void);
>  int filter_dmesg(void);
> -int remount_resctrlfs(bool mum_resctrlfs);
>  int get_resource_id(int cpu_no, int *resource_id);
> +int mount_resctrlfs(void);
>  int umount_resctrlfs(void);
>  int validate_bw_report_request(char *bw_report);
>  bool validate_resctrl_feature_request(const char *resctrl_val);
> diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
> index a421d045de08..3f26d2279f75 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(true);
> +	res = mount_resctrlfs();
>  	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(true);
> +	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(true);
> +	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(true);
> +	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 b3a05488d360..f622245adafe 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
>   */

Since it is not obviously a "failure" I do think it will help to
add to the comments that resctrl already being mounted is treated as
a 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 treats "ret == 0" as a failure. What about -ENXIO? It seems to
me that only "ret == -ENOENT" is "success".

>  
>  	ksft_print_msg("Mounting resctrl to \"%s\"\n", RESCTRL_PATH);
>  	ret = mount("resctrl", RESCTRL_PATH, "resctrl", 0, NULL);


Reinette