Re: [PATCH v6 1/2] libtraceevent: sync state char array with the kernel

[Steven Rostedt <rostedt@goodmis.org>]

On Thu, 7 Dec 2023 10:52:35 +0800
Ze Gao <zegao2021@gmail.com> wrote:

> On Thu, Dec 7, 2023 at 6:54 AM Steven Rostedt <rostedt@goodmis.org> wrote:
> >
> > On Wed,  2 Aug 2023 23:09:54 -0400
> > Ze Gao <zegao2021@gmail.com> wrote:
> >  
> > As your update never made it into the kernel I never pulled these patches
> > in. But we do have an issue with the state getting out of sync with what
> > the kernel actually uses.
> >
> > I did this "hack" but it seems to work. What I do is to look at the
> > sched_switch event print parsing arguments, find the __print_flags
> > arguments for the "prev_state" field. And then parse them, creating a new
> > output and I use that for the write_state() function.  
> 
> Thanks for your updates, I remembered to reply with my "hacks"  which
> fix both libtraceevent and "perf sched timehist --state".
> FYI,  for libtraceevent in this thread:
> https://lore.kernel.org/all/CAD8CoPDqZrd2HOUCVkrOq9JgJEQwQnqBn9zD1A3M4aKLZ4mLvg@mail.gmail.com

And somehow I missed this reply :-/

Sorry about that.

> and here for perf:
> https://lore.kernel.org/all/CAD8CoPDqZrd2HOUCVkrOq9JgJEQwQnqBn9zD1A3M4aKLZ4mLvg@mail.gmail.com/
> 
> My only concern at that time was the very hacky way to pinpoint the
> TEP_PRINT_FLAGS due to the lack of knowledge of libtraceevent design.
> So I was not actively seeking to push my fixes to the upstream.
> 
> > I guess this should work for you too?  
> 
> I'll give it a try definitely.
> 
> > I still need to pull in this patch and use that as the default. I'll do
> > that before applying this one, and have your above update be the default.
> >
> > Thanks!  
> 
> 
> > -- Steve
> >
> > diff --git a/plugins/plugin_sched_switch.c b/plugins/plugin_sched_switch.c
> > index 8752caea5c38..760d224f5507 100644
> > --- a/plugins/plugin_sched_switch.c
> > +++ b/plugins/plugin_sched_switch.c
> > @@ -9,13 +9,132 @@
> >  #include "event-parse.h"
> >  #include "trace-seq.h"
> >
> > -static void write_state(struct trace_seq *s, int val)
> > +static const char *convert_sym(struct tep_print_flag_sym *sym)
> >  {
> > -       const char states[] = "SDTtZXxW";
> > +       static char save_states[33];  
> 
> how about a gobal counter instead of fixed sized constant here,
> and we can warn if it outbounds, u know, just in case.

I picked 33 to be 1 greater than 32 as that's the max size of prev_state on
32 bit systems. I should change that to be a macro though.

> 
> > +       memset(save_states, 0, sizeof(save_states));
> > +
> > +       /* This is the flags for the prev_state_field, now make them into a string */
> > +       for (; sym; sym = sym->next) {
> > +               long bitmask = strtoul(sym->value, NULL, 0);
> > +               int i;
> > +
> > +               for (i = 0; !(bitmask & 1); i++)
> > +                       bitmask >>= 1;
> > +
> > +               if (i > 32)
> > +                       continue; // warn?
> > +
> > +               save_states[i] = sym->str[0];
> > +       }
> > +
> > +       return save_states;
> > +}
> > +


> > +static void write_state(struct trace_seq *s, struct tep_format_field *field,
> > +                       struct tep_record *record)
> > +{
> > +       static struct tep_format_field *prev_state_field;
> > +       static const char *states;
> > +       unsigned long long val;
> >         int found = 0;
> > +       int len;
> >         int i;
> >
> > -       for (i = 0; i < (sizeof(states) - 1); i++) {
> > +       if (field != prev_state_field) {
> > +               states = get_states(field);  
> 
> I think we only need to build the task_state_char_array once,
> and there are no other possibilities here. Correct me if I'm stupid :)

libtracecmd can be used for multiple trace.dat files, that in theory can
have more than one version of sched_switch. And yes this can be called with
two different versions of prev_state.

  trace-cmd report -i trace1.dat -i trace2.dat

This is commonly done to see host and guest trace data interleaved, where
the host and guest could have different versions of sched_switch.

> So here we can check if the char array is built, if so, we can
> directly look it up in the mapping, right?
> 
> > +               if (!states)
> > +                       states = "SDTtZXxW";  
> 
> I think I've done experiments on the very old kernel in which the TP_print
> is brought in the sched_switch tracepoint. The "hack" works for it too,
> so this should not happen. Anyway, I'll give it a try and keep this

I agree that "this should not happen" but I prefer to be safe than sorry,
and always try to write robust code. If in never happens we just waste a
branch, if it does happen, it can give some weird result. I don't know what
is in the future.

> thread updated
> if anything goes wrong.

Thanks,

-- Steve