Re: Co-existing XDP generic and native mode? (Re: [PATCH bpf-next v5 5/8] xdp: Provide extack messages when prog attachment failed)
From: Jakub Kicinski <hidden>
Date: 2019-02-01 21:44:11
Subsystem:
networking [general], the rest · Maintainers:
"David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Linus Torvalds
On Fri, 1 Feb 2019 22:33:22 +0100, Daniel Borkmann wrote:
On 02/01/2019 07:47 PM, Jakub Kicinski wrote:quoted
quoted
These are only refactor ideas, so if you can argue why your internal feature request for simultaneous generic and native make more sense, then I'm open for allowing this ?The request was actually to enable xdpoffload and xdpgeneric at the same time. I'm happy to have that as another HW offload exclusive for now :)The latter is probably fine, though what's the concrete use case? :)
I think it was more of a "I expected this to work, since driver+offload worked" than a feature request. Looking back at it it was filed as bug I converted it to feature.
Reason we kept native vs generic separate is mainly so that native XDP drivers are discouraged to punt missing features to generic hook instead of properly implementing them in native mode.
Agreed, although one could make a counter argument that the performance should be a strong enough incentive and we shouldn't stop people for experimenting and prototyping :) The code change looks simple enough:
diff --git a/net/core/dev.c b/net/core/dev.c
index 8e276e0192a1..ce4880e5e95d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c@@ -7976,11 +7976,13 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, enum bpf_netdev_command query; struct bpf_prog *prog = NULL; bpf_op_t bpf_op, bpf_chk; + bool offload; int err; ASSERT_RTNL(); - query = flags & XDP_FLAGS_HW_MODE ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG; + offload = flags & XDP_FLAGS_HW_MODE; + query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG; bpf_op = bpf_chk = ops->ndo_bpf; if (!bpf_op && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE)))
@@ -7991,8 +7993,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, bpf_chk = generic_xdp_install; if (fd >= 0) { - if (__dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG) || - __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW)) + if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) return -EEXIST; if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && __dev_xdp_query(dev, bpf_op, query))
@@ -8003,8 +8004,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, if (IS_ERR(prog)) return PTR_ERR(prog); - if (!(flags & XDP_FLAGS_HW_MODE) && - bpf_prog_is_dev_bound(prog->aux)) { + if (!offload && bpf_prog_is_dev_bound(prog->aux)) { NL_SET_ERR_MSG(extack, "using device-bound program without HW_MODE flag is not supported"); bpf_prog_put(prog); return -EINVAL;
Do you think we shouldn't do it?