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

[Namhyung Kim <namhyung@kernel.org>]

On Wed, Jan 17, 2024 at 7:15 PM Ze Gao <zegao2021@gmail.com> wrote:
>
> On Thu, Jan 18, 2024 at 11:00 AM Ze Gao <zegao2021@gmail.com> wrote:
> >
> > On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > Hello,
> > >
> > > On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <zegao2021@gmail.com> 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,
>
> FWIW,  commit 378ef0f5d9d7f4 ("perf build: Use libtraceevent
> from the system") has proved this as well.

Right, but I think we can enable perf sched without libtraceevent
for minimal features like record only.  But that doesn't belong to
this change set.

>
> > 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?

Oh, I realized that all the affected codes are under the #ifdef
properly then maybe it's ok for now.  But I prefer moving the
code if you're ok.  Maybe I can accept this code as is and you
can work on the refactoring later.  Does that work for you?

Thanks,
Namhyung