Linux-Trace-Devel Archive on lore.kernel.org
 help / color / Atom feed
* ftrace histogram sorting broken on BE architecures
@ 2019-12-11 12:33 Sven Schnelle
  2019-12-11 15:35 ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Sven Schnelle @ 2019-12-11 12:33 UTC (permalink / raw)
  To: linux-trace-devel

Hi List,

i was looking into a ftracetest failure on s390:

# ./ftracetest test.d/trigger/trigger-hist.tc
=== Ftrace unit tests ===
[1] event trigger - test histogram trigger	[FAIL]
[2] (instance)  event trigger - test histogram trigger	[FAIL]

from the -vvv log: ++ fail 'sort param on sched_process_fork did not work'

# cat events/sched/sched_process_fork/hist

# event histogram
#
# trigger info: hist:keys=parent_pid,child_pid:vals=hitcount:sort=child_pid:size=2048 [active]
#

{ parent_pid:       1406, child_pid:       1428 } hitcount:          1
{ parent_pid:       1406, child_pid:       1430 } hitcount:          1
{ parent_pid:       1406, child_pid:       1427 } hitcount:          1
{ parent_pid:       1406, child_pid:       1432 } hitcount:          1
{ parent_pid:       1406, child_pid:       1431 } hitcount:          1
{ parent_pid:       1406, child_pid:       1429 } hitcount:          1

So the test is right, the entries are not sorted. After digging into the
ftrace code i noticed that integer values always get extended to 64 bit
in event_hist_trigger(), but cmp_entries_key() from tracing_map.c uses the
type of the field (which is a pid_t, and therefore 4 bytes).

On Little Endian this doesn't hurt, but on BE s390 this makes the compare
function compare 4 zero bytes, which is the reason why sorting doesn't
work. As a test i forced the compare function used in cmp_entries_key() to
tracing_map_cmp_s64(), which made the ftrace tests pass.

I also tested this on 64 bit parisc with the same results, so the architecture
doesn't seem make a difference (besides LE vs. BE)

Any thoughts on how to fix this? I'm not sure whether i fully understand the
ftrace maps... ;-)

Best,
Sven

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-11 12:33 ftrace histogram sorting broken on BE architecures Sven Schnelle
@ 2019-12-11 15:35 ` Steven Rostedt
  2019-12-11 16:09   ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2019-12-11 15:35 UTC (permalink / raw)
  To: Sven Schnelle; +Cc: linux-trace-devel, LKML, Tom Zanussi

On Wed, 11 Dec 2019 13:33:16 +0100
Sven Schnelle <svens@stackframe.org> wrote:

> Hi List,

Hi Sven,

> 
> i was looking into a ftracetest failure on s390:
> 
> # ./ftracetest test.d/trigger/trigger-hist.tc
> === Ftrace unit tests ===
> [1] event trigger - test histogram trigger	[FAIL]
> [2] (instance)  event trigger - test histogram trigger	[FAIL]
> 
> from the -vvv log: ++ fail 'sort param on sched_process_fork did not work'
> 
> # cat events/sched/sched_process_fork/hist
> 
> # event histogram
> #
> # trigger info: hist:keys=parent_pid,child_pid:vals=hitcount:sort=child_pid:size=2048 [active]
> #
> 
> { parent_pid:       1406, child_pid:       1428 } hitcount:          1
> { parent_pid:       1406, child_pid:       1430 } hitcount:          1
> { parent_pid:       1406, child_pid:       1427 } hitcount:          1
> { parent_pid:       1406, child_pid:       1432 } hitcount:          1
> { parent_pid:       1406, child_pid:       1431 } hitcount:          1
> { parent_pid:       1406, child_pid:       1429 } hitcount:          1
> 
> So the test is right, the entries are not sorted. After digging into the
> ftrace code i noticed that integer values always get extended to 64 bit
> in event_hist_trigger(), but cmp_entries_key() from tracing_map.c uses the
> type of the field (which is a pid_t, and therefore 4 bytes).
> 
> On Little Endian this doesn't hurt, but on BE s390 this makes the compare
> function compare 4 zero bytes, which is the reason why sorting doesn't
> work. As a test i forced the compare function used in cmp_entries_key() to
> tracing_map_cmp_s64(), which made the ftrace tests pass.
> 
> I also tested this on 64 bit parisc with the same results, so the architecture
> doesn't seem make a difference (besides LE vs. BE)
> 
> Any thoughts on how to fix this? I'm not sure whether i fully understand the
> ftrace maps... ;-)

Your analysis makes sense. I'll take a deeper look at it.

Thanks for reporting it!

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-11 15:35 ` Steven Rostedt
@ 2019-12-11 16:09   ` Steven Rostedt
  2019-12-11 16:37     ` Tom Zanussi
                       ` (2 more replies)
  0 siblings, 3 replies; 16+ messages in thread
From: Steven Rostedt @ 2019-12-11 16:09 UTC (permalink / raw)
  To: Sven Schnelle; +Cc: linux-trace-devel, LKML, Tom Zanussi

On Wed, 11 Dec 2019 10:35:57 -0500
Steven Rostedt <rostedt@goodmis.org> wrote:

> > Any thoughts on how to fix this? I'm not sure whether i fully understand the
> > ftrace maps... ;-)  
> 
> Your analysis makes sense. I'll take a deeper look at it.

Sven,

Does this patch fix it for you?

Tom,

Correct me if I'm wrong, from what I can tell, all sums and keys are
u64 unless they are a string. Thus, I believe this patch should not
have any issues.

-- Steve

diff --git a/kernel/trace/tracing_map.c b/kernel/trace/tracing_map.c
index 9a1c22310323..9e31bfc818ff 100644
--- a/kernel/trace/tracing_map.c
+++ b/kernel/trace/tracing_map.c
@@ -148,8 +148,8 @@ static int tracing_map_cmp_atomic64(void *val_a, void *val_b)
 #define DEFINE_TRACING_MAP_CMP_FN(type)					\
 static int tracing_map_cmp_##type(void *val_a, void *val_b)		\
 {									\
-	type a = *(type *)val_a;					\
-	type b = *(type *)val_b;					\
+	type a = (type)(*(u64 *)val_a);					\
+	type b = (type)(*(u64 *)val_b);					\
 									\
 	return (a > b) ? 1 : ((a < b) ? -1 : 0);			\
 }

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-11 16:09   ` Steven Rostedt
@ 2019-12-11 16:37     ` Tom Zanussi
  2019-12-11 17:00       ` Steven Rostedt
  2019-12-11 18:14     ` Sven Schnelle
  2019-12-12 19:17     ` Tom Zanussi
  2 siblings, 1 reply; 16+ messages in thread
From: Tom Zanussi @ 2019-12-11 16:37 UTC (permalink / raw)
  To: Steven Rostedt, Sven Schnelle; +Cc: linux-trace-devel, LKML

Hi Steve, Sven,

On Wed, 2019-12-11 at 11:09 -0500, Steven Rostedt wrote:
> On Wed, 11 Dec 2019 10:35:57 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > Any thoughts on how to fix this? I'm not sure whether i fully
> > > understand the
> > > ftrace maps... ;-)  
> > 
> > Your analysis makes sense. I'll take a deeper look at it.
> 
> Sven,
> 
> Does this patch fix it for you?
> 
> Tom,
> 
> Correct me if I'm wrong, from what I can tell, all sums and keys are
> u64 unless they are a string. Thus, I believe this patch should not
> have any issues.

The sums are u64, but the keys may not be.  I'll take a look and see,
but I'm out today and won't be able to look into it until tomorrow, if
that's ok.

Tom

> 
> -- Steve
> 
> diff --git a/kernel/trace/tracing_map.c b/kernel/trace/tracing_map.c
> index 9a1c22310323..9e31bfc818ff 100644
> --- a/kernel/trace/tracing_map.c
> +++ b/kernel/trace/tracing_map.c
> @@ -148,8 +148,8 @@ static int tracing_map_cmp_atomic64(void *val_a,
> void *val_b)
>  #define DEFINE_TRACING_MAP_CMP_FN(type)				
> 	\
>  static int tracing_map_cmp_##type(void *val_a, void *val_b)		
> \
>  {									
> \
> -	type a = *(type *)val_a;					
> \
> -	type b = *(type *)val_b;					
> \
> +	type a = (type)(*(u64 *)val_a);				
> 	\
> +	type b = (type)(*(u64 *)val_b);				
> 	\
>  									
> \
>  	return (a > b) ? 1 : ((a < b) ? -1 : 0);			
> \
>  }

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-11 16:37     ` Tom Zanussi
@ 2019-12-11 17:00       ` Steven Rostedt
  2019-12-11 19:26         ` Tom Zanussi
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2019-12-11 17:00 UTC (permalink / raw)
  To: Tom Zanussi; +Cc: Sven Schnelle, linux-trace-devel, LKML

On Wed, 11 Dec 2019 10:37:18 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> > Tom,
> > 
> > Correct me if I'm wrong, from what I can tell, all sums and keys are
> > u64 unless they are a string. Thus, I believe this patch should not
> > have any issues.  
> 
> The sums are u64, but the keys may not be.  I'll take a look and see,

Are they? I see in create_key_field:

	key_size = ALIGN(key_size, sizeof(u64));

Which to me seems that we'll have nothing smaller than sizeof(u64).

> but I'm out today and won't be able to look into it until tomorrow, if
> that's ok.

No rush.

I'll start the testing of this patch if you come back and say its
fine ;-) That takes a full day.

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-11 16:09   ` Steven Rostedt
  2019-12-11 16:37     ` Tom Zanussi
@ 2019-12-11 18:14     ` Sven Schnelle
  2019-12-12 19:17     ` Tom Zanussi
  2 siblings, 0 replies; 16+ messages in thread
From: Sven Schnelle @ 2019-12-11 18:14 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: linux-trace-devel, LKML, Tom Zanussi

Hi Steven,

On Wed, Dec 11, 2019 at 11:09:59AM -0500, Steven Rostedt wrote:
> On Wed, 11 Dec 2019 10:35:57 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > Any thoughts on how to fix this? I'm not sure whether i fully understand the
> > > ftrace maps... ;-)  
> > 
> > Your analysis makes sense. I'll take a deeper look at it.
> 
> Does this patch fix it for you?

Yes, it does. Thanks for looking into this.

> Correct me if I'm wrong, from what I can tell, all sums and keys are
> u64 unless they are a string. Thus, I believe this patch should not
> have any issues.

I'll retest if Tom comes up with another patch.

Thanks,
Sven

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-11 17:00       ` Steven Rostedt
@ 2019-12-11 19:26         ` Tom Zanussi
  2019-12-12 18:07           ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: Tom Zanussi @ 2019-12-11 19:26 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Sven Schnelle, linux-trace-devel, LKML

Hi Steve,

On Wed, 2019-12-11 at 12:00 -0500, Steven Rostedt wrote:
> On Wed, 11 Dec 2019 10:37:18 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > > Tom,
> > > 
> > > Correct me if I'm wrong, from what I can tell, all sums and keys
> > > are
> > > u64 unless they are a string. Thus, I believe this patch should
> > > not
> > > have any issues.  
> > 
> > The sums are u64, but the keys may not be.  I'll take a look and
> > see,
> 
> Are they? I see in create_key_field:
> 
> 	key_size = ALIGN(key_size, sizeof(u64));
> 
> Which to me seems that we'll have nothing smaller than sizeof(u64).
> 

Yeah, that makes them effectively u64 in this case, so I'd agree.

Tom


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-11 19:26         ` Tom Zanussi
@ 2019-12-12 18:07           ` Steven Rostedt
  2019-12-12 19:15             ` Tom Zanussi
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2019-12-12 18:07 UTC (permalink / raw)
  To: Tom Zanussi; +Cc: Sven Schnelle, linux-trace-devel, LKML

On Wed, 11 Dec 2019 13:26:03 -0600
Tom Zanussi <zanussi@kernel.org> wrote:

> > Are they? I see in create_key_field:
> > 
> > 	key_size = ALIGN(key_size, sizeof(u64));
> > 
> > Which to me seems that we'll have nothing smaller than sizeof(u64).
> >   
> 
> Yeah, that makes them effectively u64 in this case, so I'd agree.

Hi Tom,

Does this mean it's good to go? It just passed my test suite, and I can
send it to Linus now.

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-12 18:07           ` Steven Rostedt
@ 2019-12-12 19:15             ` Tom Zanussi
  0 siblings, 0 replies; 16+ messages in thread
From: Tom Zanussi @ 2019-12-12 19:15 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Sven Schnelle, linux-trace-devel, LKML

Hi Steve,

On Thu, 2019-12-12 at 13:07 -0500, Steven Rostedt wrote:
> On Wed, 11 Dec 2019 13:26:03 -0600
> Tom Zanussi <zanussi@kernel.org> wrote:
> 
> > > Are they? I see in create_key_field:
> > > 
> > > 	key_size = ALIGN(key_size, sizeof(u64));
> > > 
> > > Which to me seems that we'll have nothing smaller than
> > > sizeof(u64).
> > >   
> > 
> > Yeah, that makes them effectively u64 in this case, so I'd agree.
> 
> Hi Tom,
> 
> Does this mean it's good to go? It just passed my test suite, and I
> can
> send it to Linus now.
> 

Yes, I think so.  I'll go and ack the patch.

Thanks,

Tom

> -- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-11 16:09   ` Steven Rostedt
  2019-12-11 16:37     ` Tom Zanussi
  2019-12-11 18:14     ` Sven Schnelle
@ 2019-12-12 19:17     ` Tom Zanussi
  2019-12-16 15:47       ` David Laight
  2 siblings, 1 reply; 16+ messages in thread
From: Tom Zanussi @ 2019-12-12 19:17 UTC (permalink / raw)
  To: Steven Rostedt, Sven Schnelle; +Cc: linux-trace-devel, LKML

On Wed, 2019-12-11 at 11:09 -0500, Steven Rostedt wrote:
> On Wed, 11 Dec 2019 10:35:57 -0500
> Steven Rostedt <rostedt@goodmis.org> wrote:
> 
> > > Any thoughts on how to fix this? I'm not sure whether i fully
> > > understand the
> > > ftrace maps... ;-)  
> > 
> > Your analysis makes sense. I'll take a deeper look at it.
> 
> Sven,
> 
> Does this patch fix it for you?
> 
> Tom,
> 
> Correct me if I'm wrong, from what I can tell, all sums and keys are
> u64 unless they are a string. Thus, I believe this patch should not
> have any issues.
> 
> -- Steve
> 
> diff --git a/kernel/trace/tracing_map.c b/kernel/trace/tracing_map.c
> index 9a1c22310323..9e31bfc818ff 100644
> --- a/kernel/trace/tracing_map.c
> +++ b/kernel/trace/tracing_map.c
> @@ -148,8 +148,8 @@ static int tracing_map_cmp_atomic64(void *val_a,
> void *val_b)
>  #define DEFINE_TRACING_MAP_CMP_FN(type)				
> 	\
>  static int tracing_map_cmp_##type(void *val_a, void *val_b)		
> \
>  {									
> \
> -	type a = *(type *)val_a;					
> \
> -	type b = *(type *)val_b;					
> \
> +	type a = (type)(*(u64 *)val_a);				
> 	\
> +	type b = (type)(*(u64 *)val_b);				
> 	\
>  									
> \
>  	return (a > b) ? 1 : ((a < b) ? -1 : 0);			
> \
>  }

Acked-by: Tom Zanussi <zanussi@kernel.org>


^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: ftrace histogram sorting broken on BE architecures
  2019-12-12 19:17     ` Tom Zanussi
@ 2019-12-16 15:47       ` David Laight
  2019-12-16 16:05         ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: David Laight @ 2019-12-16 15:47 UTC (permalink / raw)
  To: Tom Zanussi, Steven Rostedt, Sven Schnelle; +Cc: linux-trace-devel, LKML

> From: Tom Zanussi
> Sent: 12 December 2019 19:17
> On Wed, 2019-12-11 at 11:09 -0500, Steven Rostedt wrote:
> > On Wed, 11 Dec 2019 10:35:57 -0500
> > Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > > > Any thoughts on how to fix this? I'm not sure whether i fully
> > > > understand the
> > > > ftrace maps... ;-)
> > >
> > > Your analysis makes sense. I'll take a deeper look at it.
> >
> > Sven,
> >
> > Does this patch fix it for you?
> >
> > Tom,
> >
> > Correct me if I'm wrong, from what I can tell, all sums and keys are
> > u64 unless they are a string. Thus, I believe this patch should not
> > have any issues.
...
> > --- a/kernel/trace/tracing_map.c
> > +++ b/kernel/trace/tracing_map.c
> > @@ -148,8 +148,8 @@ static int tracing_map_cmp_atomic64(void *val_a,
> > void *val_b)
> >  #define DEFINE_TRACING_MAP_CMP_FN(type) 	\
> >  static int tracing_map_cmp_##type(void *val_a, void *val_b) \
> >  { \
> > -	type a = *(type *)val_a; \
> > -	type b = *(type *)val_b; \
> > +	type a = (type)(*(u64 *)val_a); 	\
> > +	type b = (type)(*(u64 *)val_b); 	\
> > \
> >  	return (a > b) ? 1 : ((a < b) ? -1 : 0); \
> >  }

That looks so horrid/wrong it can't be right on both BE and LE.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-16 15:47       ` David Laight
@ 2019-12-16 16:05         ` Steven Rostedt
  2019-12-16 17:06           ` David Laight
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2019-12-16 16:05 UTC (permalink / raw)
  To: David Laight; +Cc: Tom Zanussi, Sven Schnelle, linux-trace-devel, LKML

On Mon, 16 Dec 2019 15:47:12 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

> > From: Tom Zanussi
> > Sent: 12 December 2019 19:17
> > On Wed, 2019-12-11 at 11:09 -0500, Steven Rostedt wrote:  
> > > On Wed, 11 Dec 2019 10:35:57 -0500
> > > Steven Rostedt <rostedt@goodmis.org> wrote:
> > >  
> > > > > Any thoughts on how to fix this? I'm not sure whether i fully
> > > > > understand the
> > > > > ftrace maps... ;-)  
> > > >
> > > > Your analysis makes sense. I'll take a deeper look at it.  
> > >
> > > Sven,
> > >
> > > Does this patch fix it for you?
> > >
> > > Tom,
> > >
> > > Correct me if I'm wrong, from what I can tell, all sums and keys are
> > > u64 unless they are a string. Thus, I believe this patch should not
> > > have any issues.  
> ...
> > > --- a/kernel/trace/tracing_map.c
> > > +++ b/kernel/trace/tracing_map.c
> > > @@ -148,8 +148,8 @@ static int tracing_map_cmp_atomic64(void *val_a,
> > > void *val_b)
> > >  #define DEFINE_TRACING_MAP_CMP_FN(type) 	\
> > >  static int tracing_map_cmp_##type(void *val_a, void *val_b) \
> > >  { \
> > > -	type a = *(type *)val_a; \
> > > -	type b = *(type *)val_b; \
> > > +	type a = (type)(*(u64 *)val_a); 	\
> > > +	type b = (type)(*(u64 *)val_b); 	\
> > > \
> > >  	return (a > b) ? 1 : ((a < b) ? -1 : 0); \
> > >  }  
> 
> That looks so horrid/wrong it can't be right on both BE and LE.

Well, the original is obviously not right for both BE and LE, but the
fix is:

	type a = (type)(*(u64 *)val_a);

Which breaks down to:

		(u64 *)val_a - make val_a a pointer to a u64 number

all values were written as u64.

	u64 data = (u64)original_val_a

Where original_val_a could be a byte, short, int, long or long long.

Now that we have (u64 *)val_a, we dereference it:

		*(u64 *)val_a - which gives us a u64 number.

This u64 number should be the same as the u64 data.

Then we convert this as:

		(type)(*(u64 *)val_a)

Taking the u64 number we retrieved and converted it back to the
original type that was recorded.

In other words, if the following should work:


	type a = x;
	u64 data = (u64)a;
	u64 *ptr = &data;
	u64 b_data = *ptr;
	type b = (type)b_data;

If b == a above, then there should be nothing wrong with this patch.

The compiler should know how to map those type conversions properly for
both BE and LE.

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: ftrace histogram sorting broken on BE architecures
  2019-12-16 16:05         ` Steven Rostedt
@ 2019-12-16 17:06           ` David Laight
  2019-12-16 18:29             ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: David Laight @ 2019-12-16 17:06 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Tom Zanussi, Sven Schnelle, linux-trace-devel, LKML

From: Steven Rostedt
> Sent: 16 December 2019 16:06
> On Mon, 16 Dec 2019 15:47:12 +0000
> David Laight <David.Laight@ACULAB.COM> wrote:
> 
> > > From: Tom Zanussi
> > > Sent: 12 December 2019 19:17
> > > On Wed, 2019-12-11 at 11:09 -0500, Steven Rostedt wrote:
> > > > On Wed, 11 Dec 2019 10:35:57 -0500
> > > > Steven Rostedt <rostedt@goodmis.org> wrote:
> > > >
> > > > > > Any thoughts on how to fix this? I'm not sure whether i fully
> > > > > > understand the
> > > > > > ftrace maps... ;-)
> > > > >
> > > > > Your analysis makes sense. I'll take a deeper look at it.
> > > >
> > > > Sven,
> > > >
> > > > Does this patch fix it for you?
> > > >
> > > > Tom,
> > > >
> > > > Correct me if I'm wrong, from what I can tell, all sums and keys are
> > > > u64 unless they are a string. Thus, I believe this patch should not
> > > > have any issues.
> > ...
> > > > --- a/kernel/trace/tracing_map.c
> > > > +++ b/kernel/trace/tracing_map.c
> > > > @@ -148,8 +148,8 @@ static int tracing_map_cmp_atomic64(void *val_a,
> > > > void *val_b)
> > > >  #define DEFINE_TRACING_MAP_CMP_FN(type) 	\
> > > >  static int tracing_map_cmp_##type(void *val_a, void *val_b) \
> > > >  { \
> > > > -	type a = *(type *)val_a; \
> > > > -	type b = *(type *)val_b; \
> > > > +	type a = (type)(*(u64 *)val_a); 	\
> > > > +	type b = (type)(*(u64 *)val_b); 	\
> > > > \
> > > >  	return (a > b) ? 1 : ((a < b) ? -1 : 0); \
> > > >  }
> >
> > That looks so horrid/wrong it can't be right on both BE and LE.
> 
> Well, the original is obviously not right for both BE and LE, but the
> fix is:
> 
> 	type a = (type)(*(u64 *)val_a);
> 
> Which breaks down to:
> 
> 		(u64 *)val_a - make val_a a pointer to a u64 number
> 
> all values were written as u64.
> 
> 	u64 data = (u64)original_val_a
> 
> Where original_val_a could be a byte, short, int, long or long long.

I'd sort of guessed that, but then the pointer type passed to tracing_map_cmp_##type()
will always be 'u64 *' (since the field the address is taken of must be that type).
Then the (u64 *) casts are no longer needed.

Possibly you can just pass the u64 values to:
tracing_map_cmp_##type(type a, type b)
{
	return a > b ? 1 : a < b ? -1 : 0;
}

The high bit masking and sign extension is then implicit in the call.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-16 17:06           ` David Laight
@ 2019-12-16 18:29             ` Steven Rostedt
  2019-12-17 10:05               ` David Laight
  0 siblings, 1 reply; 16+ messages in thread
From: Steven Rostedt @ 2019-12-16 18:29 UTC (permalink / raw)
  To: David Laight; +Cc: Tom Zanussi, Sven Schnelle, linux-trace-devel, LKML

On Mon, 16 Dec 2019 17:06:50 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

> > Where original_val_a could be a byte, short, int, long or long long.  
> 
> I'd sort of guessed that, but then the pointer type passed to tracing_map_cmp_##type()
> will always be 'u64 *' (since the field the address is taken of must be that type).
> Then the (u64 *) casts are no longer needed.
> 
> Possibly you can just pass the u64 values to:
> tracing_map_cmp_##type(type a, type b)
> {
> 	return a > b ? 1 : a < b ? -1 : 0;
> }
> 
> The high bit masking and sign extension is then implicit in the call.

But these are used to pass into a compare function that takes compare
functions that are something other than numbers. They can be pointers
to strings.

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

* RE: ftrace histogram sorting broken on BE architecures
  2019-12-16 18:29             ` Steven Rostedt
@ 2019-12-17 10:05               ` David Laight
  2019-12-18 15:33                 ` Steven Rostedt
  0 siblings, 1 reply; 16+ messages in thread
From: David Laight @ 2019-12-17 10:05 UTC (permalink / raw)
  To: Steven Rostedt; +Cc: Tom Zanussi, Sven Schnelle, linux-trace-devel, LKML

From: Steven Rostedt
> Sent: 16 December 2019 18:29
> On Mon, 16 Dec 2019 17:06:50 +0000
> David Laight <David.Laight@ACULAB.COM> wrote:
> 
> > > Where original_val_a could be a byte, short, int, long or long long.
> >
> > I'd sort of guessed that, but then the pointer type passed to tracing_map_cmp_##type()
> > will always be 'u64 *' (since the field the address is taken of must be that type).
> > Then the (u64 *) casts are no longer needed.
> >
> > Possibly you can just pass the u64 values to:
> > tracing_map_cmp_##type(type a, type b)
> > {
> > 	return a > b ? 1 : a < b ? -1 : 0;
> > }
> >
> > The high bit masking and sign extension is then implicit in the call.
> 
> But these are used to pass into a compare function that takes compare
> functions that are something other than numbers. They can be pointers
> to strings.

In that case I think I'd embed the u64 inside a structure and pass the structure
address to the compare function.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)


^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: ftrace histogram sorting broken on BE architecures
  2019-12-17 10:05               ` David Laight
@ 2019-12-18 15:33                 ` Steven Rostedt
  0 siblings, 0 replies; 16+ messages in thread
From: Steven Rostedt @ 2019-12-18 15:33 UTC (permalink / raw)
  To: David Laight; +Cc: Tom Zanussi, Sven Schnelle, linux-trace-devel, LKML

On Tue, 17 Dec 2019 10:05:57 +0000
David Laight <David.Laight@ACULAB.COM> wrote:

> From: Steven Rostedt
> > Sent: 16 December 2019 18:29
> > On Mon, 16 Dec 2019 17:06:50 +0000
> > David Laight <David.Laight@ACULAB.COM> wrote:
> >   
> > > > Where original_val_a could be a byte, short, int, long or long long.  
> > >
> > > I'd sort of guessed that, but then the pointer type passed to tracing_map_cmp_##type()
> > > will always be 'u64 *' (since the field the address is taken of must be that type).
> > > Then the (u64 *) casts are no longer needed.
> > >
> > > Possibly you can just pass the u64 values to:
> > > tracing_map_cmp_##type(type a, type b)
> > > {
> > > 	return a > b ? 1 : a < b ? -1 : 0;
> > > }
> > >
> > > The high bit masking and sign extension is then implicit in the call.  
> > 
> > But these are used to pass into a compare function that takes compare
> > functions that are something other than numbers. They can be pointers
> > to strings.  
> 
> In that case I think I'd embed the u64 inside a structure and pass the structure
> address to the compare function.
> 

It's something we can clean up later. As this is going to be marked for
stable, and the code still works, I would like to keep the change as
simple as possible.

Thanks!

-- Steve

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, back to index

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-11 12:33 ftrace histogram sorting broken on BE architecures Sven Schnelle
2019-12-11 15:35 ` Steven Rostedt
2019-12-11 16:09   ` Steven Rostedt
2019-12-11 16:37     ` Tom Zanussi
2019-12-11 17:00       ` Steven Rostedt
2019-12-11 19:26         ` Tom Zanussi
2019-12-12 18:07           ` Steven Rostedt
2019-12-12 19:15             ` Tom Zanussi
2019-12-11 18:14     ` Sven Schnelle
2019-12-12 19:17     ` Tom Zanussi
2019-12-16 15:47       ` David Laight
2019-12-16 16:05         ` Steven Rostedt
2019-12-16 17:06           ` David Laight
2019-12-16 18:29             ` Steven Rostedt
2019-12-17 10:05               ` David Laight
2019-12-18 15:33                 ` Steven Rostedt

Linux-Trace-Devel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-trace-devel/0 linux-trace-devel/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-trace-devel linux-trace-devel/ https://lore.kernel.org/linux-trace-devel \
		linux-trace-devel@vger.kernel.org
	public-inbox-index linux-trace-devel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-trace-devel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git