* [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range
@ 2025-01-08 21:00 Namhyung Kim
2025-01-08 21:00 ` [PATCH 2/2] perf ftrace: Fix display for range of the first bucket Namhyung Kim
2025-01-09 7:53 ` [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range Gabriele Monaco
0 siblings, 2 replies; 10+ messages in thread
From: Namhyung Kim @ 2025-01-08 21:00 UTC (permalink / raw)
To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang
Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML,
linux-perf-users, Gabriele Monaco
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.
Before:
$ sudo ./perf ftrace latency -a -T do_futex sleep 1
# DURATION | COUNT | GRAPH |
0 - 0 us | 0 | |
1 - 2 us | 0 | |
2 - 4 us | 0 | |
4 - 8 us | 0 | |
8 - 16 us | 0 | |
16 - 32 us | 0 | |
32 - 64 us | 0 | |
64 - 128 us | 0 | |
128 - 256 us | 0 | |
256 - 512 us | 0 | |
512 - 1024 us | 0 | |
1 - 2 ms | 0 | |
2 - 4 ms | 0 | |
4 - 8 ms | 0 | |
8 - 16 ms | 0 | |
16 - 32 ms | 0 | |
32 - 64 ms | 0 | |
64 - 128 ms | 0 | |
128 - 256 ms | 0 | |
256 - 512 ms | 0 | |
512 - 1024 ms | 0 | |
1 - ... s | 1353 | ############################################## |
After:
$ sudo ./perf ftrace latency -a -T do_futex sleep 1
# DURATION | COUNT | GRAPH |
0 - 0 us | 321 | ########### |
1 - 2 us | 132 | #### |
2 - 4 us | 202 | ####### |
4 - 8 us | 188 | ###### |
8 - 16 us | 16 | |
16 - 32 us | 12 | |
32 - 64 us | 30 | # |
64 - 128 us | 98 | ### |
128 - 256 us | 53 | # |
256 - 512 us | 57 | ## |
512 - 1024 us | 9 | |
1 - 2 ms | 9 | |
2 - 4 ms | 1 | |
4 - 8 ms | 98 | ### |
8 - 16 ms | 5 | |
16 - 32 ms | 7 | |
32 - 64 ms | 32 | # |
64 - 128 ms | 10 | |
128 - 256 ms | 10 | |
256 - 512 ms | 2 | |
512 - 1024 ms | 0 | |
1 - ... s | 0 | |
Fixes: 690a052a6d85c530 ("perf ftrace latency: Add --max-latency option")
Cc: Gabriele Monaco <gmonaco@redhat.com>
Signed-off-by: Namhyung Kim <namhyung@kernel.org>
---
tools/perf/builtin-ftrace.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c
index 08c1cc429b27e169..90cf2c9915a7e35b 100644
--- a/tools/perf/builtin-ftrace.c
+++ b/tools/perf/builtin-ftrace.c
@@ -796,8 +796,10 @@ static void make_histogram(struct perf_ftrace *ftrace, int buckets[],
// than the min latency desired.
if (num > 0) // 1st entry: [ 1 unit .. bucket_range units ]
i = num / ftrace->bucket_range + 1;
+ if (num >= max_latency - min_latency)
+ i = NUM_BUCKET -1;
}
- if (i >= NUM_BUCKET || num >= max_latency - min_latency)
+ if (i >= NUM_BUCKET)
i = NUM_BUCKET - 1;
num += min_latency;
@@ -1738,7 +1740,7 @@ int cmd_ftrace(int argc, const char **argv)
ret = -EINVAL;
goto out_delete_filters;
}
- if (!ftrace.min_latency) {
+ if (ftrace.bucket_range && !ftrace.min_latency) {
/* default min latency should be the bucket range */
ftrace.min_latency = ftrace.bucket_range;
}
@@ -1749,7 +1751,7 @@ int cmd_ftrace(int argc, const char **argv)
ret = -EINVAL;
goto out_delete_filters;
}
- if (!ftrace.max_latency) {
+ if (ftrace.bucket_range && !ftrace.max_latency) {
/* default max latency should depend on bucket range and num_buckets */
ftrace.max_latency = (NUM_BUCKET - 2) * ftrace.bucket_range +
ftrace.min_latency;
--
2.47.1.613.gc27f4b7a9f-goog
^ permalink raw reply related [flat|nested] 10+ messages in thread* [PATCH 2/2] perf ftrace: Fix display for range of the first bucket 2025-01-08 21:00 [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range Namhyung Kim @ 2025-01-08 21:00 ` Namhyung Kim 2025-01-10 14:08 ` Arnaldo Carvalho de Melo 2025-01-09 7:53 ` [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range Gabriele Monaco 1 sibling, 1 reply; 10+ messages in thread From: Namhyung Kim @ 2025-01-08 21:00 UTC (permalink / raw) To: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Gabriele Monaco When min_latency is not given, it prints 0 - 0. It should be 0 - 1. Before: $ sudo ./perf ftrace latency -a -T do_futex sleep 1 # DURATION | COUNT | GRAPH | 0 - 0 us | 321 | ########### | ... After: $ sudo ./perf ftrace latency -a -T do_futex sleep 1 # DURATION | COUNT | GRAPH | 0 - 1 us | 699 | ############ | ... Fixes: b875b6bf608589 ("perf ftrace latency: Introduce --min-latency to narrow down into a latency range") Cc: Gabriele Monaco <gmonaco@redhat.com> Signed-off-by: Namhyung Kim <namhyung@kernel.org> --- tools/perf/builtin-ftrace.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c index 90cf2c9915a7e35b..cfd770ec72867d77 100644 --- a/tools/perf/builtin-ftrace.c +++ b/tools/perf/builtin-ftrace.c @@ -840,7 +840,7 @@ static void display_histogram(struct perf_ftrace *ftrace, int buckets[]) bar_len = buckets[0] * bar_total / total; printf(" %4d - %4d %s | %10d | %.*s%*s |\n", - 0, min_latency, use_nsec ? "ns" : "us", + 0, min_latency ?: 1, use_nsec ? "ns" : "us", buckets[0], bar_len, bar, bar_total - bar_len, ""); for (i = 1; i < NUM_BUCKET - 1; i++) { -- 2.47.1.613.gc27f4b7a9f-goog ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] perf ftrace: Fix display for range of the first bucket 2025-01-08 21:00 ` [PATCH 2/2] perf ftrace: Fix display for range of the first bucket Namhyung Kim @ 2025-01-10 14:08 ` Arnaldo Carvalho de Melo 2025-01-10 14:11 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 10+ messages in thread From: Arnaldo Carvalho de Melo @ 2025-01-10 14:08 UTC (permalink / raw) To: Namhyung Kim Cc: Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Gabriele Monaco On Wed, Jan 08, 2025 at 01:00:15PM -0800, Namhyung Kim wrote: > When min_latency is not given, it prints 0 - 0. It should be 0 - 1. > > Before: > $ sudo ./perf ftrace latency -a -T do_futex sleep 1 > # DURATION | COUNT | GRAPH | > 0 - 0 us | 321 | ########### | > ... > > After: > $ sudo ./perf ftrace latency -a -T do_futex sleep 1 > # DURATION | COUNT | GRAPH | > 0 - 1 us | 699 | ############ | > ... > > Fixes: b875b6bf608589 ("perf ftrace latency: Introduce --min-latency to narrow down into a latency range") I couldn't find the above cset in perf-tools not perf-tools-next nor in upstream, I fixed it up to: Fixes: 08b875b6bf608589 ("perf ftrace latency: Introduce --min-latency to narrow down into a latency range") Thanks, - Arnaldo > Cc: Gabriele Monaco <gmonaco@redhat.com> > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > --- > tools/perf/builtin-ftrace.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c > index 90cf2c9915a7e35b..cfd770ec72867d77 100644 > --- a/tools/perf/builtin-ftrace.c > +++ b/tools/perf/builtin-ftrace.c > @@ -840,7 +840,7 @@ static void display_histogram(struct perf_ftrace *ftrace, int buckets[]) > bar_len = buckets[0] * bar_total / total; > > printf(" %4d - %4d %s | %10d | %.*s%*s |\n", > - 0, min_latency, use_nsec ? "ns" : "us", > + 0, min_latency ?: 1, use_nsec ? "ns" : "us", > buckets[0], bar_len, bar, bar_total - bar_len, ""); > > for (i = 1; i < NUM_BUCKET - 1; i++) { > -- > 2.47.1.613.gc27f4b7a9f-goog ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] perf ftrace: Fix display for range of the first bucket 2025-01-10 14:08 ` Arnaldo Carvalho de Melo @ 2025-01-10 14:11 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 10+ messages in thread From: Arnaldo Carvalho de Melo @ 2025-01-10 14:11 UTC (permalink / raw) To: Namhyung Kim Cc: Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users, Gabriele Monaco On Fri, Jan 10, 2025 at 11:08:48AM -0300, Arnaldo Carvalho de Melo wrote: > On Wed, Jan 08, 2025 at 01:00:15PM -0800, Namhyung Kim wrote: > > When min_latency is not given, it prints 0 - 0. It should be 0 - 1. > > > > Before: > > $ sudo ./perf ftrace latency -a -T do_futex sleep 1 > > # DURATION | COUNT | GRAPH | > > 0 - 0 us | 321 | ########### | > > ... > > > > After: > > $ sudo ./perf ftrace latency -a -T do_futex sleep 1 > > # DURATION | COUNT | GRAPH | > > 0 - 1 us | 699 | ############ | > > ... > > > > Fixes: b875b6bf608589 ("perf ftrace latency: Introduce --min-latency to narrow down into a latency range") > > I couldn't find the above cset in perf-tools not perf-tools-next nor in > upstream, I fixed it up to: It is just in perf-tools-next, so probably I did some rebase and that explains this. Anyway, that should be stable now. - Arnaldo > Fixes: 08b875b6bf608589 ("perf ftrace latency: Introduce --min-latency to narrow down into a latency range") > > Thanks, > > - Arnaldo > > > Cc: Gabriele Monaco <gmonaco@redhat.com> > > Signed-off-by: Namhyung Kim <namhyung@kernel.org> > > --- > > tools/perf/builtin-ftrace.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/tools/perf/builtin-ftrace.c b/tools/perf/builtin-ftrace.c > > index 90cf2c9915a7e35b..cfd770ec72867d77 100644 > > --- a/tools/perf/builtin-ftrace.c > > +++ b/tools/perf/builtin-ftrace.c > > @@ -840,7 +840,7 @@ static void display_histogram(struct perf_ftrace *ftrace, int buckets[]) > > bar_len = buckets[0] * bar_total / total; > > > > printf(" %4d - %4d %s | %10d | %.*s%*s |\n", > > - 0, min_latency, use_nsec ? "ns" : "us", > > + 0, min_latency ?: 1, use_nsec ? "ns" : "us", > > buckets[0], bar_len, bar, bar_total - bar_len, ""); > > > > for (i = 1; i < NUM_BUCKET - 1; i++) { > > -- > > 2.47.1.613.gc27f4b7a9f-goog ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range 2025-01-08 21:00 [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range Namhyung Kim 2025-01-08 21:00 ` [PATCH 2/2] perf ftrace: Fix display for range of the first bucket Namhyung Kim @ 2025-01-09 7:53 ` Gabriele Monaco 2025-01-10 0:46 ` Namhyung Kim 1 sibling, 1 reply; 10+ messages in thread From: Gabriele Monaco @ 2025-01-09 7:53 UTC (permalink / raw) To: Namhyung Kim, Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang Cc: Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range 2025-01-09 7:53 ` [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range Gabriele Monaco @ 2025-01-10 0:46 ` Namhyung Kim 2025-01-10 10:09 ` Gabriele Monaco 0 siblings, 1 reply; 10+ messages in thread From: Namhyung Kim @ 2025-01-10 0:46 UTC (permalink / raw) To: Gabriele Monaco Cc: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Thu, Jan 09, 2025 at 08:53:02AM +0100, Gabriele Monaco wrote: > 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. It looks good. One thing I concern is 'num += min_latency' before do_inc. I put it there to make it symmetric to 'num -= min_latency' so it should go to inside the block too. Or you could factor it out as a function like 'i = get_bucket_index(num)' so that it can keep the original num for the stats. Thanks, Namhyung > > --- > 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range 2025-01-10 0:46 ` Namhyung Kim @ 2025-01-10 10:09 ` Gabriele Monaco 2025-01-10 14:03 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 10+ messages in thread From: Gabriele Monaco @ 2025-01-10 10:09 UTC (permalink / raw) To: Namhyung Kim Cc: Arnaldo Carvalho de Melo, Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users 2025-01-10T00:46:49Z Namhyung Kim <namhyung@kernel.org>: > On Thu, Jan 09, 2025 at 08:53:02AM +0100, Gabriele Monaco wrote: >> 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. > > It looks good. One thing I concern is 'num += min_latency' before > do_inc. I put it there to make it symmetric to 'num -= min_latency' > so it should go to inside the block too. > > Or you could factor it out as a function like 'i = get_bucket_index(num)' > so that it can keep the original num for the stats. > Good point, I can have a deeper look at that. But I'd say it can come as a cleanup patch later. I have a couple more changes in mind and this would be no longer related to your changes. Thanks, Gabriele ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range 2025-01-10 10:09 ` Gabriele Monaco @ 2025-01-10 14:03 ` Arnaldo Carvalho de Melo 2025-01-10 15:11 ` Gabriele Monaco 0 siblings, 1 reply; 10+ messages in thread From: Arnaldo Carvalho de Melo @ 2025-01-10 14:03 UTC (permalink / raw) To: Gabriele Monaco Cc: Namhyung Kim, Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Fri, Jan 10, 2025 at 10:09:14AM +0000, Gabriele Monaco wrote: > 2025-01-10T00:46:49Z Namhyung Kim <namhyung@kernel.org>: > > > On Thu, Jan 09, 2025 at 08:53:02AM +0100, Gabriele Monaco wrote: > >> 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. > > > > It looks good. One thing I concern is 'num += min_latency' before > > do_inc. I put it there to make it symmetric to 'num -= min_latency' > > so it should go to inside the block too. > > > > Or you could factor it out as a function like 'i = get_bucket_index(num)' > > so that it can keep the original num for the stats. > > > > Good point, I can have a deeper look at that. But I'd say it can come as a cleanup patch later. > I have a couple more changes in mind and this would be no longer related to your changes. I'm tentatively taking this as an: Acked-by: Gabriele Monaco <gmonaco@redhat.com> But it would be great to have it as a Reviewed-by and perhaps a Tested-by, provided explicitely in response to this thread, ok? Thanks, - Arnaldo ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range 2025-01-10 14:03 ` Arnaldo Carvalho de Melo @ 2025-01-10 15:11 ` Gabriele Monaco 2025-01-10 18:13 ` Arnaldo Carvalho de Melo 0 siblings, 1 reply; 10+ messages in thread From: Gabriele Monaco @ 2025-01-10 15:11 UTC (permalink / raw) To: Arnaldo Carvalho de Melo Cc: Namhyung Kim, Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Fri, 2025-01-10 at 11:03 -0300, Arnaldo Carvalho de Melo wrote: > On Fri, Jan 10, 2025 at 10:09:14AM +0000, Gabriele Monaco wrote: > > 2025-01-10T00:46:49Z Namhyung Kim <namhyung@kernel.org>: > > > > > On Thu, Jan 09, 2025 at 08:53:02AM +0100, Gabriele Monaco wrote: > > > > 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. > > > > > > It looks good. One thing I concern is 'num += min_latency' > > > before > > > do_inc. I put it there to make it symmetric to 'num -= > > > min_latency' > > > so it should go to inside the block too. > > > > > > Or you could factor it out as a function like 'i = > > > get_bucket_index(num)' > > > so that it can keep the original num for the stats. > > > > > > > Good point, I can have a deeper look at that. But I'd say it can > > come as a cleanup patch later. > > I have a couple more changes in mind and this would be no longer > > related to your changes. > > I'm tentatively taking this as an: > > Acked-by: Gabriele Monaco <gmonaco@redhat.com> > > But it would be great to have it as a Reviewed-by and perhaps a > Tested-by, provided explicitely in response to this thread, ok? > > Thanks, > > - Arnaldo > I did test after applying both patches, went through the code and confirm my test worked as expected, and I confirm the issue is there before patching. I tested also in between, so feel free to add to both patches: Reviewed-by: Gabriele Monaco <gmonaco@redhat.com> Tested-by: Gabriele Monaco <gmonaco@redhat.com> (I'm assuming you are referring to 1/2 and 2/2 and not the little patch I sent in the first answer) Thanks, Gabriele ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range 2025-01-10 15:11 ` Gabriele Monaco @ 2025-01-10 18:13 ` Arnaldo Carvalho de Melo 0 siblings, 0 replies; 10+ messages in thread From: Arnaldo Carvalho de Melo @ 2025-01-10 18:13 UTC (permalink / raw) To: Gabriele Monaco Cc: Namhyung Kim, Ian Rogers, Kan Liang, Jiri Olsa, Adrian Hunter, Peter Zijlstra, Ingo Molnar, LKML, linux-perf-users On Fri, Jan 10, 2025 at 04:11:05PM +0100, Gabriele Monaco wrote: > > On Fri, 2025-01-10 at 11:03 -0300, Arnaldo Carvalho de Melo wrote: > > On Fri, Jan 10, 2025 at 10:09:14AM +0000, Gabriele Monaco wrote: > > > 2025-01-10T00:46:49Z Namhyung Kim <namhyung@kernel.org>: > > > > > > > On Thu, Jan 09, 2025 at 08:53:02AM +0100, Gabriele Monaco wrote: > > > > > 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. > > > > > > > > It looks good. One thing I concern is 'num += min_latency' > > > > before > > > > do_inc. I put it there to make it symmetric to 'num -= > > > > min_latency' > > > > so it should go to inside the block too. > > > > > > > > Or you could factor it out as a function like 'i = > > > > get_bucket_index(num)' > > > > so that it can keep the original num for the stats. > > > > > > > > > > Good point, I can have a deeper look at that. But I'd say it can > > > come as a cleanup patch later. > > > I have a couple more changes in mind and this would be no longer > > > related to your changes. > > > > I'm tentatively taking this as an: > > > > Acked-by: Gabriele Monaco <gmonaco@redhat.com> > > > > But it would be great to have it as a Reviewed-by and perhaps a > > Tested-by, provided explicitely in response to this thread, ok? > > > > Thanks, > > > > - Arnaldo > > > > I did test after applying both patches, went through the code and > confirm my test worked as expected, and I confirm the issue is there > before patching. I tested also in between, so feel free to add to both > patches: > > Reviewed-by: Gabriele Monaco <gmonaco@redhat.com> > Tested-by: Gabriele Monaco <gmonaco@redhat.com> Thanks, added. > (I'm assuming you are referring to 1/2 and 2/2 and not the little patch Yeah, Namhyung's 1/2 and 2/2 patches, these: https://lore.kernel.org/r/20250108210015.1188531-1-namhyung@kernel.org https://lore.kernel.org/r/20250108210015.1188531-2-namhyung@kernel.org - Arnaldo > I sent in the first answer) > > Thanks, > Gabriele ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2025-01-10 18:13 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-01-08 21:00 [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range Namhyung Kim 2025-01-08 21:00 ` [PATCH 2/2] perf ftrace: Fix display for range of the first bucket Namhyung Kim 2025-01-10 14:08 ` Arnaldo Carvalho de Melo 2025-01-10 14:11 ` Arnaldo Carvalho de Melo 2025-01-09 7:53 ` [PATCH 1/2] perf ftrace: Check min/max latency only with bucket range Gabriele Monaco 2025-01-10 0:46 ` Namhyung Kim 2025-01-10 10:09 ` Gabriele Monaco 2025-01-10 14:03 ` Arnaldo Carvalho de Melo 2025-01-10 15:11 ` Gabriele Monaco 2025-01-10 18:13 ` Arnaldo Carvalho de Melo
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).