On 4/22/21 11:08 AM, Toke Høiland-Jørgensen wrote:
Daniel Borkmann <daniel@xxxxxxxxxxxxx> writes:
On 4/21/21 9:48 PM, Toke Høiland-Jørgensen wrote:
Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> writes:
On Tue, Apr 20, 2021 at 12:37 PM Kumar Kartikeya Dwivedi
<memxor@xxxxxxxxx> wrote:
[...]
---
tools/lib/bpf/libbpf.h | 44 ++++++
tools/lib/bpf/libbpf.map | 3 +
tools/lib/bpf/netlink.c | 319 ++++++++++++++++++++++++++++++++++++++-
3 files changed, 360 insertions(+), 6 deletions(-)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index bec4e6a6e31d..b4ed6a41ea70 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -16,6 +16,8 @@
#include <stdbool.h>
#include <sys/types.h> // for size_t
#include <linux/bpf.h>
+#include <linux/pkt_sched.h>
+#include <linux/tc_act/tc_bpf.h>
apart from those unused macros below, are these needed in public API header?
#include "libbpf_common.h"
@@ -775,6 +777,48 @@ LIBBPF_API int bpf_linker__add_file(struct bpf_linker *linker, const char *filen
LIBBPF_API int bpf_linker__finalize(struct bpf_linker *linker);
LIBBPF_API void bpf_linker__free(struct bpf_linker *linker);
+/* Convenience macros for the clsact attach hooks */
+#define BPF_TC_CLSACT_INGRESS TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_INGRESS)
+#define BPF_TC_CLSACT_EGRESS TC_H_MAKE(TC_H_CLSACT, TC_H_MIN_EGRESS)
these seem to be used only internally, why exposing them in public
API?
No they're "aliases" for when you want to attach the filter directly to
the interface (and thus install the clsact qdisc as the root). You can
also use the filter with an existing qdisc (most commonly HTB), in which
case you need to specify the qdisc handle as the root. We have a few
examples of this use case:
https://github.com/xdp-project/bpf-examples/tree/master/traffic-pacing-edt
and
https://github.com/xdp-project/xdp-cpumap-tc
I'm a bit puzzled, could you elaborate on your use case on why you wouldn't
use the tc egress hook for those especially given it's guaranteed to run
outside of root qdisc lock?
Jesper can correct me if I'm wrong, but I think the first one of the
links above is basically his implementation of just that EDT-based
shaper. And it works reasonably well, except you don't get the nice
per-flow scheduling and sparse flow prioritisation like in FQ-CoDel
unless you implement that yourself in BPF when you set the timestamps
(and that is by no means trivial to implement).
So if you want to use any of the features of the existing qdiscs (I have
also been suggesting to people that they use tc_bpf if they want to
customise sch_cake's notion of flows or shaping tiers), you need to be
able to attach the filter to an existing qdisc. Sure, this means you're
still stuck behind the qdisc lock, but for some applications that is
fine (not everything is a data centre, some devices don't have that many
CPUs anyway; and as the second example above shows, you can get around
the qdisc lock by some clever use of partitioning of flows using
cpumap).
So what this boils down to is, we should keep the 'parent' parameter not
just as an egress/ingress enum, but also as a field the user can fill
out. I'm fine with moving the latter into the opts struct, though, so
maybe the function parameter can be an enum like:
enum bpf_tc_attach_point {
BPF_TC_CLSACT_INGRESS,
BPF_TC_CLSACT_EGRESS,
BPF_TC_QDISC_PARENT
};
where if you set the last one you have to fill in the parent in opts?
Fair enough, I still think this is a bit backwards and should be discouraged
given the constraints, but if you have an actual need for it ... I'd rather
simplify API naming, the fact that it's clsact is an implementation detail
and shouldn't matter to a user, like:
enum bpf_tc_attach_point {
BPF_TC_INGRESS,
BPF_TC_EGRESS,
BPF_TC_CUSTOM_PARENT,
};
For BPF_TC_INGRESS and BPF_TC_EGRESS, I would enforce opts parent parameter
to be /zero/ from the API.
Thanks,
Daniel
