* [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" @ 2020-11-20 7:39 Salvatore Bonaccorso 2020-11-20 8:34 ` Greg Kroah-Hartman 2020-11-20 9:54 ` Andrey Zhizhikin 0 siblings, 2 replies; 17+ messages in thread From: Salvatore Bonaccorso @ 2020-11-20 7:39 UTC (permalink / raw) To: stable Cc: Salvatore Bonaccorso, Leo Yan, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman This reverts commit 168200b6d6ea0cb5765943ec5da5b8149701f36a upstream. (but only from 4.19.y) The original commit introduces a build failure as seen on Debian buster when compiled with gcc (Debian 8.3.0-6) 8.3.0: $ LC_ALL=C.UTF-8 ARCH=x86 make perf [...] Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h' CC util/cs-etm-decoder/cs-etm-decoder.o CC util/intel-pt.o util/cs-etm-decoder/cs-etm-decoder.c: In function 'cs_etm_decoder__buffer_packet': util/cs-etm-decoder/cs-etm-decoder.c:287:24: error: 'traceid_list' undeclared (first use in this function); did you mean 'trace_event'? inode = intlist__find(traceid_list, trace_chan_id); ^~~~~~~~~~~~ trace_event util/cs-etm-decoder/cs-etm-decoder.c:287:24: note: each undeclared identifier is reported only once for each function it appears in make[6]: *** [/build/linux-stable/tools/build/Makefile.build:97: util/cs-etm-decoder/cs-etm-decoder.o] Error 1 make[5]: *** [/build/linux-stable/tools/build/Makefile.build:139: cs-etm-decoder] Error 2 make[5]: *** Waiting for unfinished jobs.... make[4]: *** [/build/linux-stable/tools/build/Makefile.build:139: util] Error 2 make[3]: *** [Makefile.perf:633: libperf-in.o] Error 2 make[2]: *** [Makefile.perf:206: sub-make] Error 2 make[1]: *** [Makefile:70: all] Error 2 make: *** [Makefile:77: perf] Error 2 Link: https://lore.kernel.org/stable/20201114083501.GA468764@eldamar.lan/ Cc: Leo Yan <leo.yan@linaro.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Suzuki Poulouse <suzuki.poulose@arm.com> Cc: Tor Jeremiassen <tor@ti.com> Cc: linux-arm-kernel@lists.infradead.org Cc: Arnaldo Carvalho de Melo <acme@redhat.com> Cc: Guenter Roeck <linux@roeck-us.net> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: <stable@vger.kernel.org> # 4.19.y Signed-off-by: Salvatore Bonaccorso <carnil@debian.org> --- tools/perf/util/cs-etm.c | 3 --- tools/perf/util/cs-etm.h | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index ad33b99f5d21..7b5e15cc6b71 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -87,9 +87,6 @@ struct cs_etm_queue { struct cs_etm_packet *packet; }; -/* RB tree for quick conversion between traceID and metadata pointers */ -static struct intlist *traceid_list; - static int cs_etm__update_queues(struct cs_etm_auxtrace *etm); static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm, pid_t tid, u64 time_); diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index c7ef97b198c7..37f8d48179ca 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -53,6 +53,9 @@ enum { CS_ETMV4_PRIV_MAX, }; +/* RB tree for quick conversion between traceID and CPUs */ +struct intlist *traceid_list; + #define KiB(x) ((x) * 1024) #define MiB(x) ((x) * 1024 * 1024) -- 2.29.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 7:39 [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" Salvatore Bonaccorso @ 2020-11-20 8:34 ` Greg Kroah-Hartman 2020-11-20 9:54 ` Andrey Zhizhikin 1 sibling, 0 replies; 17+ messages in thread From: Greg Kroah-Hartman @ 2020-11-20 8:34 UTC (permalink / raw) To: Salvatore Bonaccorso Cc: stable, Leo Yan, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck On Fri, Nov 20, 2020 at 08:39:09AM +0100, Salvatore Bonaccorso wrote: > This reverts commit 168200b6d6ea0cb5765943ec5da5b8149701f36a upstream. > (but only from 4.19.y) Thanks for this, now queued up. greg k-h ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 7:39 [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" Salvatore Bonaccorso 2020-11-20 8:34 ` Greg Kroah-Hartman @ 2020-11-20 9:54 ` Andrey Zhizhikin 2020-11-20 11:19 ` Leo Yan 2020-11-20 13:34 ` Salvatore Bonaccorso 1 sibling, 2 replies; 17+ messages in thread From: Andrey Zhizhikin @ 2020-11-20 9:54 UTC (permalink / raw) To: Salvatore Bonaccorso Cc: stable, Leo Yan, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > 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. > > The original commit introduces a build failure as seen on Debian buster > when compiled with gcc (Debian 8.3.0-6) 8.3.0: > > $ LC_ALL=C.UTF-8 ARCH=x86 make perf > [...] > Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h' > CC util/cs-etm-decoder/cs-etm-decoder.o > CC util/intel-pt.o > util/cs-etm-decoder/cs-etm-decoder.c: In function 'cs_etm_decoder__buffer_packet': > util/cs-etm-decoder/cs-etm-decoder.c:287:24: error: 'traceid_list' undeclared (first use in this function); did you mean 'trace_event'? > inode = intlist__find(traceid_list, trace_chan_id); > ^~~~~~~~~~~~ > trace_event > util/cs-etm-decoder/cs-etm-decoder.c:287:24: note: each undeclared identifier is reported only once for each function it appears in > make[6]: *** [/build/linux-stable/tools/build/Makefile.build:97: util/cs-etm-decoder/cs-etm-decoder.o] Error 1 > make[5]: *** [/build/linux-stable/tools/build/Makefile.build:139: cs-etm-decoder] Error 2 > make[5]: *** Waiting for unfinished jobs.... > make[4]: *** [/build/linux-stable/tools/build/Makefile.build:139: util] Error 2 > make[3]: *** [Makefile.perf:633: libperf-in.o] Error 2 > make[2]: *** [Makefile.perf:206: sub-make] Error 2 > make[1]: *** [Makefile:70: all] Error 2 > make: *** [Makefile:77: perf] Error 2 > > Link: https://lore.kernel.org/stable/20201114083501.GA468764@eldamar.lan/ > Cc: Leo Yan <leo.yan@linaro.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Jiri Olsa <jolsa@redhat.com> > Cc: Mark Rutland <mark.rutland@arm.com> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Suzuki Poulouse <suzuki.poulose@arm.com> > Cc: Tor Jeremiassen <tor@ti.com> > Cc: linux-arm-kernel@lists.infradead.org > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: <stable@vger.kernel.org> # 4.19.y > Signed-off-by: Salvatore Bonaccorso <carnil@debian.org> > --- > tools/perf/util/cs-etm.c | 3 --- > tools/perf/util/cs-etm.h | 3 +++ > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index ad33b99f5d21..7b5e15cc6b71 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -87,9 +87,6 @@ struct cs_etm_queue { > struct cs_etm_packet *packet; > }; > > -/* RB tree for quick conversion between traceID and metadata pointers */ > -static struct intlist *traceid_list; > - > static int cs_etm__update_queues(struct cs_etm_auxtrace *etm); > static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm, > pid_t tid, u64 time_); > diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h > index c7ef97b198c7..37f8d48179ca 100644 > --- a/tools/perf/util/cs-etm.h > +++ b/tools/perf/util/cs-etm.h > @@ -53,6 +53,9 @@ enum { > CS_ETMV4_PRIV_MAX, > }; > > +/* RB tree for quick conversion between traceID and CPUs */ > +struct intlist *traceid_list; > + > #define KiB(x) ((x) * 1024) > #define MiB(x) ((x) * 1024 * 1024) > > -- > 2.29.2 > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 9:54 ` Andrey Zhizhikin @ 2020-11-20 11:19 ` Leo Yan 2020-11-20 12:15 ` Andrey Zhizhikin 2020-11-20 13:34 ` Salvatore Bonaccorso 1 sibling, 1 reply; 17+ messages in thread From: Leo Yan @ 2020-11-20 11:19 UTC (permalink / raw) To: Andrey Zhizhikin Cc: Salvatore Bonaccorso, stable, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote: > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > 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. The original commit has a dependency for commit 95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata"). If the commit 95c6fe970a01 is not backported on v4.19.y, then I think reverting in this patch is the right way to do. Thanks, Leo > > The original commit introduces a build failure as seen on Debian buster > > when compiled with gcc (Debian 8.3.0-6) 8.3.0: > > > > $ LC_ALL=C.UTF-8 ARCH=x86 make perf > > [...] > > Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h' > > CC util/cs-etm-decoder/cs-etm-decoder.o > > CC util/intel-pt.o > > util/cs-etm-decoder/cs-etm-decoder.c: In function 'cs_etm_decoder__buffer_packet': > > util/cs-etm-decoder/cs-etm-decoder.c:287:24: error: 'traceid_list' undeclared (first use in this function); did you mean 'trace_event'? > > inode = intlist__find(traceid_list, trace_chan_id); > > ^~~~~~~~~~~~ > > trace_event > > util/cs-etm-decoder/cs-etm-decoder.c:287:24: note: each undeclared identifier is reported only once for each function it appears in > > make[6]: *** [/build/linux-stable/tools/build/Makefile.build:97: util/cs-etm-decoder/cs-etm-decoder.o] Error 1 > > make[5]: *** [/build/linux-stable/tools/build/Makefile.build:139: cs-etm-decoder] Error 2 > > make[5]: *** Waiting for unfinished jobs.... > > make[4]: *** [/build/linux-stable/tools/build/Makefile.build:139: util] Error 2 > > make[3]: *** [Makefile.perf:633: libperf-in.o] Error 2 > > make[2]: *** [Makefile.perf:206: sub-make] Error 2 > > make[1]: *** [Makefile:70: all] Error 2 > > make: *** [Makefile:77: perf] Error 2 > > > > Link: https://lore.kernel.org/stable/20201114083501.GA468764@eldamar.lan/ > > Cc: Leo Yan <leo.yan@linaro.org> > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > > Cc: Jiri Olsa <jolsa@redhat.com> > > Cc: Mark Rutland <mark.rutland@arm.com> > > Cc: Namhyung Kim <namhyung@kernel.org> > > Cc: Peter Zijlstra <peterz@infradead.org> > > Cc: Suzuki Poulouse <suzuki.poulose@arm.com> > > Cc: Tor Jeremiassen <tor@ti.com> > > Cc: linux-arm-kernel@lists.infradead.org > > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > > Cc: Guenter Roeck <linux@roeck-us.net> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: <stable@vger.kernel.org> # 4.19.y > > Signed-off-by: Salvatore Bonaccorso <carnil@debian.org> > > --- > > tools/perf/util/cs-etm.c | 3 --- > > tools/perf/util/cs-etm.h | 3 +++ > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > > index ad33b99f5d21..7b5e15cc6b71 100644 > > --- a/tools/perf/util/cs-etm.c > > +++ b/tools/perf/util/cs-etm.c > > @@ -87,9 +87,6 @@ struct cs_etm_queue { > > struct cs_etm_packet *packet; > > }; > > > > -/* RB tree for quick conversion between traceID and metadata pointers */ > > -static struct intlist *traceid_list; > > - > > static int cs_etm__update_queues(struct cs_etm_auxtrace *etm); > > static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm, > > pid_t tid, u64 time_); > > diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h > > index c7ef97b198c7..37f8d48179ca 100644 > > --- a/tools/perf/util/cs-etm.h > > +++ b/tools/perf/util/cs-etm.h > > @@ -53,6 +53,9 @@ enum { > > CS_ETMV4_PRIV_MAX, > > }; > > > > +/* RB tree for quick conversion between traceID and CPUs */ > > +struct intlist *traceid_list; > > + > > #define KiB(x) ((x) * 1024) > > #define MiB(x) ((x) * 1024 * 1024) > > > > -- > > 2.29.2 > > > > > -- > Regards, > Andrey. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 11:19 ` Leo Yan @ 2020-11-20 12:15 ` Andrey Zhizhikin 0 siblings, 0 replies; 17+ messages in thread From: Andrey Zhizhikin @ 2020-11-20 12:15 UTC (permalink / raw) To: Leo Yan Cc: Salvatore Bonaccorso, stable, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman On Fri, Nov 20, 2020 at 12:19 PM Leo Yan <leo.yan@linaro.org> wrote: > > On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote: > > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > 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. > > The original commit has a dependency for commit 95c6fe970a01 ("perf > cs-etm: Change tuple from traceID-CPU# to traceID-metadata"). If the > commit 95c6fe970a01 is not backported on v4.19.y, then I think reverting > in this patch is the right way to do. Commit 95c6fe970a01 (perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata) did not change a definition of traceid_list in tools/perf/util/cs-etm.h header file rather a comment above it. Definition of traceid_list in tools/perf/util/cs-etm.h header file was done in commit cd8bfd8c973ea (perf tools: Add processing of coresight metadata) which appears in all branches starting from linux-4.16.y. The issue with this definition of traceid_list in header file would break perf builds with newer gcc versions if this commit would be reverted, so I believe that than rather reverting - commit 95c6fe970a01 ("perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata") should be backported onto 4.19.y instead. Or am I missing something else here? > > Thanks, > Leo > > > > The original commit introduces a build failure as seen on Debian buster > > > when compiled with gcc (Debian 8.3.0-6) 8.3.0: > > > > > > $ LC_ALL=C.UTF-8 ARCH=x86 make perf > > > [...] > > > Warning: Kernel ABI header at 'tools/include/uapi/linux/bpf.h' differs from latest version at 'include/uapi/linux/bpf.h' > > > CC util/cs-etm-decoder/cs-etm-decoder.o > > > CC util/intel-pt.o > > > util/cs-etm-decoder/cs-etm-decoder.c: In function 'cs_etm_decoder__buffer_packet': > > > util/cs-etm-decoder/cs-etm-decoder.c:287:24: error: 'traceid_list' undeclared (first use in this function); did you mean 'trace_event'? > > > inode = intlist__find(traceid_list, trace_chan_id); > > > ^~~~~~~~~~~~ > > > trace_event > > > util/cs-etm-decoder/cs-etm-decoder.c:287:24: note: each undeclared identifier is reported only once for each function it appears in > > > make[6]: *** [/build/linux-stable/tools/build/Makefile.build:97: util/cs-etm-decoder/cs-etm-decoder.o] Error 1 > > > make[5]: *** [/build/linux-stable/tools/build/Makefile.build:139: cs-etm-decoder] Error 2 > > > make[5]: *** Waiting for unfinished jobs.... > > > make[4]: *** [/build/linux-stable/tools/build/Makefile.build:139: util] Error 2 > > > make[3]: *** [Makefile.perf:633: libperf-in.o] Error 2 > > > make[2]: *** [Makefile.perf:206: sub-make] Error 2 > > > make[1]: *** [Makefile:70: all] Error 2 > > > make: *** [Makefile:77: perf] Error 2 > > > > > > Link: https://lore.kernel.org/stable/20201114083501.GA468764@eldamar.lan/ > > > Cc: Leo Yan <leo.yan@linaro.org> > > > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > > > Cc: Jiri Olsa <jolsa@redhat.com> > > > Cc: Mark Rutland <mark.rutland@arm.com> > > > Cc: Namhyung Kim <namhyung@kernel.org> > > > Cc: Peter Zijlstra <peterz@infradead.org> > > > Cc: Suzuki Poulouse <suzuki.poulose@arm.com> > > > Cc: Tor Jeremiassen <tor@ti.com> > > > Cc: linux-arm-kernel@lists.infradead.org > > > Cc: Arnaldo Carvalho de Melo <acme@redhat.com> > > > Cc: Guenter Roeck <linux@roeck-us.net> > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > > Cc: <stable@vger.kernel.org> # 4.19.y > > > Signed-off-by: Salvatore Bonaccorso <carnil@debian.org> > > > --- > > > tools/perf/util/cs-etm.c | 3 --- > > > tools/perf/util/cs-etm.h | 3 +++ > > > 2 files changed, 3 insertions(+), 3 deletions(-) > > > > > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > > > index ad33b99f5d21..7b5e15cc6b71 100644 > > > --- a/tools/perf/util/cs-etm.c > > > +++ b/tools/perf/util/cs-etm.c > > > @@ -87,9 +87,6 @@ struct cs_etm_queue { > > > struct cs_etm_packet *packet; > > > }; > > > > > > -/* RB tree for quick conversion between traceID and metadata pointers */ > > > -static struct intlist *traceid_list; > > > - > > > static int cs_etm__update_queues(struct cs_etm_auxtrace *etm); > > > static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm, > > > pid_t tid, u64 time_); > > > diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h > > > index c7ef97b198c7..37f8d48179ca 100644 > > > --- a/tools/perf/util/cs-etm.h > > > +++ b/tools/perf/util/cs-etm.h > > > @@ -53,6 +53,9 @@ enum { > > > CS_ETMV4_PRIV_MAX, > > > }; > > > > > > +/* RB tree for quick conversion between traceID and CPUs */ > > > +struct intlist *traceid_list; > > > + > > > #define KiB(x) ((x) * 1024) > > > #define MiB(x) ((x) * 1024 * 1024) > > > > > > -- > > > 2.29.2 > > > > > > > > > -- > > Regards, > > Andrey. -- Regards, Andrey. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 9:54 ` Andrey Zhizhikin 2020-11-20 11:19 ` Leo Yan @ 2020-11-20 13:34 ` Salvatore Bonaccorso 2020-11-20 14:29 ` Andrey Zhizhikin 1 sibling, 1 reply; 17+ messages in thread From: Salvatore Bonaccorso @ 2020-11-20 13:34 UTC (permalink / raw) To: Andrey Zhizhikin Cc: stable, Leo Yan, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman Hi Andrey, On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote: > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > 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. Regards, Salvatore ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 13:34 ` Salvatore Bonaccorso @ 2020-11-20 14:29 ` Andrey Zhizhikin 2020-11-20 15:53 ` Salvatore Bonaccorso 0 siblings, 1 reply; 17+ messages in thread From: Andrey Zhizhikin @ 2020-11-20 14:29 UTC (permalink / raw) To: Salvatore Bonaccorso Cc: stable, Leo Yan, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman Hello Salvatore, On Fri, Nov 20, 2020 at 2:34 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > Hi Andrey, > > On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote: > > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > 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? > > Regards, > Salvatore -- Regards, Andrey. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 14:29 ` Andrey Zhizhikin @ 2020-11-20 15:53 ` Salvatore Bonaccorso 2020-11-20 16:31 ` Andrey Zhizhikin 2020-11-22 13:43 ` Leo Yan 0 siblings, 2 replies; 17+ messages in thread From: Salvatore Bonaccorso @ 2020-11-20 15:53 UTC (permalink / raw) To: Andrey Zhizhikin Cc: stable, Leo Yan, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman Hi Andrey, On Fri, Nov 20, 2020 at 03:29:39PM +0100, Andrey Zhizhikin wrote: > Hello Salvatore, > > On Fri, Nov 20, 2020 at 2:34 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > Hi Andrey, > > > > On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote: > > > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > > > 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? Regards, Salvatore From 7d6b3668d8ae5d3aea8827670ade8ac43b92db4a Mon Sep 17 00:00:00 2001 From: Leo Yan <leo.yan@linaro.org> Date: Tue, 29 Jan 2019 20:28:39 +0800 Subject: [PATCH] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream. If packet processing wants to know the packet is bound with which ETM version, it needs to access metadata to decide that based on metadata magic number; but we cannot simply to use CPU logic ID number as index to access metadata sequential array, especially when system have hotplugged off CPUs, the metadata array are only allocated for online CPUs but not offline CPUs, so the CPU logic number doesn't match with its index in the array. This patch is to change tuple from traceID-CPU# to traceID-metadata, thus it can use the tuple to retrieve metadata pointer according to traceID. For safe accessing metadata fields, this patch provides helper function cs_etm__get_cpu() which is used to return CPU number according to traceID; cs_etm_decoder__buffer_packet() is the first consumer for this helper function. Signed-off-by: Leo Yan <leo.yan@linaro.org> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Mike Leach <mike.leach@linaro.org> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Robert Walker <robert.walker@arm.com> Cc: Suzuki K Poulouse <suzuki.poulose@arm.com> Cc: coresight ml <coresight@lists.linaro.org> Cc: linux-arm-kernel@lists.infradead.org Link: http://lkml.kernel.org/r/20190129122842.32041-6-leo.yan@linaro.org Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> [Salvatore Bonaccorso: Drop comment change in tools/perf/util/cs-etm.h which was already changed with b801d568c7d8 ("perf cs-etm: Move definition of 'traceid_list' global variable from header file")] Signed-off-by: Salvatore Bonaccorso <carnil@debian.org> --- .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 8 +++--- tools/perf/util/cs-etm.c | 26 ++++++++++++++----- tools/perf/util/cs-etm.h | 7 +++++ 3 files changed, 30 insertions(+), 11 deletions(-) diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index 938def6d0bb9..f540037eb705 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -278,14 +278,12 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, enum cs_etm_sample_type sample_type) { u32 et = 0; - struct int_node *inode = NULL; + int cpu; if (decoder->packet_count >= MAX_BUFFER - 1) return OCSD_RESP_FATAL_SYS_ERR; - /* Search the RB tree for the cpu associated with this traceID */ - inode = intlist__find(traceid_list, trace_chan_id); - if (!inode) + if (cs_etm__get_cpu(trace_chan_id, &cpu) < 0) return OCSD_RESP_FATAL_SYS_ERR; et = decoder->tail; @@ -296,7 +294,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, decoder->packet_buffer[et].sample_type = sample_type; decoder->packet_buffer[et].exc = false; decoder->packet_buffer[et].exc_ret = false; - decoder->packet_buffer[et].cpu = *((int *)inode->priv); + decoder->packet_buffer[et].cpu = cpu; decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR; decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR; diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index ad33b99f5d21..3275b8dc9344 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -94,6 +94,20 @@ static int cs_etm__update_queues(struct cs_etm_auxtrace *etm); static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm, pid_t tid, u64 time_); +int cs_etm__get_cpu(u8 trace_chan_id, int *cpu) +{ + struct int_node *inode; + u64 *metadata; + + inode = intlist__find(traceid_list, trace_chan_id); + if (!inode) + return -EINVAL; + + metadata = inode->priv; + *cpu = (int)metadata[CS_ETM_CPU]; + return 0; +} + static void cs_etm__packet_dump(const char *pkt_string) { const char *color = PERF_COLOR_BLUE; @@ -233,7 +247,7 @@ static void cs_etm__free(struct perf_session *session) cs_etm__free_events(session); session->auxtrace = NULL; - /* First remove all traceID/CPU# nodes for the RB tree */ + /* First remove all traceID/metadata nodes for the RB tree */ intlist__for_each_entry_safe(inode, tmp, traceid_list) intlist__remove(traceid_list, inode); /* Then the RB tree itself */ @@ -1319,9 +1333,9 @@ int cs_etm__process_auxtrace_info(union perf_event *event, 0xffffffff); /* - * Create an RB tree for traceID-CPU# tuple. Since the conversion has - * to be made for each packet that gets decoded, optimizing access in - * anything other than a sequential array is worth doing. + * Create an RB tree for traceID-metadata tuple. Since the conversion + * has to be made for each packet that gets decoded, optimizing access + * in anything other than a sequential array is worth doing. */ traceid_list = intlist__new(NULL); if (!traceid_list) { @@ -1387,8 +1401,8 @@ int cs_etm__process_auxtrace_info(union perf_event *event, err = -EINVAL; goto err_free_metadata; } - /* All good, associate the traceID with the CPU# */ - inode->priv = &metadata[j][CS_ETM_CPU]; + /* All good, associate the traceID with the metadata pointer */ + inode->priv = metadata[j]; } /* diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index c7ef97b198c7..97c3152f5bfd 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -66,6 +66,7 @@ static const u64 __perf_cs_etmv4_magic = 0x4040404040404040ULL; #ifdef HAVE_CSTRACE_SUPPORT int cs_etm__process_auxtrace_info(union perf_event *event, struct perf_session *session); +int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); #else static inline int cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused, @@ -73,6 +74,12 @@ cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused, { return -1; } + +static inline int cs_etm__get_cpu(u8 trace_chan_id __maybe_unused, + int *cpu __maybe_unused) +{ + return -1; +} #endif #endif -- 2.29.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 15:53 ` Salvatore Bonaccorso @ 2020-11-20 16:31 ` Andrey Zhizhikin 2020-11-20 18:30 ` Salvatore Bonaccorso 2020-11-22 13:43 ` Leo Yan 1 sibling, 1 reply; 17+ messages in thread From: Andrey Zhizhikin @ 2020-11-20 16:31 UTC (permalink / raw) To: Salvatore Bonaccorso Cc: stable, Leo Yan, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman Hello Salvatore, On Fri, Nov 20, 2020 at 4:53 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > Hi Andrey, > > On Fri, Nov 20, 2020 at 03:29:39PM +0100, Andrey Zhizhikin wrote: > > Hello Salvatore, > > > > On Fri, Nov 20, 2020 at 2:34 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > Hi Andrey, > > > > > > On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote: > > > > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > > > > > 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") Yes, I believe this would be the correct course of action here; this should cover the regression you've encountered and should ensure that perf builds on both the "old" and "new" gcc versions. > > ? > > Leo ist that what you were proposing? > > Regards, > Salvatore > > From 7d6b3668d8ae5d3aea8827670ade8ac43b92db4a Mon Sep 17 00:00:00 2001 > From: Leo Yan <leo.yan@linaro.org> > Date: Tue, 29 Jan 2019 20:28:39 +0800 > Subject: [PATCH] perf cs-etm: Change tuple from traceID-CPU# to > traceID-metadata > > commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream. > > If packet processing wants to know the packet is bound with which ETM > version, it needs to access metadata to decide that based on metadata > magic number; but we cannot simply to use CPU logic ID number as index > to access metadata sequential array, especially when system have > hotplugged off CPUs, the metadata array are only allocated for online > CPUs but not offline CPUs, so the CPU logic number doesn't match with > its index in the array. > > This patch is to change tuple from traceID-CPU# to traceID-metadata, > thus it can use the tuple to retrieve metadata pointer according to > traceID. > > For safe accessing metadata fields, this patch provides helper function > cs_etm__get_cpu() which is used to return CPU number according to > traceID; cs_etm_decoder__buffer_packet() is the first consumer for this > helper function. > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Jiri Olsa <jolsa@redhat.com> > Cc: Mike Leach <mike.leach@linaro.org> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Robert Walker <robert.walker@arm.com> > Cc: Suzuki K Poulouse <suzuki.poulose@arm.com> > Cc: coresight ml <coresight@lists.linaro.org> > Cc: linux-arm-kernel@lists.infradead.org > Link: http://lkml.kernel.org/r/20190129122842.32041-6-leo.yan@linaro.org > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > [Salvatore Bonaccorso: Drop comment change in tools/perf/util/cs-etm.h > which was already changed with b801d568c7d8 ("perf cs-etm: Move > definition of 'traceid_list' global variable from header file")] > Signed-off-by: Salvatore Bonaccorso <carnil@debian.org> > --- > .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 8 +++--- > tools/perf/util/cs-etm.c | 26 ++++++++++++++----- > tools/perf/util/cs-etm.h | 7 +++++ > 3 files changed, 30 insertions(+), 11 deletions(-) > > diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > index 938def6d0bb9..f540037eb705 100644 > --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c > @@ -278,14 +278,12 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, > enum cs_etm_sample_type sample_type) > { > u32 et = 0; > - struct int_node *inode = NULL; > + int cpu; > > if (decoder->packet_count >= MAX_BUFFER - 1) > return OCSD_RESP_FATAL_SYS_ERR; > > - /* Search the RB tree for the cpu associated with this traceID */ > - inode = intlist__find(traceid_list, trace_chan_id); > - if (!inode) > + if (cs_etm__get_cpu(trace_chan_id, &cpu) < 0) > return OCSD_RESP_FATAL_SYS_ERR; > > et = decoder->tail; > @@ -296,7 +294,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, > decoder->packet_buffer[et].sample_type = sample_type; > decoder->packet_buffer[et].exc = false; > decoder->packet_buffer[et].exc_ret = false; > - decoder->packet_buffer[et].cpu = *((int *)inode->priv); > + decoder->packet_buffer[et].cpu = cpu; > decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR; > decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR; > > diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c > index ad33b99f5d21..3275b8dc9344 100644 > --- a/tools/perf/util/cs-etm.c > +++ b/tools/perf/util/cs-etm.c > @@ -94,6 +94,20 @@ static int cs_etm__update_queues(struct cs_etm_auxtrace *etm); > static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm, > pid_t tid, u64 time_); > > +int cs_etm__get_cpu(u8 trace_chan_id, int *cpu) > +{ > + struct int_node *inode; > + u64 *metadata; > + > + inode = intlist__find(traceid_list, trace_chan_id); > + if (!inode) > + return -EINVAL; > + > + metadata = inode->priv; > + *cpu = (int)metadata[CS_ETM_CPU]; > + return 0; > +} > + > static void cs_etm__packet_dump(const char *pkt_string) > { > const char *color = PERF_COLOR_BLUE; > @@ -233,7 +247,7 @@ static void cs_etm__free(struct perf_session *session) > cs_etm__free_events(session); > session->auxtrace = NULL; > > - /* First remove all traceID/CPU# nodes for the RB tree */ > + /* First remove all traceID/metadata nodes for the RB tree */ > intlist__for_each_entry_safe(inode, tmp, traceid_list) > intlist__remove(traceid_list, inode); > /* Then the RB tree itself */ > @@ -1319,9 +1333,9 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > 0xffffffff); > > /* > - * Create an RB tree for traceID-CPU# tuple. Since the conversion has > - * to be made for each packet that gets decoded, optimizing access in > - * anything other than a sequential array is worth doing. > + * Create an RB tree for traceID-metadata tuple. Since the conversion > + * has to be made for each packet that gets decoded, optimizing access > + * in anything other than a sequential array is worth doing. > */ > traceid_list = intlist__new(NULL); > if (!traceid_list) { > @@ -1387,8 +1401,8 @@ int cs_etm__process_auxtrace_info(union perf_event *event, > err = -EINVAL; > goto err_free_metadata; > } > - /* All good, associate the traceID with the CPU# */ > - inode->priv = &metadata[j][CS_ETM_CPU]; > + /* All good, associate the traceID with the metadata pointer */ > + inode->priv = metadata[j]; > } > > /* > diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h > index c7ef97b198c7..97c3152f5bfd 100644 > --- a/tools/perf/util/cs-etm.h > +++ b/tools/perf/util/cs-etm.h > @@ -66,6 +66,7 @@ static const u64 __perf_cs_etmv4_magic = 0x4040404040404040ULL; > #ifdef HAVE_CSTRACE_SUPPORT > int cs_etm__process_auxtrace_info(union perf_event *event, > struct perf_session *session); > +int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); > #else > static inline int > cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused, > @@ -73,6 +74,12 @@ cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused, > { > return -1; > } > + > +static inline int cs_etm__get_cpu(u8 trace_chan_id __maybe_unused, > + int *cpu __maybe_unused) > +{ > + return -1; > +} > #endif > > #endif > -- > 2.29.2 > -- Regards, Andrey. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 16:31 ` Andrey Zhizhikin @ 2020-11-20 18:30 ` Salvatore Bonaccorso 2020-11-20 19:29 ` Andrey Zhizhikin 0 siblings, 1 reply; 17+ messages in thread From: Salvatore Bonaccorso @ 2020-11-20 18:30 UTC (permalink / raw) To: Andrey Zhizhikin Cc: stable, Leo Yan, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman Hi Andrey, On Fri, Nov 20, 2020 at 05:31:59PM +0100, Andrey Zhizhikin wrote: > Hello Salvatore, > > On Fri, Nov 20, 2020 at 4:53 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > Hi Andrey, > > > > On Fri, Nov 20, 2020 at 03:29:39PM +0100, Andrey Zhizhikin wrote: > > > Hello Salvatore, > > > > > > On Fri, Nov 20, 2020 at 2:34 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > > > Hi Andrey, > > > > > > > > On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote: > > > > > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > > > > > > > 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") > > Yes, I believe this would be the correct course of action here; this > should cover the regression you've encountered and should ensure that > perf builds on both the "old" and "new" gcc versions. Although perf tools in v4.19.y won't compile with recent GCCs. Greg did already queued up the first part of it, so the revert. I think we can pick the later two commits again up after the v4.19.159 release? Regards, Salvatore ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 18:30 ` Salvatore Bonaccorso @ 2020-11-20 19:29 ` Andrey Zhizhikin 0 siblings, 0 replies; 17+ messages in thread From: Andrey Zhizhikin @ 2020-11-20 19:29 UTC (permalink / raw) To: Salvatore Bonaccorso Cc: stable, Leo Yan, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman Hello Salvatore, On Fri, Nov 20, 2020 at 7:30 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > Hi Andrey, > > On Fri, Nov 20, 2020 at 05:31:59PM +0100, Andrey Zhizhikin wrote: > > Hello Salvatore, > > > > On Fri, Nov 20, 2020 at 4:53 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > Hi Andrey, > > > > > > On Fri, Nov 20, 2020 at 03:29:39PM +0100, Andrey Zhizhikin wrote: > > > > Hello Salvatore, > > > > > > > > On Fri, Nov 20, 2020 at 2:34 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > > > > > Hi Andrey, > > > > > > > > > > On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote: > > > > > > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > > > > > > > > > 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") > > > > Yes, I believe this would be the correct course of action here; this > > should cover the regression you've encountered and should ensure that > > perf builds on both the "old" and "new" gcc versions. > > Although perf tools in v4.19.y won't compile with recent GCCs. > > Greg did already queued up the first part of it, so the revert. I > think we can pick the later two commits again up after the v4.19.159 > release? Sounds reasonable to me. > > Regards, > Salvatore -- Regards, Andrey. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" 2020-11-20 15:53 ` Salvatore Bonaccorso 2020-11-20 16:31 ` Andrey Zhizhikin @ 2020-11-22 13:43 ` Leo Yan 2020-11-25 20:12 ` [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Salvatore Bonaccorso 1 sibling, 1 reply; 17+ messages in thread From: Leo Yan @ 2020-11-22 13:43 UTC (permalink / raw) To: Salvatore Bonaccorso Cc: Andrey Zhizhikin, stable, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Guenter Roeck, Greg Kroah-Hartman 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: > > Hello Salvatore, > > > > On Fri, Nov 20, 2020 at 2:34 PM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > Hi Andrey, > > > > > > On Fri, Nov 20, 2020 at 10:54:22AM +0100, Andrey Zhizhikin wrote: > > > > On Fri, Nov 20, 2020 at 8:39 AM Salvatore Bonaccorso <carnil@debian.org> wrote: > > > > > > > > > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata 2020-11-22 13:43 ` Leo Yan @ 2020-11-25 20:12 ` Salvatore Bonaccorso 2020-11-25 20:12 ` [PATCH 2/2] perf cs-etm: Move definition of 'traceid_list' global variable from header file Salvatore Bonaccorso 2020-11-25 20:23 ` [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Salvatore Bonaccorso 0 siblings, 2 replies; 17+ messages in thread From: Salvatore Bonaccorso @ 2020-11-25 20:12 UTC (permalink / raw) To: stable Cc: Andrey Zhizhikin, Leo Yan, Mathieu Poirier, Alexander Shishkin, Jiri Olsa, Mike Leach, Namhyung Kim, Robert Walker, Suzuki K Poulouse, coresight ml, linux-arm-kernel, Arnaldo Carvalho de Melo, Salvatore Bonaccorso From: Leo Yan <leo.yan@linaro.org> commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream. If packet processing wants to know the packet is bound with which ETM version, it needs to access metadata to decide that based on metadata magic number; but we cannot simply to use CPU logic ID number as index to access metadata sequential array, especially when system have hotplugged off CPUs, the metadata array are only allocated for online CPUs but not offline CPUs, so the CPU logic number doesn't match with its index in the array. This patch is to change tuple from traceID-CPU# to traceID-metadata, thus it can use the tuple to retrieve metadata pointer according to traceID. For safe accessing metadata fields, this patch provides helper function cs_etm__get_cpu() which is used to return CPU number according to traceID; cs_etm_decoder__buffer_packet() is the first consumer for this helper function. Signed-off-by: Leo Yan <leo.yan@linaro.org> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Mike Leach <mike.leach@linaro.org> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Robert Walker <robert.walker@arm.com> Cc: Suzuki K Poulouse <suzuki.poulose@arm.com> Cc: coresight ml <coresight@lists.linaro.org> Cc: linux-arm-kernel@lists.infradead.org Link: http://lkml.kernel.org/r/20190129122842.32041-6-leo.yan@linaro.org Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> [Salvatore Bonaccorso: Adjust for context changes in tools/perf/util/cs-etm-decoder/cs-etm-decoder.c] Signed-off-by: Salvatore Bonaccorso <carnil@debian.org> --- .../perf/util/cs-etm-decoder/cs-etm-decoder.c | 8 +++--- tools/perf/util/cs-etm.c | 26 ++++++++++++++----- tools/perf/util/cs-etm.h | 9 ++++++- 3 files changed, 31 insertions(+), 12 deletions(-) diff --git a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c index 938def6d0bb9..f540037eb705 100644 --- a/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c +++ b/tools/perf/util/cs-etm-decoder/cs-etm-decoder.c @@ -278,14 +278,12 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, enum cs_etm_sample_type sample_type) { u32 et = 0; - struct int_node *inode = NULL; + int cpu; if (decoder->packet_count >= MAX_BUFFER - 1) return OCSD_RESP_FATAL_SYS_ERR; - /* Search the RB tree for the cpu associated with this traceID */ - inode = intlist__find(traceid_list, trace_chan_id); - if (!inode) + if (cs_etm__get_cpu(trace_chan_id, &cpu) < 0) return OCSD_RESP_FATAL_SYS_ERR; et = decoder->tail; @@ -296,7 +294,7 @@ cs_etm_decoder__buffer_packet(struct cs_etm_decoder *decoder, decoder->packet_buffer[et].sample_type = sample_type; decoder->packet_buffer[et].exc = false; decoder->packet_buffer[et].exc_ret = false; - decoder->packet_buffer[et].cpu = *((int *)inode->priv); + decoder->packet_buffer[et].cpu = cpu; decoder->packet_buffer[et].start_addr = CS_ETM_INVAL_ADDR; decoder->packet_buffer[et].end_addr = CS_ETM_INVAL_ADDR; diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 7b5e15cc6b71..5cde3956e19a 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -91,6 +91,20 @@ static int cs_etm__update_queues(struct cs_etm_auxtrace *etm); static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm, pid_t tid, u64 time_); +int cs_etm__get_cpu(u8 trace_chan_id, int *cpu) +{ + struct int_node *inode; + u64 *metadata; + + inode = intlist__find(traceid_list, trace_chan_id); + if (!inode) + return -EINVAL; + + metadata = inode->priv; + *cpu = (int)metadata[CS_ETM_CPU]; + return 0; +} + static void cs_etm__packet_dump(const char *pkt_string) { const char *color = PERF_COLOR_BLUE; @@ -230,7 +244,7 @@ static void cs_etm__free(struct perf_session *session) cs_etm__free_events(session); session->auxtrace = NULL; - /* First remove all traceID/CPU# nodes for the RB tree */ + /* First remove all traceID/metadata nodes for the RB tree */ intlist__for_each_entry_safe(inode, tmp, traceid_list) intlist__remove(traceid_list, inode); /* Then the RB tree itself */ @@ -1316,9 +1330,9 @@ int cs_etm__process_auxtrace_info(union perf_event *event, 0xffffffff); /* - * Create an RB tree for traceID-CPU# tuple. Since the conversion has - * to be made for each packet that gets decoded, optimizing access in - * anything other than a sequential array is worth doing. + * Create an RB tree for traceID-metadata tuple. Since the conversion + * has to be made for each packet that gets decoded, optimizing access + * in anything other than a sequential array is worth doing. */ traceid_list = intlist__new(NULL); if (!traceid_list) { @@ -1384,8 +1398,8 @@ int cs_etm__process_auxtrace_info(union perf_event *event, err = -EINVAL; goto err_free_metadata; } - /* All good, associate the traceID with the CPU# */ - inode->priv = &metadata[j][CS_ETM_CPU]; + /* All good, associate the traceID with the metadata pointer */ + inode->priv = metadata[j]; } /* diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index 37f8d48179ca..fb5fc6538b7f 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -53,7 +53,7 @@ enum { CS_ETMV4_PRIV_MAX, }; -/* RB tree for quick conversion between traceID and CPUs */ +/* RB tree for quick conversion between traceID and metadata pointers */ struct intlist *traceid_list; #define KiB(x) ((x) * 1024) @@ -69,6 +69,7 @@ static const u64 __perf_cs_etmv4_magic = 0x4040404040404040ULL; #ifdef HAVE_CSTRACE_SUPPORT int cs_etm__process_auxtrace_info(union perf_event *event, struct perf_session *session); +int cs_etm__get_cpu(u8 trace_chan_id, int *cpu); #else static inline int cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused, @@ -76,6 +77,12 @@ cs_etm__process_auxtrace_info(union perf_event *event __maybe_unused, { return -1; } + +static inline int cs_etm__get_cpu(u8 trace_chan_id __maybe_unused, + int *cpu __maybe_unused) +{ + return -1; +} #endif #endif -- 2.29.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH 2/2] perf cs-etm: Move definition of 'traceid_list' global variable from header file 2020-11-25 20:12 ` [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Salvatore Bonaccorso @ 2020-11-25 20:12 ` Salvatore Bonaccorso 2020-11-25 20:23 ` [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Salvatore Bonaccorso 1 sibling, 0 replies; 17+ messages in thread From: Salvatore Bonaccorso @ 2020-11-25 20:12 UTC (permalink / raw) To: stable Cc: Andrey Zhizhikin, Leo Yan, Thomas Backlund, Mathieu Poirier, Mike Leach, Alexander Shishkin, Jiri Olsa, Mark Rutland, Namhyung Kim, Peter Zijlstra, Suzuki Poulouse, Tor Jeremiassen, linux-arm-kernel, Arnaldo Carvalho de Melo, Salvatore Bonaccorso From: Leo Yan <leo.yan@linaro.org> commit 168200b6d6ea0cb5765943ec5da5b8149701f36a upstream. The variable 'traceid_list' is defined in the header file cs-etm.h, if multiple C files include cs-etm.h the compiler might complaint for multiple definition of 'traceid_list'. To fix multiple definition error, move the definition of 'traceid_list' into cs-etm.c. Fixes: cd8bfd8c973e ("perf tools: Add processing of coresight metadata") Reported-by: Thomas Backlund <tmb@mageia.org> Signed-off-by: Leo Yan <leo.yan@linaro.org> Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> Reviewed-by: Mike Leach <mike.leach@linaro.org> Tested-by: Mike Leach <mike.leach@linaro.org> Tested-by: Thomas Backlund <tmb@mageia.org> Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> Cc: Jiri Olsa <jolsa@redhat.com> Cc: Mark Rutland <mark.rutland@arm.com> Cc: Namhyung Kim <namhyung@kernel.org> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Suzuki Poulouse <suzuki.poulose@arm.com> Cc: Tor Jeremiassen <tor@ti.com> Cc: linux-arm-kernel@lists.infradead.org Link: http://lore.kernel.org/lkml/20200505133642.4756-1-leo.yan@linaro.org Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> Signed-off-by: Salvatore Bonaccorso <carnil@debian.org> --- tools/perf/util/cs-etm.c | 3 +++ tools/perf/util/cs-etm.h | 3 --- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/tools/perf/util/cs-etm.c b/tools/perf/util/cs-etm.c index 5cde3956e19a..3275b8dc9344 100644 --- a/tools/perf/util/cs-etm.c +++ b/tools/perf/util/cs-etm.c @@ -87,6 +87,9 @@ struct cs_etm_queue { struct cs_etm_packet *packet; }; +/* RB tree for quick conversion between traceID and metadata pointers */ +static struct intlist *traceid_list; + static int cs_etm__update_queues(struct cs_etm_auxtrace *etm); static int cs_etm__process_timeless_queues(struct cs_etm_auxtrace *etm, pid_t tid, u64 time_); diff --git a/tools/perf/util/cs-etm.h b/tools/perf/util/cs-etm.h index fb5fc6538b7f..97c3152f5bfd 100644 --- a/tools/perf/util/cs-etm.h +++ b/tools/perf/util/cs-etm.h @@ -53,9 +53,6 @@ enum { CS_ETMV4_PRIV_MAX, }; -/* RB tree for quick conversion between traceID and metadata pointers */ -struct intlist *traceid_list; - #define KiB(x) ((x) * 1024) #define MiB(x) ((x) * 1024 * 1024) -- 2.29.2 ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata 2020-11-25 20:12 ` [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Salvatore Bonaccorso 2020-11-25 20:12 ` [PATCH 2/2] perf cs-etm: Move definition of 'traceid_list' global variable from header file Salvatore Bonaccorso @ 2020-11-25 20:23 ` Salvatore Bonaccorso 2020-11-26 1:35 ` Leo Yan 1 sibling, 1 reply; 17+ messages in thread From: Salvatore Bonaccorso @ 2020-11-25 20:23 UTC (permalink / raw) To: stable Cc: Andrey Zhizhikin, Leo Yan, Mathieu Poirier, Alexander Shishkin, Jiri Olsa, Mike Leach, Namhyung Kim, Robert Walker, Suzuki K Poulouse, coresight ml, linux-arm-kernel, Arnaldo Carvalho de Melo On Wed, Nov 25, 2020 at 09:12:14PM +0100, Salvatore Bonaccorso wrote: > From: Leo Yan <leo.yan@linaro.org> > > commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream. > > If packet processing wants to know the packet is bound with which ETM > version, it needs to access metadata to decide that based on metadata > magic number; but we cannot simply to use CPU logic ID number as index > to access metadata sequential array, especially when system have > hotplugged off CPUs, the metadata array are only allocated for online > CPUs but not offline CPUs, so the CPU logic number doesn't match with > its index in the array. > > This patch is to change tuple from traceID-CPU# to traceID-metadata, > thus it can use the tuple to retrieve metadata pointer according to > traceID. > > For safe accessing metadata fields, this patch provides helper function > cs_etm__get_cpu() which is used to return CPU number according to > traceID; cs_etm_decoder__buffer_packet() is the first consumer for this > helper function. > > Signed-off-by: Leo Yan <leo.yan@linaro.org> > Reviewed-by: Mathieu Poirier <mathieu.poirier@linaro.org> > Cc: Alexander Shishkin <alexander.shishkin@linux.intel.com> > Cc: Jiri Olsa <jolsa@redhat.com> > Cc: Mike Leach <mike.leach@linaro.org> > Cc: Namhyung Kim <namhyung@kernel.org> > Cc: Robert Walker <robert.walker@arm.com> > Cc: Suzuki K Poulouse <suzuki.poulose@arm.com> > Cc: coresight ml <coresight@lists.linaro.org> > Cc: linux-arm-kernel@lists.infradead.org > Link: http://lkml.kernel.org/r/20190129122842.32041-6-leo.yan@linaro.org > Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com> > [Salvatore Bonaccorso: Adjust for context changes in > tools/perf/util/cs-etm-decoder/cs-etm-decoder.c] > Signed-off-by: Salvatore Bonaccorso <carnil@debian.org> That's a fallout on my end. Should have said: This is for 4.19 specifically to be queued. Background: in https://lore.kernel.org/stable/20201014135627.GA3698844@kroah.com/ 168200b6d6ea0cb5765943ec5da5b8149701f36a was queued up for v4.19.y but the prerequeisite commit was not included and so resulted in build failures with gcc 8.3.0. The commit was later on reverted but in this thread it was asked to actually make it possible to compile the file as well with more recent gcc versions. Those two patches to be applied in 4.19.y only pick up a backport of the rerequisite commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 (PATCH 1) followed up by the cherry-pick of 168200b6d6ea0cb5765943ec5da5b8149701f36a again. Regards, Salvatore ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata 2020-11-25 20:23 ` [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Salvatore Bonaccorso @ 2020-11-26 1:35 ` Leo Yan 2020-11-26 4:52 ` Salvatore Bonaccorso 0 siblings, 1 reply; 17+ messages in thread From: Leo Yan @ 2020-11-26 1:35 UTC (permalink / raw) To: Salvatore Bonaccorso Cc: stable, Andrey Zhizhikin, Mathieu Poirier, Alexander Shishkin, Jiri Olsa, Mike Leach, Namhyung Kim, Robert Walker, Suzuki K Poulouse, coresight ml, linux-arm-kernel, Arnaldo Carvalho de Melo On Wed, Nov 25, 2020 at 09:23:23PM +0100, Salvatore Bonaccorso wrote: > On Wed, Nov 25, 2020 at 09:12:14PM +0100, Salvatore Bonaccorso wrote: > > From: Leo Yan <leo.yan@linaro.org> > > > > commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream. [...] > That's a fallout on my end. Should have said: This is for 4.19 > specifically to be queued. > > Background: in > https://lore.kernel.org/stable/20201014135627.GA3698844@kroah.com/ > 168200b6d6ea0cb5765943ec5da5b8149701f36a was queued up for v4.19.y but > the prerequeisite commit was not included and so resulted in build > failures with gcc 8.3.0. > > The commit was later on reverted but in this thread it was asked to > actually make it possible to compile the file as well with more recent > gcc versions. > > Those two patches to be applied in 4.19.y only pick up a backport of > the rerequisite commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 (PATCH > 1) followed up by the cherry-pick of > 168200b6d6ea0cb5765943ec5da5b8149701f36a again. Since the patch 01 is minor tweaked due to context difference, I manually compared it with original patch and looks good to me. Thank you for the back porting, Leo ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata 2020-11-26 1:35 ` Leo Yan @ 2020-11-26 4:52 ` Salvatore Bonaccorso 0 siblings, 0 replies; 17+ messages in thread From: Salvatore Bonaccorso @ 2020-11-26 4:52 UTC (permalink / raw) To: Leo Yan Cc: stable, Andrey Zhizhikin, Mathieu Poirier, Alexander Shishkin, Jiri Olsa, Mike Leach, Namhyung Kim, Robert Walker, Suzuki K Poulouse, coresight ml, linux-arm-kernel, Arnaldo Carvalho de Melo Hi Leo, On Thu, Nov 26, 2020 at 09:35:22AM +0800, Leo Yan wrote: > On Wed, Nov 25, 2020 at 09:23:23PM +0100, Salvatore Bonaccorso wrote: > > On Wed, Nov 25, 2020 at 09:12:14PM +0100, Salvatore Bonaccorso wrote: > > > From: Leo Yan <leo.yan@linaro.org> > > > > > > commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 upstream. > > [...] > > > That's a fallout on my end. Should have said: This is for 4.19 > > specifically to be queued. > > > > Background: in > > https://lore.kernel.org/stable/20201014135627.GA3698844@kroah.com/ > > 168200b6d6ea0cb5765943ec5da5b8149701f36a was queued up for v4.19.y but > > the prerequeisite commit was not included and so resulted in build > > failures with gcc 8.3.0. > > > > The commit was later on reverted but in this thread it was asked to > > actually make it possible to compile the file as well with more recent > > gcc versions. > > > > Those two patches to be applied in 4.19.y only pick up a backport of > > the rerequisite commit 95c6fe970a0160cb770c5dce9f80311b42d030c0 (PATCH > > 1) followed up by the cherry-pick of > > 168200b6d6ea0cb5765943ec5da5b8149701f36a again. > > Since the patch 01 is minor tweaked due to context difference, I > manually compared it with original patch and looks good to me. > > Thank you for the back porting, Welcome, given I'm unfamiliar with the codebasis for perf the explicit acknowledgement nothing looks wrong was appreciated. Regards, Salvatore ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-11-26 4:53 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-20 7:39 [PATCH] Revert "perf cs-etm: Move definition of 'traceid_list' global variable from header file" Salvatore Bonaccorso 2020-11-20 8:34 ` Greg Kroah-Hartman 2020-11-20 9:54 ` Andrey Zhizhikin 2020-11-20 11:19 ` Leo Yan 2020-11-20 12:15 ` Andrey Zhizhikin 2020-11-20 13:34 ` Salvatore Bonaccorso 2020-11-20 14:29 ` Andrey Zhizhikin 2020-11-20 15:53 ` Salvatore Bonaccorso 2020-11-20 16:31 ` Andrey Zhizhikin 2020-11-20 18:30 ` Salvatore Bonaccorso 2020-11-20 19:29 ` Andrey Zhizhikin 2020-11-22 13:43 ` Leo Yan 2020-11-25 20:12 ` [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Salvatore Bonaccorso 2020-11-25 20:12 ` [PATCH 2/2] perf cs-etm: Move definition of 'traceid_list' global variable from header file Salvatore Bonaccorso 2020-11-25 20:23 ` [PATCH 1/2] perf cs-etm: Change tuple from traceID-CPU# to traceID-metadata Salvatore Bonaccorso 2020-11-26 1:35 ` Leo Yan 2020-11-26 4:52 ` Salvatore Bonaccorso
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).