Patchwork [5/7] powerpc/perf: Define big-endian version of perf_mem_data_src

login
register
mail settings
Submitter sukadev@linux.vnet.ibm.com
Date Aug. 10, 2013, 5:51 p.m.
Message ID <20130810175153.GF15551@us.ibm.com>
Download mbox | patch
Permalink /patch/266250/
State Superseded
Headers show

Comments

sukadev@linux.vnet.ibm.com - Aug. 10, 2013, 5:51 p.m.
[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(-)
Vince Weaver - Aug. 11, 2013, 2:34 a.m.
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
sukadev@linux.vnet.ibm.com - Aug. 11, 2013, 5:15 p.m.
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
Michael Ellerman - Aug. 11, 2013, 11:57 p.m.
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
Vince Weaver - Aug. 12, 2013, 3:19 a.m.
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

Patch

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 */