Re: [PATCH bpf-next] uprobes: Fix the xol slots reserved for uretprobe trampoline

["Liao, Chang" <liaochang1@huawei.com>]

Hi Jiri and Oleg,

在 2024/6/20 19:27, Liao, Chang 写道:
> 
> 
> 在 2024/6/20 17:06, Jiri Olsa 写道:
>> On Thu, Jun 20, 2024 at 10:36:02AM +0200, Oleg Nesterov wrote:
>>> On 06/20, Liao, Chang wrote:
>>>>
>>>> However, when i asm porting uretprobe trampoline to arm64
>>>> to explore its benefits on that architecture, i discovered the problem that
>>>> single slot is not large enought for trampoline code.
>>
>> ah ok, makes sense now.. x86_64 has the slot big enough for the trampoline,
>> but arm64 does not
>>
>>>
>>> Ah, but then I'd suggest to make the changelog more clear. It looks as
>>> if the problem was introduced by the patch from Jiri. Note that we was
>>> confused as well ;)
>>>
>>> And,
>>>
>>> 	+	/* Reserve enough slots for the uretprobe trampoline */
>>> 	+	for (slot_nr = 0;
>>> 	+	     slot_nr < max((insns_size / UPROBE_XOL_SLOT_BYTES), 1);
>>> 	+	     slot_nr++)
>>>
>>> this doesn't look right. Just suppose that insns_size = UPROBE_XOL_SLOT_BYTES + 1.
>>> I'd suggest DIV_ROUND_UP(insns_size, UPROBE_XOL_SLOT_BYTES).
>>>
>>> And perhaps it would be better to send this change along with
>>> uretprobe_trampoline_for_arm64 ?
>>
>> +1, also I'm curious what's the gain on arm64?
> 
> I am currently finalizing the uretprobe trampoline and syscall implementation on arm64.
> While i have addressed most of issues, there are stiil a few bugs that reguire more effort.
> Once these are fixed, i will use Redis to evaluate the performance gains on arm64. In the
> next revision, i will submit a patchset that includes all relevant code changs, testcases
> and benchmark data, which will allows a comprehensive review and dicussion.

This is an update on the development of uretprobe syscall for ARM64 architecture.

I've recently completed a basic implementation of the uretprobe syscall and trampoline
code for ARM64, with the uprobe syscall selftest passed. This allow me to revisit the
performance benchmark using uretprobe. With running Redis benchmark a Kunpeng server,
I observed a slight performance gain with the uretprobe syscall on ARM64. The performance
test spawned a Redis-server and Redis-benchmark on seperate cores within the same NUMA
node. The Redis-server handled each SET and GET request from Redis-benchmark which triggered
three uretprobe events with attached bpftrace program that increments the counter.
Here is the benchmark result:

On Kunpeng916 (Hi1616), 4 NUMA nodes, 64 Cores @ 2.4GHz :

-------------------------------------------------------------------------------
Test case       |  No uretprobe |  uretprobe(breakpoint) |  uretprobe (syscall)
===============================================================================
Redis SET (RPS) |  47025        |  40619   -13.6%        |  40670   -13.5%
-------------------------------------------------------------------------------
Redis GET (RPS) |  46715        |  41426   -11.3%        |  41274   -11.6%
-------------------------------------------------------------------------------

The detailed test scripts and bpf program are available upon any request.

Additionally, I've attempted to optimize the implementation of the uretprobe syscall and
trampoline, but the cause of the lower than expected performance gain compared to x86
remains unclear. Further investigation is necessary to identify potentail bottlenecks or
inefficiencies specific to ARM64. It is grateful for any insights or suggestions the
community might have on the potential reasons for the performance difference between
ARM64 and X86. The patch for the uretprobe syscall is attached below for reference.

---------------------------%<----------------------------
diff --git a/arch/arm64/kernel/probes/Makefile b/arch/arm64/kernel/probes/Makefile
index 8e4be92e25b1..059f38c0857f 100644
--- a/arch/arm64/kernel/probes/Makefile
+++ b/arch/arm64/kernel/probes/Makefile
@@ -3,4 +3,5 @@ obj-$(CONFIG_KPROBES)		+= kprobes.o decode-insn.o	\
 				   kprobes_trampoline.o		\
 				   simulate-insn.o
 obj-$(CONFIG_UPROBES)		+= uprobes.o decode-insn.o	\
+				   uprobes_trampoline.o		\
 				   simulate-insn.o
diff --git a/arch/arm64/kernel/probes/uprobes.c b/arch/arm64/kernel/probes/uprobes.c
index d49aef2657cd..632f97afd50f 100644
--- a/arch/arm64/kernel/probes/uprobes.c
+++ b/arch/arm64/kernel/probes/uprobes.c
@@ -5,12 +5,69 @@
 #include <linux/highmem.h>
 #include <linux/ptrace.h>
 #include <linux/uprobes.h>
+#include <linux/syscalls.h>
 #include <asm/cacheflush.h>

 #include "decode-insn.h"

 #define UPROBE_INV_FAULT_CODE	UINT_MAX

+extern char uretprobe_trampoline[] __read_mostly;
+extern char uretprobe_trampoline_end[] __read_mostly;
+extern char uretprobe_trampoline_svc[] __read_mostly;
+
+void *arch_uprobe_trampoline(unsigned long *psize)
+{
+	static uprobe_opcode_t insn = UPROBE_SWBP_INSN;
+	struct pt_regs *regs = task_pt_regs(current);
+
+	if (!compat_user_mode(regs)) {
+		*psize = uretprobe_trampoline_end - uretprobe_trampoline;
+		return uretprobe_trampoline;
+	}
+
+	*psize = UPROBE_SWBP_INSN_SIZE;
+	return &insn;
+}
+
+static unsigned long syscall_at_uprobe_trampoline(void)
+{
+	unsigned long tramp = uprobe_get_trampoline_vaddr();
+
+	return tramp + (uretprobe_trampoline_svc - uretprobe_trampoline);
+}
+
+SYSCALL_DEFINE0(uretprobe)
+{
+	int err;
+	struct pt_regs *regs = task_pt_regs(current);
+
+	if (compat_user_mode(regs))
+		goto sigill;
+
+	/* ensure uretprobe syscall invoked from uretprobe trampoline */
+	if ((regs->pc - AARCH64_INSN_SIZE) != syscall_at_uprobe_trampoline())
+		goto sigill;
+
+	/* restore the clobbered context used to invoke uretprobe syscall */
+	err = copy_from_user(&regs->regs[8], (void __user *)(regs->sp - 8),
+			     sizeof(regs->regs[8]));
+	if (err)
+		goto sigill;
+
+	uprobe_handle_trampoline(regs);
+
+	/* restore the real LR before return to the caller. */
+	regs->regs[30] = regs->pc;
+
+	/* use the real return value */
+	return regs->regs[0];
+
+sigill:
+	force_sig(SIGILL);
+	return -1;
+}
+
 void arch_uprobe_copy_ixol(struct page *page, unsigned long vaddr,
 		void *src, unsigned long len)
 {
diff --git a/arch/arm64/kernel/probes/uprobes_trampoline.S b/arch/arm64/kernel/probes/uprobes_trampoline.S
new file mode 100644
index 000000000000..670d4d9e97ec
--- /dev/null
+++ b/arch/arm64/kernel/probes/uprobes_trampoline.S
@@ -0,0 +1,21 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * trampoline entry and return code for uretprobes.
+ */
+
+#include <linux/linkage.h>
+#include <asm/asm-bug.h>
+#include <asm/assembler.h>
+#include <asm/unistd.h>
+
+	.text
+
+SYM_CODE_START(uretprobe_trampoline)
+	str x8, [sp, #-8]
+	mov x8, #__NR_uretprobe
+
+SYM_CODE_START(uretprobe_trampoline_svc)
+	svc #0x000
+
+SYM_CODE_START(uretprobe_trampoline_end)
+	nop
--------------------------->%----------------------------

Thanks.

> 
>>
>> thanks,
>> jirka
> 

-- 
BR
Liao, Chang