* [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 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 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-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).