Re: [PATCH 0/4] perf sched: Fix task state report

[Ze Gao]

On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <namhyung@xxxxxxxxxx> wrote:
>
> Hello,
>
> On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <zegao2021@xxxxxxxxx> wrote:
> >
> > Hi,
> >
> > The problems of task state report in both libtraceevent
> > and perf sched has been reported in [1]. In short, they
> > parsed the wrong state due to relying on the outdated
> > hardcoded state string to interpret the raw bitmask
> > from the record, which left the messes to maintain the
> > backward compatibilities for both tools.
> >
> > [1] has not managed to make itself into the kernel, the
> > problems and the solutions are well studied though.
> >
> > Luckily, as suggested by Steven, perf/libtraceevent
> > records the print format, especially the __print_flags()
> > part of the in-kernel tracepoint sched_switch in its
> > metadata, and we have a chance to build the state str
> > on the fly by parsing it.
> >
> > Now that libtraceevent has landed this solution in [2],
> > we now apply the same idea to perf as well.
>
> Thanks for your work.  But perf links libtraceevent
> conditionally so you need to make sure if it works without
> that too.

Yes, I've tested with NO_LIBTRACEEVENT=1, and it turns
out perf removes perf sched subcmd without libtraceevent,
which explains why the compiler does not complain no
evsel__intval() defined when !HAVE_LIBTRACEEVENT
given the fact so many references of evsel__intval() in
builtin-sched.c.

Here evsel__taskstate() uses the exact assumption as
evsel__intval(), so I put it next to it for clarity and it works
without a doubt.

> I think all libtraceevent related stuff should be in the
> util/trace-event.c which is included only if the library is
> available.  Maybe util/trace-event-parse.c is a better
> place but then you need to tweak the python-ext-sources
> and Makefile.perf for the case it's not available.

Thanks for pointing this out. I will do the hack if you insist
on this move :D. But I think the current version is clear
enough, otherwise we need to move all the parts guarded
by #ifdef HAVE_LIBTRACEEVENT out for complete decoupling.
What do you think of it?

Thanks,
        -- Ze

> Thanks,
> Namhyung
>
> >
> > Regards,
> >
> >         -- Ze
> >
> > [1]: https://lore.kernel.org/lkml/20230803083352.1585-1-zegao@xxxxxxxxxxx/
> > [2]: https://lore.kernel.org/linux-trace-devel/20231224140732.7d41698d@xxxxxxxxxxxxxxxxxxxx/
> >
> > Ze Gao (4):
> >   perf sched: Sync state char array with the kernel
> >   perf util: Add helpers to parse task state string from libtraceevent
> >   perf util: Add evsel__taskstate() to parse the task state info instead
> >   perf sched: Commit to evsel__taskstate() to parse task state info
> >
> >  tools/perf/builtin-sched.c |  57 +++------------
> >  tools/perf/util/evsel.c    | 146 +++++++++++++++++++++++++++++++++++++
> >  tools/perf/util/evsel.h    |   1 +
> >  3 files changed, 157 insertions(+), 47 deletions(-)
> >
> > --
> > 2.41.0
> >