On Thu, Jan 25, 2024 at 12:59 PM Jamal Hadi Salim <jhs@xxxxxxxxxxxx> wrote: > > On Thu, Jan 25, 2024 at 10:47 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > > > On 1/24/24 3:40 PM, Jamal Hadi Salim wrote: > > > On Wed, Jan 24, 2024 at 8:59 AM Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote: > > >> On 1/22/24 8:48 PM, Jamal Hadi Salim wrote: > > [...] > > >>> > > >>> It should also be noted that it is feasible to split some of the ingress > > >>> datapath into XDP first and more into TC later (as was shown above for > > >>> example where the parser runs at XDP level). YMMV. > > >>> Regardless of choice of which scheme to use, none of these will affect > > >>> UAPI. It will all depend on whether you generate code to load on XDP vs > > >>> tc, etc. > > >>> > > >>> Co-developed-by: Victor Nogueira <victor@xxxxxxxxxxxx> > > >>> Signed-off-by: Victor Nogueira <victor@xxxxxxxxxxxx> > > >>> Co-developed-by: Pedro Tammela <pctammela@xxxxxxxxxxxx> > > >>> Signed-off-by: Pedro Tammela <pctammela@xxxxxxxxxxxx> > > >>> Signed-off-by: Jamal Hadi Salim <jhs@xxxxxxxxxxxx> > > >> > > >> My objections from last iterations still stand, and I also added a nak, > > >> so please do not just drop it with new revisions.. from the v10 as you > > >> wrote you added further code but despite the various community feedback > > >> the design still stands as before, therefore: > > >> > > >> Nacked-by: Daniel Borkmann <daniel@xxxxxxxxxxxxx> > > > > > > We didnt make code changes - but did you read the cover letter and the > > > extended commentary in this patch's commit log? We should have > > > mentioned it in the changes log. It did respond to your comments. > > > There's text that says "the filter manages the lifetime of the > > > pipeline" - which in the future could include not only tc but XDP but > > > also the hardware path (in the form of a file that gets loaded). I am > > > not sure if that message is clear. Your angle being this is layer > > > violation. In the last discussion i asked you for suggestions and we > > > went the tcx route, which didnt make sense, and then you didnt > > > respond. > > [...] > > > > >> Also as mentioned earlier I don't think tc should hold references on > > >> XDP programs in here. It doesn't make any sense aside from the fact > > >> that the cls_p4 is also not doing anything with it. This is something > > >> that a user space control plane should be doing i.e. managing a XDP > > >> link on the target device. > > > > > > This is the same argument about layer violation that you made earlier. > > > The filter manages the p4 pipeline - i.e it's not just about the ebpf > > > blob(s) but for example in the future (discussions are still ongoing > > > with vendors who have P4 NICs) a filter could be loaded to also > > > specify the location of the hardware blob. > > > > Ah, so there is a plan to eventually add HW offload support for cls_p4? > > Or is this only specifiying a location of a blob through some opaque > > cookie value from user space? > > Current thought process is it will be something along these lines (the > commit provides more details): > > tc filter add block 22 ingress protocol all prio 1 p4 pname simple_l3 \ > prog type hw filename "mypnameprog.o" ... \ > prog type xdp obj $PARSER.o section parser/xdp pinned_link > /sys/fs/bpf/mylink \ > action bpf obj $PROGNAME.o section prog/tc-ingress > > These discussions are still ongoing - but that is the current > consensus. Note: we are not pushing any code for that, but hope it > paints the bigger picture.... > The idea is the cls p4 owns the lifetime of the pipeline. Installing > the filter instantiates the p4 pipeline "simple_l3" and triggers a lot > of the refcounts to make sure the pipeline and its components stays > alive. > There could be multiple such filters - when someone deletes the last > filter, then it is safe to delete the pipeline. > Essentially the filter manages the lifetime of the pipeline. > > > > I would be happy with a suggestion that gets us moving forward with > > > that context in mind. > > > > My question on the above is mainly what does it bring you to hold a > > reference on the XDP program? There is no guarantee that something else > > will get loaded onto XDP, and then eventually the cls_p4 is the only > > entity holding the reference but w/o 'purpose'. We do have BPF links > > and the user space component orchestrating all this needs to create > > and pin the BPF link in BPF fs, for example. An artificial reference > > on XDP prog feels similar as if you'd hold a reference on an inode > > out of tc.. Again, that should be delegated to the control plane you > > have running interacting with the compiler which then manages and > > loads its artifacts. What if you would also need to set up some > > netfilter rules for the SW pipeline, would you then embed this too? > > Sorry, a slight tangent first: > P4 is self-contained, there are a handful of objects that are defined > by the spec (externs, actions, tables, etc) and we model them in the > patchset, so that part is self-contained. For the extra richness such > as the netfilter example you quoted - based on my many years of > experience deploying SDN - using daemons(sorry if i am reading too > much in what I think you are implying) for control is not the best > option i.e you need all kinds of coordination - for example where do > you store state, what happens when the daemon dies, how do you > graceful restarts etc. Based on that, if i can put things in the > kernel (which is essentially a "perpetual daemon", unless the kernel > crashes) it's a lot simpler to manage as a source of truth especially > when there is not that much info. There is a limit when there are > multiple pieces (to use your netfilter example) because you need > another layer to coordinate things. > > Re: the XDP part - our key reason is mostly managerial, in that the > filter is the lifetime manager of the pipeline; and that if i dump > that filter i can see all the details in regards to the pipeline(tc, > XDP and in future hw, etc) in one spot. You are right, the link > pinning is our protection from someone replacing the XDP prog (this > was a tip from Toke in the early days) and the comparison of tc > holding inode is apropos. > There's some history: in the early days we were also using metadata > which comes from the XDP program at the tc layer if more processing > was to be done (and there was extra metadata which told us which XDP > prog produced it which we would vet before trusting the metadata). > Given all the above, we should still be able to hold this info without > necessarily holding the extra refcount and be able to see this detail. > So we can remove the refcounting. > Daniel? cheers, jamal > cheers, > jamal > > > Thanks, > > Daniel
