ftrace histogram sorting broken on BE architecures

ftrace histogram sorting broken on BE architecures

[Sven Schnelle]

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

Re: ftrace histogram sorting broken on BE architecures

[Steven Rostedt]

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

Re: ftrace histogram sorting broken on BE architecures

[Steven Rostedt]

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);			\
 }

Re: ftrace histogram sorting broken on BE architecures

[Tom Zanussi]

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);			
> \
>  }

Re: ftrace histogram sorting broken on BE architecures

[Steven Rostedt]

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

Re: ftrace histogram sorting broken on BE architecures

[Tom Zanussi]

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

Re: ftrace histogram sorting broken on BE architecures

[Steven Rostedt]

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

Re: ftrace histogram sorting broken on BE architecures

[Tom Zanussi]

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

Re: ftrace histogram sorting broken on BE architecures

[Sven Schnelle]

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

Re: ftrace histogram sorting broken on BE architecures

[Tom Zanussi]

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>

RE: ftrace histogram sorting broken on BE architecures

[David Laight]

> 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)

Re: ftrace histogram sorting broken on BE architecures

[Steven Rostedt]

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

RE: ftrace histogram sorting broken on BE architecures

[David Laight]

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)

Re: ftrace histogram sorting broken on BE architecures

[Steven Rostedt]

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

RE: ftrace histogram sorting broken on BE architecures

[David Laight]

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)

Re: ftrace histogram sorting broken on BE architecures

[Steven Rostedt]

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