Messages in this thread | | | From | Namhyung Kim <> | Date | Thu, 18 Jan 2024 15:53:34 -0800 | Subject | Re: [PATCH 0/4] perf sched: Fix task state report |
| |
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
| |