Re: [PATCH 2/3] perf report: Add raw report support for s390 auxiliary trace

[Arnaldo Carvalho de Melo <acme@kernel.org>]

Em Wed, Aug 08, 2018 at 12:59:21PM -0300, Arnaldo Carvalho de Melo escreveu:
> Em Wed, Aug 08, 2018 at 12:53:28PM -0300, Arnaldo Carvalho de Melo escreveu:
> > Em Wed, Aug 08, 2018 at 08:39:58AM +0200, Thomas-Mich Richter escreveu:
> > > On 08/08/2018 05:37 AM, mpe@ellerman.id.au wrote:
> > > > Thomas Richter <tmricht@linux.ibm.com> writes:
> > > >> Add support for s390 auxiliary trace support.
> > > >> Use 'perf record -e rbd000' to create the perf.data file.
> > > >> The event also has the symbolic name SF_CYCLES_BASIC_DIAG,
> > > >> using 'perf record -e SF_CYCLES_BASIC_DIAG' is equivalent.
> > > > ...
> > > >>
> > > >> Signed-off-by: Thomas Richter <tmricht@linux.ibm.com>
> > > >> Reviewed-by: Hendrik Brueckner <brueckner@linux.ibm.com>
> > > >> ---
> > > >>  tools/perf/util/s390-cpumsf-kernel.h |  71 ++++++++
> > > >>  tools/perf/util/s390-cpumsf.c        | 244 ++++++++++++++++++++++++++-
> > > >>  2 files changed, 314 insertions(+), 1 deletion(-)
> > > >>  create mode 100644 tools/perf/util/s390-cpumsf-kernel.h
> > > > 
> > > > 
> > > > I'm seeing a build break on ppc64le which seems to be caused by this
> > > > commit (I haven't bisected):
> > > > 
> > > >   util/s390-cpumsf.c: In function ‘trailer_timestamp’:
> > > >   util/s390-cpumsf.c:222:2: error: dereferencing type-punned pointer will break strict-aliasing rules [-Werror=strict-aliasing]
> > > >     return *((unsigned long long *) &te->timestamp[te->t]);
> > > >     ^
> > > > 
> > > > 
> > > > And on powerpc big endian:
> > > >   In file included from util/cpumap.h:10:0,
> > > >                    from util/s390-cpumsf.c:150:
> > > >   util/s390-cpumsf.c: In function ‘s390_cpumsf_basic_show’:
> > > >   util/s390-cpumsf.c:187:10: error: format ‘%lx’ expects argument of type ‘long unsigned int’, but argument 4 has type ‘size_t {aka unsigned int}’ [-Werror=format=]
> > > >      pr_err("Invalid AUX trace basic entry [%#08lx]\n", pos);
> > > >             ^
> > > > 
> > > > cheers
> > > > 
> > > 
> > > Can you please try this patch. Thanks a lot
> > 
> > Ok, this was making the build fail as well on some containers here:
> 
> Ok, failed in another container:
> 
>   11 centos:6                      : gcc (GCC) 4.4.7 20120313 (Red Hat 4.4.7-18)
> 
>   CC       /tmp/build/perf/tests/sample-parsing.o
> cc1: warnings being treated as errors
> In file included from util/s390-cpumsf.c:161:
> util/s390-cpumsf-kernel.h:16: error: packed attribute is unnecessary for 'def'
> util/s390-cpumsf-kernel.h:25: error: packed attribute is unnecessary for 'CL'
> util/s390-cpumsf-kernel.h:28: error: packed attribute is unnecessary for 'ia'
> util/s390-cpumsf-kernel.h:29: error: packed attribute is unnecessary for 'gpp'
> util/s390-cpumsf-kernel.h:30: error: packed attribute is unnecessary for 'hpp'
> util/s390-cpumsf-kernel.h:34: error: packed attribute is unnecessary for 'def'
> mv: cannot stat `/tmp/build/perf/util/.s390-cpumsf.o.tmp': No such file or directory
> make[4]: *** [/tmp/build/perf/util/s390-cpumsf.o] Error 1

So, this is how it shows using that __packed you have:

[acme@seventh perf]$ pahole -C hws_basic_entry ~/bin/perf
struct hws_basic_entry {
	unsigned int               def:16;               /*     0:16  4 */
	unsigned int               R:4;                  /*     0:12  4 */
	unsigned int               U:4;                  /*     0: 8  4 */
	unsigned int               z:2;                  /*     0: 6  4 */
	unsigned int               T:1;                  /*     0: 5  4 */
	unsigned int               W:1;                  /*     0: 4  4 */
	unsigned int               P:1;                  /*     0: 3  4 */
	unsigned int               AS:2;                 /*     0: 1  4 */
	unsigned int               I:1;                  /*     0: 0  4 */
	unsigned int               CL:2;                 /*     4:30  4 */
	unsigned int               prim_asn:16;          /*     4: 0  4 */

	/* XXX 14 bits hole, try to pack */

	long long unsigned int     ia;                   /*     8     8 */
	long long unsigned int     gpp;                  /*    16     8 */
	long long unsigned int     hpp;                  /*    24     8 */

	/* size: 32, cachelines: 1, members: 14 */
	/* bit holes: 1, sum bit holes: 14 bits */
	/* last cacheline: 32 bytes */
};
[acme@seventh perf]$

And:

[acme@seventh perf]$ pahole -C hws_diag_entry ~/bin/perf
struct hws_diag_entry {
	unsigned int               def:16;               /*     0:16  4 */
	unsigned int               R:15;                 /*     0: 1  4 */
	unsigned int               I:1;                  /*     0: 0  4 */
	u8                         data[0];              /*     4     0 */

	/* size: 4, cachelines: 1, members: 4 */
	/* last cacheline: 4 bytes */
};
[acme@seventh perf]$ 

Now, if we apply this patch:

[acme@seventh perf]$ pahole -C hws_basic_entry ~/bin/perf
struct hws_basic_entry {
	unsigned int               def:16;               /*     0:16  4 */
	unsigned int               R:4;                  /*     0:12  4 */
	unsigned int               U:4;                  /*     0: 8  4 */
	unsigned int               z:2;                  /*     0: 6  4 */
	unsigned int               T:1;                  /*     0: 5  4 */
	unsigned int               W:1;                  /*     0: 4  4 */
	unsigned int               P:1;                  /*     0: 3  4 */
	unsigned int               AS:2;                 /*     0: 1  4 */
	unsigned int               I:1;                  /*     0: 0  4 */
	unsigned int               CL:2;                 /*     4:30  4 */
	unsigned int               prim_asn:16;          /*     4: 0  4 */

	/* XXX 14 bits hole, try to pack */

	long long unsigned int     ia;                   /*     8     8 */
	long long unsigned int     gpp;                  /*    16     8 */
	long long unsigned int     hpp;                  /*    24     8 */

	/* size: 32, cachelines: 1, members: 14 */
	/* bit holes: 1, sum bit holes: 14 bits */
	/* last cacheline: 32 bytes */
};
[acme@seventh perf]$ 

See, no need for packing, that old gcc version is right, and the other
struct:

[acme@seventh perf]$ pahole -C hws_diag_entry ~/bin/perf
struct hws_diag_entry {
	unsigned int               def:16;               /*     0:16  4 */
	unsigned int               R:15;                 /*     0: 1  4 */
	unsigned int               I:1;                  /*     0: 0  4 */
	u8                         data[0];              /*     4     0 */

	/* size: 4, cachelines: 1, members: 4 */
	/* last cacheline: 4 bytes */
};
[acme@seventh perf]$

No need for __packed.

I'm removing that to avoid having this failling in compilers that have
such a warning, since we default to Werror.

Holler if there is something I'missing :-)

- Arnaldo