Re: [PATCH] tracing/user_events: Fix the order of the fields in the trace output

[sunliming]

OK，I got it.

Beau Belgrave <beaub@xxxxxxxxxxxxxxxxxxx> 于2023年5月26日周五 00:53写道：
>
> On Thu, May 25, 2023 at 01:40:32PM +0800, sunliming wrote:
> > Commit 4bec284cc0b9 ("tracing/user_events: Use print_format_fields() for
> > trace output") use print_event_fields() as safe and gives user readable
> > output. However, due to the insertion of the struct ftrace_event_field
> > structure into the field linked list from the header, the trace output
> > oder of fields of user events is reversed. Fix the problem by insertint
> > to the tail of field linked list.
> >
> > Signed-off-by: sunliming <sunliming@xxxxxxxxxx>
> > ---
> >  kernel/trace/trace_events_user.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/kernel/trace/trace_events_user.c b/kernel/trace/trace_events_user.c
> > index aacd22c1e9f8..e9e2ec3c7613 100644
> > --- a/kernel/trace/trace_events_user.c
> > +++ b/kernel/trace/trace_events_user.c
> > @@ -972,7 +972,7 @@ static int user_event_add_field(struct user_event *user, const char *type,
> >       if (filter_type == FILTER_OTHER)
> >               field->filter_type = filter_assign_type(type);
> >
> > -     list_add(&field->link, &user->fields);
> > +     list_add_tail(&field->link, &user->fields);
> >
> >       /*
> >        * Min size from user writes that are required, this does not include
> > --
> > 2.25.1
>
> Thanks for the patch, however, this breaks the tracefs format file. The
> fields are required to be put in backwards since it walks them
> backwards.
>
> Example using this:
> echo 'u:test u32 a; u32 b;' > dynamic_events
> cat /sys/kernel/tracing/events/user_events/test/format
>
> Before this change:
> name: test
> ...
> format:
>         field:unsigned short common_type;       offset:0;       size:2; signed:0;
>         field:unsigned char common_flags;       offset:2;       size:1; signed:0;
>         field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
>         field:int common_pid;   offset:4;       size:4; signed:1;
>
>         field:u32 a;    offset:8;       size:4; signed:0;
>         field:u32 b;    offset:12;      size:4; signed:0;
>
> print fmt: "a=%u b=%u", REC->a, REC->b
>
> After this change:
> name: test
> ...
> format:
>         field:unsigned short common_type;       offset:0;       size:2; signed:0;
>         field:unsigned char common_flags;       offset:2;       size:1; signed:0;
>         field:unsigned char common_preempt_count;       offset:3;       size:1; signed:0;
>         field:int common_pid;   offset:4;       size:4; signed:1;
>
>         field:u32 b;    offset:12;      size:4; signed:0;
>         field:u32 a;    offset:8;       size:4; signed:0;
>
> print fmt: "b=%u a=%u", REC->b, REC->a
>
> I do agree though, that print_fields() is doing it backwards. Can you
> please fix the print_fields() function instead? (It should walk the list
> of fields backwards like tracefs format file does).
>
> Steven can then Ack that work, since it's isolated there.
>
> Thanks,
> -Beau