Received-SPF: pass (google.com: domain of linux-kernel+bounces-32294-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Precedence: bulk
MIME-Version: 1.0
References: <20240115072306.303993-1-zegao@tencent.com> <CAM9d7cgXKaKTnfx9YJae2qqv0pXtWLVQPBN=2H0WZkenBjHFnQ@mail.gmail.com>
 <CAD8CoPA6xBT9hbGZoviyyQm1bwn2Oh=v8QCMjxkMMOSN2zfC-A@mail.gmail.com>
 <CAD8CoPA0YgVHVW1Ya1O0-vhmpCbiw2h3i29ayzKmg4YTu-zeKQ@mail.gmail.com>
 <CAM9d7ciiKeVJzCjNyJp8hDsMe2ZiVyOKT3ses_SQMfdEQnWiog@mail.gmail.com>
 <CAD8CoPCSDcB+-qvwEZGOrdi01C3W8N3dYN3XQUQ2jrpn1mSf=Q@mail.gmail.com> <CAM9d7cigWxAN_nmT2rO-o7YG0hkmPNjDPeLtsn=z1YqRSjMEjA@mail.gmail.com>
In-Reply-To: <CAM9d7cigWxAN_nmT2rO-o7YG0hkmPNjDPeLtsn=z1YqRSjMEjA@mail.gmail.com>
From: Ze Gao <zegao2021@gmail.com>
Date: Mon, 22 Jan 2024 11:08:15 +0800
Message-ID: <CAD8CoPChgx_h9KU4nBJ+0__g6J=LvTBuMdbc0-F0dHz4=kjY0g@mail.gmail.com>
Subject: Re: [PATCH 0/4] perf sched: Fix task state report
To: Namhyung Kim <namhyung@kernel.org>
Cc: Adrian Hunter <adrian.hunter@intel.com>, 
	Alexander Shishkin <alexander.shishkin@linux.intel.com>, 
	Arnaldo Carvalho de Melo <acme@kernel.org>, Ian Rogers <irogers@google.com>, Ingo Molnar <mingo@redhat.com>, 
	Jiri Olsa <jolsa@kernel.org>, Mark Rutland <mark.rutland@arm.com>, 
	Peter Zijlstra <peterz@infradead.org>, Steven Rostedt <rostedt@goodmis.org>, linux-kernel@vger.kernel.org, 
	linux-perf-users@vger.kernel.org, Ze Gao <zegao@tencent.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Sat, Jan 20, 2024 at 6:45=E2=80=AFAM Namhyung Kim <namhyung@kernel.org> =
wrote:
>
> On Thu, Jan 18, 2024 at 5:54=E2=80=AFPM Ze Gao <zegao2021@gmail.com> wrot=
e:
> >
> > On Fri, Jan 19, 2024 at 7:53=E2=80=AFAM Namhyung Kim <namhyung@kernel.o=
rg> wrote:
> > >
> > > On Wed, Jan 17, 2024 at 7:15=E2=80=AFPM Ze Gao <zegao2021@gmail.com> =
wrote:
> > > >
> > > > On Thu, Jan 18, 2024 at 11:00=E2=80=AFAM Ze Gao <zegao2021@gmail.co=
m> wrote:
> > > > >
> > > > > On Wed, Jan 17, 2024 at 9:35=E2=80=AFAM Namhyung Kim <namhyung@ke=
rnel.org> wrote:
> > > > > >
> > > > > > Hello,
> > > > > >
> > > > > > On Sun, Jan 14, 2024 at 11:23=E2=80=AFPM Ze Gao <zegao2021@gmai=
l.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=3D1, 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