Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20230525054032.29392-1-sunliming@kylinos.cn> <20230525165329.GA82@W11-BEAU-MD.localdomain>
In-Reply-To: <20230525165329.GA82@W11-BEAU-MD.localdomain>
From:   sunliming <kelulanainsley@gmail.com>
Date:   Fri, 26 May 2023 09:05:49 +0800
Message-ID: <CAJncD7SnEr9FSz7VwDx3k9jeamDqAe0e1Ce+9TubjDEO3S0eJg@mail.gmail.com>
Subject: Re: [PATCH] tracing/user_events: Fix the order of the fields in the
 trace output
To:     Beau Belgrave <beaub@linux.microsoft.com>
Cc:     mhiramat@kernel.org, rostedt@goodmis.org,
        linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

OK=EF=BC=8CI got it.

Beau Belgrave <beaub@linux.microsoft.com> =E4=BA=8E2023=E5=B9=B45=E6=9C=882=
6=E6=97=A5=E5=91=A8=E4=BA=94 00:53=E5=86=99=E9=81=93=EF=BC=9A
>
> On Thu, May 25, 2023 at 01:40:32PM +0800, sunliming wrote:
> > Commit 4bec284cc0b9 ("tracing/user_events: Use print_format_fields() fo=
r
> > 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@kylinos.cn>
> > ---
> >  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_even=
ts_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 =3D=3D FILTER_OTHER)
> >               field->filter_type =3D 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 inc=
lude
> > --
> > 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; s=
igned:0;
>         field:unsigned char common_flags;       offset:2;       size:1; s=
igned:0;
>         field:unsigned char common_preempt_count;       offset:3;       s=
ize: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=3D%u b=3D%u", REC->a, REC->b
>
> After this change:
> name: test
> ...
> format:
>         field:unsigned short common_type;       offset:0;       size:2; s=
igned:0;
>         field:unsigned char common_flags;       offset:2;       size:1; s=
igned:0;
>         field:unsigned char common_preempt_count;       offset:3;       s=
ize: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=3D%u a=3D%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