Re: [PATCH bpf-next 3/5] libbpf: add low level TC-BPF API

Daniel Borkmann [off-list ref] writes:

On 3/30/21 10:39 PM, Andrii Nakryiko wrote:

quoted

On Sun, Mar 28, 2021 at 1:11 AM Kumar Kartikeya Dwivedi
[off-list ref] wrote:

quoted

On Sun, Mar 28, 2021 at 10:12:40AM IST, Andrii Nakryiko wrote:

quoted

Is there some succinct but complete enough documentation/tutorial/etc
that I can reasonably read to understand kernel APIs provided by TC
(w.r.t. BPF, of course). I'm trying to wrap my head around this and
whether API makes sense or not. Please share links, if you have some.

Hi Andrii,

Unfortunately for the kernel API part, I couldn't find any when I was working
on this. So I had to read the iproute2 tc code (tc_filter.c, f_bpf.c,
m_action.c, m_bpf.c) and the kernel side bits (cls_api.c, cls_bpf.c, act_api.c,
act_bpf.c) to grok anything I didn't understand. There's also similar code in
libnl (lib/route/{act,cls}.c).

Other than that, these resources were useful (perhaps you already went through
some/all of them):

https://docs.cilium.io/en/latest/bpf/#tc-traffic-control
https://qmonnet.github.io/whirl-offload/2020/04/11/tc-bpf-direct-action/
tc(8), and tc-bpf(8) man pages

I hope this is helpful!

Thanks! I'll take a look. Sorry, I'm a bit behind with all the stuff,
trying to catch up.

I was just wondering if it would be more natural instead of having
_dev _block variants and having to specify __u32 ifindex, __u32
parent_id, __u32 protocol, to have some struct specifying TC
"destination"? Maybe not, but I thought I'd bring this up early. So
you'd have just bpf_tc_cls_attach(), and you'd so something like

bpf_tc_cls_attach(prog_fd, TC_DEV(ifindex, parent_id, protocol))

or

bpf_tc_cls_attach(prog_fd, TC_BLOCK(block_idx, protocol))

? Or it's taking it too far?

But even if not, I think detaching can be unified between _dev and
_block, can't it?

Do we even need the _block variant? I would rather prefer to take the chance
and make it as simple as possible, and only iff really needed extend with
other APIs, for example:

   bpf_tc_attach(prog_fd, ifindex, {INGRESS,EGRESS});

Internally, this will create the sch_clsact qdisc & cls_bpf filter instance
iff not present yet, and attach to a default prio 1 handle 1, and _always_ in
direct-action mode. This is /as simple as it gets/ and we don't need to bother
users with more complex tc/cls_bpf internals unless desired. For example,
extended APIs could add prio/parent so that multi-prog can be attached to a
single cls_bpf instance, but even that could be a second step, imho.

While I'm all for simplifying where possible, the question becomes at
what level? I.e., we initially figured we'd expose (most of) the netlink
API in the low-level API (patch 3 in the series) and then have the
bpf_program__* level API be the simple "just attach" one...

We could simplify the low-level one further, of course, for instance by
getting rid of the block stuff entirely, but I don't see much value in
leaving out the support for prio/parent in the bpf_tc_cls_* - we'd have
to make the API extensible so it could be added later anyway, so why not
just include it from the get-go (especially as Kumar has already written
the code?)

-Toke