Le 08/12/2023 à 17:30, Naveen N Rao a écrit :

Function profile sequence on powerpc includes two instructions at the
beginning of each function:

	mflr	r0
	bl	ftrace_caller

The call to ftrace_caller() gets nop'ed out during kernel boot and is
patched in when ftrace is enabled.

There are two issues with this:
1. The 'mflr r0' instruction at the beginning of each function remains
    even though ftrace is not being used.
2. When ftrace is activated, we return from ftrace_caller() with a bctr
    instruction to preserve r0 and LR, resulting in the link stack
    becoming unbalanced.

To address (1), we have tried to nop'out the 'mflr r0' instruction when
nop'ing out the call to ftrace_caller() and restoring it when enabling
ftrace. But, that required additional synchronization slowing down
ftrace activation. It also left an additional nop instruction at the
beginning of each function and that wasn't desirable on 32-bit powerpc.

Instead of that, move the function profile sequence out-of-line leaving
a single nop at function entry. On ftrace activation, the nop is changed
to an unconditional branch to the out-of-line sequence that in turn
calls ftrace_caller(). This removes the need for complex synchronization
during ftrace activation and simplifies the code. More importantly, this
improves performance of the kernel when ftrace is not in use.

To address (2), change the ftrace trampoline to return with a 'blr'
instruction with the original return address in r0 intact. Then, an
additional 'mtlr r0' instruction in the function profile sequence can
move the correct return address back to LR.

With the above two changes, the function profile sequence now looks like
the following:

  [func:		# GEP -- 64-bit powerpc, optional
	addis	r2,r12,imm1
	addi	r2,r2,imm2]
   tramp:
	mflr	r0
	bl	ftrace_caller
	mtlr	r0
	b	func
	nop
	[nop]	# 64-bit powerpc only
   func:		# LEP
	nop

On 32-bit powerpc, the ftrace mcount trampoline is now completely
outside the function. This is also the case on 64-bit powerpc for
functions that do not need a GEP. However, for functions that need a
GEP, the additional instructions are inserted between the GEP and the
LEP. Since we can only have a fixed number of instructions between GEP
and LEP, we choose to emit 6 instructions. Four of those instructions
are used for the function profile sequence and two instruction slots are
reserved for implementing support for DYNAMIC_FTRACE_WITH_CALL_OPS. On
32-bit powerpc, we emit one additional nop for this purpose resulting in
a total of 5 nops before function entry.

To enable ftrace, the nop at function entry is changed to an
unconditional branch to 'tramp'. The call to ftrace_caller() may be
updated to ftrace_regs_caller() depending on the registered ftrace ops.
On 64-bit powerpc, we additionally change the instruction at 'tramp' to
'mflr r0' from an unconditional branch back to func+4. This is so that
functions entered through the GEP can skip the function profile sequence
unless ftrace is enabled.

With the context_switch microbenchmark on a P9 machine, there is a
performance improvement of ~6% with this patch applied, going from 650k
context switches to 690k context switches without ftrace enabled. With
ftrace enabled, the performance was similar at 86k context switches.

Wondering how significant that context_switch micorbenchmark is.

I ran it on both mpc885 and mpc8321 and I'm a bit puzzled by some of the 
results:
# ./context_switch --no-fp
Using threads with yield on cpus 0/0 touching FP:no altivec:no vector:no 
vdso:no

On 885, I get the following results before and after your patch.

CONFIG_FTRACE not selected : 44,9k
CONFIG_FTRACE selected, before : 32,8k
CONFIG_FTRACE selected, after : 33,6k

All this is with CONFIG_INIT_STACK_ALL_ZERO which is the default. But 
when I select CONFIG_INIT_STACK_NONE, the CONFIG_FTRACE not selected 
result is only 34,4.

On 8321:

CONFIG_FTRACE not selected : 100,3k
CONFIG_FTRACE selected, before : 72,5k
CONFIG_FTRACE selected, after : 116k

So the results look odd to me.

The downside of this approach is the increase in vmlinux size,
especially on 32-bit powerpc. We now emit 3 additional instructions for
each function (excluding the one or two instructions for supporting
DYNAMIC_FTRACE_WITH_CALL_OPS). On 64-bit powerpc with the current
implementation of -fpatchable-function-entry though, this is not
avoidable since we are forced to emit 6 instructions between the GEP and
the LEP even if we are to only support DYNAMIC_FTRACE_WITH_CALL_OPS.

The increase is almost 5% on the few 32 bits defconfig I have tested. 
That's a lot.

On 32 bits powerpc, only the e500 has a link stack that could end up 
being unbalanced. Could we keep the bctr and avoid the mtlr and the jump 
in the trampoline ?

On 8xx a NOP is one cycle, a taken branch is 2 cycles, but the second 
cycle is a bubble that most of the time gets filled by following 
operations. On the 83xx, branches and NOPs are supposed to be seamless.

So is that out-of-line trampoline really worth it ? Maybe keeping the 
ftrace instructions at the begining and just replacing the mflr by an 
jump when ftrace is off would help reduce the ftrace insns by one more 
instruction.

Signed-off-by: Naveen N Rao <naveen@kernel.org>
---

Can you replace by IS_ENABLED() ?

+		/*
+		 * Heuristic: look for the 'mtlr r0' instruction assuming ftrace init is done.
+		 * If it is not found, look for two consecutive nops after the GEP.
+		 * Longer term, we really should be parsing the symbol table to determine LEP.
+		 */
+		if ((*(insn+4) == PPC_RAW_MTLR(_R0)) ||
+		    ((*(insn+2) == PPC_RAW_NOP() && *(insn+3) == PPC_RAW_NOP())))
+			return (unsigned long)(insn + 8);
+#endif
  		return (unsigned long)(insn + 2);
-	else
+	} else {
  		return (unsigned long)func;
+	}
  #elif defined(CONFIG_PPC64_ELF_ABI_V1)
  	/*
  	 * On PPC64 ABIv1 the function pointer actually points to the

Can it be based on IS_ENABLED(CONFIG_PPC64) instead ?

+		PPC_RAW_BL(0), /* bl ftrace_caller */
+		PPC_RAW_MTLR(_R0), /* also see update ppc_function_entry() */
+		PPC_RAW_BRANCH(FTRACE_MCOUNT_TRAMP_OFFSET - MCOUNT_INSN_SIZE * 2)
+	};
+