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
@@ -140,7 +140,6 @@ int cat_perf_miss_val(int cpu_no, int n, char *cache_type)
struct resctrl_val_param param = {
.resctrl_val = CAT_STR,
.cpu_no = cpu_no,
- .mum_resctrlfs = false,
.setup = cat_setup,
};
@@ -113,7 +113,6 @@ int cmt_resctrl_val(int cpu_no, int n, char **benchmark_cmd)
.ctrlgrp = "c1",
.mongrp = "m1",
.cpu_no = cpu_no,
- .mum_resctrlfs = false,
.filename = RESULT_FILE_NAME,
.mask = ~(long_mask << n) & long_mask,
.span = cache_size * n / count_of_bits,
@@ -154,7 +154,6 @@ int mba_schemata_change(int cpu_no, char *bw_report, char **benchmark_cmd)
.ctrlgrp = "c1",
.mongrp = "m1",
.cpu_no = cpu_no,
- .mum_resctrlfs = true,
.filename = RESULT_FILE_NAME,
.bw_report = bw_report,
.setup = mba_setup
@@ -123,7 +123,6 @@ int mbm_bw_change(int span, int cpu_no, char *bw_report, char **benchmark_cmd)
.mongrp = "m1",
.span = span,
.cpu_no = cpu_no,
- .mum_resctrlfs = true,
.filename = RESULT_FILE_NAME,
.bw_report = bw_report,
.setup = mbm_setup
@@ -53,7 +53,6 @@
* @mongrp: Name of the monitor group (mon grp)
* @cpu_no: CPU number to which the benchmark would be binded
* @span: Memory bytes accessed in each benchmark iteration
- * @mum_resctrlfs: Should the resctrl FS be remounted?
* @filename: Name of file to which the o/p should be written
* @bw_report: Bandwidth report type (reads vs writes)
* @setup: Call back function to setup test environment
@@ -64,7 +63,6 @@ struct resctrl_val_param {
char mongrp[64];
int cpu_no;
unsigned long span;
- bool mum_resctrlfs;
char filename[64];
char *bw_report;
unsigned long mask;
@@ -84,8 +82,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);
@@ -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();
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;
@@ -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);