* [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 an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.