Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Thu, 2 Jun 2022 00:13:31 +0900
From:   Masami Hiramatsu (Google) <mhiramat@kernel.org>
To:     Jeff Xie <xiehuan09@gmail.com>
Cc:     Steven Rostedt <rostedt@goodmis.org>, mingo@redhat.com,
        Tom Zanussi <zanussi@kernel.org>, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v10 2/4] trace/objtrace: Get the value of the object
Message-Id: <20220602001331.fab92b2dcd2c9aaad800ddae@kernel.org>
In-Reply-To: <CAEr6+EAU_FUvCqZqqpp_hC5ichV9WJ+Up+N6ya5KW_CbqbRGFA@mail.gmail.com>
References: <20220512170008.1301613-1-xiehuan09@gmail.com>
        <20220512170008.1301613-3-xiehuan09@gmail.com>
        <20220522232216.bb1451fb6efc18c2bccc8d09@kernel.org>
        <CAEr6+EAU_FUvCqZqqpp_hC5ichV9WJ+Up+N6ya5KW_CbqbRGFA@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Tue, 31 May 2022 15:13:24 +0800
Jeff Xie <xiehuan09@gmail.com> wrote:

> Hi Masami and steve,
> 
> On Sun, May 22, 2022 at 10:22 PM Masami Hiramatsu <mhiramat@kernel.org> wrote:
> >
> > Hi Jeff,
> >
> > On Fri, 13 May 2022 01:00:06 +0800
> > Jeff Xie <xiehuan09@gmail.com> wrote:
> >
> > [...]
> > > @@ -175,9 +271,27 @@ trace_object_trigger(struct event_trigger_data *data,
> > >
> > >       field = obj_data->field;
> > >       memcpy(&obj, rec + field->offset, sizeof(obj));
> > > -     set_trace_object(obj, tr);
> > > +     /* set the offset from the special object and the type size of the value*/
> > > +     set_trace_object(obj, obj_data->obj_offset,
> > > +                     obj_data->obj_value_type_size, tr);
> > >  }
> > >
> > > +static const struct objtrace_fetch_type objtrace_fetch_types[] = {
> > > +     {"u8", 1},
> > > +     {"s8", 1},
> > > +     {"x8", 1},
> > > +     {"u16", 2},
> > > +     {"s16", 2},
> > > +     {"x16", 2},
> > > +     {"u32", 4},
> > > +     {"s32", 4},
> > > +     {"x32", 4},
> > > +     {"u64", 8},
> > > +     {"s64", 8},
> > > +     {"x64", 8},
> >
> > Hmm, as far as I can see, you don't distinguish the prefix 'u','s','x'.
> > If so, please support only 'x' at this moment. kprobe events supports
> > those types, and it distinguishes the types when printing the logged
> > data. E.g. 's16' shows '-1' for 0xffff, but 'x16' shows '0xffff'.
> > You can add another patch to support such different types afterwards.
> 
> I feel to let the objtrace trigger to distinguish the prefix 'u', 's',
> 'x', It seems a very challenging work ;-)
> I spent a lot of time thinking, I would like to add a callback
> function(print function) in the struct trace_object_entry  for each
> data type.
> Not sure if this is possible or allowed, as I haven't seen any example
> like this to add function in the  struct *_entry  ;-)

Hmm, I don't recommend this, becuase this event record can be exposed
to user space as binary data. So please do not put such a function
pointer which will be used in the ftrace directly.
Instead, add a new event type of the object-trace for each type-prefix,
since each of them has different print-fmt.

Anyway I would like to ask you is to share the next version of the
series without that improvement. You can improve it after merging the
basic feature. No need to stop the series until all possible feature
set are implemented (unless it will change the user-exposed interface
much.)

> 
> The following is part of the code I have prepared. I don't know if you
> can give any suggestions or wait until I submit the next version to
> discuss.

But thanks for sharing the code. This helps me to understand what you
are trying :)

Thank you,


> 
> <snip>
> diff --git a/kernel/trace/trace_entries.h b/kernel/trace/trace_entries.h
> index 2407c45a568c..5f8289e26f91 100644
> --- a/kernel/trace/trace_entries.h
> +++ b/kernel/trace/trace_entries.h
> @@ -414,6 +414,7 @@ FTRACE_ENTRY(object, trace_object_entry,
>                 __field(        unsigned long,          parent_ip       )
>                 __field(        unsigned long,          object          )
>                 __field(        unsigned long,          value           )
> +               __field(        unsigned long,          print           )
>         ),
> 
> +/* get the type size for the special object */
> +struct objtrace_fetch_type {
> +       char *name;
> +       int type_size;
> +       int is_signed;
> +       print_type_func_t       print;
> +};
> +
> 
>  static const struct objtrace_fetch_type objtrace_fetch_types[] = {
> -       {"x8", 1},
> -       {"x16", 2},
> -       {"x32", 4},
> -       {"x64", 8},
> -       {NULL, 0}
> +       {"u8", 1, 0, PRINT_TYPE_FUNC_NAME(u8)},
> +       {"s8", 1, 1, PRINT_TYPE_FUNC_NAME(s8)},
> +       {"x8", 1, 0, PRINT_TYPE_FUNC_NAME(x8)},
> +       {"u16", 2, 0, PRINT_TYPE_FUNC_NAME(u16)},
> +       {"s16", 2, 1, PRINT_TYPE_FUNC_NAME(s16)},
> +       {"x16", 2, 0, PRINT_TYPE_FUNC_NAME(x16)},
> +       {"u32", 4, 0, PRINT_TYPE_FUNC_NAME(u32)},
> +       {"s32", 4, 1, PRINT_TYPE_FUNC_NAME(s32)},
> +       {"x32", 4, 0, PRINT_TYPE_FUNC_NAME(x32)},
> +       {"u64", 8, 0, PRINT_TYPE_FUNC_NAME(u64)},
> +       {"s64", 8, 1, PRINT_TYPE_FUNC_NAME(s64)},
> +       {"x64", 8, 1, PRINT_TYPE_FUNC_NAME(x64)},
> +       {NULL, 0, 0, NULL}
>  };
> </snip>
> 
> > > +     {}
> >
> > If this array is null terminated, please explictly do that, like
> >
> >         {NULL, 0},
> >
> > for readability.
> >
> > Thank you,
> >
> > --
> > Masami Hiramatsu (Google) <mhiramat@kernel.org>
> 
> Thanks,
> JeffXie


-- 
Masami Hiramatsu (Google) <mhiramat@kernel.org>