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?
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?
Thanks,
Daniel
