perf tool: fix endianness handling of u32 data in samples
PATCH v2 1/1 perf tool: fix endianness handling of u32 data in samples
Currently, analyzing PPC data files on x86 the cpu field is always 0 and the tid and pid are backwards. For example, analyzing a PPC file on PPC the pid/tid fields show:
rsyslogd 1210/1212
and analyzing the same PPC file using an x86 perf binary shows:
rsyslogd 1212/1210
The problem is that the swap_op method for samples is perf_event__all64_swap which assumes all elements in the sample_data struct are u64s. cpu, tid and pid are u32s and need to be handled individually. Given that the swap is done before the sample is parsed, the simplest solution is to undo the 64-bit swap of those elements when the sample is parsed and do the proper swap.
The RAW data field is generic and perf cannot have programmatic knowledge of how to treat that data. Instead a warning is given to the user.
Thanks to Anton Blanchard for providing a data file for a mult-CPU PPC system so I could verify the fix for the CPU fields.
v1 -> v2:
- added a union for undoing the byte-swap on u64 and redoing swap on u32's to address compiler errors (see git commit 65014ab3)
--- tools/perf/builtin-test.c | 2 +- tools/perf/util/event.h | 2 +- tools/perf/util/evsel.c | 61 ++++++++++++++++++++++++++++++++++++-------- tools/perf/util/session.h | 3 +- 4 files changed, 54 insertions(+), 14 deletions(-)
tools/perf/builtin-test.c +1 -1
@@ -561,7 +561,7 @@ static int test__basic_mmap(void) } err =perf_event__parse_sample (event, attr.sample_type, sample_size, - false, &sample); + false, &sample, false);if (err) {pr_err ("Can't parse sample, err = %d\n" , err); goto out_munmap;
tools/perf/util/event.h +1 -1
@@ -186,6 +186,6 @@ const char *perf_event__name(unsigned int id);int perf_event__parse_sample (const union perf_event *event,u64 type,int sample_size,bool sample_id_all, -struct perf_sample *sample); +struct perf_sample *sample,bool swapped);#endif /* __PERF_RECORD_H */
tools/perf/util/evsel.c +50 -11
100 lines not on this page · raw message
tools/perf/util/session.h +2 -1
@@ -162,7 +162,8 @@ static inline int perf_session__parse_sample(struct perf_session *session, {return perf_event__parse_sample (event, session->sample_type, session->sample_size, - session->sample_id_all, sample); + session->sample_id_all, sample, + session->header.needs_swap); } struct perf_evsel *perf_session__find_first_evtype(struct perf_session *session,
Signature · 1 line
1.7.6
Em Fri, Sep 02, 2011 at 10:03:46AM -0600, David Ahern escreveu:
Quoted 6 lines
tools/perf/util/evsel.c +4
-1if (type &PERF_SAMPLE_RAW ) { -u32 *p = (u32 *)array; + u.val64 = *array; +if (swapped) { +static bool show_warn = true; +
tools/perf/util/evsel.c int perf_event__parse_sample(const union perf_event *event, u64 type, · line 466
Quoted 10 lines
tools/perf/util/evsel.c +10
-0+/* undo swap of u64, then swap on individual u32s */ + u.val64 =bswap_64 (u.val64); + u.val32[0 ] =bswap_32 (u.val32[0 ]); + u.val32[1 ] =bswap_32 (u.val32[1 ]); + +if (show_warn) { +pr_warning ("Endianness of raw data not corrected!\n" ); + show_warn = false; + } + }
Can you use WARN_ONCE? Would become:
if (WARN_ONCE(swapped, "Endianness of raw data not corrected!\n")) {
/* undo swap of u64, then swap on individual u32s */
u.val64 = bswap_64(u.val64);
u.val32[0] = bswap_32(u.val32[0]);
u.val32[1] = bswap_32(u.val32[1]);
}We have that and its the usual idiom in the kernel proper :-)
See tools/perf/util/include/asm/bug.h and net/can/af_can.c can_rcv for an example.
Thanks,
- Arnaldo
Quoted 20 lines · Arnaldo Carvalho de Melo
Em Fri, Sep 02, 2011 at 10:03:46AM -0600, David Ahern escreveu:
Quoted 6 lines
tools/perf/util/evsel.c +4
-1if (type &PERF_SAMPLE_RAW ) { -u32 *p = (u32 *)array; + u.val64 = *array; +if (swapped) { +static bool show_warn = true; +tools/perf/util/evsel.c int perf_event__parse_sample(const union perf_event *event, u64 type, · line 466
Quoted 10 lines
tools/perf/util/evsel.c +10
-0+/* undo swap of u64, then swap on individual u32s */ + u.val64 =bswap_64 (u.val64); + u.val32[0 ] =bswap_32 (u.val32[0 ]); + u.val32[1 ] =bswap_32 (u.val32[1 ]); + +if (show_warn) { +pr_warning ("Endianness of raw data not corrected!\n" ); + show_warn = false; + } + }Can you use
WARN_ONCE? Would become:
Quoted 6 lines
if (WARN_ONCE(swapped, "Endianness of raw data not corrected!\n")) {
/* undo swap of u64, then swap on individual u32s */
u.val64 = bswap_64(u.val64);
u.val32[0] = bswap_32(u.val32[0]);
u.val32[1] = bswap_32(u.val32[1]);
}That's not quite what we need. The bswap's happen all the time; the warn once is to tell the user one time that samples in the file contain raw data and those cannot be programmatically adjusted for endianness.
ie., more like:
WARN_ONCE(swapped, "Endianness of raw data not corrected!\n");
and no action taken (the 'if (WARN_ONCE())'part).
David
Quoted 9 lines
We have that and its the usual idiom in the kernel proper :-)
See
tools/perf/util/include/asm/bug.handnet/can/af_can.ccan_rcvfor an example.Thanks,
- Arnaldo
Em Fri, Sep 02, 2011 at 01:12:30PM -0600, David Ahern escreveu:
Quoted 28 lines · Arnaldo Carvalho de Melo
Quoted 20 lines
Em Fri, Sep 02, 2011 at 10:03:46AM -0600, David Ahern escreveu:
Quoted 6 lines
tools/perf/util/evsel.c +4
-1if (type &PERF_SAMPLE_RAW ) { -u32 *p = (u32 *)array; + u.val64 = *array; +if (swapped) { +static bool show_warn = true; +tools/perf/util/evsel.c int perf_event__parse_sample(const union perf_event *event, u64 type, · line 466
Quoted 10 lines
tools/perf/util/evsel.c +10
-0+/* undo swap of u64, then swap on individual u32s */ + u.val64 =bswap_64 (u.val64); + u.val32[0 ] =bswap_32 (u.val32[0 ]); + u.val32[1 ] =bswap_32 (u.val32[1 ]); + +if (show_warn) { +pr_warning ("Endianness of raw data not corrected!\n" ); + show_warn = false; + } + }Can you use
WARN_ONCE? Would become:Quoted 6 lines
if (WARN_ONCE(swapped, "Endianness of raw data not corrected!\n")) { /* undo swap of u64, then swap on individual u32s */ u.val64 = bswap_64(u.val64); u.val32[0] = bswap_32(u.val32[0]); u.val32[1] = bswap_32(u.val32[1]); }
Quoted 7 lines
That's not quite what we need. The bswap's happen all the time; the warn once is to tell the user one time that samples in the file contain raw data and those cannot be programmatically adjusted for endianness.
ie., more like:
WARN_ONCE(swapped, "Endianness of raw data not corrected!\n");and no action taken (the 'if (WARN_ONCE())'part).
Look again:
#define WARN_ONCE(condition, format...) ({ \
static bool __warned; \
int __ret_warn_once = !!(condition); \
\
if (unlikely(__ret_warn_once)) \
if (WARN(!__warned, format)) \
__warned = true; \
unlikely(__ret_warn_once); \
}) See that ({ }) construct? It evaluates to what is in its last statement, which is...
unlikely(__ret_warn_once);
Forget about the unlikely, __ret_warn_once is:
!!condition
I.e. it always evaluates to what is passed as condition, so in fact it could be seen as:
if (swapped) {
/* undo swap of u64, then swap on individual
u32s */
u.val64 = bswap_64(u.val64);
u.val32[0] = bswap_32(u.val32[0]);
u.val32[1] = bswap_32(u.val32[1]);
}The rest is the boilerplate needed to warn the user the first time condition is true.
- Arnaldo
Quoted 33 lines · Arnaldo Carvalho de Melo
Look again:
#define WARN_ONCE(condition, format...) ({ \ static bool __warned; \ int __ret_warn_once = !!(condition); \ \ if (unlikely(__ret_warn_once)) \ if (WARN(!__warned, format)) \ __warned = true; \ unlikely(__ret_warn_once); \ })See that ({ }) construct? It evaluates to what is in its last statement, which is...
unlikely(__ret_warn_once);Forget about the unlikely,
__ret_warn_onceis:!!condition
I.e. it always evaluates to what is passed as condition, so in fact it could be seen as:
if (swapped) { /* undo swap of u64, then swap on individual u32s */ u.val64 = bswap_64(u.val64); u.val32[0] = bswap_32(u.val32[0]); u.val32[1] = bswap_32(u.val32[1]); }
Quoted 4 lines
The rest is the boilerplate needed to warn the user the first time condition is true.
- Arnaldo
Ok, I get it now.
David