Re: [PATCH net-next v15 14/15] p4tc: add set of P4TC table kfuncs

[John Fastabend <john.fastabend@gmail.com>]

Daniel Borkmann wrote:
> On 4/8/24 2:19 PM, Jamal Hadi Salim wrote:
> > We add an initial set of kfuncs to allow interactions from eBPF programs
> > to the P4TC domain.
> > 
> > - bpf_p4tc_tbl_read: Used to lookup a table entry from a BPF
> > program installed in TC. To find the table entry we take in an skb, the
> > pipeline ID, the table ID, a key and a key size.
> > We use the skb to get the network namespace structure where all the
> > pipelines are stored. After that we use the pipeline ID and the table
> > ID, to find the table. We then use the key to search for the entry.
> > We return an entry on success and NULL on failure.
> > 
> > - xdp_p4tc_tbl_read: Used to lookup a table entry from a BPF
> > program installed in XDP. To find the table entry we take in an xdp_md,
> > the pipeline ID, the table ID, a key and a key size.
> > We use struct xdp_md to get the network namespace structure where all
> > the pipelines are stored. After that we use the pipeline ID and the table
> > ID, to find the table. We then use the key to search for the entry.
> > We return an entry on success and NULL on failure.
> > 
> > - bpf_p4tc_entry_create: Used to create a table entry from a BPF
> > program installed in TC. To create the table entry we take an skb, the
> > pipeline ID, the table ID, a key and its size, and an action which will
> > be associated with the new entry.
> > We return 0 on success and a negative errno on failure
> > 
> > - xdp_p4tc_entry_create: Used to create a table entry from a BPF
> > program installed in XDP. To create the table entry we take an xdp_md, the
> > pipeline ID, the table ID, a key and its size, and an action which will
> > be associated with the new entry.
> > We return 0 on success and a negative errno on failure
> > 
> > - bpf_p4tc_entry_create_on_miss: conforms to PNA "add on miss".
> > First does a lookup using the passed key and upon a miss will add the entry
> > to the table.
> > We return 0 on success and a negative errno on failure
> > 
> > - xdp_p4tc_entry_create_on_miss: conforms to PNA "add on miss".
> > First does a lookup using the passed key and upon a miss will add the entry
> > to the table.
> > We return 0 on success and a negative errno on failure
> > 
> > - bpf_p4tc_entry_update: Used to update a table entry from a BPF
> > program installed in TC. To update the table entry we take an skb, the
> > pipeline ID, the table ID, a key and its size, and an action which will
> > be associated with the new entry.
> > We return 0 on success and a negative errno on failure
> > 
> > - xdp_p4tc_entry_update: Used to update a table entry from a BPF
> > program installed in XDP. To update the table entry we take an xdp_md, the
> > pipeline ID, the table ID, a key and its size, and an action which will
> > be associated with the new entry.
> > We return 0 on success and a negative errno on failure
> > 
> > - bpf_p4tc_entry_delete: Used to delete a table entry from a BPF
> > program installed in TC. To delete the table entry we take an skb, the
> > pipeline ID, the table ID, a key and a key size.
> > We return 0 on success and a negative errno on failure
> > 
> > - xdp_p4tc_entry_delete: Used to delete a table entry from a BPF
> > program installed in XDP. To delete the table entry we take an xdp_md, the
> > pipeline ID, the table ID, a key and a key size.
> > We return 0 on success and a negative errno on failure
> > 
> > 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.
> > 
> > Co-developed-by: Victor Nogueira <victor@mojatatu.com>
> > Signed-off-by: Victor Nogueira <victor@mojatatu.com>
> > Co-developed-by: Pedro Tammela <pctammela@mojatatu.com>
> > Signed-off-by: Pedro Tammela <pctammela@mojatatu.com>
> > Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
> > Reviewed-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> > Nacked-by: Alexei Starovoitov <alexei.starovoitov@gmail.com>
> > Acked-by: Toke Høiland-Jørgensen <toke@redhat.com>
> 
> Given the many reasons stated earlier & for the record:
> 
> Nacked-by: Daniel Borkmann <daniel@iogearbox.net>
> 

Same for me. For reasons already given tldr,

 . p4 can be done already in xdp/tc with p4c backend
 . not clear how hardware offload would be done
 . shimming control path through 'tc' seems unnecessary
 . related kfuncs duplicate map operations already there
 . disagree with pipeline design e.g. single xdp.o is optimal
 . keeping control path in userspace will be more flexible.

Nacked-by: John Fastabend <john.fastabend@gmail.com>