Date:   Thu, 27 Oct 2022 03:01:32 -0600
From:   Daniel Xu <dxu@...uu.xyz>
To:     Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     Andrii Nakryiko <andrii.nakryiko@...il.com>,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Stanislav Fomichev <sdf@...gle.com>, bpf <bpf@...r.kernel.org>,
        Nikolay Aleksandrov <razor@...ckwall.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Andrii Nakryiko <andrii@...nel.org>,
        Martin KaFai Lau <martin.lau@...ux.dev>,
        John Fastabend <john.fastabend@...il.com>,
        Joanne Koong <joannelkoong@...il.com>,
        Kumar Kartikeya Dwivedi <memxor@...il.com>,
        Joe Stringer <joe@...ium.io>,
        Network Development <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf-next 01/10] bpf: Add initial fd-based API to attach
 tc BPF programs

Hi Alexei,

On Fri, Oct 14, 2022 at 08:38:52AM -0700, Alexei Starovoitov wrote:
> On Thu, Oct 13, 2022 at 11:30 AM Andrii Nakryiko
> <andrii.nakryiko@...il.com> wrote:
> >

[...]

> > No one proposed a slight variation on what Daniel was proposing with
> > prios that might work just as well. So for completeness, what if
> > instead of specifying 0 or explicit prio, we allow specifying either:
> >   - explicit prio, and if that prio is taken -- fail
> >   - min_prio, and kernel will find smallest untaken prio >= min_prio;
> > we can also define that min_prio=-1 means append as the very last one.
> >
> > So if someone needs to be the very first -- explicitly request prio=1.
> > If wants to be last: prio=0, min_prio=-1. If we want to observe, we
> > can do something like min_prio=50 to leave a bunch of slots free for
> > some other programs for which exact order matters.
> 
> Daniel, was suggesting more or less the same thing.
> My point is that prio is an unnecessary concept and uapi
> will be stuck with it. Including query interface
> and bpftool printing it.

I apologize if I'm piling onto the bikeshedding, but I've been working a
lot more with TC bpf lately so I thought I'd offer some thoughts.

I quite like the intent of this patchset; it'll help simply using TC bpf
greatly. I also think what Andrii is suggesting makes a lot of sense. My
biggest gripe with TC priorities is that:

1. "Priority" is a rather arbitrary concept and hard to come up with
values for.

2. The default replace-on-collision semantic (IIRC) is error prone as
evidenced by this patch's motivation.

My suggestion here is to rename "priority" -> "position". Maybe it's
just me but I think "priority" is too vague of a concept whereas a
0-indexed "position" is rather unambiguous.

> 
> > This whole before/after FD interface seems a bit hypothetical as well,
> > tbh.
> 
> The fd approach is not better. It's not more flexible.
> That was not the point.
> The point is that fd does not add stuff to uapi that
> bpftool has to print and later the user has to somehow interpret.
> prio is that magic number that users would have to understand,
> but for them it's meaningless. The users want to see the order
> of progs on query and select the order on attach.

While I appreciate how the FD based approach leaves less confusing
values for bpftool to dump, I see a small semantic ambiguity with it:

For example, say we start with a single prog, A. Then add B as
"after-A".  Then add C as "before-B". It's unclear what'll happen.
Either you invalidate B's guarantees or you return an error. If you
invalidate, that's unfortunate. If you error, how does userspace retry?
You'd have to express all the existing relationships to the user thru
through bpftool or something. Whereas with Andrii's proposal it's
unambiguous.

[...]

Thanks,
Daniel

Powered by blists - more mailing lists