* 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 related [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, other threads:[~2019-12-18 15:33 UTC | newest]
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
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).