Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file"

Hi Salvatore, Andrey,

On Fri, Nov 20, 2020 at 04:53:17PM +0100, Salvatore Bonaccorso wrote:

Hi Andrey,

On Fri, Nov 20, 2020 at 03:29:39PM +0100, Andrey Zhizhikin wrote:

quoted

Hello Salvatore,

On Fri, Nov 20, 2020 at 2:34 PM Salvatore Bonaccorso [off-list ref] wrote:

quoted

Hi Andrey,

On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote:

quoted

On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso [off-list ref] wrote:

quoted

This reverts commit 168200b6d6ea0cb5765943ec5da5b8149701f36a upstream.
(but only from 4.19.y)

This revert would fail the build of 4.19.y with gcc10, I believe the
original commit was introduced to address exactly this case. If this
is intended behavior that 4.19.y is not compiled with newer gcc
versions - then this revert is OK.

TTBOMK, this would not regress the build for newer gcc (specifically
gcc10) as 4.19.158 is failing perf tool builds there as well (without
the above commit reverted). Just as an example v4.19.y does not have
cff20b3151cc ("perf tests bp_account: Make global variable static")
which is there in v5.6-rc6 to fix build failures with 10.0.1.

But it did regress builds with older gcc's as for instance used in
Debian buster (gcc 8.3.0) since 4.19.152.

Do I possibly miss something? If there is a solution to make it build
with newer GCCs and *not* regress previously working GCC versions then
this is surely the best outcome though.

I guess (and from what I understand in Leo's reply), porting of
95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to
traceID-metadata") should solve the issue for both older and newer gcc
versions.

The breakage is now in
[tools/perf/util/cs-etm-decoder/cs-etm-decoder.c] file (which uses
traceid_list inside). This is solved with the above commit, which
concealed traceid_list internally inside [tools/perf/util/cs-etm.c]
file and exposed to [tools/perf/util/cs-etm-decoder/cs-etm-decoder.c]
via cs_etm__get_cpu() call.

Can you try out to port that commit to see if that would solve your
regression?

So something like the following will compile as well with the older
gcc version.

I realize: I mainline the order of the commits was:

95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata")
168200b6d6ea ("perf cs-etm: Move definition of 'traceid_list' global variable from header f
ile")

But to v4.19.y only 168200b6d6ea was backported, and while that was
done I now realize the comment was also changed including the change
fom 95c6fe970a01.

Thus the proposed backported patch would drop the change in
tools/perf/util/cs-etm.c to the comment as this was already done.
Thecnically currently the comment would be wrong, because it reads:

/* RB tree for quick conversion between traceID and metadata pointers */

but backport of 95c6fe970a01 is not included.

Would the right thing to do thus be:

- Revert b801d568c7d8 "perf cs-etm: Move definition of 'traceid_list' global variable from header file"
- Backport 95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata")
- Backport 168200b6d6ea ("perf cs-etm: Move definition of 'traceid_list' global variable from header file")

?

Leo ist that what you were proposing?

Though this isn't my proposing, I totally agree with this :)

Just some notes: prior to apply the commit 95c6fe970a01,
tools/perf/util/cs-etm-decoder/cs-etm-decoder.c is the only source
file which uses the variable "traceid_list"; after applied the commit
95c6fe970a01, the code for using the variable "traceid_list" has been
moved out from tools/perf/util/cs-etm-decoder/cs-etm-decoder.c and
moved in tools/perf/util/cs-etm.c.

So the commit 168200b6d6ea moves the definition of "traceid_list" from
the header file to the source file tools/perf/util/cs-etm.c and it
defines the variable as "static".

As you mentioned, backporting 95c6fe970a01 and 168200b6d6ea can fix
both for the older (8.3.0) and new GCC (10.0.1).  And I confirmed this
should not cause logic issue.

Thanks for looking at this issue.

Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel