Message-ID: <87wn9egx3d.fsf@toke.dk>
Date:   Wed, 05 Oct 2022 16:32:22 +0200
From:   Toke Høiland-Jørgensen <toke@...hat.com>
To:     Daniel Borkmann <daniel@...earbox.net>, bpf@...r.kernel.org
Cc:     razor@...ckwall.org, ast@...nel.org, andrii@...nel.org,
        martin.lau@...ux.dev, john.fastabend@...il.com,
        joannelkoong@...il.com, memxor@...il.com, joe@...ium.io,
        netdev@...r.kernel.org
Subject: Re: [PATCH bpf-next 01/10] bpf: Add initial fd-based API to attach
 tc BPF programs

Daniel Borkmann <daniel@...earbox.net> writes:

> On 10/5/22 12:33 PM, Toke Høiland-Jørgensen wrote:
>> Daniel Borkmann <daniel@...earbox.net> writes:
>> 
>>> As part of the feedback from LPC, there was a suggestion to provide a
>>> name for this infrastructure to more easily differ between the classic
>>> cls_bpf attachment and the fd-based API. As for most, the XDP vs tc
>>> layer is already the default mental model for the pkt processing
>>> pipeline. We refactored this with an xtc internal prefix aka 'express
>>> traffic control' in order to avoid to deviate too far (and 'express'
>>> given its more lightweight/faster entry point).
>> 
>> Woohoo, bikeshed time! :)
>> 
>> I am OK with having a separate name for this, but can we please pick one
>> that doesn't sound like 'XDP' when you say it out loud? You really don't
>> have to mumble much for 'XDP' and 'XTC' to sound exactly alike; this is
>> bound to lead to confusion!
>> 
>> Alternatives, in the same vein:
>> - ltc (lightweight)
>> - etc (extended/express/ebpf/et cetera ;))
>> - tcx (keep the cool X, but put it at the end)
>
> Hehe, yeah agree, I don't have a strong opinion, but tcx (or just sticking
> with tc) is fully okay to me.

Either is fine with me; I don't have any strong opinions either, other
than "not XTC" ;)

>> [...]
>> 
>>> +/* (Simplified) user return codes for tc prog type.
>>> + * A valid tc program must return one of these defined values. All other
>>> + * return codes are reserved for future use. Must remain compatible with
>>> + * their TC_ACT_* counter-parts. For compatibility in behavior, unknown
>>> + * return codes are mapped to TC_NEXT.
>>> + */
>>> +enum tc_action_base {
>>> +	TC_NEXT		= -1,
>>> +	TC_PASS		= 0,
>>> +	TC_DROP		= 2,
>>> +	TC_REDIRECT	= 7,
>>> +};
>> 
>> Looking at things like this, though, I wonder if having a separate name
>> (at least if it's too prominent) is not just going to be more confusing
>> than not? I.e., we go out of our way to make it compatible with existing
>> TC-BPF programs (which is a good thing!), so do we really need a
>> separate name? Couldn't it just be an implementation detail that "it's
>> faster now"?
>
> Yep, faster is an implementation detail; and developers can stick to existing
> opcodes. I added this here given Andrii suggested to add the action codes as
> enum so they land in vmlinux BTF. My thinking was that if we go this route,
> we could also make them more user friendly. This part is 100% optional,
> but for new developers it might lower the barrier a bit I was hoping given
> it makes it clear which subset of actions BPF supports explicitly and with
> less cryptic name.

Oh, I didn't mean that we shouldn't define these helpers; that's totally
fine, and probably useful. Just that when everything is named 'TC'
anyway, having a different name (like TCX) is maybe not that important
anyway?

>> Oh, and speaking of compatibility should 'tc' (the iproute2 binary) be
>> taught how to display these new bpf_link attachments so that users can
>> see that they're there?
>
> Sounds reasonable, I can follow-up with the iproute2 support as well.

Cool!

-Toke

Powered by blists - more mailing lists