ppc64 ftrace: mark data_access callees "notrace" (pt.1)

PATCH In review Torsten Duwe 5 participants 16 messages May 13, 2015 – Jun 8, 2015

Subsystems (4)

PowerPC Kbuild Tracing ftrace

lore

Share & tools

lore

Torsten Duwe

author

2015-05-13 16:11 UTC #0 lore raw

PATCH 1/1 ppc64 ftrace: mark data_access callees "notrace" (pt.1)

In order to avoid an endless recursion, functions that may get called from the data access handler must not call into tracing functions, which may cause data access faults ;-)

Advancing from my previous approach that lavishly compiled whole subdirs without the profiling switches, this is more fine-grained (but probably yet incomplete). This patch is necessary albeit not sufficient for FTRACE_WITH_REGS on ppc64.

Patch is against 3.17; fuzz+force should do no harm.

Signed-off-by Torsten Duwe <[email protected]>

arch/powerpc/kernel/process.c +1 -1

index bf44ae9..29ed85f 100644
@@ -717,7 +717,7 @@ static inline void __switch_to_tm(struct task_struct *prev)
  * don't know which of the checkpointed state and the transactional
  * state to use.
  */
-void restore_tm_state(struct pt_regs *regs)
+notrace void restore_tm_state(struct pt_regs *regs)
 {
 	unsigned long msr_diff;

arch/powerpc/mm/hash_utils_64.c +8 -8

index daee7f4..a8e5b8c 100644
@@ -846,7 +846,7 @@ void early_init_mmu_secondary(void)
 /*
  * Called by asm hashtable.S for doing lazy icache flush
  */
-unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)
+notrace unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)
 {
 	struct page *page;

@@ -867,7 +867,7 @@ unsigned int hash_page_do_lazy_icache(unsigned int pp, pte_t pte, int trap)
 }
 
 #ifdef CONFIG_PPC_MM_SLICES
-unsigned int get_paca_psize(unsigned long addr)
+notrace unsigned int get_paca_psize(unsigned long addr)
 {
 	u64 lpsizes;
 	unsigned char *hpsizes;

@@ -896,7 +896,7 @@ unsigned int get_paca_psize(unsigned long addr)
  * For now this makes the whole process use 4k pages.
  */
 #ifdef CONFIG_PPC_64K_PAGES
-void demote_segment_4k(struct mm_struct *mm, unsigned long addr)
+notrace void demote_segment_4k(struct mm_struct *mm, unsigned long addr)
 {
 	if (get_slice_psize(mm, addr) == MMU_PAGE_4K)
 		return;

@@ -919,7 +919,7 @@ void demote_segment_4k(struct mm_struct *mm, unsigned long addr)
  * Result is 0: full permissions, _PAGE_RW: read-only,
  * _PAGE_USER or _PAGE_USER|_PAGE_RW: no access.
  */
-static int subpage_protection(struct mm_struct *mm, unsigned long ea)
+static notrace int subpage_protection(struct mm_struct *mm, unsigned long ea)
 {
 	struct subpage_prot_table *spt = &mm->context.spt;
 	u32 spp = 0;

@@ -967,7 +967,7 @@ void hash_failure_debug(unsigned long ea, unsigned long access,
 		trap, vsid, ssize, psize, lpsize, pte);
 }
 
-static void check_paca_psize(unsigned long ea, struct mm_struct *mm,
+static notrace void check_paca_psize(unsigned long ea, struct mm_struct *mm,
 			     int psize, bool user_region)
 {
 	if (user_region) {

@@ -989,7 +989,7 @@ static void check_paca_psize(unsigned long ea, struct mm_struct *mm,
  * -1 - critical hash insertion error
  * -2 - access not permitted by subpage protection mechanism
  */
-int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
+notrace int hash_page(unsigned long ea, unsigned long access, unsigned long trap)
 {
 	enum ctx_state prev_state = exception_enter();
 	pgd_t *pgdir;

@@ -1269,7 +1269,7 @@ out_exit:
 /* WARNING: This is called from hash_low_64.S, if you change this prototype,
  *          do not forget to update the assembly call site !
  */
-void flush_hash_page(unsigned long vpn, real_pte_t pte, int psize, int ssize,
+notrace void flush_hash_page(unsigned long vpn, real_pte_t pte, int psize, int ssize,
 		     int local)
 {
 	unsigned long hash, index, shift, hidx, slot;

@@ -1343,7 +1343,7 @@ void low_hash_fault(struct pt_regs *regs, unsigned long address, int rc)
 	exception_exit(prev_state);
 }
 
-long hpte_insert_repeating(unsigned long hash, unsigned long vpn,
+notrace long hpte_insert_repeating(unsigned long hash, unsigned long vpn,
 			   unsigned long pa, unsigned long rflags,
 			   unsigned long vflags, int psize, int ssize)
 {

arch/powerpc/mm/hugetlbpage-hash64.c +1 -1

index a5bcf93..ea911b2 100644
@@ -18,7 +18,7 @@ extern long hpte_insert_repeating(unsigned long hash, unsigned long vpn,
 				  unsigned long pa, unsigned long rlags,
 				  unsigned long vflags, int psize, int ssize);
 
-int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
+notrace int __hash_page_huge(unsigned long ea, unsigned long access, unsigned long vsid,
 		     pte_t *ptep, unsigned long trap, int local, int ssize,
 		     unsigned int shift, unsigned int mmu_psize)
 {

arch/powerpc/mm/hugetlbpage.c +2 -2

index 7e70ae9..4fe2338 100644
@@ -905,7 +905,7 @@ static int __init hugetlbpage_init(void)
 #endif
 module_init(hugetlbpage_init);
 
-void flush_dcache_icache_hugepage(struct page *page)
+notrace void flush_dcache_icache_hugepage(struct page *page)
 {
 	int i;
 	void *start;

@@ -936,7 +936,7 @@ void flush_dcache_icache_hugepage(struct page *page)
  * we can follow the address down to the the page and take a ref on it.
  */
 
-pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea, unsigned *shift)
+notrace pte_t *find_linux_pte_or_hugepte(pgd_t *pgdir, unsigned long ea, unsigned *shift)
 {
 	pgd_t pgd, *pgdp;
 	pud_t pud, *pudp;

arch/powerpc/mm/mem.c +1 -1

index e0f7a18..05f66b1 100644
@@ -400,7 +400,7 @@ void flush_dcache_page(struct page *page)
 }
 EXPORT_SYMBOL(flush_dcache_page);
 
-void flush_dcache_icache_page(struct page *page)
+notrace void flush_dcache_icache_page(struct page *page)
 {
 #ifdef CONFIG_HUGETLB_PAGE
 	if (PageCompound(page)) {

arch/powerpc/mm/pgtable_64.c +1 -1

index c8d709a..72bd991 100644
@@ -444,7 +444,7 @@ static void page_table_free_rcu(void *table)
 	}
 }
 
-void pgtable_free_tlb(struct mmu_gather *tlb, void *table, int shift)
+notrace void pgtable_free_tlb(struct mmu_gather *tlb, void *table, int shift)
 {
 	unsigned long pgf = (unsigned long)table;

arch/powerpc/mm/slb.c +3 -3

index 0399a67..5ccc3c8 100644
@@ -94,7 +94,7 @@ static inline void create_shadowed_slbe(unsigned long ea, int ssize,
 		     : "memory" );
 }
 
-static void __slb_flush_and_rebolt(void)
+static notrace void __slb_flush_and_rebolt(void)
 {
 	/* If you change this make sure you change SLB_NUM_BOLTED
 	 * and PR KVM appropriately too. */

@@ -134,7 +134,7 @@ static void __slb_flush_and_rebolt(void)
 		     : "memory");
 }
 
-void slb_flush_and_rebolt(void)
+notrace void slb_flush_and_rebolt(void)
 {
 
 	WARN_ON(!irqs_disabled());

@@ -149,7 +149,7 @@ void slb_flush_and_rebolt(void)
 	get_paca()->slb_cache_ptr = 0;
 }
 
-void slb_vmalloc_update(void)
+notrace void slb_vmalloc_update(void)
 {
 	unsigned long vflags;

arch/powerpc/mm/slice.c +4 -4

index b0c75cc..c1efc4c 100644
@@ -74,8 +74,8 @@ static void slice_print_mask(const char *label, struct slice_mask mask) {}
 
 #endif
 
-static struct slice_mask slice_range_to_mask(unsigned long start,
-					     unsigned long len)
+static notrace struct slice_mask slice_range_to_mask(unsigned long start,
+						     unsigned long len)
 {
 	unsigned long end = start + len - 1;
 	struct slice_mask ret = { 0, 0 };

@@ -564,7 +564,7 @@ unsigned long arch_get_unmapped_area_topdown(struct file *filp,
 				       current->mm->context.user_psize, 1);
 }
 
-unsigned int get_slice_psize(struct mm_struct *mm, unsigned long addr)
+notrace unsigned int get_slice_psize(struct mm_struct *mm, unsigned long addr)
 {
 	unsigned char *hpsizes;
 	int index, mask_index;

@@ -676,7 +676,7 @@ void slice_set_psize(struct mm_struct *mm, unsigned long address,
 #endif
 }
 
-void slice_set_range_psize(struct mm_struct *mm, unsigned long start,
+notrace void slice_set_range_psize(struct mm_struct *mm, unsigned long start,
 			   unsigned long len, unsigned int psize)
 {
 	struct slice_mask mask = slice_range_to_mask(start, len);

Michael Ellerman

reviewer

2015-05-15 01:34 UTC #1 lore raw

Quoted 8 lines · Torsten Duwe

In order to avoid an endless recursion, functions that may get called from the data access handler must not call into tracing functions, which may cause data access faults ;-)
Advancing from my previous approach that lavishly compiled whole subdirs without the profiling switches, this is more fine-grained (but probably yet incomplete). This patch is necessary albeit not sufficient for FTRACE_WITH_REGS on ppc64.

There's got to be a better solution than this. The chance that you've correctly annotated every function is basically 0, and the chance that we correctly add it to every new or modififed function in the future is also 0.

I don't mean that as a criticism of you, but rather the technique. For starters I don't see any annotations in 32-bit code, or in the BookE code etc.

Can you give us more details on what goes wrong without these annotations?

cheers

Torsten Duwe

2015-05-15 08:45 UTC #2 lore raw

Quoted 5 lines · Michael Ellerman

Quoted 4 lines · Torsten Duwe
In order to avoid an endless recursion, functions that may get called from the data access handler must not call into tracing functions, which may cause data access faults ;-)

Quoted 7 lines

Quoted 4 lines
Advancing from my previous approach that lavishly compiled whole subdirs without the profiling switches, this is more fine-grained (but probably yet incomplete). This patch is necessary albeit not sufficient for FTRACE_WITH_REGS on ppc64.
There's got to be a better solution than this. The chance that you've correctly annotated every function is basically 0, and the chance that we correctly add

Well, I used an automated static code analysis to find these, so from that point the chances to find all the relevant funcs is significantly > 0.

Quoted 1 line

it to every new or modififed function in the future is also 0.

Yes, this worries me, too. This may lead to very obscure and confusing breakage :-(

Quoted 1 line

I don't mean that as a criticism of you, but rather the technique. For starters

No problem, I don't take this personally. I'd also prefer a more elegant solution, but the problem seems to stem from this particular hardware.

Quoted 1 line

I don't see any annotations in 32-bit code, or in the BookE code etc.

Exactly, for a start. 32-bit & friends would be another run, for all hardware where the MMU does not fully autoload. What does sparc do, btw?

Quoted 1 line

Can you give us more details on what goes wrong without these annotations?

e.g. ftrace tries to note that a function has been called. The memory location of the tracing framework that is to record this does not yet have an HTAB entry -> data access fault. Should any of the functions involved in the HTAB handling be profiled, ftrace will try to note that function call into some RAM location, which might still not have an entry, etc...

I've seen this lead into an endless recursion, unless, like I wrote and patched before, disabled tracing in all the relevant source dirs. This looked like overkill to me, hence the machine-aided approach to find the exact set of functions affected.

Can you think of a better approach?

Torsten

Torsten Duwe

author

2015-05-16 08:05 UTC #3 lore raw

Quoted 5 lines · Torsten Duwe

Quoted 2 lines · Michael Ellerman
There's got to be a better solution than this.
Can you think of a better approach?

Maybe a per thread variable to lock out a recursion into tracing? Thanks for your doubt.

Torsten

Jiri Kosina

2015-05-18 12:30 UTC #4 lore raw

Quoted 6 lines · Torsten Duwe

Quoted 3 lines
Quoted 1 line
There's got to be a better solution than this.
Can you think of a better approach?
Maybe a per thread variable to lock out a recursion into tracing? Thanks for your doubt.

ftrace already handles recursion protection by itself (depending on the per-ftrace-ops FTRACE_OPS_FL_RECURSION_SAFE flag).

It's however not really well-defined what to do when recursion would happen. Therefore __notrace__ annotation, that just completely avoid such situation by making tracing impossible, looks like saner general solution to me.

Signature · 2 lines

Jiri Kosina
SUSE Labs

Michael Ellerman

reviewer

2015-05-19 03:27 UTC #5 lore raw

Quoted 11 lines · Jiri Kosina

Quoted 6 lines · Torsten Duwe
Quoted 3 lines
Quoted 1 line
There's got to be a better solution than this.
Can you think of a better approach?
Maybe a per thread variable to lock out a recursion into tracing? Thanks for your doubt.
ftrace already handles recursion protection by itself (depending on the per-ftrace-ops FTRACE_OPS_FL_RECURSION_SAFE flag).

OK, so I wonder why that's not working for us?

Quoted 4 lines

It's however not really well-defined what to do when recursion would happen. Therefore __notrace__ annotation, that just completely avoid such situation by making tracing impossible, looks like saner general solution to me.

I disagree. Correctly annotating all functions that might be called ever and for all time is a maintenance nightmare and is never going to work in the long term.

cheers

Jiri Kosina

2015-05-19 09:52 UTC #6 lore raw

Quoted 4 lines · Michael Ellerman

Quoted 2 lines
ftrace already handles recursion protection by itself (depending on the per-ftrace-ops FTRACE_OPS_FL_RECURSION_SAFE flag).
OK, so I wonder why that's not working for us?

The situation when traced function recurses to itself is different from the situation when tracing core infrastrcuture would recurse to itself while performing tracing.

Quoted 8 lines

Quoted 4 lines
It's however not really well-defined what to do when recursion would happen. Therefore __notrace__ annotation, that just completely avoid such situation by making tracing impossible, looks like saner general solution to me.
I disagree. Correctly annotating all functions that might be called ever and for all time is a maintenance nightmare and is never going to work in the long term.

All the functions called by ftrace must be marked as notrace, there is no way out of it.

Signature · 2 lines

Jiri Kosina
SUSE Labs

Torsten Duwe

author

2015-06-03 13:03 UTC #7 lore raw

cover ppc64 ftrace implementation

Quoted 10 lines · Jiri Kosina

Quoted 4 lines · Michael Ellerman
Quoted 2 lines
ftrace already handles recursion protection by itself (depending on the per-ftrace-ops FTRACE_OPS_FL_RECURSION_SAFE flag).
OK, so I wonder why that's not working for us?
The situation when traced function recurses to itself is different from the situation when tracing core infrastrcuture would recurse to itself while performing tracing.

I have used this inspiration to add a catch-all parachute for ftrace_caller, see my last reply. It reappears here as patch 4/4. Expect noticable performance impact compared to the selected "notrace" attributation discussed here. This should still be done in a second step especially for the hardware assistance functions I mentioned.

Torsten

Torsten Duwe

author

2015-06-03 13:08 UTC #8 lore raw

Re: [PATCH 1/4] ppc64 ftrace implementation

Implement ftrace on ppc64

Signed-off-by Torsten Duwe <[email protected]>

arch/powerpc/include/asm/ftrace.h +5 -0

index e366187..6111191 100644
@@ -46,6 +46,8 @@
 extern void _mcount(void);
 
 #ifdef CONFIG_DYNAMIC_FTRACE
+# define FTRACE_ADDR ((unsigned long)ftrace_caller+8)
+# define FTRACE_REGS_ADDR FTRACE_ADDR
 static inline unsigned long ftrace_call_adjust(unsigned long addr)
 {
        /* reloction of mcount call site is the same as the address */

@@ -58,6 +60,9 @@ struct dyn_arch_ftrace {
 #endif /*  CONFIG_DYNAMIC_FTRACE */
 #endif /* __ASSEMBLY__ */
 
+#ifdef CONFIG_DYNAMIC_FTRACE
+#define ARCH_SUPPORTS_FTRACE_OPS 1
+#endif
 #endif
 
 #if defined(CONFIG_FTRACE_SYSCALLS) && defined(CONFIG_PPC64) && !defined(__ASSEMBLY__)

arch/powerpc/kernel/entry_64.S +92 ~~-20~~

146 lines not on this page · raw message

arch/powerpc/kernel/ftrace.c +59 -8

115 lines not on this page · raw message

arch/powerpc/kernel/module_64.c +30 -3

index 6838451..1428ad8 100644
@@ -138,12 +138,22 @@ static u32 ppc64_stub_insns[] = {
 	0x4e800420			/* bctr */
 };
 
+/* In case of _mcount calls or dynamic ftracing, Do not save the
+   current callee's TOC (in R2) again into the original caller's stack
+   frame during this trampoline hop. The stack frame already holds
+   that of the original caller.  _mcount and ftrace_caller will take
+   care of this TOC value themselves.
+*/
+#define SQUASH_TOC_SAVE_INSN(trampoline_addr) \
+	((struct ppc64_stub_entry*)(trampoline_addr))-> \
+	jump[2] = PPC_INST_NOP;
+
 #ifdef CONFIG_DYNAMIC_FTRACE
 
 static u32 ppc64_stub_mask[] = {
 	0xffff0000,
 	0xffff0000,
-	0xffffffff,
+	0x00000000,
 	0xffffffff,
 #if !defined(_CALL_ELF) || _CALL_ELF != 2
 	0xffffffff,

@@ -170,6 +180,9 @@ bool is_module_trampoline(u32 *p)
 		if ((insna & mask) != (insnb & mask))
 			return false;
 	}
+	if (insns[2] != ppc64_stub_insns[2] &&
+	    insns[2] != PPC_INST_NOP )
+		return false;
 
 	return true;
 }

@@ -475,6 +488,14 @@ static unsigned long stub_for_addr(Elf64_Shdr *sechdrs,
 static int restore_r2(u32 *instruction, struct module *me)
 {
 	if (*instruction != PPC_INST_NOP) {
+
+		/* -mprofile_kernel sequence starting with mflr r0; std r0, LRSAVE(r1) */
+		if (instruction[-3] == 0x7c0802a6 && instruction[-2] == 0xf8010010) {
+			/* Nothing to be done here, it's a _mcount call location
+			   and r2 will have to be restored in the _mcount function */
+			return 2;
+		};
+
 		pr_err("%s: Expect noop after relocate, got %08x\n",
 		       me->name, *instruction);
 		return 0;

@@ -490,7 +511,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 		       unsigned int relsec,
 		       struct module *me)
 {
-	unsigned int i;
+	unsigned int i, r2;
 	Elf64_Rela *rela = (void *)sechdrs[relsec].sh_addr;
 	Elf64_Sym *sym;
 	unsigned long *location;

@@ -603,8 +624,11 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 				value = stub_for_addr(sechdrs, value, me);
 				if (!value)
 					return -ENOENT;
-				if (!restore_r2((u32 *)location + 1, me))
+				if (!(r2 = restore_r2((u32 *)location + 1, me)))
 					return -ENOEXEC;
+				/* Squash the TOC saver for profiler calls */
+				if (!strcmp("_mcount", strtab+sym->st_name))
+					SQUASH_TOC_SAVE_INSN(value);
 			} else
 				value += local_entry_offset(sym);

@@ -665,6 +689,9 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
 	me->arch.tramp = stub_for_addr(sechdrs,
 				       (unsigned long)ftrace_caller,
 				       me);
+	/* ftrace_caller will take care of the TOC;
+	   do not clobber original caller's value. */
+	SQUASH_TOC_SAVE_INSN(me->arch.tramp);
 #endif
 
 	return 0;

Steven Rostedt

maintainer

2015-06-08 15:30 UTC #9 lore raw

Re: [PATCH 1/4] ppc64 ftrace implementation

Why are your patches all as replies?

Quoted 1 line · On Wed, 3 Jun 2015 15:08:29 +0200 Torsten Duwe <[email protected]> wrote:

Implement ftrace on ppc64

What do you mean "Implement ftrace on ppc64"? It's already implemented.

I'm not even going to bother looking at this patch because I have no idea what its purpose is.

-- Steve

Quoted 3 lines

Signed-off-by Torsten Duwe <[email protected]>

Torsten Duwe

author

2015-06-08 15:57 UTC #10 lore raw

Re: [PATCH 1/4] ppc64 ftrace implementation

Quoted 2 lines · Steven Rostedt

Why are your patches all as replies?

Because I cared too much about the threading and the series that the "Re:" escaped me.

Quoted 3 lines

Quoted 1 line
Implement ftrace on ppc64
What do you mean "Implement ftrace on ppc64"? It's already implemented.

Sorry, too deep in the details. It's DYNAMIC_FTRACE_WITH_REGS, actually. Using gcc's -mprofile-kernel to call into ftrace...

Quoted 2 lines

I'm not even going to bother looking at this patch because I have no idea what its purpose is.

Understandable ;-)

So: Implement DYNAMIC_FTRACE_WITH_REGS, in order to prepare for live patching. Other obvious things I forgot to mention?

Torsten

Torsten Duwe

author

2015-06-03 13:10 UTC #11 lore raw

Re: [PATCH 2/4] ppc64 ftrace configuration

In reply to Torsten Duwe

Add Kconfig variables and Makefile magic for ftrace with -mprofile-kernel

Signed-off-by Torsten Duwe <[email protected]>

Makefile +4 -1

index 3d16bcc..bbd5e87 100644
@@ -733,7 +733,10 @@ export CC_FLAGS_FTRACE
 ifdef CONFIG_HAVE_FENTRY
 CC_USING_FENTRY	:= $(call cc-option, -mfentry -DCC_USING_FENTRY)
 endif
-KBUILD_CFLAGS	+= $(CC_FLAGS_FTRACE) $(CC_USING_FENTRY)
+ifdef CONFIG_HAVE_MPROFILE_KERNEL
+CC_USING_MPROFILE_KERNEL	:= $(call cc-option, -mprofile-kernel -DCC_USING_MPROFILE_KERNEL)
+endif
+KBUILD_CFLAGS	+= $(CC_FLAGS_FTRACE) $(CC_USING_FENTRY) $(CC_USING_MPROFILE_KERNEL)
 KBUILD_AFLAGS	+= $(CC_USING_FENTRY)
 ifdef CONFIG_DYNAMIC_FTRACE
 	ifdef CONFIG_HAVE_C_RECORDMCOUNT

arch/powerpc/Kconfig +2 -0

index 22b0940..566f204 100644
@@ -94,8 +94,10 @@ config PPC
 	select OF_RESERVED_MEM
 	select HAVE_FTRACE_MCOUNT_RECORD
 	select HAVE_DYNAMIC_FTRACE
+	select HAVE_DYNAMIC_FTRACE_WITH_REGS
 	select HAVE_FUNCTION_TRACER
 	select HAVE_FUNCTION_GRAPH_TRACER
+	select HAVE_MPROFILE_KERNEL
 	select SYSCTL_EXCEPTION_TRACE
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select VIRT_TO_BUS if !PPC64

kernel/trace/Kconfig +5 -0

index a5da09c..dd53f3d 100644
@@ -52,6 +52,11 @@ config HAVE_FENTRY
 	help
 	  Arch supports the gcc options -pg with -mfentry
 
+config HAVE_MPROFILE_KERNEL
+	bool
+	help
+	  Arch supports the gcc options -pg with -mprofile-kernel
+
 config HAVE_C_RECORDMCOUNT
 	bool
 	help

Torsten Duwe

author

2015-06-03 13:15 UTC #12 lore raw

Re: [PATCH 3/4] ppc64 ftrace: spare early boot and low level code

In reply to Torsten Duwe

Using -mprofile-kernel on early boot code not only confuses the checker but is also useless, as the infrastructure is not yet in place. Proceed like with -pg, equally with time.o and ftrace itself.

Signed-off-by Torsten Duwe <[email protected]>

arch/powerpc/kernel/Makefile +6 -6

index 502cf69..fb33fc5 100644
@@ -17,14 +17,14 @@ endif
 
 ifdef CONFIG_FUNCTION_TRACER
 # Do not trace early boot code
-CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog
-CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_cputable.o = -pg -mno-sched-epilog -mprofile-kernel
+CFLAGS_REMOVE_prom_init.o = -pg -mno-sched-epilog -mprofile-kernel
+CFLAGS_REMOVE_btext.o = -pg -mno-sched-epilog -mprofile-kernel
+CFLAGS_REMOVE_prom.o = -pg -mno-sched-epilog -mprofile-kernel
 # do not trace tracer code
-CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_ftrace.o = -pg -mno-sched-epilog -mprofile-kernel
 # timers used by tracing
-CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog
+CFLAGS_REMOVE_time.o = -pg -mno-sched-epilog -mprofile-kernel
 endif
 
 obj-y				:= cputable.o ptrace.o syscalls.o \

Torsten Duwe

author

2015-06-03 13:22 UTC #13 lore raw

Re: [PATCH 4/4] ppc64 ftrace recursion protection

In reply to Torsten Duwe

As suggested by You and Jikos, a flag in task_struct's trace_recursion is used to block a tracer function to recurse into itself, especially on a data access fault. This should catch all functions called by the fault handlers which are not yet attributed notrace.

Signed-off-by Torsten Duwe <[email protected]>

arch/powerpc/kernel/asm-offsets.c +1 -0

index 4717859..ae10752 100644
@@ -72,6 +72,7 @@ int main(void)
 	DEFINE(THREAD, offsetof(struct task_struct, thread));
 	DEFINE(MM, offsetof(struct task_struct, mm));
 	DEFINE(MMCONTEXTID, offsetof(struct mm_struct, context.id));
+	DEFINE(TASK_TRACEREC, offsetof(struct task_struct, trace_recursion));
 #ifdef CONFIG_PPC64
 	DEFINE(AUDITCONTEXT, offsetof(struct task_struct, audit_context));
 	DEFINE(SIGSEGV, SIGSEGV);

arch/powerpc/kernel/entry_64.S +13 -2

index a4132ef..4768104 100644
@@ -1202,7 +1202,13 @@ _GLOBAL(ftrace_caller)
 	SAVE_8GPRS(16,r1)
 	SAVE_8GPRS(24,r1)
 
-
+	ld	r3, PACACURRENT(r13)
+	ld	r4, TASK_TRACEREC(r3)
+	andi.	r5, r4, 0x0010 // ( 1 << TRACE_FTRACE_BIT )
+	ori	r4, r4, 0x0010
+	std	r4, TASK_TRACEREC(r3)
+	bne	3f		// ftrace in progress - avoid recursion!
+	
 	LOAD_REG_IMMEDIATE(r3,function_trace_op)
 	ld	r5,0(r3)

@@ -1224,9 +1230,14 @@ ftrace_call:
 	bl	ftrace_stub
 	nop
 
+	ld	r3, PACACURRENT(r13)
+	ld	r4, TASK_TRACEREC(r3)
+	andi.	r4, r4, 0xffef // ~( 1 << TRACE_FTRACE_BIT )
+	std	r4, TASK_TRACEREC(r3)
+
 	ld	r3, _NIP(r1)
 	mtlr	r3
-
+3:
 	REST_8GPRS(0,r1)
 	REST_8GPRS(8,r1)
 	REST_8GPRS(16,r1)

Torsten Duwe

author

2015-05-20 09:03 UTC #14 lore raw

In reply to Michael Ellerman

Quoted 6 lines · Michael Ellerman

Quoted 3 lines · Jiri Kosina
ftrace already handles recursion protection by itself (depending on the per-ftrace-ops FTRACE_OPS_FL_RECURSION_SAFE flag).
OK, so I wonder why that's not working for us?

IIRC a data access fault happens just before that flag is looked at ;-)

I'm now thinking about a hybrid solution: mark the most critical functions "notrace", especially those directly involved with MMU loading, and add a per-thread flag to catch the not-so-obvious cases.

Torsten

Torsten Duwe

author

2015-05-26 14:35 UTC #15 lore raw

Quoted 6 lines · Torsten Duwe

Quoted 5 lines · Michael Ellerman
Quoted 3 lines · Jiri Kosina
ftrace already handles recursion protection by itself (depending on the per-ftrace-ops FTRACE_OPS_FL_RECURSION_SAFE flag).

Quoted 7 lines

Quoted 1 line
OK, so I wonder why that's not working for us?
IIRC a data access fault happens just before that flag is looked at ;-)
I'm now thinking about a hybrid solution: mark the most critical functions "notrace", especially those directly involved with MMU loading, and add a per-thread flag to catch the not-so-obvious cases.

I realised the trace_recursion in the "current" task struct is not so far away, and it should not fault, right? So that part of the solution would look like this on top of my previous ftrace patch set.
It would impact performance further so I'd stick with the "notrace" on certain hot functions, like hash_page. What do you think?

Torsten

arch/powerpc/kernel/asm-offsets.c +1 -0

index 4717859..ae10752 100644
@@ -72,6 +72,7 @@ int main(void)
 	DEFINE(THREAD, offsetof(struct task_struct, thread));
 	DEFINE(MM, offsetof(struct task_struct, mm));
 	DEFINE(MMCONTEXTID, offsetof(struct mm_struct, context.id));
+	DEFINE(TASK_TRACEREC, offsetof(struct task_struct, trace_recursion));
 #ifdef CONFIG_PPC64
 	DEFINE(AUDITCONTEXT, offsetof(struct task_struct, audit_context));
 	DEFINE(SIGSEGV, SIGSEGV);

arch/powerpc/kernel/entry_64.S +13 -2

index a4132ef..7842092 100644
@@ -1202,7 +1202,13 @@ _GLOBAL(ftrace_caller)
 	SAVE_8GPRS(16,r1)
 	SAVE_8GPRS(24,r1)
 
-
+	ld	r3, PACACURRENT(r13)
+	ld	r4, TASK_TRACEREC(r3)
+	andi.	r5, r4, 0x0010 // ( 1 << TRACE_FTRACE_BIT )
+	ori	r4, r4, 0x0010
+	std	r4, TASK_TRACEREC(r3)
+	bne	3f		// ftrace in progress - avoid recursion!
+	
 	LOAD_REG_IMMEDIATE(r3,function_trace_op)
 	ld	r5,0(r3)

@@ -1224,9 +1230,14 @@ ftrace_call:
 	bl	ftrace_stub
 	nop
 
+	ld	r3, PACACURRENT(r13)
+	ld	r4, TASK_TRACEREC(r3)
+	andi	r4, r4, 0xffef // ~( 1 << TRACE_FTRACE_BIT )
+	std	r4, TASK_TRACEREC(r3)
+
 	ld	r3, _NIP(r1)
 	mtlr	r3
-
+3:
 	REST_8GPRS(0,r1)
 	REST_8GPRS(8,r1)
 	REST_8GPRS(16,r1)