Message ID | 160588912738.2817268.9380466634324530673.stgit@firesoul (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | BPF |
Headers | show |
Series | bpf: New approach for BPF MTU handling | expand |
Context | Check | Description |
---|---|---|
netdev/apply | fail | Patch does not apply to bpf-next |
netdev/tree_selection | success | Clearly marked for bpf-next |
On Fri, Nov 20, 2020 at 05:18:47PM +0100, Jesper Dangaard Brouer wrote: > Adding selftest for BPF-helper bpf_check_mtu(). Making sure > it can be used from both XDP and TC. > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > tools/testing/selftests/bpf/prog_tests/check_mtu.c | 37 ++++++++++++++++++++ > tools/testing/selftests/bpf/progs/test_check_mtu.c | 33 ++++++++++++++++++ > 2 files changed, 70 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/check_mtu.c > create mode 100644 tools/testing/selftests/bpf/progs/test_check_mtu.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/check_mtu.c b/tools/testing/selftests/bpf/prog_tests/check_mtu.c > new file mode 100644 > index 000000000000..09b8f986a17b > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/check_mtu.c > @@ -0,0 +1,37 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2020 Red Hat */ > +#include <uapi/linux/bpf.h> > +#include <linux/if_link.h> > +#include <test_progs.h> > + > +#include "test_check_mtu.skel.h" > +#define IFINDEX_LO 1 > + > +void test_check_mtu_xdp(struct test_check_mtu *skel) > +{ > + int err = 0; > + int fd; > + > + fd = bpf_program__fd(skel->progs.xdp_use_helper); > + err = bpf_set_link_xdp_fd(IFINDEX_LO, fd, XDP_FLAGS_SKB_MODE); > + if (CHECK_FAIL(err)) > + return; > + > + bpf_set_link_xdp_fd(IFINDEX_LO, -1, 0); > +} > + > +void test_check_mtu(void) > +{ > + struct test_check_mtu *skel; > + > + skel = test_check_mtu__open_and_load(); > + if (CHECK_FAIL(!skel)) { > + perror("test_check_mtu__open_and_load"); > + return; > + } > + > + if (test__start_subtest("bpf_check_mtu XDP-attach")) > + test_check_mtu_xdp(skel); > + > + test_check_mtu__destroy(skel); > +} > diff --git a/tools/testing/selftests/bpf/progs/test_check_mtu.c b/tools/testing/selftests/bpf/progs/test_check_mtu.c > new file mode 100644 > index 000000000000..ab97ec925a32 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_check_mtu.c > @@ -0,0 +1,33 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2020 Red Hat */ > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > + > +#include <stddef.h> > +#include <stdint.h> > + > +char _license[] SEC("license") = "GPL"; > + > +SEC("xdp") > +int xdp_use_helper(struct xdp_md *ctx) > +{ > + uint32_t mtu_len = 0; > + int delta = 20; > + > + if (bpf_check_mtu(ctx, 0, &mtu_len, delta, 0)) { > + return XDP_ABORTED; > + } > + return XDP_PASS; > +} > + > +SEC("classifier") > +int tc_use_helper(struct __sk_buff *ctx) > +{ > + uint32_t mtu_len = 0; > + int delta = -20; > + > + if (bpf_check_mtu(ctx, 0, &mtu_len, delta, 0)) { > + return BPF_DROP; > + } > + return BPF_OK; > +} Patches 7 and 8 are not adequate as tests. They do not testing the functionality of earlier patches. bpf_check_mtu() could be returning random value and "tests" 7 and 8 would still pass. Please fix.
On Fri, Nov 20, 2020 at 8:21 AM Jesper Dangaard Brouer <brouer@redhat.com> wrote: > > Adding selftest for BPF-helper bpf_check_mtu(). Making sure > it can be used from both XDP and TC. > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > --- > tools/testing/selftests/bpf/prog_tests/check_mtu.c | 37 ++++++++++++++++++++ > tools/testing/selftests/bpf/progs/test_check_mtu.c | 33 ++++++++++++++++++ > 2 files changed, 70 insertions(+) > create mode 100644 tools/testing/selftests/bpf/prog_tests/check_mtu.c > create mode 100644 tools/testing/selftests/bpf/progs/test_check_mtu.c > > diff --git a/tools/testing/selftests/bpf/prog_tests/check_mtu.c b/tools/testing/selftests/bpf/prog_tests/check_mtu.c > new file mode 100644 > index 000000000000..09b8f986a17b > --- /dev/null > +++ b/tools/testing/selftests/bpf/prog_tests/check_mtu.c > @@ -0,0 +1,37 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2020 Red Hat */ > +#include <uapi/linux/bpf.h> > +#include <linux/if_link.h> > +#include <test_progs.h> > + > +#include "test_check_mtu.skel.h" > +#define IFINDEX_LO 1 > + > +void test_check_mtu_xdp(struct test_check_mtu *skel) this should be static func, otherwise it's treated as an independent selftest. > +{ > + int err = 0; > + int fd; > + > + fd = bpf_program__fd(skel->progs.xdp_use_helper); > + err = bpf_set_link_xdp_fd(IFINDEX_LO, fd, XDP_FLAGS_SKB_MODE); > + if (CHECK_FAIL(err)) please use CHECK() or one of ASSERT_xxx() helpers. CHECK_FAIL() should be used for high-volume unlikely to fail test (i.e., very rarely). > + return; > + > + bpf_set_link_xdp_fd(IFINDEX_LO, -1, 0); > +} > + > +void test_check_mtu(void) > +{ > + struct test_check_mtu *skel; > + > + skel = test_check_mtu__open_and_load(); > + if (CHECK_FAIL(!skel)) { > + perror("test_check_mtu__open_and_load"); > + return; > + } > + > + if (test__start_subtest("bpf_check_mtu XDP-attach")) > + test_check_mtu_xdp(skel); > + > + test_check_mtu__destroy(skel); > +} > diff --git a/tools/testing/selftests/bpf/progs/test_check_mtu.c b/tools/testing/selftests/bpf/progs/test_check_mtu.c > new file mode 100644 > index 000000000000..ab97ec925a32 > --- /dev/null > +++ b/tools/testing/selftests/bpf/progs/test_check_mtu.c > @@ -0,0 +1,33 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* Copyright (c) 2020 Red Hat */ > +#include <linux/bpf.h> > +#include <bpf/bpf_helpers.h> > + > +#include <stddef.h> > +#include <stdint.h> > + > +char _license[] SEC("license") = "GPL"; > + > +SEC("xdp") > +int xdp_use_helper(struct xdp_md *ctx) > +{ > + uint32_t mtu_len = 0; > + int delta = 20; > + > + if (bpf_check_mtu(ctx, 0, &mtu_len, delta, 0)) { > + return XDP_ABORTED; > + } nit: unnecessary {} for single-line if; same below > + return XDP_PASS; > +} > + > +SEC("classifier") > +int tc_use_helper(struct __sk_buff *ctx) > +{ > + uint32_t mtu_len = 0; > + int delta = -20; > + > + if (bpf_check_mtu(ctx, 0, &mtu_len, delta, 0)) { > + return BPF_DROP; > + } > + return BPF_OK; > +} > >
On Fri, 20 Nov 2020 23:41:11 -0800 Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > On Fri, Nov 20, 2020 at 8:21 AM Jesper Dangaard Brouer > <brouer@redhat.com> wrote: > > > > Adding selftest for BPF-helper bpf_check_mtu(). Making sure > > it can be used from both XDP and TC. > > > > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> > > --- > > tools/testing/selftests/bpf/prog_tests/check_mtu.c | 37 ++++++++++++++++++++ > > tools/testing/selftests/bpf/progs/test_check_mtu.c | 33 ++++++++++++++++++ > > 2 files changed, 70 insertions(+) > > create mode 100644 tools/testing/selftests/bpf/prog_tests/check_mtu.c > > create mode 100644 tools/testing/selftests/bpf/progs/test_check_mtu.c > > > > diff --git a/tools/testing/selftests/bpf/prog_tests/check_mtu.c b/tools/testing/selftests/bpf/prog_tests/check_mtu.c > > new file mode 100644 > > index 000000000000..09b8f986a17b > > --- /dev/null > > +++ b/tools/testing/selftests/bpf/prog_tests/check_mtu.c > > @@ -0,0 +1,37 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* Copyright (c) 2020 Red Hat */ > > +#include <uapi/linux/bpf.h> > > +#include <linux/if_link.h> > > +#include <test_progs.h> > > + > > +#include "test_check_mtu.skel.h" > > +#define IFINDEX_LO 1 > > + > > +void test_check_mtu_xdp(struct test_check_mtu *skel) > > this should be static func, otherwise it's treated as an independent selftest. Ok, fixed. > > +{ > > + int err = 0; > > + int fd; > > + > > + fd = bpf_program__fd(skel->progs.xdp_use_helper); > > + err = bpf_set_link_xdp_fd(IFINDEX_LO, fd, XDP_FLAGS_SKB_MODE); > > + if (CHECK_FAIL(err)) > > please use CHECK() or one of ASSERT_xxx() helpers. CHECK_FAIL() should > be used for high-volume unlikely to fail test (i.e., very rarely). I could not get CHECK() macro working. I now realize that this is because I've not defined a global static variable named 'duration'. static __u32 duration; I wonder, are there any best-practice documentation or blogpost on howto write these bpf-selftests? Below signature is the compile error for others to Google for, and solution above. - Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer $ make TEST-OBJ [test_progs] check_mtu.test.o In file included from /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/prog_tests/check_mtu.c:6: /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/prog_tests/check_mtu.c: In function ‘test_check_mtu’: ./test_progs.h:129:25: error: ‘duration’ undeclared (first use in this function) 129 | _CHECK(condition, tag, duration, format) | ^~~~~~~~ ./test_progs.h:111:25: note: in definition of macro ‘_CHECK’ 111 | __func__, tag, duration); \ | ^~~~~~~~ /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/prog_tests/check_mtu.c:33:6: note: in expansion of macro ‘CHECK’ 33 | if (CHECK(!skel, "open and load skel", "failed")) | ^~~~~ ./test_progs.h:129:25: note: each undeclared identifier is reported only once for each function it appears in 129 | _CHECK(condition, tag, duration, format) | ^~~~~~~~ ./test_progs.h:111:25: note: in definition of macro ‘_CHECK’ 111 | __func__, tag, duration); \ | ^~~~~~~~ /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/prog_tests/check_mtu.c:33:6: note: in expansion of macro ‘CHECK’ 33 | if (CHECK(!skel, "open and load skel", "failed")) | ^~~~~ make: *** [Makefile:396: /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/check_mtu.test.o] Error 1
On 11/24/20 6:33 AM, Jesper Dangaard Brouer wrote: > On Fri, 20 Nov 2020 23:41:11 -0800 > Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote: > >> On Fri, Nov 20, 2020 at 8:21 AM Jesper Dangaard Brouer >> <brouer@redhat.com> wrote: >>> >>> Adding selftest for BPF-helper bpf_check_mtu(). Making sure >>> it can be used from both XDP and TC. >>> >>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> >>> --- >>> tools/testing/selftests/bpf/prog_tests/check_mtu.c | 37 ++++++++++++++++++++ >>> tools/testing/selftests/bpf/progs/test_check_mtu.c | 33 ++++++++++++++++++ >>> 2 files changed, 70 insertions(+) >>> create mode 100644 tools/testing/selftests/bpf/prog_tests/check_mtu.c >>> create mode 100644 tools/testing/selftests/bpf/progs/test_check_mtu.c >>> >>> diff --git a/tools/testing/selftests/bpf/prog_tests/check_mtu.c b/tools/testing/selftests/bpf/prog_tests/check_mtu.c >>> new file mode 100644 >>> index 000000000000..09b8f986a17b >>> --- /dev/null >>> +++ b/tools/testing/selftests/bpf/prog_tests/check_mtu.c >>> @@ -0,0 +1,37 @@ >>> +// SPDX-License-Identifier: GPL-2.0 >>> +/* Copyright (c) 2020 Red Hat */ >>> +#include <uapi/linux/bpf.h> >>> +#include <linux/if_link.h> >>> +#include <test_progs.h> >>> + >>> +#include "test_check_mtu.skel.h" >>> +#define IFINDEX_LO 1 >>> + >>> +void test_check_mtu_xdp(struct test_check_mtu *skel) >> >> this should be static func, otherwise it's treated as an independent selftest. > > Ok, fixed. > >>> +{ >>> + int err = 0; >>> + int fd; >>> + >>> + fd = bpf_program__fd(skel->progs.xdp_use_helper); >>> + err = bpf_set_link_xdp_fd(IFINDEX_LO, fd, XDP_FLAGS_SKB_MODE); >>> + if (CHECK_FAIL(err)) >> >> please use CHECK() or one of ASSERT_xxx() helpers. CHECK_FAIL() should >> be used for high-volume unlikely to fail test (i.e., very rarely). > > I could not get CHECK() macro working. I now realize that this is > because I've not defined a global static variable named 'duration'. > > static __u32 duration; > > I wonder, are there any best-practice documentation or blogpost on > howto write these bpf-selftests? The 'duration' in old days is used to measure performance. Today most of selftests actually do not need this. We do not have doc/blogpost for this. The best is to look at other files under prog_tests to see how they handle duration ... > > > Below signature is the compile error for others to Google for, and > solution above. > - > Best regards, > Jesper Dangaard Brouer > MSc.CS, Principal Kernel Engineer at Red Hat > LinkedIn: http://www.linkedin.com/in/brouer > > > $ make > TEST-OBJ [test_progs] check_mtu.test.o > In file included from /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/prog_tests/check_mtu.c:6: > /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/prog_tests/check_mtu.c: In function ‘test_check_mtu’: > ./test_progs.h:129:25: error: ‘duration’ undeclared (first use in this function) > 129 | _CHECK(condition, tag, duration, format) > | ^~~~~~~~ > ./test_progs.h:111:25: note: in definition of macro ‘_CHECK’ > 111 | __func__, tag, duration); \ > | ^~~~~~~~ > /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/prog_tests/check_mtu.c:33:6: note: in expansion of macro ‘CHECK’ > 33 | if (CHECK(!skel, "open and load skel", "failed")) > | ^~~~~ > ./test_progs.h:129:25: note: each undeclared identifier is reported only once for each function it appears in > 129 | _CHECK(condition, tag, duration, format) > | ^~~~~~~~ > ./test_progs.h:111:25: note: in definition of macro ‘_CHECK’ > 111 | __func__, tag, duration); \ > | ^~~~~~~~ > /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/prog_tests/check_mtu.c:33:6: note: in expansion of macro ‘CHECK’ > 33 | if (CHECK(!skel, "open and load skel", "failed")) > | ^~~~~ > make: *** [Makefile:396: /home/jbrouer/git/kernel/bpf-next/tools/testing/selftests/bpf/check_mtu.test.o] Error 1 >
diff --git a/tools/testing/selftests/bpf/prog_tests/check_mtu.c b/tools/testing/selftests/bpf/prog_tests/check_mtu.c new file mode 100644 index 000000000000..09b8f986a17b --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/check_mtu.c @@ -0,0 +1,37 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2020 Red Hat */ +#include <uapi/linux/bpf.h> +#include <linux/if_link.h> +#include <test_progs.h> + +#include "test_check_mtu.skel.h" +#define IFINDEX_LO 1 + +void test_check_mtu_xdp(struct test_check_mtu *skel) +{ + int err = 0; + int fd; + + fd = bpf_program__fd(skel->progs.xdp_use_helper); + err = bpf_set_link_xdp_fd(IFINDEX_LO, fd, XDP_FLAGS_SKB_MODE); + if (CHECK_FAIL(err)) + return; + + bpf_set_link_xdp_fd(IFINDEX_LO, -1, 0); +} + +void test_check_mtu(void) +{ + struct test_check_mtu *skel; + + skel = test_check_mtu__open_and_load(); + if (CHECK_FAIL(!skel)) { + perror("test_check_mtu__open_and_load"); + return; + } + + if (test__start_subtest("bpf_check_mtu XDP-attach")) + test_check_mtu_xdp(skel); + + test_check_mtu__destroy(skel); +} diff --git a/tools/testing/selftests/bpf/progs/test_check_mtu.c b/tools/testing/selftests/bpf/progs/test_check_mtu.c new file mode 100644 index 000000000000..ab97ec925a32 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_check_mtu.c @@ -0,0 +1,33 @@ +// SPDX-License-Identifier: GPL-2.0 +/* Copyright (c) 2020 Red Hat */ +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> + +#include <stddef.h> +#include <stdint.h> + +char _license[] SEC("license") = "GPL"; + +SEC("xdp") +int xdp_use_helper(struct xdp_md *ctx) +{ + uint32_t mtu_len = 0; + int delta = 20; + + if (bpf_check_mtu(ctx, 0, &mtu_len, delta, 0)) { + return XDP_ABORTED; + } + return XDP_PASS; +} + +SEC("classifier") +int tc_use_helper(struct __sk_buff *ctx) +{ + uint32_t mtu_len = 0; + int delta = -20; + + if (bpf_check_mtu(ctx, 0, &mtu_len, delta, 0)) { + return BPF_DROP; + } + return BPF_OK; +}
Adding selftest for BPF-helper bpf_check_mtu(). Making sure it can be used from both XDP and TC. Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com> --- tools/testing/selftests/bpf/prog_tests/check_mtu.c | 37 ++++++++++++++++++++ tools/testing/selftests/bpf/progs/test_check_mtu.c | 33 ++++++++++++++++++ 2 files changed, 70 insertions(+) create mode 100644 tools/testing/selftests/bpf/prog_tests/check_mtu.c create mode 100644 tools/testing/selftests/bpf/progs/test_check_mtu.c