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

  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.

Regards,

        -- Ze

[1]: https://lore.kernel.org/lkml/20230803083352.1585-1-zegao@tencent.com/
[2]: https://lore.kernel.org/linux-trace-devel/20231224140732.7d41698d@rorschach.local.home/

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(-)
  

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.

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,
Namhyung

>
> Regards,
>
>         -- Ze
>
> [1]: https://lore.kernel.org/lkml/20230803083352.1585-1-zegao@tencent.com/
> [2]: https://lore.kernel.org/linux-trace-devel/20231224140732.7d41698d@rorschach.local.home/
>
> 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
>
  

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,
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@tencent.com/
> > [2]: https://lore.kernel.org/linux-trace-devel/20231224140732.7d41698d@rorschach.local.home/
> >
> > 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
> >
  

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.

Regards,
        -- Ze

> 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@tencentcom/
> > > [2]: https://lore.kernel.org/linux-trace-devel/20231224140732.7d41698d@rorschach.local.home/
> > >
> > > 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
> > >
  

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
  

On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> 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@kernelorg> 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

Sounds great!

> can work on the refactoring later.  Does that work for you?

Absolutely! Will send the following refactoring patches soon. :D

Cheers,
        -- Ze

> Thanks,
> Namhyung
  

On Thu, Jan 18, 2024 at 5:54 PM Ze Gao <zegao2021@gmail.com> wrote:
>
> On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > 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
>
> Sounds great!
>
> > can work on the refactoring later.  Does that work for you?
>
> Absolutely! Will send the following refactoring patches soon. :D

Thanks, but your patches don't apply cleanly.  Could you please
rebase it onto the current perf-tools-next tree?

Thanks,
Namhyung
  

On Sat, Jan 20, 2024 at 6:45 AM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Thu, Jan 18, 2024 at 5:54 PM Ze Gao <zegao2021@gmail.com> wrote:
> >
> > On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <namhyung@kernel.org> wrote:
> > >
> > > 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
> >
> > Sounds great!
> >
> > > can work on the refactoring later.  Does that work for you?
> >
> > Absolutely! Will send the following refactoring patches soon. :D
>
> Thanks, but your patches don't apply cleanly.  Could you please
> rebase it onto the current perf-tools-next tree?

Oops, that is kinda weird. I've tested and managed to cherry-picked all 4
patches onto branch perf-tools-next in [1], with no conflicts being
hit.  Maybe I used the wrong branch tip?

FWIW:  the tip I rebase onto is

d988c9f511af (perf/perf-tools-next) MAINTAINERS: Add Namhyung as
tools/perf/ co-maintainer

[1]:  https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/

Regards,
        -- Ze

> Thanks,
> Namhyung
  

On Mon, Jan 22, 2024 at 11:08 AM Ze Gao <zegao2021@gmail.com> wrote:
>
> On Sat, Jan 20, 2024 at 6:45 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Thu, Jan 18, 2024 at 5:54 PM Ze Gao <zegao2021@gmail.com> wrote:
> > >
> > > On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <namhyung@kernelorg> wrote:
> > > >
> > > > 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
> > >
> > > Sounds great!
> > >
> > > > can work on the refactoring later.  Does that work for you?
> > >
> > > Absolutely! Will send the following refactoring patches soon. :D
> >
> > Thanks, but your patches don't apply cleanly.  Could you please
> > rebase it onto the current perf-tools-next tree?
>
> Oops, that is kinda weird. I've tested and managed to cherry-picked all 4
> patches onto branch perf-tools-next in [1], with no conflicts being
> hit.  Maybe I used the wrong branch tip?

For reference, it ends up like:
    https://github.com/zegao96/linux/commits/perf-tools-next/

> FWIW:  the tip I rebase onto is
>
> d988c9f511af (perf/perf-tools-next) MAINTAINERS: Add Namhyung as
> tools/perf/ co-maintainer
>
> [1]:  https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/
>
> Regards,
>         -- Ze
>
> > Thanks,
> > Namhyung
  

On Mon, Jan 22, 2024 at 11:08 AM Ze Gao <zegao2021@gmail.com> wrote:
>
> On Sat, Jan 20, 2024 at 6:45 AM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Thu, Jan 18, 2024 at 5:54 PM Ze Gao <zegao2021@gmail.com> wrote:
> > >
> > > On Fri, Jan 19, 2024 at 7:53 AM Namhyung Kim <namhyung@kernelorg> wrote:
> > > >
> > > > 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
> > >
> > > Sounds great!
> > >
> > > > can work on the refactoring later.  Does that work for you?
> > >
> > > Absolutely! Will send the following refactoring patches soon. :D
> >
> > Thanks, but your patches don't apply cleanly.  Could you please
> > rebase it onto the current perf-tools-next tree?
>
> Oops, that is kinda weird. I've tested and managed to cherry-picked all 4
> patches onto branch perf-tools-next in [1], with no conflicts being

Please forget about this. Looks like git-cherry-pick is smarter to
figure out where to
apply when in the same repo ( or than git-am ?).

Anyway I've resent a v2 series:
    https://lore.kernel.org/all/20240122070859.1394479-2-zegao@tencent.com/

Tested and verified. Hope this time i don't mess it up :)

Thanks,
        -- Ze

> hit.  Maybe I used the wrong branch tip?
>
> FWIW:  the tip I rebase onto is
>
> d988c9f511af (perf/perf-tools-next) MAINTAINERS: Add Namhyung as
> tools/perf/ co-maintainer
>
> [1]:  https://git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/
>
> Regards,
>         -- Ze
>
> > Thanks,
> > Namhyung