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;
Date:   Thu, 25 May 2023 09:53:29 -0700
From:   Beau Belgrave <beaub@linux.microsoft.com>
To:     sunliming <sunliming@kylinos.cn>
Cc:     mhiramat@kernel.org, rostedt@goodmis.org,
        linux-trace-kernel@vger.kernel.org, linux-kernel@vger.kernel.org,
        kelulanainsley@gmail.com
Subject: Re: [PATCH] tracing/user_events: Fix the order of the fields in the
 trace output
Message-ID: <20230525165329.GA82@W11-BEAU-MD.localdomain>
References: <20230525054032.29392-1-sunliming@kylinos.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230525054032.29392-1-sunliming@kylinos.cn>
Precedence: bulk

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@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_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