linux-kernel - Re: [PATCH v3 5/7] selftests/resctrl: Make benchmark command const and build it with pointers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <159d8e07-cab7-9430-b83f-9c6de4d519d4@intel.com>
Date:   Wed, 30 Aug 2023 10:47:19 -0700
From:   Reinette Chatre <reinette.chatre@...el.com>
To:     Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
CC:     Shuah Khan <skhan@...uxfoundation.org>,
        <linux-kselftest@...r.kernel.org>, Shuah Khan <shuah@...nel.org>,
        Maciej Wieczor-Retman <maciej.wieczor-retman@...el.com>,
        Fenghua Yu <fenghua.yu@...el.com>,
        "Babu Moger" <babu.moger@....com>,
        LKML <linux-kernel@...r.kernel.org>,
        Shaopeng Tan <tan.shaopeng@...fujitsu.com>
Subject: Re: [PATCH v3 5/7] selftests/resctrl: Make benchmark command const
 and build it with pointers

Hi Ilpo,

On 8/30/2023 1:59 AM, Ilpo Järvinen wrote:
> On Tue, 29 Aug 2023, Reinette Chatre wrote:
>> This is a tricky one. If I understand correctly this goto target makes
>> some assumptions about the state (no test plan created yet) and exit
>> reason (it has to be skipped). A temporary variable is also thrown into
>> the mix.
> 
> So in the end the symmetry proved to be not as simple as was depicted 
> earlier but "tricky"... I tried to warn about this and it's why I wished 
> to avoid the allocation entirely. Without allocation, there would have not 
> been need for the temporary variable nor adjusting the control flow with 
> that label.

hmmm ... I do not see why an allocation forces the use of a temporary
variable and a change in control (more below).

> 
>> Can this not be simplified by moving the snippet where
>> benchmark_cmd[] is initialized to fill_buf to be just before the tests 
>> are run? Perhaps right before ksft_set_plan()?
> 
> So I throw a temporary variable into the mix (has_ben) to keep track when 
> benchmark_cmd needs to be initialized to the default command? It doesn't 
> play well with what I've in queue after this when user parameters are 
> collected into a struct which is initialized to default value by a helper 
> function before any argument processing. That is, initializing the 
> parameters to defaults needs to be split before and after the parameter 
> parsing code.

No new temporary variable is needed. Of course, I do not have insight into
what is further down in your queue but based on this work I do think it
can be simplified. Since code is easier to consider, the snippet below
applies on top of this series and shows what I was proposing:


diff --git a/tools/testing/selftests/resctrl/resctrl_tests.c b/tools/testing/selftests/resctrl/resctrl_tests.c
index ae9001ef7b0a..8033eabb9aa8 100644
--- a/tools/testing/selftests/resctrl/resctrl_tests.c
+++ b/tools/testing/selftests/resctrl/resctrl_tests.c
@@ -170,11 +170,10 @@ static void run_cat_test(int cpu_no, int no_of_bits)
 int main(int argc, char **argv)
 {
 	bool mbm_test = true, mba_test = true, cmt_test = true;
+	const char *benchmark_cmd[BENCHMARK_ARGS] = {};
 	int c, cpu_no = 1, i, no_of_bits = 0;
-	const char *benchmark_cmd[BENCHMARK_ARGS];
 	char *span_str = NULL;
 	bool cat_test = true;
-	char *skip_reason;
 	int tests = 0;
 	int ret;
 
@@ -247,17 +246,6 @@ int main(int argc, char **argv)
 		}
 	}
 
-	/* If no benchmark is given by "-b" argument, use fill_buf. */
-	benchmark_cmd[0] = "fill_buf";
-	ret = asprintf(&span_str, "%u", DEFAULT_SPAN);
-	if (ret < 0)
-		ksft_exit_fail_msg("Out of memory!\n");
-	benchmark_cmd[1] = span_str;
-	benchmark_cmd[2] = "1";
-	benchmark_cmd[3] = "0";
-	benchmark_cmd[4] = "false";
-	benchmark_cmd[5] = NULL;
-
 last_arg:
 
 	ksft_print_header();
@@ -267,23 +255,30 @@ int main(int argc, char **argv)
 	 * 1. We write to resctrl FS
 	 * 2. We execute perf commands
 	 */
-	if (geteuid() != 0) {
-		skip_reason = "Not running as root. Skipping...\n";
-		goto free_span;
-	}
+	if (geteuid() != 0)
+		return ksft_exit_skip("Not running as root. Skipping...\n");
 
-	if (!check_resctrlfs_support()) {
-		skip_reason = "resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n";
-		goto free_span;
-	}
+	if (!check_resctrlfs_support())
+		return ksft_exit_skip("resctrl FS does not exist. Enable X86_CPU_RESCTRL config option.\n");
 
-	if (umount_resctrlfs()) {
-		skip_reason = "resctrl FS unmount failed.\n";
-		goto free_span;
-	}
+	if (umount_resctrlfs())
+		return ksft_exit_skip("resctrl FS unmount failed.\n");
 
 	filter_dmesg();
 
+	if (!benchmark_cmd[0]) {
+		/* If no benchmark is given by "-b" argument, use fill_buf. */
+		benchmark_cmd[0] = "fill_buf";
+		ret = asprintf(&span_str, "%u", DEFAULT_SPAN);
+		if (ret < 0)
+			ksft_exit_fail_msg("Out of memory!\n");
+		benchmark_cmd[1] = span_str;
+		benchmark_cmd[2] = "1";
+		benchmark_cmd[3] = "0";
+		benchmark_cmd[4] = "false";
+		benchmark_cmd[5] = NULL;
+	}
+
 	ksft_set_plan(tests ? : 4);
 
 	if (mbm_test)
@@ -300,8 +295,4 @@ int main(int argc, char **argv)
 
 	free(span_str);
 	ksft_finished();
-
-free_span:
-	free(span_str);
-	return ksft_exit_skip(skip_reason);
 }