diff mbox series

[RFC,bpf-next,4/5] selftests/bpf: Add test for USDT parse of xmm reg

Message ID 20220512074321.2090073-5-davemarchevsky@fb.com (mailing list archive)
State RFC
Delegated to: BPF
Headers show
Series bpf: add get_reg_val helper | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR fail merge-conflict
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/apply fail Patch does not apply to bpf-next

Commit Message

Dave Marchevsky May 12, 2022, 7:43 a.m. UTC 
Validate that bpf_get_reg_val helper solves the motivating problem of
this patch series: USDT args passed through xmm regs. The userspace
portion of the test forces STAP_PROBE macro to use %xmm0 and %xmm1 regs
to pass a float and an int, which the bpf-side successfully reads using
BPF_USDT.

In the wild I discovered a sanely-configured USDT in Fedora libpthread
using xmm regs to pass scalar values, likely due to register pressure.
urandom_read_lib_xmm mimics this by using -ffixed-$REG flag to mark
r11-r14 unusable and passing many USDT args.

Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
---
 tools/testing/selftests/bpf/Makefile          |  8 ++-
 tools/testing/selftests/bpf/prog_tests/usdt.c |  7 +++
 .../selftests/bpf/progs/test_urandom_usdt.c   | 13 ++++
 tools/testing/selftests/bpf/urandom_read.c    |  3 +
 .../selftests/bpf/urandom_read_lib_xmm.c      | 62 +++++++++++++++++++
 5 files changed, 91 insertions(+), 2 deletions(-)
 create mode 100644 tools/testing/selftests/bpf/urandom_read_lib_xmm.c

Comments

Andrii Nakryiko May 16, 2022, 11:31 p.m. UTC | #1 
On Thu, May 12, 2022 at 12:43 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
>
> Validate that bpf_get_reg_val helper solves the motivating problem of
> this patch series: USDT args passed through xmm regs. The userspace
> portion of the test forces STAP_PROBE macro to use %xmm0 and %xmm1 regs
> to pass a float and an int, which the bpf-side successfully reads using
> BPF_USDT.
>
> In the wild I discovered a sanely-configured USDT in Fedora libpthread
> using xmm regs to pass scalar values, likely due to register pressure.
> urandom_read_lib_xmm mimics this by using -ffixed-$REG flag to mark
> r11-r14 unusable and passing many USDT args.
>
> Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> ---
>  tools/testing/selftests/bpf/Makefile          |  8 ++-
>  tools/testing/selftests/bpf/prog_tests/usdt.c |  7 +++
>  .../selftests/bpf/progs/test_urandom_usdt.c   | 13 ++++
>  tools/testing/selftests/bpf/urandom_read.c    |  3 +
>  .../selftests/bpf/urandom_read_lib_xmm.c      | 62 +++++++++++++++++++
>  5 files changed, 91 insertions(+), 2 deletions(-)
>  create mode 100644 tools/testing/selftests/bpf/urandom_read_lib_xmm.c
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index 6bbc03161544..19246e34dfe1 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -172,10 +172,14 @@ $(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c
>         $(call msg,LIB,,$@)
>         $(Q)$(CC) $(CFLAGS) -fPIC $(LDFLAGS) $^ $(LDLIBS) --shared -o $@
>
> -$(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so
> +$(OUTPUT)/liburandom_read_xmm.so: urandom_read_lib_xmm.c
> +       $(call msg,LIB,,$@)
> +       $(Q)$(CC) -O0 -ffixed-r11 -ffixed-r12 -ffixed-r13 -ffixed-r14 -fPIC $(LDFLAGS) $^ $(LDLIBS) --shared -o $@

this looks very x86-specific, but we support other architectures as well

looking at sdt.h, it seems like STAP_PROBEx() macros support being
called from assembly code, I wonder if it would be better to try to
figure out how to use it from assembly and use some xmm register
directly in inline assembly? I have never done that before, but am
hopeful :)

> +
> +$(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so $(OUTPUT)/liburandom_read_xmm.so
>         $(call msg,BINARY,,$@)
>         $(Q)$(CC) $(CFLAGS) $(LDFLAGS) $(filter %.c,$^)                        \
> -                 liburandom_read.so $(LDLIBS)                                 \
> +                 liburandom_read.so liburandom_read_xmm.so $(LDLIBS)          \
>                   -Wl,-rpath=. -Wl,--build-id=sha1 -o $@
>
>  $(OUTPUT)/bpf_testmod.ko: $(VMLINUX_BTF) $(wildcard bpf_testmod/Makefile bpf_testmod/*.[ch])
> diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c
> index a71f51bdc08d..f98749ac74a7 100644
> --- a/tools/testing/selftests/bpf/prog_tests/usdt.c
> +++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
> @@ -385,6 +385,12 @@ static void subtest_urandom_usdt(bool auto_attach)
>                         goto cleanup;
>                 skel->links.urandlib_read_with_sema = l;
>
> +               l = bpf_program__attach_usdt(skel->progs.urandlib_xmm_reg_read,
> +                                            urand_pid, "./liburandom_read_xmm.so",
> +                                            "urandlib", "xmm_reg_read", NULL);
> +               if (!ASSERT_OK_PTR(l, "urandlib_xmm_reg_read"))
> +                       goto cleanup;
> +               skel->links.urandlib_xmm_reg_read = l;
>         }
>
>         /* trigger urandom_read USDTs */
> @@ -402,6 +408,7 @@ static void subtest_urandom_usdt(bool auto_attach)
>         ASSERT_EQ(bss->urandlib_read_with_sema_call_cnt, 1, "urandlib_w_sema_cnt");
>         ASSERT_EQ(bss->urandlib_read_with_sema_buf_sz_sum, 256, "urandlib_w_sema_sum");
>
> +       ASSERT_EQ(bss->urandlib_xmm_reg_read_buf_sz_sum, 256, "liburandom_read_xmm.so");
>  cleanup:
>         if (urand_pipe)
>                 pclose(urand_pipe);
> diff --git a/tools/testing/selftests/bpf/progs/test_urandom_usdt.c b/tools/testing/selftests/bpf/progs/test_urandom_usdt.c
> index 3539b02bd5f7..575761863eb6 100644
> --- a/tools/testing/selftests/bpf/progs/test_urandom_usdt.c
> +++ b/tools/testing/selftests/bpf/progs/test_urandom_usdt.c
> @@ -67,4 +67,17 @@ int BPF_USDT(urandlib_read_with_sema, int iter_num, int iter_cnt, int buf_sz)
>         return 0;
>  }
>
> +int urandlib_xmm_reg_read_buf_sz_sum;

nit: empty line here

> +SEC("usdt/./liburandom_read_xmm.so:urandlib:xmm_reg_read")
> +int BPF_USDT(urandlib_xmm_reg_read, int *f1, int *f2, int *f3, int a, int b,
> +                                    int c /*should be float */, int d, int e,
> +                                    int f, int g, int h, int buf_sz)
> +{
> +       if (urand_pid != (bpf_get_current_pid_tgid() >> 32))
> +               return 0;
> +
> +       __sync_fetch_and_add(&urandlib_xmm_reg_read_buf_sz_sum, buf_sz);
> +       return 0;
> +}
> +

[...]
Alexei Starovoitov May 17, 2022, 1:17 a.m. UTC | #2 
On Mon, May 16, 2022 at 04:31:55PM -0700, Andrii Nakryiko wrote:
> On Thu, May 12, 2022 at 12:43 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
> >
> > Validate that bpf_get_reg_val helper solves the motivating problem of
> > this patch series: USDT args passed through xmm regs. The userspace
> > portion of the test forces STAP_PROBE macro to use %xmm0 and %xmm1 regs
> > to pass a float and an int, which the bpf-side successfully reads using
> > BPF_USDT.
> >
> > In the wild I discovered a sanely-configured USDT in Fedora libpthread
> > using xmm regs to pass scalar values, likely due to register pressure.
> > urandom_read_lib_xmm mimics this by using -ffixed-$REG flag to mark
> > r11-r14 unusable and passing many USDT args.
> >
> > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> > ---
> >  tools/testing/selftests/bpf/Makefile          |  8 ++-
> >  tools/testing/selftests/bpf/prog_tests/usdt.c |  7 +++
> >  .../selftests/bpf/progs/test_urandom_usdt.c   | 13 ++++
> >  tools/testing/selftests/bpf/urandom_read.c    |  3 +
> >  .../selftests/bpf/urandom_read_lib_xmm.c      | 62 +++++++++++++++++++
> >  5 files changed, 91 insertions(+), 2 deletions(-)
> >  create mode 100644 tools/testing/selftests/bpf/urandom_read_lib_xmm.c
> >
> > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > index 6bbc03161544..19246e34dfe1 100644
> > --- a/tools/testing/selftests/bpf/Makefile
> > +++ b/tools/testing/selftests/bpf/Makefile
> > @@ -172,10 +172,14 @@ $(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c
> >         $(call msg,LIB,,$@)
> >         $(Q)$(CC) $(CFLAGS) -fPIC $(LDFLAGS) $^ $(LDLIBS) --shared -o $@
> >
> > -$(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so
> > +$(OUTPUT)/liburandom_read_xmm.so: urandom_read_lib_xmm.c
> > +       $(call msg,LIB,,$@)
> > +       $(Q)$(CC) -O0 -ffixed-r11 -ffixed-r12 -ffixed-r13 -ffixed-r14 -fPIC $(LDFLAGS) $^ $(LDLIBS) --shared -o $@
> 
> this looks very x86-specific, but we support other architectures as well
> 
> looking at sdt.h, it seems like STAP_PROBEx() macros support being
> called from assembly code, I wonder if it would be better to try to
> figure out how to use it from assembly and use some xmm register
> directly in inline assembly? I have never done that before, but am
> hopeful :)

stap_probe in asm won't help cross arch issue.
It's better to stay with C.
Maybe some makefile magic can make the above x86 only?
The test should be skipped on other archs anyway.
Andrii Nakryiko May 18, 2022, 11:56 p.m. UTC | #3 
On Mon, May 16, 2022 at 6:17 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Mon, May 16, 2022 at 04:31:55PM -0700, Andrii Nakryiko wrote:
> > On Thu, May 12, 2022 at 12:43 AM Dave Marchevsky <davemarchevsky@fb.com> wrote:
> > >
> > > Validate that bpf_get_reg_val helper solves the motivating problem of
> > > this patch series: USDT args passed through xmm regs. The userspace
> > > portion of the test forces STAP_PROBE macro to use %xmm0 and %xmm1 regs
> > > to pass a float and an int, which the bpf-side successfully reads using
> > > BPF_USDT.
> > >
> > > In the wild I discovered a sanely-configured USDT in Fedora libpthread
> > > using xmm regs to pass scalar values, likely due to register pressure.
> > > urandom_read_lib_xmm mimics this by using -ffixed-$REG flag to mark
> > > r11-r14 unusable and passing many USDT args.
> > >
> > > Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>
> > > ---
> > >  tools/testing/selftests/bpf/Makefile          |  8 ++-
> > >  tools/testing/selftests/bpf/prog_tests/usdt.c |  7 +++
> > >  .../selftests/bpf/progs/test_urandom_usdt.c   | 13 ++++
> > >  tools/testing/selftests/bpf/urandom_read.c    |  3 +
> > >  .../selftests/bpf/urandom_read_lib_xmm.c      | 62 +++++++++++++++++++
> > >  5 files changed, 91 insertions(+), 2 deletions(-)
> > >  create mode 100644 tools/testing/selftests/bpf/urandom_read_lib_xmm.c
> > >
> > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > index 6bbc03161544..19246e34dfe1 100644
> > > --- a/tools/testing/selftests/bpf/Makefile
> > > +++ b/tools/testing/selftests/bpf/Makefile
> > > @@ -172,10 +172,14 @@ $(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c
> > >         $(call msg,LIB,,$@)
> > >         $(Q)$(CC) $(CFLAGS) -fPIC $(LDFLAGS) $^ $(LDLIBS) --shared -o $@
> > >
> > > -$(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so
> > > +$(OUTPUT)/liburandom_read_xmm.so: urandom_read_lib_xmm.c
> > > +       $(call msg,LIB,,$@)
> > > +       $(Q)$(CC) -O0 -ffixed-r11 -ffixed-r12 -ffixed-r13 -ffixed-r14 -fPIC $(LDFLAGS) $^ $(LDLIBS) --shared -o $@
> >
> > this looks very x86-specific, but we support other architectures as well
> >
> > looking at sdt.h, it seems like STAP_PROBEx() macros support being
> > called from assembly code, I wonder if it would be better to try to
> > figure out how to use it from assembly and use some xmm register
> > directly in inline assembly? I have never done that before, but am
> > hopeful :)
>
> stap_probe in asm won't help cross arch issue.
> It's better to stay with C.
> Maybe some makefile magic can make the above x86 only?
> The test should be skipped on other archs anyway.

It seems easier (or rather cleaner) to maintain arch-specific pieces
within .c file through #if __x86_64__ instead of doing equivalent
changes to Makefile. Suggestion for using assembly was due to needing
to make sure xmmX register is used and desired to avoid maintaining
arch-specific compile-time flags like -ffixed-r11. But truth be told I
have zero idea how STAP_PROBE() use from assembly would look like, so
just a suggestion to consider.
diff mbox series

Patch

diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 6bbc03161544..19246e34dfe1 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -172,10 +172,14 @@  $(OUTPUT)/liburandom_read.so: urandom_read_lib1.c urandom_read_lib2.c
 	$(call msg,LIB,,$@)
 	$(Q)$(CC) $(CFLAGS) -fPIC $(LDFLAGS) $^ $(LDLIBS) --shared -o $@
 
-$(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so
+$(OUTPUT)/liburandom_read_xmm.so: urandom_read_lib_xmm.c
+	$(call msg,LIB,,$@)
+	$(Q)$(CC) -O0 -ffixed-r11 -ffixed-r12 -ffixed-r13 -ffixed-r14 -fPIC $(LDFLAGS) $^ $(LDLIBS) --shared -o $@
+
+$(OUTPUT)/urandom_read: urandom_read.c urandom_read_aux.c $(OUTPUT)/liburandom_read.so $(OUTPUT)/liburandom_read_xmm.so
 	$(call msg,BINARY,,$@)
 	$(Q)$(CC) $(CFLAGS) $(LDFLAGS) $(filter %.c,$^)			       \
-		  liburandom_read.so $(LDLIBS)	       			       \
+		  liburandom_read.so liburandom_read_xmm.so $(LDLIBS)          \
 		  -Wl,-rpath=. -Wl,--build-id=sha1 -o $@
 
 $(OUTPUT)/bpf_testmod.ko: $(VMLINUX_BTF) $(wildcard bpf_testmod/Makefile bpf_testmod/*.[ch])
diff --git a/tools/testing/selftests/bpf/prog_tests/usdt.c b/tools/testing/selftests/bpf/prog_tests/usdt.c
index a71f51bdc08d..f98749ac74a7 100644
--- a/tools/testing/selftests/bpf/prog_tests/usdt.c
+++ b/tools/testing/selftests/bpf/prog_tests/usdt.c
@@ -385,6 +385,12 @@  static void subtest_urandom_usdt(bool auto_attach)
 			goto cleanup;
 		skel->links.urandlib_read_with_sema = l;
 
+		l = bpf_program__attach_usdt(skel->progs.urandlib_xmm_reg_read,
+					     urand_pid, "./liburandom_read_xmm.so",
+					     "urandlib", "xmm_reg_read", NULL);
+		if (!ASSERT_OK_PTR(l, "urandlib_xmm_reg_read"))
+			goto cleanup;
+		skel->links.urandlib_xmm_reg_read = l;
 	}
 
 	/* trigger urandom_read USDTs */
@@ -402,6 +408,7 @@  static void subtest_urandom_usdt(bool auto_attach)
 	ASSERT_EQ(bss->urandlib_read_with_sema_call_cnt, 1, "urandlib_w_sema_cnt");
 	ASSERT_EQ(bss->urandlib_read_with_sema_buf_sz_sum, 256, "urandlib_w_sema_sum");
 
+	ASSERT_EQ(bss->urandlib_xmm_reg_read_buf_sz_sum, 256, "liburandom_read_xmm.so");
 cleanup:
 	if (urand_pipe)
 		pclose(urand_pipe);
diff --git a/tools/testing/selftests/bpf/progs/test_urandom_usdt.c b/tools/testing/selftests/bpf/progs/test_urandom_usdt.c
index 3539b02bd5f7..575761863eb6 100644
--- a/tools/testing/selftests/bpf/progs/test_urandom_usdt.c
+++ b/tools/testing/selftests/bpf/progs/test_urandom_usdt.c
@@ -67,4 +67,17 @@  int BPF_USDT(urandlib_read_with_sema, int iter_num, int iter_cnt, int buf_sz)
 	return 0;
 }
 
+int urandlib_xmm_reg_read_buf_sz_sum;
+SEC("usdt/./liburandom_read_xmm.so:urandlib:xmm_reg_read")
+int BPF_USDT(urandlib_xmm_reg_read, int *f1, int *f2, int *f3, int a, int b,
+				     int c /*should be float */, int d, int e,
+				     int f, int g, int h, int buf_sz)
+{
+	if (urand_pid != (bpf_get_current_pid_tgid() >> 32))
+		return 0;
+
+	__sync_fetch_and_add(&urandlib_xmm_reg_read_buf_sz_sum, buf_sz);
+	return 0;
+}
+
 char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/urandom_read.c b/tools/testing/selftests/bpf/urandom_read.c
index e92644d0fa75..0ee7aad014e1 100644
--- a/tools/testing/selftests/bpf/urandom_read.c
+++ b/tools/testing/selftests/bpf/urandom_read.c
@@ -20,6 +20,8 @@  void urand_read_without_sema(int iter_num, int iter_cnt, int read_sz);
 /* these are coming from urandom_read_lib{1,2}.c */
 void urandlib_read_with_sema(int iter_num, int iter_cnt, int read_sz);
 void urandlib_read_without_sema(int iter_num, int iter_cnt, int read_sz);
+/* defined in urandom_read_lib_xmm.c */
+void urandlib_read_xmm_args(int iter_num, int iter_cnt, int read_sz);
 
 unsigned short urand_read_with_sema_semaphore SEC(".probes");
 
@@ -39,6 +41,7 @@  void urandom_read(int fd, int count)
 		/* trigger USDTs defined in shared lib */
 		urandlib_read_without_sema(i, count, BUF_SIZE);
 		urandlib_read_with_sema(i, count, BUF_SIZE);
+		urandlib_read_xmm_args(i, count, BUF_SIZE);
 	}
 }
 
diff --git a/tools/testing/selftests/bpf/urandom_read_lib_xmm.c b/tools/testing/selftests/bpf/urandom_read_lib_xmm.c
new file mode 100644
index 000000000000..d5f9c9cf74e7
--- /dev/null
+++ b/tools/testing/selftests/bpf/urandom_read_lib_xmm.c
@@ -0,0 +1,62 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2022 Meta Platforms, Inc. and affiliates. */
+
+#define STAP_SDT_ARG_CONSTRAINT norx
+/* For x86_64, this was changed from 'nor' default to 'norfxy' in systemtap
+ * commit eaa15b047 ("PR27829: Support floating point values passed through sdt.h markers")
+ * then changed to 'norx' in commit 1d3653936f ("sys/sdt.h fp constraints cont'd, x86-64 edition")
+ * It's not necessary to set STAP_SDT_ARG_CONSTRAINT for newer systemtap to see
+ * xmm regs used in this program
+ */
+
+#include "sdt.h"
+
+int *f1(void)
+{
+	return (int *)100;
+}
+
+int *f2(void)
+{
+	return (int *)200;
+}
+
+int *f3(void)
+{
+	return (int *)300;
+}
+
+/* Compile w/ -ffixed-r11 -ffixed-r12 -ffixed-r13 -ffixed-r14 -O0 */
+void urandlib_read_xmm_args(int iter_num, int iter_cnt, int read_sz)
+{
+	volatile int a, b, d, e, f, g, h, i;
+	a = b = d = e = f = g = h = 300;
+	i = read_sz;
+	volatile float c = 100;
+
+	STAP_PROBE12(urandlib, xmm_reg_read, f1(), f2(), f3(), a, b, c, d, e, f, g, h, i);
+}
+
+/*
+ * `readelf -n` results:
+ * On a test host outside of kernel toolchain (Ubuntu 20.04, 5.13.0-39-generic, gcc 9.4.0-1ubuntu1~20.04.1)
+ * w/ STAP_SDT_ARG_CONSTRAINT norfxy
+ * 	using system sdt.h:
+ * 		Provider: urandlib
+ * 		Name: xmm_reg_read
+ * 		Location: 0x00000000000011d8, Base: 0x0000000000002008, Semaphore: 0x0000000000000000
+ * 		Arguments: 8@%rbx 8@%r15 8@%xmm1 -4@%edx -4@%ecx 4@%xmm0 -4@%esi -4@%edi -4@%r8d -4@%r9d -4@%r10d -4@%eax
+ *
+ * 	Using a newer systemtap's sdt.h (commit acca4ae47 ("Don't run tls tests if debuginfo is missing")):
+ * 		Provider: urandlib
+ * 		Name: xmm_reg_read
+ * 		Location: 0x00000000000011d8, Base: 0x0000000000002008, Semaphore: 0x0000000000000000
+ * 		Arguments: 8@%rbx 8@%r15 8@%xmm1 -4@%edx -4@%ecx 4f@%xmm0 -4@%esi -4@%edi -4@%r8d -4@%r9d -4@%r10d -4@%eax
+ *
+ * Kernel toolchain:
+ * STAP_SDT_ARG_CONSTRAINT norfxy causes compiler error (due to the 'f'), so using 'norx'
+ * 		Provider: urandlib
+ * 		Name: xmm_reg_read
+ * 		Location: 0x0000000000000717, Base: 0x0000000000000738, Semaphore: 0x0000000000000000
+ * 		Arguments: 8@%rbx 8@%r15 8@-72(%rbp) -4@%edx -4@%ecx 4f@%xmm0 -4@%esi -4@%edi -4@%r8d -4@%r9d -4@%r10d -4@%xmm1
+ */