linux-kernel - [PATCH 2/2] perf stat: fix unexpected delay behaviour

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: <20220729161244.10522-2-adrian.herrera@arm.com>
Date:   Fri, 29 Jul 2022 16:12:44 +0000
From:   Adrián Herrera Arcila <adrian.herrera@....com>
To:     linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        acme@...nel.org
Cc:     peterz@...radead.org, mingo@...hat.com, mark.rutland@....com,
        alexander.shishkin@...ux.intel.com, jolsa@...nel.org,
        namhyung@...nel.org, leo.yan@...aro.org, songliubraving@...com,
        james.clark@....com,
        Adrián Herrera Arcila <adrian.herrera@....com>
Subject: [PATCH 2/2] perf stat: fix unexpected delay behaviour

The described --delay behaviour is to delay the enablement of events, but
not the execution of the command, if one is passed, which is incorrectly
the current behaviour.

This patch decouples the enablement from the delay, and enables events
before or after launching the workload dependent on the options passed
by the user. This code structure is inspired by that in perf-record, and
tries to be consistent with it.

Link: https://lore.kernel.org/linux-perf-users/7BFD066E-B0A8-49D4-B635-379328F0CF4C@fb.com
Fixes: d0a0a511493d ("perf stat: Fix forked applications enablement of counters")
Signed-off-by: Adrián Herrera Arcila <adrian.herrera@....com>
---
 tools/perf/builtin-stat.c | 56 ++++++++++++++++++++++-----------------
 1 file changed, 32 insertions(+), 24 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index 318ffd119dad..f98c823b16dd 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -559,7 +559,7 @@ static bool handle_interval(unsigned int interval, int *times)
 	return false;
 }
 
-static int enable_counters(void)
+static int enable_bpf_counters(void)
 {
 	struct evsel *evsel;
 	int err;
@@ -572,28 +572,6 @@ static int enable_counters(void)
 		if (err)
 			return err;
 	}
-
-	if (stat_config.initial_delay < 0) {
-		pr_info(EVLIST_DISABLED_MSG);
-		return 0;
-	}
-
-	if (stat_config.initial_delay > 0) {
-		pr_info(EVLIST_DISABLED_MSG);
-		usleep(stat_config.initial_delay * USEC_PER_MSEC);
-	}
-
-	/*
-	 * We need to enable counters only if:
-	 * - we don't have tracee (attaching to task or cpu)
-	 * - we have initial delay configured
-	 */
-	if (!target__none(&target) || stat_config.initial_delay) {
-		if (!all_counters_use_bpf)
-			evlist__enable(evsel_list);
-		if (stat_config.initial_delay > 0)
-			pr_info(EVLIST_ENABLED_MSG);
-	}
 	return 0;
 }
 
@@ -966,10 +944,24 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 			return err;
 	}
 
-	err = enable_counters();
+	err = enable_bpf_counters();
 	if (err)
 		return -1;
 
+	/*
+	 * Enable events manually here if perf-stat is run:
+	 * 1. with a target (any of --all-cpus, --cpu, --pid or --tid)
+	 * 2. without measurement delay (no --delay)
+	 * 3. without all events associated to BPF
+	 *
+	 * This is because if run with a target, events are not enabled
+	 * on exec if a workload is passed, and because there is no delay
+	 * we ensure to enable them before the workload starts
+	 */
+	if (!target__none(&target) && !stat_config.initial_delay &&
+	    !all_counters_use_bpf)
+		evlist__enable(evsel_list);
+
 	/* Exec the command, if any */
 	if (forks)
 		evlist__start_workload(evsel_list);
@@ -977,6 +969,22 @@ static int __run_perf_stat(int argc, const char **argv, int run_idx)
 	t0 = rdclock();
 	clock_gettime(CLOCK_MONOTONIC, &ref_time);
 
+	/*
+	 * If a measurement delay was specified, start it, and if positive,
+	 * enable events manually after. We respect the delay even if all
+	 * events are associated to BPF
+	 */
+	if (stat_config.initial_delay) {
+		/* At this point, events are guaranteed disabled */
+		pr_info(EVLIST_DISABLED_MSG);
+		if (stat_config.initial_delay > 0) {
+			usleep(stat_config.initial_delay * USEC_PER_MSEC);
+			if (!all_counters_use_bpf)
+				evlist__enable(evsel_list);
+			pr_info(EVLIST_ENABLED_MSG);
+		}
+	}
+
 	if (forks) {
 		if (interval || timeout || evlist__ctlfd_initialized(evsel_list))
 			status = dispatch_events(forks, timeout, interval, &times);
-- 
2.36.1