linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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
Date: Thu, 2 Jun 2022 00:13:31 +0900	[thread overview]
Message-ID: <20220602001331.fab92b2dcd2c9aaad800ddae@kernel.org> (raw)
In-Reply-To: <CAEr6+EAU_FUvCqZqqpp_hC5ichV9WJ+Up+N6ya5KW_CbqbRGFA@mail.gmail.com>

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>

  reply	other threads:[~2022-06-01 15:13 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-12 17:00 [PATCH v10 0/4] trace: Introduce objtrace trigger to trace the kernel object Jeff Xie
2022-05-12 17:00 ` [PATCH v10 1/4] trace: Add trace any " Jeff Xie
2022-05-13  2:01   ` kernel test robot
2022-05-18 13:48     ` Masami Hiramatsu
2022-05-18 14:17       ` Jeff Xie
2022-05-26 23:42         ` Masami Hiramatsu
2022-05-27 12:00           ` Jeff Xie
2022-05-13  4:50   ` kernel test robot
2022-05-21 15:25   ` Masami Hiramatsu
2022-05-21 17:25     ` Jeff Xie
2022-05-12 17:00 ` [PATCH v10 2/4] trace/objtrace: Get the value of the object Jeff Xie
2022-05-22 14:22   ` Masami Hiramatsu
2022-05-23  1:12     ` Jeff Xie
2022-05-31  7:13     ` Jeff Xie
2022-06-01 15:13       ` Masami Hiramatsu [this message]
2022-06-02 16:23         ` Jeff Xie
2022-05-12 17:00 ` [PATCH v10 3/4] trace/objtrace: Add testcases for objtrace Jeff Xie
2022-05-12 17:00 ` [PATCH v10 4/4] trace/objtrace: Add documentation " Jeff Xie

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20220602001331.fab92b2dcd2c9aaad800ddae@kernel.org \
    --to=mhiramat@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=rostedt@goodmis.org \
    --cc=xiehuan09@gmail.com \
    --cc=zanussi@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
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).