| Message ID | 20130810175153.GF15551@us.ibm.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On Sat, 10 Aug 2013, Sukadev Bhattiprolu wrote: > > include/uapi/linux/perf_event.h | 55 +++++++++++++++++++++++++++++++++++++++ > 1 files changed, 55 insertions(+), 0 deletions(-) > +#define __PERF_LE 1234 > +#define __PERF_BE 4321 > + > +#if defined(__KERNEL__) I could be wrong, but I thought files under uapi weren't supposed to contain __KERNEL__ code. Wasn't that the whole point of uapi? Also having the perf_event interface depend on endianess just seems like a complicated mess. Can't we just declare the interface to be a certain endianess and have the kernel byte-swap as necessary? Vince
Vince Weaver [vince@deater.net] wrote: | On Sat, 10 Aug 2013, Sukadev Bhattiprolu wrote: | | > | > include/uapi/linux/perf_event.h | 55 +++++++++++++++++++++++++++++++++++++++ | > 1 files changed, 55 insertions(+), 0 deletions(-) | | > +#define __PERF_LE 1234 | > +#define __PERF_BE 4321 | > + | > +#if defined(__KERNEL__) | | I could be wrong, but I thought files under uapi weren't supposed to | contain __KERNEL__ code. Wasn't that the whole point of uapi? | | Also having the perf_event interface depend on endianess just seems like a | complicated mess. Can't we just declare the interface to be a certain | endianess and have the kernel byte-swap as necessary? Except for the __KERNEL__ check, it looked like this approach would keep the kernel and user code same. Would it complicate user space ? I tried to avoid the __KERNEL__ check hack, but like I tried to explain in the patch, user space and kernel do the endian check differently. And, there are about ~300 sites in the kernel with __*ENDIAN checks Sukadev
On Sat, Aug 10, 2013 at 10:34:58PM -0400, Vince Weaver wrote: > On Sat, 10 Aug 2013, Sukadev Bhattiprolu wrote: > > > > > include/uapi/linux/perf_event.h | 55 +++++++++++++++++++++++++++++++++++++++ > > 1 files changed, 55 insertions(+), 0 deletions(-) > > > +#define __PERF_LE 1234 > > +#define __PERF_BE 4321 > > + > > +#if defined(__KERNEL__) > > I could be wrong, but I thought files under uapi weren't supposed to > contain __KERNEL__ code. Wasn't that the whole point of uapi? Yes. > Also having the perf_event interface depend on endianess just seems like a > complicated mess. Can't we just declare the interface to be a certain > endianess and have the kernel byte-swap as necessary? Yes I think so. The interface is already defined and it's little endian, so on big endian we just need to swap. The only part I'm not clear on is how things are handled in perf userspace, it seems to already do some byte swapping. cheers
On Mon, 12 Aug 2013, Michael Ellerman wrote: > > Yes I think so. The interface is already defined and it's little endian, > so on big endian we just need to swap. > > The only part I'm not clear on is how things are handled in perf > userspace, it seems to already do some byte swapping. It would be nice to clarify this. "struct perf_branch_entry" also has bitfields like this, though to make things more confusing that structure isn't exported via the uapi header so it's not clear how userspace code is supposed to interpret the values. As you say it gets complicated with perf userspace, especially in cases where you record the data on big-endian but then try to analyze the results on a little-endian machine. It would be nice to get confirmation that these bitfields will always be little-endian. I guess they currently are by definition because only x86/pebs sets data.data_src.val so far? Vince
diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index 62c25a2..8497c51 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -19,6 +19,47 @@ #include <asm/byteorder.h> /* + * Kernel and userspace check for endianness in incompatible ways. + * In user space, <endian.h> defines both __BIG_ENDIAN and __LITTLE_ENDIAN + * but sets __BYTE_ORDER to one or the other. So user space uses checks are: + * + * #if __BYTE_ORDER == __LITTLE_ENDIAN + * + * In the kernel, __BYTE_ORDER is undefined, so using the above check doesn't + * work. Further, kernel code assumes that exactly one of __BIG_ENDIAN and + * __LITTLE_ENDIAN is defined. So the kernel checks are like: + * + * #if defined(__LITTLE_ENDIAN) + * + * But we can't use that check in user space since __LITTLE_ENDIAN (and + * __BIG_ENDIAN) are always defined. + * + * Since some perf data structures depend on endianness _and_ are shared + * between kernel and user, perf needs its own notion of endian macros (at + * least until user and kernel endian checks converge). + */ +#define __PERF_LE 1234 +#define __PERF_BE 4321 + +#if defined(__KERNEL__) + +#if defined(__LITTLE_ENDIAN) +#define __PERF_BYTE_ORDER __PERF_LE +#elif defined(__BIG_ENDIAN) +#define __PERF_BYTE_ORDER __PERF_BE +#endif + +#else /* __KERNEL__ */ + +#if __BYTE_ORDER == __LITTLE_ENDIAN +#define __PERF_BYTE_ORDER __PERF_LE +#elif __BYTE_ORDER == __BIG_ENDIAN +#define __PERF_BYTE_ORDER __PERF_BE +#endif + +#endif /* __KERNEL__ */ + +/* * User-space ABI bits: */ @@ -659,6 +700,7 @@ enum perf_callchain_context { #define PERF_FLAG_FD_OUTPUT (1U << 1) #define PERF_FLAG_PID_CGROUP (1U << 2) /* pid=cgroup id, per-cpu mode only */ +#if __PERF_BYTE_ORDER == __PERF_LE union perf_mem_data_src { __u64 val; struct { @@ -670,6 +712,19 @@ union perf_mem_data_src { mem_rsvd:31; }; }; +#elif __PERF_BYTE_ORDER == __PERF_BE +union perf_mem_data_src { + __u64 val; + struct { + __u64 mem_rsvd:31, + mem_dtlb:7, /* tlb access */ + mem_lock:2, /* lock instr */ + mem_snoop:5, /* snoop mode */ + mem_lvl:14, /* memory hierarchy level */ + mem_op:5; /* type of opcode */ + }; +}; +#endif /* type of opcode (load/store/prefetch,code) */ #define PERF_MEM_OP_NA 0x01 /* not available */
[PATCH 5/7] powerpc/perf: Define big-endian version of perf_mem_data_src perf_mem_data_src is an union that is initialized via the ->val field and accessed via the bitmap fields. For this to work on big endian platforms, we also need a big-endian represenation of perf_mem_data_src. Cc: Stephane Eranian <eranian@google.com> Cc: Paul Mckerras <paulus@samba.org> Cc: Michael Ellerman <michael@ellerman.id.au> Signed-off-by: Sukadev Bhattiprolu <sukadev@linux.vnet.ibm.com> --- Thanks to input from Stephane Eranian and Michael Ellerman. include/uapi/linux/perf_event.h | 55 +++++++++++++++++++++++++++++++++++++++ 1 files changed, 55 insertions(+), 0 deletions(-)