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

[Gabriele Monaco]

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