All of lore.kernel.org
 help / color / mirror / Atom feed
* [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 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.