Re: [PATCH bpf-next v2 4/8] xsk: register XDP sockets at bind(), and add new AF_XDP BPF helper

On 2021-01-20 15:54, Toke Høiland-Jørgensen wrote:

Björn Töpel [off-list ref] writes:

quoted

On 2021-01-20 13:50, Toke Høiland-Jørgensen wrote:

quoted

Björn Töpel [off-list ref] writes:

quoted

diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index c001766adcbc..bbc7d9a57262 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h

@@ -3836,6 +3836,12 @@ union bpf_attr {
    *	Return
    *		A pointer to a struct socket on success or NULL if the file is
    *		not a socket.
+ *
+ * long bpf_redirect_xsk(struct xdp_buff *xdp_md, u64 action)
+ *	Description
+ *		Redirect to the registered AF_XDP socket.
+ *	Return
+ *		**XDP_REDIRECT** on success, otherwise the action parameter is returned.
    */

I think it would be better to make the second argument a 'flags'
argument and make values > XDP_TX invalid (like we do in
bpf_xdp_redirect_map() now). By allowing any value as return you lose
the ability to turn it into a flags argument later...

Yes, but that adds a run-time check. I prefer this non-checked version,
even though it is a bit less futureproof.

That...seems a bit short-sighted? :)
Can you actually see a difference in your performance numbers?

I would rather add an additional helper *if* we see the need for flags,
instead of paying for that upfront. For me, BPF is about being able to
specialize, and not having one call with tons of checks.

(Related; Going forward, the growing switch() for redirect targets in
xdp_do_redirect() is a concern for me...)

And yes, even with all those fancy branch predictors, less instructions
is still less. :-) (It shows in my ubenchs.)


Björn