linux-kernel - Re: [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range

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: <a016ab208eb3ee3821ab59a2520a2bf56645917e.camel@redhat.com>
Date: Thu, 09 Jan 2025 08:53:02 +0100
From: Gabriele Monaco <gmonaco@...hat.com>
To: Namhyung Kim <namhyung@...nel.org>, Arnaldo Carvalho de Melo
	 <acme@...nel.org>, Ian Rogers <irogers@...gle.com>, Kan Liang
	 <kan.liang@...ux.intel.com>
Cc: Jiri Olsa <jolsa@...nel.org>, Adrian Hunter <adrian.hunter@...el.com>, 
 Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...nel.org>, LKML
 <linux-kernel@...r.kernel.org>, 	linux-perf-users@...r.kernel.org
Subject: Re: [PATCH 1/2] perf ftrace: Check min/max latency only with bucket
 range

On Wed, 2025-01-08 at 13:00 -0800, Namhyung Kim wrote:
> It's an optional feature and remains 0 when bucket range is not
> given.
> And it makes the histogram goes to the last entry always because any
> latency (num) is greater than or equal to 0.

Thanks Namhyung for fixing this, something definitely slipped while
testing..

I confirm your patches work well also when the bucket range is provided but the
min latency isn't.

I'm wondering if it would be cleaner to propagate your changes (using
min/max latency only if bucket_range is provided) also to
make_histogram. That function currently works since we assume
min_latency to be always 0, which is the case but probably not
considering it altogether would look a bit better and prevent some
headache in the future.

---
 tools/perf/builtin-ftrace.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)

diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 076c703e5c334..82d63d7af9d03 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -779,19 +779,16 @@ static void make_histogram(struct perf_ftrace *ftrace, int buckets[],
 		if (ftrace->use_nsec)
 			num *= 1000;
 
-		i = 0;
-		if (num < min_latency)
-			goto do_inc;
-
-		num -= min_latency;
-
 		if (!ftrace->bucket_range) {
 			i = log2(num);
 			if (i < 0)
 				i = 0;
 		} else {
-			// Less than 1 unit (ms or ns), or, in the future,
-			// than the min latency desired.
+			i = 0;
+			if (num < min_latency)
+				goto do_inc;
+
+			num -= min_latency;
 			if (num > 0) // 1st entry: [ 1 unit .. bucket_range units ]
 				i = num / ftrace->bucket_range + 1;
 			if (num >= max_latency - min_latency)

base-commit: 257ccfaf9fbef6f54eabf1a8f8a8efcc9036439b
-- 
2.47.1