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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Thu, 18 Jan 2024 11:15:31 +0800
From: Ze Gao <zegao2021@...il.com>
To: Namhyung Kim <namhyung@...nel.org>
Cc: Adrian Hunter <adrian.hunter@...el.com>, 
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>, 
	Arnaldo Carvalho de Melo <acme@...nel.org>, Ian Rogers <irogers@...gle.com>, Ingo Molnar <mingo@...hat.com>, 
	Jiri Olsa <jolsa@...nel.org>, Mark Rutland <mark.rutland@....com>, 
	Peter Zijlstra <peterz@...radead.org>, Steven Rostedt <rostedt@...dmis.org>, linux-kernel@...r.kernel.org, 
	linux-perf-users@...r.kernel.org, Ze Gao <zegao@...cent.com>
Subject: Re: [PATCH 0/4] perf sched: Fix task state report

On Thu, Jan 18, 2024 at 11:00 AM Ze Gao <zegao2021@...il.com> wrote:
>
> On Wed, Jan 17, 2024 at 9:35 AM Namhyung Kim <namhyung@...nel.org> wrote:
> >
> > Hello,
> >
> > On Sun, Jan 14, 2024 at 11:23 PM Ze Gao <zegao2021@...il.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
> > >