2022-03-25 16:05:32

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 00/22] powerpc: ftrace optimisation and cleanup and more [v1]

This series provides optimisation and cleanup of ftrace on powerpc.

With this series ftrace activation is about 20% faster on an 8xx.

At the end of the series come additional cleanups around ppc-opcode,
that would likely conflict with this series if posted separately.

Christophe Leroy (22):
powerpc/ftrace: Refactor prepare_ftrace_return()
powerpc/ftrace: Remove redundant create_branch() calls
powerpc/code-patching: Inline is_offset_in_{cond}_branch_range()
powerpc/ftrace: Use is_offset_in_branch_range()
powerpc/code-patching: Inline create_branch()
powerpc/ftrace: Inline ftrace_modify_code()
powerpc/ftrace: Use patch_instruction() return directly
powerpc/ftrace: Make __ftrace_make_{nop/call}() common to PPC32 and
PPC64
powerpc/ftrace: Don't include ftrace.o for CONFIG_FTRACE_SYSCALLS
powerpc/ftrace: Use CONFIG_FUNCTION_TRACER instead of
CONFIG_DYNAMIC_FTRACE
powerpc/ftrace: Remove ftrace_plt_tramps[]
powerpc/ftrace: Use BRANCH_SET_LINK instead of value 1
powerpc/ftrace: Use PPC_RAW_xxx() macros instead of opencoding.
powerpc/ftrace: Use size macro instead of opencoding
powerpc/ftrace: Simplify expected_nop_sequence()
powerpc/ftrace: Minimise number of #ifdefs
powerpc/inst: Add __copy_inst_from_kernel_nofault()
powerpc/ftrace: Don't use copy_from_kernel_nofault() in
module_trampoline_target()
powerpc/inst: Remove PPC_INST_BRANCH
powerpc/modules: Use PPC_INST_BRANCH_MASK instead of opencoding
powerpc/inst: Remove PPC_INST_BL
powerpc/opcodes: Remove unused PPC_INST_XXX macros

arch/powerpc/include/asm/book3s/32/pgtable.h | 2 +-
arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +-
arch/powerpc/include/asm/code-patching.h | 53 ++-
arch/powerpc/include/asm/inst.h | 13 +-
arch/powerpc/include/asm/module.h | 6 +-
arch/powerpc/include/asm/nohash/pgtable.h | 2 +-
arch/powerpc/include/asm/ppc-opcode.h | 20 +-
arch/powerpc/include/asm/sections.h | 24 +-
arch/powerpc/kernel/module_32.c | 29 +-
arch/powerpc/kernel/module_64.c | 10 +-
arch/powerpc/kernel/trace/Makefile | 5 +-
arch/powerpc/kernel/trace/ftrace.c | 390 ++++++-------------
arch/powerpc/lib/code-patching.c | 47 ---
arch/powerpc/lib/feature-fixups.c | 2 +-
arch/powerpc/net/bpf_jit.h | 2 +-
15 files changed, 229 insertions(+), 378 deletions(-)

--
2.35.1


2022-03-25 17:59:37

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 11/22] powerpc/ftrace: Remove ftrace_plt_tramps[]

ftrace_plt_tramps table is never filled so it is useless.

Remove it.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/ftrace.c | 8 --------
1 file changed, 8 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 188f59f4ee4a..b6c5223e26b2 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -250,7 +250,6 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
int i;
ppc_inst_t op;
unsigned long ptr;
- static unsigned long ftrace_plt_tramps[NUM_FTRACE_TRAMPS];

/* Is this a known long jump tramp? */
for (i = 0; i < NUM_FTRACE_TRAMPS; i++)
@@ -259,13 +258,6 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
else if (ftrace_tramps[i] == tramp)
return 0;

- /* Is this a known plt tramp? */
- for (i = 0; i < NUM_FTRACE_TRAMPS; i++)
- if (!ftrace_plt_tramps[i])
- break;
- else if (ftrace_plt_tramps[i] == tramp)
- return -1;
-
/* New trampoline -- read where this goes */
if (copy_inst_from_kernel_nofault(&op, (void *)tramp)) {
pr_debug("Fetching opcode failed.\n");
--
2.35.1

2022-03-25 18:10:41

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 18/22] powerpc/ftrace: Don't use copy_from_kernel_nofault() in module_trampoline_target()

module_trampoline_target() is quite a hot path used when
activating/deactivating function tracer.

Avoid the heavy copy_from_kernel_nofault() by doing four calls
to copy_inst_from_kernel_nofault().

Use __copy_inst_from_kernel_nofault() for the 3 last calls. First call
is done to copy_from_kernel_nofault() to check address is within
kernel space. No risk to wrap out the top of kernel space because the
last page is never mapped so if address is in last page the first copy
will fails and the other ones will never be performed.

And also make it notrace just like all functions that call it.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/module_32.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index 2aa368ce21c9..1282cfd672f2 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -289,13 +289,19 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
}

#ifdef CONFIG_FUNCTION_TRACER
-int module_trampoline_target(struct module *mod, unsigned long addr,
- unsigned long *target)
+notrace int module_trampoline_target(struct module *mod, unsigned long addr,
+ unsigned long *target)
{
unsigned int jmp[4];

/* Find where the trampoline jumps to */
- if (copy_from_kernel_nofault(jmp, (void *)addr, sizeof(jmp)))
+ if (copy_inst_from_kernel_nofault(jmp, (void *)addr))
+ return -EFAULT;
+ if (__copy_inst_from_kernel_nofault(jmp + 1, (void *)addr + 4))
+ return -EFAULT;
+ if (__copy_inst_from_kernel_nofault(jmp + 2, (void *)addr + 8))
+ return -EFAULT;
+ if (__copy_inst_from_kernel_nofault(jmp + 3, (void *)addr + 12))
return -EFAULT;

/* verify that this is what we expect it to be */
--
2.35.1

2022-03-25 18:19:56

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 06/22] powerpc/ftrace: Inline ftrace_modify_code()

Inlining ftrace_modify_code(), it increases a bit the
size of ftrace code but brings 5% improvment on ftrace
activation.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/ftrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 41c45b9c7f39..98e82fa4980f 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -53,7 +53,7 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
return op;
}

-static int
+static inline int
ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
{
ppc_inst_t replaced;
--
2.35.1

2022-03-25 18:22:46

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 22/22] powerpc/opcodes: Remove unused PPC_INST_XXX macros

The following PPC_INST_XXX macros are not used anymore
outside ppc-opcode.h:
- PPC_INST_LD
- PPC_INST_STD
- PPC_INST_ADDIS
- PPC_INST_ADD
- PPC_INST_DIVD

Remove them.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/ppc-opcode.h | 13 ++++---------
1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index 810a28af9dce..4cdda507e816 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -285,11 +285,6 @@
#define PPC_INST_TRECHKPT 0x7c0007dd
#define PPC_INST_TRECLAIM 0x7c00075d
#define PPC_INST_TSR 0x7c0005dd
-#define PPC_INST_LD 0xe8000000
-#define PPC_INST_STD 0xf8000000
-#define PPC_INST_ADDIS 0x3c000000
-#define PPC_INST_ADD 0x7c000214
-#define PPC_INST_DIVD 0x7c0003d2
#define PPC_INST_BRANCH_COND 0x40800000

#define PPC_INST_OFFSET24_MASK 0x03fffffc
@@ -460,10 +455,10 @@
(0x100000c7 | ___PPC_RT(vrt) | ___PPC_RA(vra) | ___PPC_RB(vrb) | __PPC_RC21)
#define PPC_RAW_VCMPEQUB_RC(vrt, vra, vrb) \
(0x10000006 | ___PPC_RT(vrt) | ___PPC_RA(vra) | ___PPC_RB(vrb) | __PPC_RC21)
-#define PPC_RAW_LD(r, base, i) (PPC_INST_LD | ___PPC_RT(r) | ___PPC_RA(base) | IMM_DS(i))
+#define PPC_RAW_LD(r, base, i) (0xe8000000 | ___PPC_RT(r) | ___PPC_RA(base) | IMM_DS(i))
#define PPC_RAW_LWZ(r, base, i) (0x80000000 | ___PPC_RT(r) | ___PPC_RA(base) | IMM_L(i))
#define PPC_RAW_LWZX(t, a, b) (0x7c00002e | ___PPC_RT(t) | ___PPC_RA(a) | ___PPC_RB(b))
-#define PPC_RAW_STD(r, base, i) (PPC_INST_STD | ___PPC_RS(r) | ___PPC_RA(base) | IMM_DS(i))
+#define PPC_RAW_STD(r, base, i) (0xf8000000 | ___PPC_RS(r) | ___PPC_RA(base) | IMM_DS(i))
#define PPC_RAW_STDCX(s, a, b) (0x7c0001ad | ___PPC_RS(s) | ___PPC_RA(a) | ___PPC_RB(b))
#define PPC_RAW_LFSX(t, a, b) (0x7c00042e | ___PPC_RT(t) | ___PPC_RA(a) | ___PPC_RB(b))
#define PPC_RAW_STFSX(s, a, b) (0x7c00052e | ___PPC_RS(s) | ___PPC_RA(a) | ___PPC_RB(b))
@@ -474,8 +469,8 @@
#define PPC_RAW_ADDE(t, a, b) (0x7c000114 | ___PPC_RT(t) | ___PPC_RA(a) | ___PPC_RB(b))
#define PPC_RAW_ADDZE(t, a) (0x7c000194 | ___PPC_RT(t) | ___PPC_RA(a))
#define PPC_RAW_ADDME(t, a) (0x7c0001d4 | ___PPC_RT(t) | ___PPC_RA(a))
-#define PPC_RAW_ADD(t, a, b) (PPC_INST_ADD | ___PPC_RT(t) | ___PPC_RA(a) | ___PPC_RB(b))
-#define PPC_RAW_ADD_DOT(t, a, b) (PPC_INST_ADD | ___PPC_RT(t) | ___PPC_RA(a) | ___PPC_RB(b) | 0x1)
+#define PPC_RAW_ADD(t, a, b) (0x7c000214 | ___PPC_RT(t) | ___PPC_RA(a) | ___PPC_RB(b))
+#define PPC_RAW_ADD_DOT(t, a, b) (0x7c000214 | ___PPC_RT(t) | ___PPC_RA(a) | ___PPC_RB(b) | 0x1)
#define PPC_RAW_ADDC(t, a, b) (0x7c000014 | ___PPC_RT(t) | ___PPC_RA(a) | ___PPC_RB(b))
#define PPC_RAW_ADDC_DOT(t, a, b) (0x7c000014 | ___PPC_RT(t) | ___PPC_RA(a) | ___PPC_RB(b) | 0x1)
#define PPC_RAW_NOP() PPC_RAW_ORI(0, 0, 0)
--
2.35.1

2022-03-25 18:24:00

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 20/22] powerpc/modules: Use PPC_INST_BRANCH_MASK instead of opencoding

Use PPC_INST_BRANCH_MASK instead of opencoding.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/module_32.c | 13 ++++++-------
arch/powerpc/kernel/module_64.c | 4 ++--
2 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index 1282cfd672f2..344941855e9e 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -256,9 +256,8 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
value, (uint32_t)location);
pr_debug("Location before: %08X.\n",
*(uint32_t *)location);
- value = (*(uint32_t *)location & ~0x03fffffc)
- | ((value - (uint32_t)location)
- & 0x03fffffc);
+ value = (*(uint32_t *)location & ~PPC_INST_OFFSET24_MASK) |
+ ((value - (uint32_t)location) & PPC_INST_OFFSET24_MASK);

if (patch_instruction(location, ppc_inst(value)))
return -EFAULT;
@@ -266,10 +265,10 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
pr_debug("Location after: %08X.\n",
*(uint32_t *)location);
pr_debug("ie. jump to %08X+%08X = %08X\n",
- *(uint32_t *)location & 0x03fffffc,
- (uint32_t)location,
- (*(uint32_t *)location & 0x03fffffc)
- + (uint32_t)location);
+ *(uint32_t *)location & PPC_INST_OFFSET24_MASK,
+ (uint32_t)location,
+ (*(uint32_t *)location & PPC_INST_OFFSET24_MASK) +
+ (uint32_t)location);
break;

case R_PPC_REL32:
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index b13a72665eee..8f70a04aac6c 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -653,8 +653,8 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
}

/* Only replace bits 2 through 26 */
- value = (*(uint32_t *)location & ~0x03fffffc)
- | (value & 0x03fffffc);
+ value = (*(uint32_t *)location & ~PPC_INST_OFFSET24_MASK) |
+ (value & PPC_INST_OFFSET24_MASK);

if (patch_instruction((u32 *)location, ppc_inst(value)))
return -EFAULT;
--
2.35.1

2022-03-25 18:42:00

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 10/22] powerpc/ftrace: Use CONFIG_FUNCTION_TRACER instead of CONFIG_DYNAMIC_FTRACE

Since commit 0c0c52306f47 ("powerpc: Only support DYNAMIC_FTRACE not
static"), CONFIG_DYNAMIC_FTRACE is always selected when
CONFIG_FUNCTION_TRACER is selected.

To avoid confusion and have the reader wonder what's happen when
CONFIG_FUNCTION_TRACER is selected and CONFIG_DYNAMIC_FTRACE is not,
use CONFIG_FUNCTION_TRACER in ifdefs instead of CONFIG_DYNAMIC_FTRACE.

As CONFIG_FUNCTION_GRAPH_TRACER depends on CONFIG_FUNCTION_TRACER,
ftrace.o doesn't need to appear for both symbols in Makefile.

Then as ftrace.o is built only when CONFIG_FUNCTION_TRACER is selected
ifdef CONFIG_FUNCTION_TRACER is not needed in ftrace.c

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/book3s/32/pgtable.h | 2 +-
arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +-
arch/powerpc/include/asm/module.h | 4 ++--
arch/powerpc/include/asm/nohash/pgtable.h | 2 +-
arch/powerpc/kernel/module_32.c | 4 ++--
arch/powerpc/kernel/module_64.c | 6 +++---
arch/powerpc/kernel/trace/Makefile | 4 +---
arch/powerpc/kernel/trace/ftrace.c | 4 ----
8 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
index 772e00dc4ef1..992aed626eb4 100644
--- a/arch/powerpc/include/asm/book3s/32/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
@@ -124,7 +124,7 @@ static inline bool pte_user(pte_t pte)
* on platforms where such control is possible.
*/
#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
- defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+ defined(CONFIG_KPROBES) || defined(CONFIG_FUNCTION_TRACER)
#define PAGE_KERNEL_TEXT PAGE_KERNEL_X
#else
#define PAGE_KERNEL_TEXT PAGE_KERNEL_ROX
diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
index 875730d5af40..cf01b609572f 100644
--- a/arch/powerpc/include/asm/book3s/64/pgtable.h
+++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
@@ -169,7 +169,7 @@
* on platforms where such control is possible.
*/
#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) || \
- defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+ defined(CONFIG_KPROBES) || defined(CONFIG_FUNCTION_TRACER)
#define PAGE_KERNEL_TEXT PAGE_KERNEL_X
#else
#define PAGE_KERNEL_TEXT PAGE_KERNEL_ROX
diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
index 857d9ff24295..e6f5963fd96e 100644
--- a/arch/powerpc/include/asm/module.h
+++ b/arch/powerpc/include/asm/module.h
@@ -39,7 +39,7 @@ struct mod_arch_specific {
unsigned int init_plt_section;
#endif /* powerpc64 */

-#ifdef CONFIG_DYNAMIC_FTRACE
+#ifdef CONFIG_FUNCTION_TRACER
unsigned long tramp;
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
unsigned long tramp_regs;
@@ -68,7 +68,7 @@ struct mod_arch_specific {
# endif /* MODULE */
#endif

-#ifdef CONFIG_DYNAMIC_FTRACE
+#ifdef CONFIG_FUNCTION_TRACER
# ifdef MODULE
asm(".section .ftrace.tramp,\"ax\",@nobits; .align 3; .previous");
# endif /* MODULE */
diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
index ac75f4ab0dba..2e8cf217a191 100644
--- a/arch/powerpc/include/asm/nohash/pgtable.h
+++ b/arch/powerpc/include/asm/nohash/pgtable.h
@@ -23,7 +23,7 @@
* on platforms where such control is possible.
*/
#if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
- defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
+ defined(CONFIG_KPROBES) || defined(CONFIG_FUNCTION_TRACER)
#define PAGE_KERNEL_TEXT PAGE_KERNEL_X
#else
#define PAGE_KERNEL_TEXT PAGE_KERNEL_ROX
diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
index a0432ef46967..2aa368ce21c9 100644
--- a/arch/powerpc/kernel/module_32.c
+++ b/arch/powerpc/kernel/module_32.c
@@ -39,7 +39,7 @@ static unsigned int count_relocs(const Elf32_Rela *rela, unsigned int num)
r_addend = rela[i].r_addend;
}

-#ifdef CONFIG_DYNAMIC_FTRACE
+#ifdef CONFIG_FUNCTION_TRACER
_count_relocs++; /* add one for ftrace_caller */
#endif
return _count_relocs;
@@ -288,7 +288,7 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
return 0;
}

-#ifdef CONFIG_DYNAMIC_FTRACE
+#ifdef CONFIG_FUNCTION_TRACER
int module_trampoline_target(struct module *mod, unsigned long addr,
unsigned long *target)
{
diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
index 794720530442..b13a72665eee 100644
--- a/arch/powerpc/kernel/module_64.c
+++ b/arch/powerpc/kernel/module_64.c
@@ -207,7 +207,7 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
}
}

-#ifdef CONFIG_DYNAMIC_FTRACE
+#ifdef CONFIG_FUNCTION_TRACER
/* make the trampoline to the ftrace_caller */
relocs++;
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
@@ -372,7 +372,7 @@ static bool is_mprofile_ftrace_call(const char *name)
{
if (!strcmp("_mcount", name))
return true;
-#ifdef CONFIG_DYNAMIC_FTRACE
+#ifdef CONFIG_FUNCTION_TRACER
if (!strcmp("ftrace_caller", name))
return true;
#ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
@@ -740,7 +740,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
return 0;
}

-#ifdef CONFIG_DYNAMIC_FTRACE
+#ifdef CONFIG_FUNCTION_TRACER
int module_trampoline_target(struct module *mod, unsigned long addr,
unsigned long *target)
{
diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index fc32ec30b297..af8527538fe4 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile
@@ -14,9 +14,7 @@ obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_mprofile.o
else
obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_64_pg.o
endif
-obj-$(CONFIG_FUNCTION_TRACER) += ftrace_low.o
-obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
-obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
+obj-$(CONFIG_FUNCTION_TRACER) += ftrace_low.o ftrace.o
obj-$(CONFIG_TRACING) += trace_clock.o

obj-$(CONFIG_PPC64) += $(obj64-y)
diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 2c7e42e439bb..188f59f4ee4a 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -28,9 +28,6 @@
#include <asm/syscall.h>
#include <asm/inst.h>

-
-#ifdef CONFIG_DYNAMIC_FTRACE
-
/*
* We generally only have a single long_branch tramp and at most 2 or 3 plt
* tramps generated. But, we don't use the plt tramps currently. We also allot
@@ -783,7 +780,6 @@ int __init ftrace_dyn_arch_init(void)
return 0;
}
#endif
-#endif /* CONFIG_DYNAMIC_FTRACE */

#ifdef CONFIG_FUNCTION_GRAPH_TRACER

--
2.35.1

2022-03-25 18:42:36

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 05/22] powerpc/code-patching: Inline create_branch()

create_branch() is a good candidate for inlining because:
- Flags can be folded in.
- Range tests are likely to be already done.

Hence reducing the create_branch() to only a set of instructions.

So inline it.

It improves ftrace activation by 10%.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/code-patching.h | 22 ++++++++++++++++++++--
arch/powerpc/lib/code-patching.c | 20 --------------------
2 files changed, 20 insertions(+), 22 deletions(-)

diff --git a/arch/powerpc/include/asm/code-patching.h b/arch/powerpc/include/asm/code-patching.h
index e7c5df50cb4e..4260e89f62b1 100644
--- a/arch/powerpc/include/asm/code-patching.h
+++ b/arch/powerpc/include/asm/code-patching.h
@@ -49,8 +49,26 @@ static inline bool is_offset_in_cond_branch_range(long offset)
return offset >= -0x8000 && offset <= 0x7fff && !(offset & 0x3);
}

-int create_branch(ppc_inst_t *instr, const u32 *addr,
- unsigned long target, int flags);
+static inline int create_branch(ppc_inst_t *instr, const u32 *addr,
+ unsigned long target, int flags)
+{
+ long offset;
+
+ *instr = ppc_inst(0);
+ offset = target;
+ if (! (flags & BRANCH_ABSOLUTE))
+ offset = offset - (unsigned long)addr;
+
+ /* Check we can represent the target in the instruction format */
+ if (!is_offset_in_branch_range(offset))
+ return 1;
+
+ /* Mask out the flags and target, so they don't step on each other. */
+ *instr = ppc_inst(0x48000000 | (flags & 0x3) | (offset & 0x03FFFFFC));
+
+ return 0;
+}
+
int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
unsigned long target, int flags);
int patch_branch(u32 *addr, unsigned long target, int flags);
diff --git a/arch/powerpc/lib/code-patching.c b/arch/powerpc/lib/code-patching.c
index 58262c7e447c..7adbdb05fee7 100644
--- a/arch/powerpc/lib/code-patching.c
+++ b/arch/powerpc/lib/code-patching.c
@@ -230,26 +230,6 @@ bool is_conditional_branch(ppc_inst_t instr)
}
NOKPROBE_SYMBOL(is_conditional_branch);

-int create_branch(ppc_inst_t *instr, const u32 *addr,
- unsigned long target, int flags)
-{
- long offset;
-
- *instr = ppc_inst(0);
- offset = target;
- if (! (flags & BRANCH_ABSOLUTE))
- offset = offset - (unsigned long)addr;
-
- /* Check we can represent the target in the instruction format */
- if (!is_offset_in_branch_range(offset))
- return 1;
-
- /* Mask out the flags and target, so they don't step on each other. */
- *instr = ppc_inst(0x48000000 | (flags & 0x3) | (offset & 0x03FFFFFC));
-
- return 0;
-}
-
int create_cond_branch(ppc_inst_t *instr, const u32 *addr,
unsigned long target, int flags)
{
--
2.35.1

2022-03-25 19:01:01

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 01/22] powerpc/ftrace: Refactor prepare_ftrace_return()

When we have CONFIG_DYNAMIC_FTRACE_WITH_ARGS,
prepare_ftrace_return() is called by ftrace_graph_func()
otherwise prepare_ftrace_return() is called from assembly.

Refactor prepare_ftrace_return() into a static
__prepare_ftrace_return() that will be called by both
prepare_ftrace_return() and ftrace_graph_func().

It will allow GCC to fold __prepare_ftrace_return() inside
ftrace_graph_func().

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/ftrace.c | 12 +++++++++---
1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 4ee04aacf9f1..7a266fd469b7 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -939,8 +939,8 @@ int ftrace_disable_ftrace_graph_caller(void)
* Hook the return address and push it in the stack of return addrs
* in current thread info. Return the address we want to divert to.
*/
-unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
- unsigned long sp)
+static unsigned long
+__prepare_ftrace_return(unsigned long parent, unsigned long ip, unsigned long sp)
{
unsigned long return_hooker;
int bit;
@@ -969,7 +969,13 @@ unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
void ftrace_graph_func(unsigned long ip, unsigned long parent_ip,
struct ftrace_ops *op, struct ftrace_regs *fregs)
{
- fregs->regs.link = prepare_ftrace_return(parent_ip, ip, fregs->regs.gpr[1]);
+ fregs->regs.link = __prepare_ftrace_return(parent_ip, ip, fregs->regs.gpr[1]);
+}
+#else
+unsigned long prepare_ftrace_return(unsigned long parent, unsigned long ip,
+ unsigned long sp)
+{
+ return __prepare_ftrace_return(parent, ip, sp);
}
#endif
#endif /* CONFIG_FUNCTION_GRAPH_TRACER */
--
2.35.1

2022-03-25 19:05:56

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 19/22] powerpc/inst: Remove PPC_INST_BRANCH

Convert last users of PPC_INST_BRANCH to PPC_RAW_BRANCH()

And remove PPC_INST_BRANCH.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/include/asm/ppc-opcode.h | 3 +--
arch/powerpc/lib/feature-fixups.c | 2 +-
2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/arch/powerpc/include/asm/ppc-opcode.h b/arch/powerpc/include/asm/ppc-opcode.h
index 281754aca0a3..ada8fe17b199 100644
--- a/arch/powerpc/include/asm/ppc-opcode.h
+++ b/arch/powerpc/include/asm/ppc-opcode.h
@@ -290,7 +290,6 @@
#define PPC_INST_ADDIS 0x3c000000
#define PPC_INST_ADD 0x7c000214
#define PPC_INST_DIVD 0x7c0003d2
-#define PPC_INST_BRANCH 0x48000000
#define PPC_INST_BL 0x48000001
#define PPC_INST_BRANCH_COND 0x40800000

@@ -573,7 +572,7 @@
#define PPC_RAW_MTSPR(spr, d) (0x7c0003a6 | ___PPC_RS(d) | __PPC_SPR(spr))
#define PPC_RAW_EIEIO() (0x7c0006ac)

-#define PPC_RAW_BRANCH(addr) (PPC_INST_BRANCH | ((addr) & 0x03fffffc))
+#define PPC_RAW_BRANCH(offset) (0x48000000 | ((offset) & PPC_INST_OFFSET24_MASK))
#define PPC_RAW_BL(offset) (0x48000001 | ((offset) & PPC_INST_OFFSET24_MASK))

/* Deal with instructions that older assemblers aren't aware of */
diff --git a/arch/powerpc/lib/feature-fixups.c b/arch/powerpc/lib/feature-fixups.c
index 343a78826035..993d3f31832a 100644
--- a/arch/powerpc/lib/feature-fixups.c
+++ b/arch/powerpc/lib/feature-fixups.c
@@ -451,7 +451,7 @@ static int __do_rfi_flush_fixups(void *data)

if (types & L1D_FLUSH_FALLBACK)
/* b .+16 to fallback flush */
- instrs[0] = PPC_INST_BRANCH | 16;
+ instrs[0] = PPC_RAW_BRANCH(16);

i = 0;
if (types & L1D_FLUSH_ORI) {
--
2.35.1

2022-03-25 19:13:31

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 07/22] powerpc/ftrace: Use patch_instruction() return directly

Instead of returning -EPERM when patch_instruction() fails,
just return what patch_instruction returns.

That simplifies ftrace_modify_code():

0: 94 21 ff c0 stwu r1,-64(r1)
4: 93 e1 00 3c stw r31,60(r1)
8: 7c 7f 1b 79 mr. r31,r3
c: 40 80 00 30 bge 3c <ftrace_modify_code+0x3c>
10: 93 c1 00 38 stw r30,56(r1)
14: 7c 9e 23 78 mr r30,r4
18: 7c a4 2b 78 mr r4,r5
1c: 80 bf 00 00 lwz r5,0(r31)
20: 7c 1e 28 40 cmplw r30,r5
24: 40 82 00 34 bne 58 <ftrace_modify_code+0x58>
28: 83 c1 00 38 lwz r30,56(r1)
2c: 7f e3 fb 78 mr r3,r31
30: 83 e1 00 3c lwz r31,60(r1)
34: 38 21 00 40 addi r1,r1,64
38: 48 00 00 00 b 38 <ftrace_modify_code+0x38>
38: R_PPC_REL24 patch_instruction

Before:

0: 94 21 ff c0 stwu r1,-64(r1)
4: 93 e1 00 3c stw r31,60(r1)
8: 7c 7f 1b 79 mr. r31,r3
c: 40 80 00 4c bge 58 <ftrace_modify_code+0x58>
10: 93 c1 00 38 stw r30,56(r1)
14: 7c 9e 23 78 mr r30,r4
18: 7c a4 2b 78 mr r4,r5
1c: 80 bf 00 00 lwz r5,0(r31)
20: 7c 08 02 a6 mflr r0
24: 90 01 00 44 stw r0,68(r1)
28: 7c 1e 28 40 cmplw r30,r5
2c: 40 82 00 48 bne 74 <ftrace_modify_code+0x74>
30: 7f e3 fb 78 mr r3,r31
34: 48 00 00 01 bl 34 <ftrace_modify_code+0x34>
34: R_PPC_REL24 patch_instruction
38: 80 01 00 44 lwz r0,68(r1)
3c: 20 63 00 00 subfic r3,r3,0
40: 83 c1 00 38 lwz r30,56(r1)
44: 7c 63 19 10 subfe r3,r3,r3
48: 7c 08 03 a6 mtlr r0
4c: 83 e1 00 3c lwz r31,60(r1)
50: 38 21 00 40 addi r1,r1,64
54: 4e 80 00 20 blr

It improves ftrace activation/deactivation duration by about 3%.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/ftrace.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 98e82fa4980f..1b05d33f96c6 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -78,10 +78,7 @@ ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
}

/* replace the text with the new text */
- if (patch_instruction((u32 *)ip, new))
- return -EPERM;
-
- return 0;
+ return patch_instruction((u32 *)ip, new);
}

/*
--
2.35.1

2022-03-25 19:18:08

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 09/22] powerpc/ftrace: Don't include ftrace.o for CONFIG_FTRACE_SYSCALLS

Since commit 7bea7ac0ca01 ("powerpc/syscalls: Fix syscall tracing")
ftrace.o is not needed anymore for CONFIG_FTRACE_SYSCALLS.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/Makefile | 1 -
1 file changed, 1 deletion(-)

diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
index 542aa7a8b2b4..fc32ec30b297 100644
--- a/arch/powerpc/kernel/trace/Makefile
+++ b/arch/powerpc/kernel/trace/Makefile
@@ -17,7 +17,6 @@ endif
obj-$(CONFIG_FUNCTION_TRACER) += ftrace_low.o
obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
-obj-$(CONFIG_FTRACE_SYSCALLS) += ftrace.o
obj-$(CONFIG_TRACING) += trace_clock.o

obj-$(CONFIG_PPC64) += $(obj64-y)
--
2.35.1

2022-03-25 19:30:26

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 02/22] powerpc/ftrace: Remove redundant create_branch() calls

Since commit d5937db114e4 ("powerpc/code-patching: Fix patch_branch()
return on out-of-range failure") patch_branch() fails with -ERANGE
when trying to branch out of range.

No need to perform the test twice. Remove redundant create_branch()
calls.

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/ftrace.c | 20 --------------------
1 file changed, 20 deletions(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index 7a266fd469b7..3ce3697e8a7c 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -301,7 +301,6 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
int i;
ppc_inst_t op;
unsigned long ptr;
- ppc_inst_t instr;
static unsigned long ftrace_plt_tramps[NUM_FTRACE_TRAMPS];

/* Is this a known long jump tramp? */
@@ -344,12 +343,6 @@ static int setup_mcount_compiler_tramp(unsigned long tramp)
#else
ptr = ppc_global_function_entry((void *)ftrace_caller);
#endif
- if (create_branch(&instr, (void *)tramp, ptr, 0)) {
- pr_debug("%ps is not reachable from existing mcount tramp\n",
- (void *)ptr);
- return -1;
- }
-
if (patch_branch((u32 *)tramp, ptr, 0)) {
pr_debug("REL24 out of range!\n");
return -1;
@@ -490,7 +483,6 @@ static int
__ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
{
ppc_inst_t op[2];
- ppc_inst_t instr;
void *ip = (void *)rec->ip;
unsigned long entry, ptr, tramp;
struct module *mod = rec->arch.mod;
@@ -539,12 +531,6 @@ __ftrace_make_call(struct dyn_ftrace *rec, unsigned long addr)
return -EINVAL;
}

- /* Ensure branch is within 24 bits */
- if (create_branch(&instr, ip, tramp, BRANCH_SET_LINK)) {
- pr_err("Branch out of range\n");
- return -EINVAL;
- }
-
if (patch_branch(ip, tramp, BRANCH_SET_LINK)) {
pr_err("REL24 out of range!\n");
return -EINVAL;
@@ -770,12 +756,6 @@ __ftrace_modify_call(struct dyn_ftrace *rec, unsigned long old_addr,
return -EINVAL;
}

- /* Ensure branch is within 24 bits */
- if (create_branch(&op, (u32 *)ip, tramp, BRANCH_SET_LINK)) {
- pr_err("Branch out of range\n");
- return -EINVAL;
- }
-
if (patch_branch((u32 *)ip, tramp, BRANCH_SET_LINK)) {
pr_err("REL24 out of range!\n");
return -EINVAL;
--
2.35.1

2022-03-25 19:37:38

by Christophe Leroy

[permalink] [raw]
Subject: [PATCH v1 12/22] powerpc/ftrace: Use BRANCH_SET_LINK instead of value 1

To make it explicit, use BRANCH_SET_LINK instead of value 1
when calling create_branch().

Signed-off-by: Christophe Leroy <[email protected]>
---
arch/powerpc/kernel/trace/ftrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
index b6c5223e26b2..fdc0412c1d8a 100644
--- a/arch/powerpc/kernel/trace/ftrace.c
+++ b/arch/powerpc/kernel/trace/ftrace.c
@@ -45,7 +45,7 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
addr = ppc_function_entry((void *)addr);

/* if (link) set op to 'bl' else 'b' */
- create_branch(&op, (u32 *)ip, addr, link ? 1 : 0);
+ create_branch(&op, (u32 *)ip, addr, link ? BRANCH_SET_LINK : 0);

return op;
}
--
2.35.1

2022-04-18 09:59:20

by Naveen N. Rao

[permalink] [raw]
Subject: Re: [PATCH v1 10/22] powerpc/ftrace: Use CONFIG_FUNCTION_TRACER instead of CONFIG_DYNAMIC_FTRACE

Christophe Leroy wrote:
> Since commit 0c0c52306f47 ("powerpc: Only support DYNAMIC_FTRACE not
> static"), CONFIG_DYNAMIC_FTRACE is always selected when
> CONFIG_FUNCTION_TRACER is selected.
>
> To avoid confusion and have the reader wonder what's happen when
> CONFIG_FUNCTION_TRACER is selected and CONFIG_DYNAMIC_FTRACE is not,
> use CONFIG_FUNCTION_TRACER in ifdefs instead of CONFIG_DYNAMIC_FTRACE.
>
> As CONFIG_FUNCTION_GRAPH_TRACER depends on CONFIG_FUNCTION_TRACER,
> ftrace.o doesn't need to appear for both symbols in Makefile.
>
> Then as ftrace.o is built only when CONFIG_FUNCTION_TRACER is selected

and since it implies CONFIG_DYNAMIC_FTRACE, CONFIG_DYNAMIC_FTRACE is not
needed in ftrace.c

> ifdef CONFIG_FUNCTION_TRACER is not needed in ftrace.c
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/include/asm/book3s/32/pgtable.h | 2 +-
> arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +-
> arch/powerpc/include/asm/module.h | 4 ++--
> arch/powerpc/include/asm/nohash/pgtable.h | 2 +-
> arch/powerpc/kernel/module_32.c | 4 ++--
> arch/powerpc/kernel/module_64.c | 6 +++---
> arch/powerpc/kernel/trace/Makefile | 4 +---
> arch/powerpc/kernel/trace/ftrace.c | 4 ----
> 8 files changed, 11 insertions(+), 17 deletions(-)
>
> diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h b/arch/powerpc/include/asm/book3s/32/pgtable.h
> index 772e00dc4ef1..992aed626eb4 100644
> --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
> @@ -124,7 +124,7 @@ static inline bool pte_user(pte_t pte)
> * on platforms where such control is possible.
> */
> #if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
> - defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
> + defined(CONFIG_KPROBES) || defined(CONFIG_FUNCTION_TRACER)
> #define PAGE_KERNEL_TEXT PAGE_KERNEL_X
> #else
> #define PAGE_KERNEL_TEXT PAGE_KERNEL_ROX
> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h b/arch/powerpc/include/asm/book3s/64/pgtable.h
> index 875730d5af40..cf01b609572f 100644
> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
> @@ -169,7 +169,7 @@
> * on platforms where such control is possible.
> */
> #if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) || \
> - defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
> + defined(CONFIG_KPROBES) || defined(CONFIG_FUNCTION_TRACER)
> #define PAGE_KERNEL_TEXT PAGE_KERNEL_X
> #else
> #define PAGE_KERNEL_TEXT PAGE_KERNEL_ROX
> diff --git a/arch/powerpc/include/asm/module.h b/arch/powerpc/include/asm/module.h
> index 857d9ff24295..e6f5963fd96e 100644
> --- a/arch/powerpc/include/asm/module.h
> +++ b/arch/powerpc/include/asm/module.h
> @@ -39,7 +39,7 @@ struct mod_arch_specific {
> unsigned int init_plt_section;
> #endif /* powerpc64 */
>
> -#ifdef CONFIG_DYNAMIC_FTRACE
> +#ifdef CONFIG_FUNCTION_TRACER
> unsigned long tramp;
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> unsigned long tramp_regs;
> @@ -68,7 +68,7 @@ struct mod_arch_specific {
> # endif /* MODULE */
> #endif
>
> -#ifdef CONFIG_DYNAMIC_FTRACE
> +#ifdef CONFIG_FUNCTION_TRACER
> # ifdef MODULE
> asm(".section .ftrace.tramp,\"ax\",@nobits; .align 3; .previous");
> # endif /* MODULE */
> diff --git a/arch/powerpc/include/asm/nohash/pgtable.h b/arch/powerpc/include/asm/nohash/pgtable.h
> index ac75f4ab0dba..2e8cf217a191 100644
> --- a/arch/powerpc/include/asm/nohash/pgtable.h
> +++ b/arch/powerpc/include/asm/nohash/pgtable.h
> @@ -23,7 +23,7 @@
> * on platforms where such control is possible.
> */
> #if defined(CONFIG_KGDB) || defined(CONFIG_XMON) || defined(CONFIG_BDI_SWITCH) ||\
> - defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
> + defined(CONFIG_KPROBES) || defined(CONFIG_FUNCTION_TRACER)
> #define PAGE_KERNEL_TEXT PAGE_KERNEL_X
> #else
> #define PAGE_KERNEL_TEXT PAGE_KERNEL_ROX
> diff --git a/arch/powerpc/kernel/module_32.c b/arch/powerpc/kernel/module_32.c
> index a0432ef46967..2aa368ce21c9 100644
> --- a/arch/powerpc/kernel/module_32.c
> +++ b/arch/powerpc/kernel/module_32.c
> @@ -39,7 +39,7 @@ static unsigned int count_relocs(const Elf32_Rela *rela, unsigned int num)
> r_addend = rela[i].r_addend;
> }
>
> -#ifdef CONFIG_DYNAMIC_FTRACE
> +#ifdef CONFIG_FUNCTION_TRACER
> _count_relocs++; /* add one for ftrace_caller */
> #endif
> return _count_relocs;
> @@ -288,7 +288,7 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
> return 0;
> }
>
> -#ifdef CONFIG_DYNAMIC_FTRACE
> +#ifdef CONFIG_FUNCTION_TRACER
> int module_trampoline_target(struct module *mod, unsigned long addr,
> unsigned long *target)
> {
> diff --git a/arch/powerpc/kernel/module_64.c b/arch/powerpc/kernel/module_64.c
> index 794720530442..b13a72665eee 100644
> --- a/arch/powerpc/kernel/module_64.c
> +++ b/arch/powerpc/kernel/module_64.c
> @@ -207,7 +207,7 @@ static unsigned long get_stubs_size(const Elf64_Ehdr *hdr,
> }
> }
>
> -#ifdef CONFIG_DYNAMIC_FTRACE
> +#ifdef CONFIG_FUNCTION_TRACER
> /* make the trampoline to the ftrace_caller */
> relocs++;
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> @@ -372,7 +372,7 @@ static bool is_mprofile_ftrace_call(const char *name)
> {
> if (!strcmp("_mcount", name))
> return true;
> -#ifdef CONFIG_DYNAMIC_FTRACE
> +#ifdef CONFIG_FUNCTION_TRACER
> if (!strcmp("ftrace_caller", name))
> return true;
> #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
> @@ -740,7 +740,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
> return 0;
> }
>
> -#ifdef CONFIG_DYNAMIC_FTRACE
> +#ifdef CONFIG_FUNCTION_TRACER
> int module_trampoline_target(struct module *mod, unsigned long addr,
> unsigned long *target)
> {

The below two changes to trace/Makefile and trace/ftrace.c make
sense, but I'm not sure the above changes are necessary.

For generic code (outside arch/powerpc/kernel/trace), I think it is good
to retain the actual dependency which is DYNAMIC_FTRACE. In my view,
gating some of the above code on FUNCTION_TRACER is also confusing. The
primary implication of always selecting DYNAMIC_FTRACE is within the
powerpc trace code, and it is good to keep it that way.

> diff --git a/arch/powerpc/kernel/trace/Makefile b/arch/powerpc/kernel/trace/Makefile
> index fc32ec30b297..af8527538fe4 100644
> --- a/arch/powerpc/kernel/trace/Makefile
> +++ b/arch/powerpc/kernel/trace/Makefile
> @@ -14,9 +14,7 @@ obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_mprofile.o
> else
> obj64-$(CONFIG_FUNCTION_TRACER) += ftrace_64_pg.o
> endif
> -obj-$(CONFIG_FUNCTION_TRACER) += ftrace_low.o
> -obj-$(CONFIG_DYNAMIC_FTRACE) += ftrace.o
> -obj-$(CONFIG_FUNCTION_GRAPH_TRACER) += ftrace.o
> +obj-$(CONFIG_FUNCTION_TRACER) += ftrace_low.o ftrace.o
> obj-$(CONFIG_TRACING) += trace_clock.o
>
> obj-$(CONFIG_PPC64) += $(obj64-y)
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index 2c7e42e439bb..188f59f4ee4a 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -28,9 +28,6 @@
> #include <asm/syscall.h>
> #include <asm/inst.h>
>
> -
> -#ifdef CONFIG_DYNAMIC_FTRACE
> -
> /*
> * We generally only have a single long_branch tramp and at most 2 or 3 plt
> * tramps generated. But, we don't use the plt tramps currently. We also allot
> @@ -783,7 +780,6 @@ int __init ftrace_dyn_arch_init(void)
> return 0;
> }
> #endif
> -#endif /* CONFIG_DYNAMIC_FTRACE */
>
> #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>
> --
> 2.35.1
>

- Naveen

2022-04-18 12:37:31

by Naveen N. Rao

[permalink] [raw]
Subject: Re: [PATCH v1 06/22] powerpc/ftrace: Inline ftrace_modify_code()

Christophe Leroy wrote:
> Inlining ftrace_modify_code(), it increases a bit the
> size of ftrace code but brings 5% improvment on ftrace
> activation.
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/kernel/trace/ftrace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index 41c45b9c7f39..98e82fa4980f 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -53,7 +53,7 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
> return op;
> }
>
> -static int
> +static inline int
> ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
> {
> ppc_inst_t replaced;

I thought gcc was free to inline functions without the need for
'inline'. Don't you see this being inlined otherwise?

On the flip side, don't we need __always_inline if we want to force
inlining?


- Naveen

2022-04-18 17:54:50

by Naveen N. Rao

[permalink] [raw]
Subject: Re: [PATCH v1 07/22] powerpc/ftrace: Use patch_instruction() return directly

Christophe Leroy wrote:
> Instead of returning -EPERM when patch_instruction() fails,
> just return what patch_instruction returns.
>
> That simplifies ftrace_modify_code():
>
> 0: 94 21 ff c0 stwu r1,-64(r1)
> 4: 93 e1 00 3c stw r31,60(r1)
> 8: 7c 7f 1b 79 mr. r31,r3
> c: 40 80 00 30 bge 3c <ftrace_modify_code+0x3c>
> 10: 93 c1 00 38 stw r30,56(r1)
> 14: 7c 9e 23 78 mr r30,r4
> 18: 7c a4 2b 78 mr r4,r5
> 1c: 80 bf 00 00 lwz r5,0(r31)
> 20: 7c 1e 28 40 cmplw r30,r5
> 24: 40 82 00 34 bne 58 <ftrace_modify_code+0x58>
> 28: 83 c1 00 38 lwz r30,56(r1)
> 2c: 7f e3 fb 78 mr r3,r31
> 30: 83 e1 00 3c lwz r31,60(r1)
> 34: 38 21 00 40 addi r1,r1,64
> 38: 48 00 00 00 b 38 <ftrace_modify_code+0x38>
> 38: R_PPC_REL24 patch_instruction
>
> Before:
>
> 0: 94 21 ff c0 stwu r1,-64(r1)
> 4: 93 e1 00 3c stw r31,60(r1)
> 8: 7c 7f 1b 79 mr. r31,r3
> c: 40 80 00 4c bge 58 <ftrace_modify_code+0x58>
> 10: 93 c1 00 38 stw r30,56(r1)
> 14: 7c 9e 23 78 mr r30,r4
> 18: 7c a4 2b 78 mr r4,r5
> 1c: 80 bf 00 00 lwz r5,0(r31)
> 20: 7c 08 02 a6 mflr r0
> 24: 90 01 00 44 stw r0,68(r1)
> 28: 7c 1e 28 40 cmplw r30,r5
> 2c: 40 82 00 48 bne 74 <ftrace_modify_code+0x74>
> 30: 7f e3 fb 78 mr r3,r31
> 34: 48 00 00 01 bl 34 <ftrace_modify_code+0x34>
> 34: R_PPC_REL24 patch_instruction
> 38: 80 01 00 44 lwz r0,68(r1)
> 3c: 20 63 00 00 subfic r3,r3,0
> 40: 83 c1 00 38 lwz r30,56(r1)
> 44: 7c 63 19 10 subfe r3,r3,r3
> 48: 7c 08 03 a6 mtlr r0
> 4c: 83 e1 00 3c lwz r31,60(r1)
> 50: 38 21 00 40 addi r1,r1,64
> 54: 4e 80 00 20 blr
>
> It improves ftrace activation/deactivation duration by about 3%.
>
> Signed-off-by: Christophe Leroy <[email protected]>
> ---
> arch/powerpc/kernel/trace/ftrace.c | 5 +----
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
> index 98e82fa4980f..1b05d33f96c6 100644
> --- a/arch/powerpc/kernel/trace/ftrace.c
> +++ b/arch/powerpc/kernel/trace/ftrace.c
> @@ -78,10 +78,7 @@ ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
> }
>
> /* replace the text with the new text */
> - if (patch_instruction((u32 *)ip, new))
> - return -EPERM;
> -
> - return 0;
> + return patch_instruction((u32 *)ip, new);

I think the reason we were returning -EPERM is so that ftrace_bug() can
throw the right error message. That will change due to this patch,
though I'm not sure how much it matters. -EFAULT and -EPERM seem to
print almost the same error message.

- Naveen

2022-04-22 10:55:41

by Michael Ellerman

[permalink] [raw]
Subject: Re: [PATCH v1 06/22] powerpc/ftrace: Inline ftrace_modify_code()

"Naveen N. Rao" <[email protected]> writes:
> Christophe Leroy wrote:
>> Inlining ftrace_modify_code(), it increases a bit the
>> size of ftrace code but brings 5% improvment on ftrace
>> activation.
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>> arch/powerpc/kernel/trace/ftrace.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/trace/ftrace.c b/arch/powerpc/kernel/trace/ftrace.c
>> index 41c45b9c7f39..98e82fa4980f 100644
>> --- a/arch/powerpc/kernel/trace/ftrace.c
>> +++ b/arch/powerpc/kernel/trace/ftrace.c
>> @@ -53,7 +53,7 @@ ftrace_call_replace(unsigned long ip, unsigned long addr, int link)
>> return op;
>> }
>>
>> -static int
>> +static inline int
>> ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
>> {
>> ppc_inst_t replaced;
>
> I thought gcc was free to inline functions without the need for
> 'inline'.

Yes it is.

> On the flip side, don't we need __always_inline if we want to force
> inlining?

Yes. Since ac7c3e4ff401 ("compiler: enable CONFIG_OPTIMIZE_INLINING forcibly").

cheers

2022-04-22 19:24:07

by Steven Rostedt

[permalink] [raw]
Subject: Re: [PATCH v1 07/22] powerpc/ftrace: Use patch_instruction() return directly

On Mon, 18 Apr 2022 11:51:16 +0530
"Naveen N. Rao" <[email protected]> wrote:

> > --- a/arch/powerpc/kernel/trace/ftrace.c
> > +++ b/arch/powerpc/kernel/trace/ftrace.c
> > @@ -78,10 +78,7 @@ ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
> > }
> >
> > /* replace the text with the new text */
> > - if (patch_instruction((u32 *)ip, new))
> > - return -EPERM;
> > -
> > - return 0;
> > + return patch_instruction((u32 *)ip, new);
>
> I think the reason we were returning -EPERM is so that ftrace_bug() can

That is correct.

> throw the right error message. That will change due to this patch,
> though I'm not sure how much it matters. -EFAULT and -EPERM seem to
> print almost the same error message.

In these cases it helps to know the type of failure, as the way to debug it
is different.

-EFAULT: It failed to read it the location. This means that the memory is
likely not even mapped in, or the pointer is way off.

-EINVAL: Means that what was read did not match what was expected (the code
was already updated, pointing to the wrong location, or simply the
calculation of what to expect is incorrect).

-EPERM: Means the write failed. What was read was expected, but the
permissions to write have not been updated properly.

Differentiating the three is crucial to looking at where the issue lies
when an ftrace_bug() triggers.

-- Steve

2022-05-05 06:34:15

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v1 07/22] powerpc/ftrace: Use patch_instruction() return directly



Le 18/04/2022 à 21:44, Steven Rostedt a écrit :
> On Mon, 18 Apr 2022 11:51:16 +0530
> "Naveen N. Rao" <[email protected]> wrote:
>
>>> --- a/arch/powerpc/kernel/trace/ftrace.c
>>> +++ b/arch/powerpc/kernel/trace/ftrace.c
>>> @@ -78,10 +78,7 @@ ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
>>> }
>>>
>>> /* replace the text with the new text */
>>> - if (patch_instruction((u32 *)ip, new))
>>> - return -EPERM;
>>> -
>>> - return 0;
>>> + return patch_instruction((u32 *)ip, new);
>>
>> I think the reason we were returning -EPERM is so that ftrace_bug() can
>
> That is correct.
>
>> throw the right error message. That will change due to this patch,
>> though I'm not sure how much it matters. -EFAULT and -EPERM seem to
>> print almost the same error message.
>
> In these cases it helps to know the type of failure, as the way to debug it
> is different.
>
> -EFAULT: It failed to read it the location. This means that the memory is
> likely not even mapped in, or the pointer is way off.
>
> -EINVAL: Means that what was read did not match what was expected (the code
> was already updated, pointing to the wrong location, or simply the
> calculation of what to expect is incorrect).
>
> -EPERM: Means the write failed. What was read was expected, but the
> permissions to write have not been updated properly.
>
> Differentiating the three is crucial to looking at where the issue lies
> when an ftrace_bug() triggers.
>


Apparently no caller really care about the value returned by
patch_instruction(), the ones who check the return value just check that
it's not 0.

So the most performant would be to have patch_instruction() return
-EPERM instead of -EFAULT in case of failure.

Christophe

2022-05-05 07:32:58

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v1 06/22] powerpc/ftrace: Inline ftrace_modify_code()



Le 18/04/2022 à 08:07, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> Inlining ftrace_modify_code(), it increases a bit the
>> size of ftrace code but brings 5% improvment on ftrace
>> activation.
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>>  arch/powerpc/kernel/trace/ftrace.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/kernel/trace/ftrace.c
>> b/arch/powerpc/kernel/trace/ftrace.c
>> index 41c45b9c7f39..98e82fa4980f 100644
>> --- a/arch/powerpc/kernel/trace/ftrace.c
>> +++ b/arch/powerpc/kernel/trace/ftrace.c
>> @@ -53,7 +53,7 @@ ftrace_call_replace(unsigned long ip, unsigned long
>> addr, int link)
>>      return op;
>>  }
>>
>> -static int
>> +static inline int
>>  ftrace_modify_code(unsigned long ip, ppc_inst_t old, ppc_inst_t new)
>>  {
>>      ppc_inst_t replaced;
>
> I thought gcc was free to inline functions without the need for
> 'inline'. Don't you see this being inlined otherwise?

Yep, gcc is free to inline, but in that case it doesn't inline unless
you suggest it to do so.

>
> On the flip side, don't we need __always_inline if we want to force
> inlining?

The question is, do we want to force inlining, even with
CONFIG_CC_OPTIMIZE_FOR_SIZE ?

With 'inline', gcc seems to now inline it with
CONFIG_CC_OPTIMIZE_FOR_SPEED and still keep it out of line with
CONFIG_CC_OPTIMIZE_FOR_SIZE.

Christophe

2022-05-09 04:43:56

by Christophe Leroy

[permalink] [raw]
Subject: Re: [PATCH v1 10/22] powerpc/ftrace: Use CONFIG_FUNCTION_TRACER instead of CONFIG_DYNAMIC_FTRACE



Le 18/04/2022 à 09:00, Naveen N. Rao a écrit :
> Christophe Leroy wrote:
>> Since commit 0c0c52306f47 ("powerpc: Only support DYNAMIC_FTRACE not
>> static"), CONFIG_DYNAMIC_FTRACE is always selected when
>> CONFIG_FUNCTION_TRACER is selected.
>>
>> To avoid confusion and have the reader wonder what's happen when
>> CONFIG_FUNCTION_TRACER is selected and CONFIG_DYNAMIC_FTRACE is not,
>> use CONFIG_FUNCTION_TRACER in ifdefs instead of CONFIG_DYNAMIC_FTRACE.
>>
>> As CONFIG_FUNCTION_GRAPH_TRACER depends on CONFIG_FUNCTION_TRACER,
>> ftrace.o doesn't need to appear for both symbols in Makefile.
>>
>> Then as ftrace.o is built only when CONFIG_FUNCTION_TRACER is selected
>
> and since it implies CONFIG_DYNAMIC_FTRACE, CONFIG_DYNAMIC_FTRACE is not
> needed in ftrace.c

Ok, added to the commit message

>
>> ifdef CONFIG_FUNCTION_TRACER is not needed in ftrace.c
>>
>> Signed-off-by: Christophe Leroy <[email protected]>
>> ---
>>  arch/powerpc/include/asm/book3s/32/pgtable.h | 2 +-
>>  arch/powerpc/include/asm/book3s/64/pgtable.h | 2 +-
>>  arch/powerpc/include/asm/module.h            | 4 ++--
>>  arch/powerpc/include/asm/nohash/pgtable.h    | 2 +-
>>  arch/powerpc/kernel/module_32.c              | 4 ++--
>>  arch/powerpc/kernel/module_64.c              | 6 +++---
>>  arch/powerpc/kernel/trace/Makefile           | 4 +---
>>  arch/powerpc/kernel/trace/ftrace.c           | 4 ----
>>  8 files changed, 11 insertions(+), 17 deletions(-)
>>
>> diff --git a/arch/powerpc/include/asm/book3s/32/pgtable.h
>> b/arch/powerpc/include/asm/book3s/32/pgtable.h
>> index 772e00dc4ef1..992aed626eb4 100644
>> --- a/arch/powerpc/include/asm/book3s/32/pgtable.h
>> +++ b/arch/powerpc/include/asm/book3s/32/pgtable.h
>> @@ -124,7 +124,7 @@ static inline bool pte_user(pte_t pte)
>>   * on platforms where such control is possible.
>>   */
>>  #if defined(CONFIG_KGDB) || defined(CONFIG_XMON) ||
>> defined(CONFIG_BDI_SWITCH) ||\
>> -    defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
>> +    defined(CONFIG_KPROBES) || defined(CONFIG_FUNCTION_TRACER)
>>  #define PAGE_KERNEL_TEXT    PAGE_KERNEL_X
>>  #else
>>  #define PAGE_KERNEL_TEXT    PAGE_KERNEL_ROX
>> diff --git a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> index 875730d5af40..cf01b609572f 100644
>> --- a/arch/powerpc/include/asm/book3s/64/pgtable.h
>> +++ b/arch/powerpc/include/asm/book3s/64/pgtable.h
>> @@ -169,7 +169,7 @@
>>   * on platforms where such control is possible.
>>   */
>>  #if defined(CONFIG_KGDB) || defined(CONFIG_XMON) ||
>> defined(CONFIG_BDI_SWITCH) || \
>> -    defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
>> +    defined(CONFIG_KPROBES) || defined(CONFIG_FUNCTION_TRACER)
>>  #define PAGE_KERNEL_TEXT    PAGE_KERNEL_X
>>  #else
>>  #define PAGE_KERNEL_TEXT    PAGE_KERNEL_ROX
>> diff --git a/arch/powerpc/include/asm/module.h
>> b/arch/powerpc/include/asm/module.h
>> index 857d9ff24295..e6f5963fd96e 100644
>> --- a/arch/powerpc/include/asm/module.h
>> +++ b/arch/powerpc/include/asm/module.h
>> @@ -39,7 +39,7 @@ struct mod_arch_specific {
>>      unsigned int init_plt_section;
>>  #endif /* powerpc64 */
>>
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> +#ifdef CONFIG_FUNCTION_TRACER
>>      unsigned long tramp;
>>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>>      unsigned long tramp_regs;
>> @@ -68,7 +68,7 @@ struct mod_arch_specific {
>>  #    endif    /* MODULE */
>>  #endif
>>
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> +#ifdef CONFIG_FUNCTION_TRACER
>>  #    ifdef MODULE
>>      asm(".section .ftrace.tramp,\"ax\",@nobits; .align 3; .previous");
>>  #    endif    /* MODULE */
>> diff --git a/arch/powerpc/include/asm/nohash/pgtable.h
>> b/arch/powerpc/include/asm/nohash/pgtable.h
>> index ac75f4ab0dba..2e8cf217a191 100644
>> --- a/arch/powerpc/include/asm/nohash/pgtable.h
>> +++ b/arch/powerpc/include/asm/nohash/pgtable.h
>> @@ -23,7 +23,7 @@
>>   * on platforms where such control is possible.
>>   */
>>  #if defined(CONFIG_KGDB) || defined(CONFIG_XMON) ||
>> defined(CONFIG_BDI_SWITCH) ||\
>> -    defined(CONFIG_KPROBES) || defined(CONFIG_DYNAMIC_FTRACE)
>> +    defined(CONFIG_KPROBES) || defined(CONFIG_FUNCTION_TRACER)
>>  #define PAGE_KERNEL_TEXT    PAGE_KERNEL_X
>>  #else
>>  #define PAGE_KERNEL_TEXT    PAGE_KERNEL_ROX
>> diff --git a/arch/powerpc/kernel/module_32.c
>> b/arch/powerpc/kernel/module_32.c
>> index a0432ef46967..2aa368ce21c9 100644
>> --- a/arch/powerpc/kernel/module_32.c
>> +++ b/arch/powerpc/kernel/module_32.c
>> @@ -39,7 +39,7 @@ static unsigned int count_relocs(const Elf32_Rela
>> *rela, unsigned int num)
>>              r_addend = rela[i].r_addend;
>>          }
>>
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> +#ifdef CONFIG_FUNCTION_TRACER
>>      _count_relocs++;    /* add one for ftrace_caller */
>>  #endif
>>      return _count_relocs;
>> @@ -288,7 +288,7 @@ int apply_relocate_add(Elf32_Shdr *sechdrs,
>>      return 0;
>>  }
>>
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> +#ifdef CONFIG_FUNCTION_TRACER
>>  int module_trampoline_target(struct module *mod, unsigned long addr,
>>                   unsigned long *target)
>>  {
>> diff --git a/arch/powerpc/kernel/module_64.c
>> b/arch/powerpc/kernel/module_64.c
>> index 794720530442..b13a72665eee 100644
>> --- a/arch/powerpc/kernel/module_64.c
>> +++ b/arch/powerpc/kernel/module_64.c
>> @@ -207,7 +207,7 @@ static unsigned long get_stubs_size(const
>> Elf64_Ehdr *hdr,
>>          }
>>      }
>>
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> +#ifdef CONFIG_FUNCTION_TRACER
>>      /* make the trampoline to the ftrace_caller */
>>      relocs++;
>>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>> @@ -372,7 +372,7 @@ static bool is_mprofile_ftrace_call(const char *name)
>>  {
>>      if (!strcmp("_mcount", name))
>>          return true;
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> +#ifdef CONFIG_FUNCTION_TRACER
>>      if (!strcmp("ftrace_caller", name))
>>          return true;
>>  #ifdef CONFIG_DYNAMIC_FTRACE_WITH_REGS
>> @@ -740,7 +740,7 @@ int apply_relocate_add(Elf64_Shdr *sechdrs,
>>      return 0;
>>  }
>>
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> +#ifdef CONFIG_FUNCTION_TRACER
>>  int module_trampoline_target(struct module *mod, unsigned long addr,
>>                   unsigned long *target)
>>  {
>
> The below two changes to trace/Makefile and trace/ftrace.c make sense,
> but I'm not sure the above changes are necessary.
>
> For generic code (outside arch/powerpc/kernel/trace), I think it is good
> to retain the actual dependency which is DYNAMIC_FTRACE. In my view,
> gating some of the above code on FUNCTION_TRACER is also confusing. The
> primary implication of always selecting DYNAMIC_FTRACE is within the
> powerpc trace code, and it is good to keep it that way.

Ok I keep only the changes to trace directory for now.

>
>> diff --git a/arch/powerpc/kernel/trace/Makefile
>> b/arch/powerpc/kernel/trace/Makefile
>> index fc32ec30b297..af8527538fe4 100644
>> --- a/arch/powerpc/kernel/trace/Makefile
>> +++ b/arch/powerpc/kernel/trace/Makefile
>> @@ -14,9 +14,7 @@ obj64-$(CONFIG_FUNCTION_TRACER)        +=
>> ftrace_mprofile.o
>>  else
>>  obj64-$(CONFIG_FUNCTION_TRACER)        += ftrace_64_pg.o
>>  endif
>> -obj-$(CONFIG_FUNCTION_TRACER)        += ftrace_low.o
>> -obj-$(CONFIG_DYNAMIC_FTRACE)        += ftrace.o
>> -obj-$(CONFIG_FUNCTION_GRAPH_TRACER)    += ftrace.o
>> +obj-$(CONFIG_FUNCTION_TRACER)        += ftrace_low.o ftrace.o
>>  obj-$(CONFIG_TRACING)            += trace_clock.o
>>
>>  obj-$(CONFIG_PPC64)            += $(obj64-y)
>> diff --git a/arch/powerpc/kernel/trace/ftrace.c
>> b/arch/powerpc/kernel/trace/ftrace.c
>> index 2c7e42e439bb..188f59f4ee4a 100644
>> --- a/arch/powerpc/kernel/trace/ftrace.c
>> +++ b/arch/powerpc/kernel/trace/ftrace.c
>> @@ -28,9 +28,6 @@
>>  #include <asm/syscall.h>
>>  #include <asm/inst.h>
>>
>> -
>> -#ifdef CONFIG_DYNAMIC_FTRACE
>> -
>>  /*
>>   * We generally only have a single long_branch tramp and at most 2 or
>> 3 plt
>>   * tramps generated. But, we don't use the plt tramps currently. We
>> also allot
>> @@ -783,7 +780,6 @@ int __init ftrace_dyn_arch_init(void)
>>      return 0;
>>  }
>>  #endif
>> -#endif /* CONFIG_DYNAMIC_FTRACE */
>>
>>  #ifdef CONFIG_FUNCTION_GRAPH_TRACER
>>
>> --
>> 2.35.1
>>
>
> - Naveen