Received-SPF: pass (google.com: domain of linux-kernel+bounces-209480-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Precedence: bulk
MIME-Version: 1.0
References: <20240523121149.575616-1-jolsa@kernel.org> <CAEf4Bza-+=04GG7Tg4U4pCQ28Oy_2F_5872EPDsX6X3Y=jhEuw@mail.gmail.com>
 <CAEf4Bzbc99bwGcmtCa3iekXSvSrxMQzfnTViT5Y-dn8qbvJy7A@mail.gmail.com> <20240611064641.9021829459211782902e4fb2@kernel.org>
In-Reply-To: <20240611064641.9021829459211782902e4fb2@kernel.org>
From: Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date: Tue, 11 Jun 2024 09:30:52 +0100
Message-ID: <CAEf4BzYcwUS=7KFX5fUibS9eLT8yQxYqaWF_+sVM0YZJzBD=Sg@mail.gmail.com>
Subject: Re: [PATCHv7 bpf-next 0/9] uprobe: uretprobe speed up
To: Masami Hiramatsu <mhiramat@kernel.org>
Cc: Steven Rostedt <rostedt@goodmis.org>, Oleg Nesterov <oleg@redhat.com>, Jiri Olsa <jolsa@kernel.org>, 
	Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, 
	Andrii Nakryiko <andrii@kernel.org>, linux-kernel@vger.kernel.org, 
	linux-trace-kernel@vger.kernel.org, linux-api@vger.kernel.org, 
	linux-man@vger.kernel.org, x86@kernel.org, bpf@vger.kernel.org, 
	Song Liu <songliubraving@fb.com>, Yonghong Song <yhs@fb.com>, 
	John Fastabend <john.fastabend@gmail.com>, Peter Zijlstra <peterz@infradead.org>, 
	Thomas Gleixner <tglx@linutronix.de>, "Borislav Petkov (AMD)" <bp@alien8.de>, Ingo Molnar <mingo@redhat.com>, 
	Andy Lutomirski <luto@kernel.org>, "Edgecombe, Rick P" <rick.p.edgecombe@intel.com>, 
	Deepak Gupta <debug@rivosinc.com>, Linus Torvalds <torvalds@linux-foundation.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jun 10, 2024 at 10:46=E2=80=AFPM Masami Hiramatsu <mhiramat@kernel.=
org> wrote:
>
> On Wed, 5 Jun 2024 09:42:45 -0700
> Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> > On Fri, May 31, 2024 at 10:52=E2=80=AFAM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Thu, May 23, 2024 at 5:11=E2=80=AFAM Jiri Olsa <jolsa@kernel.org> =
wrote:
> > > >
> > > > hi,
> > > > as part of the effort on speeding up the uprobes [0] coming with
> > > > return uprobe optimization by using syscall instead of the trap
> > > > on the uretprobe trampoline.
> > > >
> > > > The speed up depends on instruction type that uprobe is installed
> > > > and depends on specific HW type, please check patch 1 for details.
> > > >
> > > > Patches 1-8 are based on bpf-next/master, but patch 2 and 3 are
> > > > apply-able on linux-trace.git tree probes/for-next branch.
> > > > Patch 9 is based on man-pages master.
> > > >
> > > > v7 changes:
> > > > - fixes in man page [Alejandro Colomar]
> > > > - fixed patch #1 fixes tag [Oleg]
> > > >
> > > > Also available at:
> > > >   https://git.kernel.org/pub/scm/linux/kernel/git/jolsa/perf.git
> > > >   uretprobe_syscall
> > > >
> > > > thanks,
> > > > jirka
> > > >
> > > >
> > > > Notes to check list items in Documentation/process/adding-syscalls.=
rst:
> > > >
> > > > - System Call Alternatives
> > > >   New syscall seems like the best way in here, because we need
> > > >   just to quickly enter kernel with no extra arguments processing,
> > > >   which we'd need to do if we decided to use another syscall.
> > > >
> > > > - Designing the API: Planning for Extension
> > > >   The uretprobe syscall is very specific and most likely won't be
> > > >   extended in the future.
> > > >
> > > >   At the moment it does not take any arguments and even if it does
> > > >   in future, it's allowed to be called only from trampoline prepare=
d
> > > >   by kernel, so there'll be no broken user.
> > > >
> > > > - Designing the API: Other Considerations
> > > >   N/A because uretprobe syscall does not return reference to kernel
> > > >   object.
> > > >
> > > > - Proposing the API
> > > >   Wiring up of the uretprobe system call is in separate change,
> > > >   selftests and man page changes are part of the patchset.
> > > >
> > > > - Generic System Call Implementation
> > > >   There's no CONFIG option for the new functionality because it
> > > >   keeps the same behaviour from the user POV.
> > > >
> > > > - x86 System Call Implementation
> > > >   It's 64-bit syscall only.
> > > >
> > > > - Compatibility System Calls (Generic)
> > > >   N/A uretprobe syscall has no arguments and is not supported
> > > >   for compat processes.
> > > >
> > > > - Compatibility System Calls (x86)
> > > >   N/A uretprobe syscall is not supported for compat processes.
> > > >
> > > > - System Calls Returning Elsewhere
> > > >   N/A.
> > > >
> > > > - Other Details
> > > >   N/A.
> > > >
> > > > - Testing
> > > >   Adding new bpf selftests and ran ltp on top of this change.
> > > >
> > > > - Man Page
> > > >   Attached.
> > > >
> > > > - Do not call System Calls in the Kernel
> > > >   N/A.
> > > >
> > > >
> > > > [0] https://lore.kernel.org/bpf/ZeCXHKJ--iYYbmLj@krava/
> > > > ---
> > > > Jiri Olsa (8):
> > > >       x86/shstk: Make return uprobe work with shadow stack
> > > >       uprobe: Wire up uretprobe system call
> > > >       uprobe: Add uretprobe syscall to speed up return probe
> > > >       selftests/x86: Add return uprobe shadow stack test
> > > >       selftests/bpf: Add uretprobe syscall test for regs integrity
> > > >       selftests/bpf: Add uretprobe syscall test for regs changes
> > > >       selftests/bpf: Add uretprobe syscall call from user space tes=
t
> > > >       selftests/bpf: Add uretprobe shadow stack test
> > > >
> > >
> > > Masami, Steven,
> > >
> > > It seems like the series is ready to go in. Are you planning to take
> > > the first 4 patches through your linux-trace tree?
> >
> > Another ping. It's been two weeks since Jiri posted the last revision
> > that got no more feedback to be addressed and everyone seems to be
> > happy with it.
>
> Sorry about late reply. I agree that this is OK to go, since no other
> comments. Let me pick this up to probes/for-next branch.
>
> >
> > This is an important speed up improvement for uprobe infrastructure in
> > general and for BPF ecosystem in particular. "Uprobes are slow" is one
> > of the top complaints from production BPF users, and sys_uretprobe
> > approach is significantly improving the situation for return uprobes
> > (aka uretprobes), potentially enabling new use cases that previously
> > could have been too expensive to trace in practice and reducing the
> > overhead of the existing ones.
> >
> > I'd appreciate the engagement from linux-trace maintainers on this
> > patch set. Given it's important for BPF and that a big part of the
> > patch set is BPF-based selftests, we'd also be happy to route all this
> > through the bpf-next tree (which would actually make logistics for us
> > much easier, but that's not the main concern). But regardless of the
> > tree, it would be nice to make a decision and go forward with it.
>
> I think it would be better to include those patches together in
> linux-tree. Can you review and ack to the last patch ? ([9/9])

Sure. Jiri, please add my ack for the entire series in the next revision:

Acked-by: Andrii Nakryiko <andrii@kernel.org>

>
> Thank you,
>
> >
> > Thank you!
> >
> > >
> > > >  arch/x86/entry/syscalls/syscall_64.tbl                      |   1 =
+
> > > >  arch/x86/include/asm/shstk.h                                |   4 =
+
> > > >  arch/x86/kernel/shstk.c                                     |  16 =
++++
> > > >  arch/x86/kernel/uprobes.c                                   | 124 =
++++++++++++++++++++++++++++-
> > > >  include/linux/syscalls.h                                    |   2 =
+
> > > >  include/linux/uprobes.h                                     |   3 =
+
> > > >  include/uapi/asm-generic/unistd.h                           |   5 =
+-
> > > >  kernel/events/uprobes.c                                     |  24 =
++++--
> > > >  kernel/sys_ni.c                                             |   2 =
+
> > > >  tools/include/linux/compiler.h                              |   4 =
+
> > > >  tools/testing/selftests/bpf/bpf_testmod/bpf_testmod.c       | 123 =
++++++++++++++++++++++++++++-
> > > >  tools/testing/selftests/bpf/prog_tests/uprobe_syscall.c     | 385 =
+++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++=
++++++++++++++++
> > > >  tools/testing/selftests/bpf/progs/uprobe_syscall.c          |  15 =
++++
> > > >  tools/testing/selftests/bpf/progs/uprobe_syscall_executed.c |  17 =
++++
> > > >  tools/testing/selftests/x86/test_shadow_stack.c             | 145 =
++++++++++++++++++++++++++++++++++
> > > >  15 files changed, 860 insertions(+), 10 deletions(-)
> > > >  create mode 100644 tools/testing/selftests/bpf/prog_tests/uprobe_s=
yscall.c
> > > >  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscal=
l.c
> > > >  create mode 100644 tools/testing/selftests/bpf/progs/uprobe_syscal=
l_executed.c
> > > >
> > > > Jiri Olsa (1):
> > > >       man2: Add uretprobe syscall page
> > > >
> > > >  man/man2/uretprobe.2 | 56 ++++++++++++++++++++++++++++++++++++++++=
++++++++++++++++
> > > >  1 file changed, 56 insertions(+)
> > > >  create mode 100644 man/man2/uretprobe.2
>
>
> --
> Masami Hiramatsu (Google) <mhiramat@kernel.org>