On Thu, Apr 4, 2024 at 5:48 AM Jamal Hadi Salim <jhs@xxxxxxxxxxxx> 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@xxxxxxxxxx> 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. So please drop all bpf integration and focus on pure p4tc.
