| Message ID | 20210325120020.236504-6-memxor@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | libbpf: Add TC-BPF API | expand |
On Thu, Mar 25, 2021 at 05:30:03PM +0530, Kumar Kartikeya Dwivedi wrote: > This adds some basic tests for the low level bpf_tc_* API and its > bpf_program__attach_tc_* wrapper on top. *_block() apis from patch 3 and 4 are not covered by this selftest. Why were they added ? And how were they tested? Pls trim your cc. bpf@vger and netdev@vger would have been enough. My main concern with this set is that it adds netlink apis to libbpf while we already agreed to split xdp manipulation pieces out of libbpf. It would be odd to add tc apis now only to split them later. I think it's better to start with new library for tc/xdp and have libbpf as a dependency on that new lib. For example we can add it as subdir in tools/lib/bpf/. Similarly I think integerating static linking into libbpf was a mistake. It should be a sub library as well. If we end up with core libbpf and ten sublibs for tc, xdp, af_xdp, linking, whatever else the users would appreciate that we don't shove single libbpf to them with a ton of features that they might never use.
Alexei Starovoitov <alexei.starovoitov@gmail.com> writes: > On Thu, Mar 25, 2021 at 05:30:03PM +0530, Kumar Kartikeya Dwivedi wrote: >> This adds some basic tests for the low level bpf_tc_* API and its >> bpf_program__attach_tc_* wrapper on top. > > *_block() apis from patch 3 and 4 are not covered by this selftest. > Why were they added ? And how were they tested? > > Pls trim your cc. bpf@vger and netdev@vger would have been enough. > > My main concern with this set is that it adds netlink apis to libbpf while > we already agreed to split xdp manipulation pieces out of libbpf. > It would be odd to add tc apis now only to split them later. We're not removing the ability to attach an XDP program via netlink from libxdp, though. This is the equivalent for TC: the minimum support to attach a program, and if you want to do more, you pull in another library or roll your own. I'm fine with cutting out more stuff and making this even more minimal (e.g., remove the block stuff and only support attach/detach on ifaces), but we figured we'd err on the side of including too much and getting some feedback from others on which bits are the essential ones to keep, and which can be dropped. > I think it's better to start with new library for tc/xdp and have > libbpf as a dependency on that new lib. > For example we can add it as subdir in tools/lib/bpf/. I agree for the higher-level stuff (though I'm not sure what that would be for TC), but right now TC programs are the only ones that cannot be attached by libbpf, which is annoying; that's what we're trying to fix. -Toke
On Sat, Mar 27, 2021 at 09:32:58PM -0700, Andrii Nakryiko wrote: > > I think it's better to start with new library for tc/xdp and have > > libbpf as a dependency on that new lib. > > For example we can add it as subdir in tools/lib/bpf/. > > > > Similarly I think integerating static linking into libbpf was a mistake. > > It should be a sub library as well. > > > > If we end up with core libbpf and ten sublibs for tc, xdp, af_xdp, linking, > > whatever else the users would appreciate that we don't shove single libbpf > > to them with a ton of features that they might never use. > > What's the concern exactly? The size of the library? Having 10 > micro-libraries has its own set of downsides, specifically? > I'm not convinced that's > a better situation for end users. And would certainly cause more > hassle for libbpf developers and packagers. For developers and packagers.. yes. For users.. quite the opposite. The skel gen and static linking must be split out before the next libbpf release. Not a single application linked with libbpf is going to use those pieces. bpftool is one and only that needs them. Hence forcing libbpf users to increase their .text with a dead code is a selfish call of libbpf developers and packagers. The user's priorities must come first. > And what did you include in "core libbpf"? I would take this opportunity to split libbpf into maintainable pieces: - libsysbpf - sys_bpf wrappers (pretty much tools/lib/bpf/bpf.c) - libbpfutil - hash, strset - libbtf - BTF read/write - libbpfelf - ELF parsing, CORE, ksym, kconfig - libbpfskel - skeleton gen used by bpftool only - libbpflink - linker used by bpftool only - libbpfnet - networking attachment via netlink including TC and XDP - libbpftrace - perfbuf, ringbuf - libxdp - Toke's xdp chaining - libxsk - af_xdp logic In the future the stack trace symbolization code can come into libbpftrace or be a part of its own lib. My upcoming loader program and signed prog generation logic can be part of libbpfskel.
On Sun, Mar 28, 2021 at 07:38:42PM -0700, Andrii Nakryiko wrote: > > See above. I don't know which hassle is libbpf for users today. You > were implying code size used for functionality users might not use > (e.g., linker). Libbpf is a very small library, <300KB. There are > users building tools for constrained embedded systems that use libbpf. > There are/were various problems mentioned, but the size of libbpf > wasn't yet one of them. We should certainly watch the code bloat, but > we are not yet at the point where library is too big for users to be > turned off. It's true that today sizeof(libbpf + libelf + libz) ~ 500k is not a concern. I'm worried what it will get to if we don't start splitting things up. Why split libxdp into its own lib? If tc attach is going to part of libbpf all things xdp should be part of libbpf as well. But af_xdp folks are probably annoyed that they need to add -lelf an -lz though they're not using them. Just a tech debt that eventually need to be paid. > > I would take this opportunity to split libbpf into maintainable pieces: > > - libsysbpf - sys_bpf wrappers (pretty much tools/lib/bpf/bpf.c) > > - libbpfutil - hash, strset > > strset and hash are internal data structures, I never intended to > expose them through public APIs. I haven't investigated, but if we > have a separate shared library (libbpfutil), I imagine we won't be > able to hide those APIs, right? In the other thread you've proposed to copy paste hash implemenation into pahole. That's not ideal. If we had libbpfutil other projects could have used that without copy-paste. > But again, let's just reflect for a second that we are talking about > the library that takes less than 300KB total. that's today. Plus mandatory libelf and libz. I would like to have libsysbpf that doesn't depend on libelf/libz for folks that don't need it. Also I'd like to see symbolizer to be included in "libbpf package". Currently it's the main component that libbcc offers, but libbpf doesn't. Say we don't split libbpf. Then symbolizer will bring some dwarf library (say libdwarves ~ 1Mbyte) and libiberty ~ 500k (for c++ demangle). Now we're looking at multi megabyte libbpf package. I think the users would benefit from smaller building blocks. Splitting into 10 mini libs is overkill, of course, but some split is necessary. I agree that moving static linking into separate lib won't really affect .text size. The point is not to reduce text, but to establish a framework where such things are possible. Then symbolizer and anything fancier that would depend on other libs can be part of "libbpf package". I mean single rpm that contains all libbpf libs. Basic libsysbpf wouldn't need libelf/z. libbpfsymbolizer would need libdwarf, etc. Maybe some libbpfnet would depend on libnl or what not.
diff --git a/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c new file mode 100644 index 000000000000..8bab56b4dea0 --- /dev/null +++ b/tools/testing/selftests/bpf/prog_tests/test_tc_bpf.c @@ -0,0 +1,261 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bpf.h> +#include <linux/err.h> +#include <bpf/libbpf.h> +#include <errno.h> +#include <stdio.h> +#include <stdlib.h> +#include <test_progs.h> +#include <linux/if_ether.h> + +#define LO_IFINDEX 1 + +static int test_tc_cls_internal(int fd, __u32 parent_id) +{ + struct bpf_tc_cls_attach_id id = {}; + struct bpf_tc_cls_info info = {}; + int ret; + DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, .handle = 1, .priority = 10, + .class_id = TC_H_MAKE(1UL << 16, 1), + .chain_index = 5); + + ret = bpf_tc_cls_attach_dev(fd, LO_IFINDEX, parent_id, ETH_P_IP, &opts, + &id); + if (CHECK_FAIL(ret < 0)) + return ret; + + ret = bpf_tc_cls_get_info_dev(fd, LO_IFINDEX, parent_id, ETH_P_IP, NULL, + &info); + if (CHECK_FAIL(ret < 0)) + goto end; + + ret = -1; + + if (CHECK_FAIL(info.id.ifindex != id.ifindex) || + CHECK_FAIL(info.id.parent_id != id.parent_id) || + CHECK_FAIL(info.id.handle != id.handle) || + CHECK_FAIL(info.id.protocol != id.protocol) || + CHECK_FAIL(info.id.chain_index != id.chain_index) || + CHECK_FAIL(info.id.priority != id.priority) || + CHECK_FAIL(info.id.ifindex != LO_IFINDEX) || + CHECK_FAIL(info.id.parent_id != parent_id) || + CHECK_FAIL(info.id.handle != 1) || + CHECK_FAIL(info.id.priority != 10) || + CHECK_FAIL(info.id.protocol != ETH_P_IP) || + CHECK_FAIL(info.class_id != TC_H_MAKE(1UL << 16, 1)) || + CHECK_FAIL(info.id.chain_index != 5)) + goto end; + + opts.direct_action = true; + ret = bpf_tc_cls_replace_dev(fd, id.ifindex, id.parent_id, id.protocol, + &opts, &id); + if (CHECK_FAIL(ret < 0)) + return ret; + +end:; + ret = bpf_tc_cls_detach_dev(&id); + CHECK_FAIL(ret < 0); + return ret; +} + +static int test_tc_cls(struct bpf_program *prog, __u32 parent_id) +{ + struct bpf_tc_cls_info info = {}; + struct bpf_link *link; + int ret; + DECLARE_LIBBPF_OPTS(bpf_tc_cls_opts, opts, .priority = 10, .handle = 1, + .class_id = TC_H_MAKE(1UL << 16, 1)); + + link = bpf_program__attach_tc_cls_dev(prog, LO_IFINDEX, parent_id, + ETH_P_ALL, &opts); + if (CHECK_FAIL(IS_ERR_OR_NULL(link))) + return PTR_ERR(link); + + ret = bpf_tc_cls_get_info_dev(bpf_program__fd(prog), LO_IFINDEX, + parent_id, ETH_P_ALL, NULL, &info); + if (CHECK_FAIL(ret < 0)) + goto end; + + ret = -1; + + if (CHECK_FAIL(info.id.ifindex != LO_IFINDEX) || + CHECK_FAIL(info.id.handle != 1) || + CHECK_FAIL(info.id.priority != 10) || + CHECK_FAIL(info.id.protocol != ETH_P_ALL) || + CHECK_FAIL(info.class_id != TC_H_MAKE(1UL << 16, 1))) + goto end; + + /* Demonstrate changing attributes (e.g. to direct action) */ + opts.class_id = TC_H_MAKE(1UL << 16, 2); + opts.direct_action = true; + + /* Disconnect as we drop to the lower level API, which invalidates the + * link. + */ + bpf_link__disconnect(link); + + ret = bpf_tc_cls_change_dev(bpf_program__fd(prog), info.id.ifindex, + info.id.parent_id, info.id.protocol, &opts, + &info.id); + if (CHECK_FAIL(ret < 0)) + goto end; + + ret = bpf_tc_cls_get_info_dev(bpf_program__fd(prog), info.id.ifindex, + info.id.parent_id, info.id.protocol, NULL, + &info); + if (CHECK_FAIL(ret < 0)) + goto end; + + ret = -1; + + if (CHECK_FAIL(info.class_id != TC_H_MAKE(1UL << 16, 2))) + goto end; + if (CHECK_FAIL((info.bpf_flags & TCA_BPF_FLAG_ACT_DIRECT) != 1)) + goto end; + + ret = bpf_tc_cls_detach_dev(&info.id); + if (CHECK_FAIL(ret < 0)) + goto end; + +end: + ret = bpf_link__destroy(link); + CHECK_FAIL(ret < 0); + return ret; +} + +static int test_tc_act_internal(int fd) +{ + struct bpf_tc_act_info info = {}; + __u32 index = 0; + int ret; + DECLARE_LIBBPF_OPTS(bpf_tc_act_opts, opts, 0); + + ret = bpf_tc_act_attach(fd, &opts, &index); + if (CHECK_FAIL(ret < 0 || !index)) + goto end; + + index = 0; + ret = bpf_tc_act_attach(fd, &opts, &index); + if (CHECK_FAIL(ret < 0 || !index)) + goto end; + + opts.index = 3; + index = 0; + ret = bpf_tc_act_attach(fd, &opts, &index); + if (CHECK_FAIL(ret < 0 || !index)) + goto end; + + index = 0; + ret = bpf_tc_act_replace(fd, &opts, &index); + if (CHECK_FAIL(ret < 0 || !index)) + goto end; + + opts.index = 1; + ret = bpf_tc_act_attach(fd, &opts, &index); + if (CHECK_FAIL(!ret || ret != -EEXIST)) { + ret = -1; + goto end; + } + + for (int i = 0; i < 3; i++) { + memset(&info, 0, sizeof(info)); + + ret = bpf_tc_act_get_info(fd, &info); + if (CHECK_FAIL(ret < 0 && ret != -ESRCH)) + goto end; + + if (CHECK_FAIL(ret == -ESRCH)) + goto end; + + if (CHECK_FAIL(info.refcnt != 1)) + goto end; + + ret = bpf_tc_act_detach(info.index); + if (CHECK_FAIL(ret < 0)) + goto end; + } + + CHECK_FAIL(bpf_tc_act_get_info(fd, &info) == -ESRCH); + +end: + ret = bpf_tc_act_detach(0); + CHECK_FAIL(ret < 0); + return ret; +} + +static int test_tc_act(struct bpf_program *prog) +{ + struct bpf_tc_act_info info = {}; + struct bpf_link *link; + int ret; + DECLARE_LIBBPF_OPTS(bpf_tc_act_opts, opts, .index = 42); + + link = bpf_program__attach_tc_act(prog, &opts); + if (CHECK_FAIL(IS_ERR_OR_NULL(link))) + return PTR_ERR(link); + + ret = bpf_tc_act_get_info(bpf_program__fd(prog), &info); + if (CHECK_FAIL(ret < 0)) + goto end; + + if (CHECK_FAIL(info.index != 42)) + goto end; + +end: + ret = bpf_link__destroy(link); + CHECK_FAIL(ret < 0); + return ret; +} + +void test_test_tc_bpf(void) +{ + const char *file = "./test_tc_bpf_kern.o"; + int cls_fd, act_fd, ret; + struct bpf_program *clsp, *actp; + struct bpf_object *obj; + + obj = bpf_object__open(file); + if (CHECK_FAIL(IS_ERR_OR_NULL(obj))) + return; + + clsp = bpf_object__find_program_by_title(obj, "classifier"); + if (CHECK_FAIL(IS_ERR_OR_NULL(clsp))) + goto end; + + actp = bpf_object__find_program_by_title(obj, "action"); + if (CHECK_FAIL(IS_ERR_OR_NULL(clsp))) + goto end; + + ret = bpf_object__load(obj); + if (CHECK_FAIL(ret < 0)) + goto end; + + cls_fd = bpf_program__fd(clsp); + act_fd = bpf_program__fd(actp); + + if (CHECK_FAIL(system("tc qdisc add dev lo clsact"))) + goto end; + + ret = test_tc_cls_internal(cls_fd, BPF_TC_CLSACT_INGRESS); + if (CHECK_FAIL(ret < 0)) + goto end; + + ret = test_tc_cls(clsp, BPF_TC_CLSACT_EGRESS); + if (CHECK_FAIL(ret < 0)) + goto end; + + system("tc qdisc del dev lo clsact"); + + ret = test_tc_act_internal(act_fd); + if (CHECK_FAIL(ret < 0)) + goto end; + + ret = test_tc_act(actp); + if (CHECK_FAIL(ret < 0)) + goto end; + +end: + bpf_object__close(obj); + return; +} diff --git a/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c new file mode 100644 index 000000000000..d39644ea0fd7 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/test_tc_bpf_kern.c @@ -0,0 +1,18 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include <linux/bpf.h> +#include <bpf/bpf_helpers.h> + +// Dummy prog to test tc_bpf API + +SEC("classifier") +int cls(struct __sk_buff *skb) +{ + return 0; +} + +SEC("action") +int act(struct __sk_buff *skb) +{ + return 0; +}