Message-ID: <CAM0EoM=SyHR-f7z8YVRknXrUsKALgx96eH-hBudo40NyeaxuoA@mail.gmail.com>
Date: Thu, 4 Apr 2024 18:59:33 -0400
From: Jamal Hadi Salim <jhs@...atatu.com>
To: Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc: Network Development <netdev@...r.kernel.org>, deb.chatterjee@...el.com, 
	Anjali Singhai Jain <anjali.singhai@...el.com>, namrata.limaye@...el.com, tom@...anda.io, 
	Marcelo Ricardo Leitner <mleitner@...hat.com>, Mahesh.Shirshyad@....com, Vipin.Jain@....com, 
	tomasz.osinski@...el.com, Jiri Pirko <jiri@...nulli.us>, 
	Cong Wang <xiyou.wangcong@...il.com>, "David S. Miller" <davem@...emloft.net>, 
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Vlad Buslov <vladbu@...dia.com>, Simon Horman <horms@...nel.org>, khalidm@...dia.com, 
	Toke Høiland-Jørgensen <toke@...hat.com>, 
	Daniel Borkmann <daniel@...earbox.net>, Martin KaFai Lau <martin.lau@...ux.dev>, victor@...atatu.com, 
	Pedro Tammela <pctammela@...atatu.com>, bpf <bpf@...r.kernel.org>
Subject: Re: [PATCH net-next v14 14/15] p4tc: add set of P4TC table kfuncs

On Thu, Apr 4, 2024 at 6:07 PM Alexei Starovoitov
<alexei.starovoitov@...il.com> wrote:
>
> On Thu, Apr 4, 2024 at 5:48 AM Jamal Hadi Salim <jhs@...atatu.com> wrote:
> >
> > We add an initial set of kfuncs to allow interactions from eBPF programs
> > to the P4TC domain.
>
> ...
>
> > Note:
> > All P4 objects are owned and reside on the P4TC side. IOW, they are
> > controlled via TC netlink interfaces and their resources are managed
> > (created, updated, freed, etc) by the TC side. As an example, the structure
> > p4tc_table_entry_act is returned to the ebpf side on table lookup. On the
> > TC side that struct is wrapped around p4tc_table_entry_act_bpf_kern.
> > A multitude of these structure p4tc_table_entry_act_bpf_kern are
> > preallocated (to match the P4 architecture, patch #9 describes some of
> > the subtleties involved) by the P4TC control plane and put in a kernel
> > pool. Their purpose is to hold the action parameters for either a table
> > entry, a global per-table "miss" and "hit" action, etc - which are
> > instantiated and updated via netlink per runtime requests. An instance of
> > the p4tc_table_entry_act_bpf_kern.p4tc_table_entry_act is returned
> > to ebpf when there is a un/successful table lookup depending on how the
> > P4 program is written. When the table entry is deleted the instance of
> > the struct p4tc_table_entry_act_bpf_kern is recycled to the pool to be
> > reused for a future table entry. The only time the pool memory is released
> > is when the pipeline is deleted.
>
> TLDR:
> Nacked-by: Alexei Starovoitov <ast@...nel.org>
>
> Please drop this patch (all p4tc kfuncs) then I can lift
> the nack and the rest will be up to Kuba/Dave to decide.
> I agree with earlier comments from John and Daniel that
> p4tc is duplicating existing logic and doesn't provide
> any additional value to the kernel, but TC has a bunch
> of code already that no one is using, so
> another 13k lines of such stuff doesn't make it much worse.
>
> What I cannot tolerate is this p4tc <-> bpf integration.
> It breaks all the normal layering and programming from bpf pov.
> Martin explained this earlier. You cannot introduce new
> objects that are instantiated by TC, yet read/write-able
> from xdp and act_bpf that act like a bpf map, but not being a map.
> Performance overhead of these kfuncs is too high.
> It's not what the XDP community is used to.
> We cannot give users such a footgun.
>
> Furthermore, act_bpf is something that we added during early
> days and it turned out to be useless. It's still
> there, it's working and supported, but it's out of place
> and in the wrong spot in the pipeline to be useful
> beyond simple examples.
> Yet, p4tc tries to integrate with act_bpf. It's a red flag
> and a sign that that none of this code saw any practical
> application beyond demo code.
> We made our fair share of design mistakes. We learned our lessons
> and not going to repeat the same ones. This one is obvious.
>

It's the same old aggression from you "if you dont do it the way i
would have done it then it is wrong". Maybe it's how Meta or Cilium do
things, but that imposition on other people is just plain rude.
I have heard you shit on tc many times and it's not just tc everything
else sucks if it is not from you. It's your prerogative. Sorry, we are
not going to use tcx because it doesnt suit our goals. We are going to
use tc actions and filters. You do you, we'll do us.

You've been going around telling people they dont have to post any
kfunc to the ebpf mailing but it is still preferred to upstream.
That's what we are doing.
You do realize we can make this kfunc an out of tree kernel module but
we opted to share with the community.
 We initially posted a solution that didnt use ebpf and the push back
was to use ebpf. I know you act like ebpf is your property but it is
not. We will use ebpf/xdp/kfuncs whenever they make sense to us. Sigh.
This community is getting more political by the day. Open source is
about scratching your itch - this is our itch.

cheers,
jamal

> So please drop all bpf integration and focus on pure p4tc.

Powered by blists - more mailing lists