Skip to contentSkip to conversation

Perf-tool/MIPS: support cross compiling of tools/perf for MIPS

PATCH RESEND Superseded Deng-Cheng Zhu 3 participants 5 messages Sep 30, 2010 – Oct 9, 2010

Subsystems (2)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

PATCH RESEND 1/1 Perf-tool/MIPS: support cross compiling of tools/perf for MIPS

(Directing this patch to Perf-events maintainers for review.)

With the kernel facility of Linux performance counters, we want the user level tool tools/perf to be cross compiled for MIPS platform. To do this, we need to include unistd.h, add rmb() and cpu_relax() in perf.h.

Your review comments are especially required for the definition of rmb():
In perf.h, we need to have a proper rmb() for _all_ MIPS platforms. And we don't have CONFIG_* things for use in here. Looking at barrier.h, rmb() goes into barrier() and __sync() for CAVIUM OCTEON and other CPUs, respectively. What's more, __sync() has different versions as well. Referring to BARRIER() in dump_tlb.c, I propose the "common" definition for perf tool rmb() in this patch. Do you have any comments?

In addition, for testing the kernel part code I sent several days ago, I was using the "particular" rmb() version for 24K/34K/74K cores:

#define rmb()           asm volatile(                           \
                                ".set   push\n\t"               \
                                ".set   noreorder\n\t"          \
                                ".set   mips2\n\t"              \
                                "sync\n\t"                      \
                                ".set   pop"                    \
                                : /* no output */               \
                                : /* no input */                \
                                : "memory")

This is the definition of __sync() for CONFIG_CPU_HAS_SYNC.

Thanks,

Deng-Cheng

---
 tools/perf/perf.h |   12 ++++++++++++
 1 files changed, 12 insertions(+), 0 deletions(-)
tools/perf/perf.h +12 -0
index 6fb379b..cd05284 100644
@@ -73,6 +73,18 @@
 #define cpu_relax()	asm volatile("":::"memory")
 #endif
 
+#ifdef __mips__
+#include "../../arch/mips/include/asm/unistd.h"
+#define rmb()		asm volatile(					\
+				".set	noreorder\n\t"			\
+				"nop;nop;nop;nop;nop;nop;nop\n\t"	\
+				".set	reorder"			\
+				: /* no output */			\
+				: /* no input */			\
+				: "memory")
+#define cpu_relax()	asm volatile("" ::: "memory")
+#endif
+
 #include <time.h>
 #include <unistd.h>
 #include <sys/types.h>
#1 lore raw
Quoted 8 lines · Deng-Cheng Zhu

(Directing this patch to Perf-events maintainers for review.)

With the kernel facility of Linux performance counters, we want the user level tool tools/perf to be cross compiled for MIPS platform. To do this, we need to include unistd.h, add rmb() and cpu_relax() in perf.h.

Quoted 8 lines

Your review comments are especially required for the definition of rmb():
In perf.h, we need to have a proper rmb() for _all_ MIPS platforms. And we don't have CONFIG_* things for use in here. Looking at barrier.h, rmb() goes into barrier() and __sync() for CAVIUM OCTEON and other CPUs, respectively. What's more, __sync() has different versions as well. Referring to BARRIER() in dump_tlb.c, I propose the "common" definition for perf tool rmb() in this patch. Do you have any comments?

In fact I do.

In user space the rmb() must expand to a SYNC instruction. I am not sure what your version in the patch is doing with all those NOPs. That is not guaranteed to do anything.

The instruction set specifications say that SYNC orders all loads and stores. This is a heaver operation than rmb() demands, but is the only universally available instruction that imposes ordering.

For processors that do not support SYNC, the kernel will emulate it, so it is safe to use in userspace. I wouldn't worry about emulation overhead though, because processors that lack SYNC probably also lack performance counters, so are not as interesting from a perf-tool point of view.

David Daney

Quoted 50 lines

In addition, for testing the kernel part code I sent several days ago, I was using the "particular" rmb() version for 24K/34K/74K cores:

#define rmb()           asm volatile(                           \
                                 ".set   push\n\t"               \
                                 ".set   noreorder\n\t"          \
                                 ".set   mips2\n\t"              \
                                 "sync\n\t"                      \
                                 ".set   pop"                    \
                                 : /* no output */               \
                                 : /* no input */                \
                                 : "memory")

This is the definition of __sync() for CONFIG_CPU_HAS_SYNC.

Thanks,

Deng-Cheng

---
  tools/perf/perf.h |   12 ++++++++++++
  1 files changed, 12 insertions(+), 0 deletions(-)
tools/perf/perf.h +12 -0
index 6fb379b..cd05284 100644
@@ -73,6 +73,18 @@
  #define cpu_relax()	asm volatile("":::"memory")
  #endif
 
+#ifdef __mips__
+#include "../../arch/mips/include/asm/unistd.h"
+#define rmb()		asm volatile(					\
+				".set	noreorder\n\t"			\
+				"nop;nop;nop;nop;nop;nop;nop\n\t"	\
+				".set	reorder"			\
+				: /* no output */			\
+				: /* no input */			\
+				: "memory")
+#define cpu_relax()	asm volatile("" ::: "memory")
+#endif
+
  #include<time.h>
  #include<unistd.h>
  #include<sys/types.h>
#2 lore raw
Quoted 3 lines · David Daney

In user space the rmb() must expand to a SYNC instruction. I am not sure what your version in the patch is doing with all those NOPs. That is not guaranteed to do anything.

That's a rather old version of the kernel rmb macro I think. The NOPs where there to enforce ordering of a mix of cached and uncached accesses on the R4400 (not R4000) where according to my reading the manual leaves it a bit unclear if a SYNC is sufficient or if the pipeline needs to be drained in addition. See version 2 of the R4000/R4400 User's Manual.

Quoted 9 lines

The instruction set specifications say that SYNC orders all loads and stores. This is a heaver operation than rmb() demands, but is the only universally available instruction that imposes ordering.

For processors that do not support SYNC, the kernel will emulate it, so it is safe to use in userspace. I wouldn't worry about emulation overhead though, because processors that lack SYNC probably also lack performance counters, so are not as interesting from a perf-tool point of view.

Yes, just use SYNC. SYNC-less processors would only be R2000/R3000 processors and a few other oddball processors which for performance optimization are totally uninteresting since years.

Ralf

#3 lore raw

Thanks guys. So let's turn the patch into the following?

---
 tools/perf/perf.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
tools/perf/perf.h +14 -0
index 6fb379b..cd05284 100644
@@ -73,6 +73,20 @@
 #define cpu_relax()	asm volatile("":::"memory")
 #endif
 
+#ifdef __mips__
+#include "../../arch/mips/include/asm/unistd.h"
+#define rmb()		asm volatile(					\
+				".set	push\n\t"			\
+				".set	noreorder\n\t"			\
+				".set	mips2\n\t"			\
+				"sync\n\t"				\
+				".set	pop"				\
+				: /* no output */			\
+				: /* no input */			\
+				: "memory")
+#define cpu_relax()	asm volatile("" ::: "memory")
+#endif
+
 #include<time.h>
 #include<unistd.h>
 #include<sys/types.h>
Quoted 25 lines · Ralf Baechle
Quoted 3 lines · David Daney

In user space the rmb() must expand to a SYNC instruction. I am not sure what your version in the patch is doing with all those NOPs. That is not guaranteed to do anything.

That's a rather old version of the kernel rmb macro I think. The NOPs where there to enforce ordering of a mix of cached and uncached accesses on the R4400 (not R4000) where according to my reading the manual leaves it a bit unclear if a SYNC is sufficient or if the pipeline needs to be drained in addition. See version 2 of the R4000/R4400 User's Manual.

Quoted 9 lines

The instruction set specifications say that SYNC orders all loads and stores. This is a heaver operation than rmb() demands, but is the only universally available instruction that imposes ordering.

For processors that do not support SYNC, the kernel will emulate it, so it is safe to use in userspace. I wouldn't worry about emulation overhead though, because processors that lack SYNC probably also lack performance counters, so are not as interesting from a perf-tool point of view.

Yes, just use SYNC. SYNC-less processors would only be R2000/R3000 processors and a few other oddball processors which for performance optimization are totally uninteresting since years.

Ralf

#4 lore raw
Quoted 18 lines · Deng-Cheng Zhu

Thanks guys. So let's turn the patch into the following?

---
 tools/perf/perf.h |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)
tools/perf/perf.h +3 -0
index 6fb379b..cd05284 100644
@@ -73,6 +73,20 @@
 #define cpu_relax()	asm volatile("":::"memory")
 #endif
 
+#ifdef __mips__
+#include "../../arch/mips/include/asm/unistd.h"
+#define rmb()		asm volatile(					\

tools/perf/perf.h line 88

Quoted 10 lines
tools/perf/perf.h +10 -0
+				".set	push\n\t"			\
+				".set	noreorder\n\t"			\
+				".set	mips2\n\t"			\
+				"sync\n\t"				\
+				".set	pop"				\
+				: /* no output */			\
+				: /* no input */			\
+				: "memory")
+#define cpu_relax()	asm volatile("" ::: "memory")
+#endif

Yes, aside of cosmetic issues this is looking good. The cosmetic issue is that there are lots of useless dot-ops in the inline assembler. That could be reduced to just .set mips2 before the SYNC and .set mips0 after.

There are some considerations wrt. to the MT ASE so I've bounced a few emails around in the meantime but that's nothing that should stop merging this patch with the rest of the perf patches.

Ralf