diff mbox series

[v4,07/19] selftests/resctrl: Refactor remount_resctrl(bool mum_resctrlfs) to mount_resctrl()

Message ID 20230713131932.133258-8-ilpo.jarvinen@linux.intel.com
State New
Headers
Series selftests/resctrl: Fixes and cleanups |

Commit Message

Ilpo Järvinen July 13, 2023, 1:19 p.m. UTC 
  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(-)

Comments

Reinette Chatre July 13, 2023, 10:57 p.m. UTC | #1 
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
Ilpo Järvinen July 14, 2023, 11:03 a.m. UTC | #2 
On Thu, 13 Jul 2023, Reinette Chatre wrote:
> 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".

Yes, it's a good catch.
Maciej Wieczor-Retman July 24, 2023, 7:12 a.m. UTC | #3 
Hi!

On 14.07.2023 13:03, Ilpo Järvinen wrote:
> On Thu, 13 Jul 2023, Reinette Chatre wrote:
>> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
>>> -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".
> 
> Yes, it's a good catch.
> 

I had an idea about a small redesign of find_resctrl_mount
return values so it is easier to see what the function tries
to accomplish.

When there is an error (-ENXIO for example) it could 
return the negative error value. When no mount is found
it could return a zero (instead of the -ENOENT error code).
Finally when a mount point was found it could return a positive
value (for example return 1). This way errors could be 
separate from regular return values and in my opinion the
function logic would be more transparent.

What do you think about it?

Kind regards
Maciej Wieczór-Retman
Ilpo Järvinen Aug. 7, 2023, 10:26 a.m. UTC | #4 
On Mon, 24 Jul 2023, Wieczor-Retman, Maciej wrote:
> On 14.07.2023 13:03, Ilpo Järvinen wrote:
> > On Thu, 13 Jul 2023, Reinette Chatre wrote:
> >> On 7/13/2023 6:19 AM, Ilpo Järvinen wrote:
> >>> -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".
> > 
> > Yes, it's a good catch.
> > 
> 
> I had an idea about a small redesign of find_resctrl_mount
> return values so it is easier to see what the function tries
> to accomplish.
> 
> When there is an error (-ENXIO for example) it could 
> return the negative error value. When no mount is found
> it could return a zero (instead of the -ENOENT error code).
> Finally when a mount point was found it could return a positive
> value (for example return 1). This way errors could be 
> separate from regular return values and in my opinion the
> function logic would be more transparent.
> 
> What do you think about it?

Yes, it's a great idea. It would be more logical and thus easier to 
comprehend.

Since this already wast applied, it has to be done in another change.
diff mbox series

Patch

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
  */
-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;
 
 	ksft_print_msg("Mounting resctrl to \"%s\"\n", RESCTRL_PATH);
 	ret = mount("resctrl", RESCTRL_PATH, "resctrl", 0, NULL);