| Message ID | 20190201001954.4130-6-maciej.fijalkowski@intel.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | xdp: Avoid unloading xdp prog not attached by sample | expand |
On Fri, 1 Feb 2019 01:19:51 +0100, Maciej Fijalkowski wrote: > In order to provide more meaningful messages to user when the process of > loading xdp program onto network interface failed, let's add extack > messages within dev_change_xdp_fd. > > Suggested-by: Jakub Kicinski <jakub.kicinski@netronome.com> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
On Fri, 1 Feb 2019 01:19:51 +0100, Maciej Fijalkowski wrote: > if (__dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG) || > - __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW)) > + __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW)) { > + NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time"); > return -EEXIST; > + } This reminds me, since we allowed native/driver and offloaded XDP programs to coexist in a25717d2b604 ("xdp: support simultaneous driver and hw XDP attachment") I got an internal feature request to also allow generic and native mode. Would anyone object to that? Apart from a touch up to test_offload.py I don't think anything would care. netlink can already carry multiple IDs, iproute2 understands it, too.. (Obviously as a follow up after this set gets merged.)
On Thu, 31 Jan 2019 19:11:01 -0800 Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > On Fri, 1 Feb 2019 01:19:51 +0100, Maciej Fijalkowski wrote: > > if (__dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG) || > > - __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW)) > > + __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW)) { > > + NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time"); > > return -EEXIST; > > + } > > This reminds me, since we allowed native/driver and offloaded XDP > programs to coexist in a25717d2b604 ("xdp: support simultaneous > driver and hw XDP attachment") I got an internal feature request > to also allow generic and native mode. Would anyone object to that? Well, I will object ;-) I have two refactor ideas [1] and [2], that depend on not allowing XDP-native and XDP-generic to co-exist. The general idea is to let XDP-native use the same fields in net_device->rx[] as XDP-generic given they (currently) cannot co-exist. The goal is (1) to move stuff out of driver code, and (2) hopefully make it easier to implement per RXq XDP progs. 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 ? [1] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp_per_rxq01.org#refactor-idea-move-xdp_rxq_info-to-net_devicenetdev_rx_queue [2] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp_per_rxq01.org#refactor-idea-xdpbpf_prog-into-netdev_rx_queuenet_device > Apart from a touch up to test_offload.py I don't think anything > would care. netlink can already carry multiple IDs, iproute2 > understands it, too.. And we did notice you added support for HW+native: [3] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp_per_rxq01.org
On Fri, 1 Feb 2019 01:19:51 +0100 Maciej Fijalkowski <maciejromanfijalkowski@gmail.com> wrote: > In order to provide more meaningful messages to user when the process of > loading xdp program onto network interface failed, let's add extack > messages within dev_change_xdp_fd. > > Suggested-by: Jakub Kicinski <jakub.kicinski@netronome.com> > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> Acked-by: Jesper Dangaard Brouer <brouer@redhat.com>
On Fri, 1 Feb 2019 08:02:36 +0100, Jesper Dangaard Brouer wrote: > On Thu, 31 Jan 2019 19:11:01 -0800 > Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > > On Fri, 1 Feb 2019 01:19:51 +0100, Maciej Fijalkowski wrote: > > > if (__dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG) || > > > - __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW)) > > > + __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW)) { > > > + NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time"); > > > return -EEXIST; > > > + } > > > > This reminds me, since we allowed native/driver and offloaded XDP > > programs to coexist in a25717d2b604 ("xdp: support simultaneous > > driver and hw XDP attachment") I got an internal feature request > > to also allow generic and native mode. Would anyone object to that? > > Well, I will object ;-) > > I have two refactor ideas [1] and [2], that depend on not allowing > XDP-native and XDP-generic to co-exist. The general idea is to let > XDP-native use the same fields in net_device->rx[] as XDP-generic given > they (currently) cannot co-exist. > The goal is (1) to move stuff out of driver code, and (2) hopefully > make it easier to implement per RXq XDP progs. You mean you'd use one pointer to keep the prog in the RXQ structure? Then some from from of an extra flag will be necessary to distinguish? I.e.: if (rxq->prog && rxq->is_native) /* got_prog */ rather than: if (rxq->native_prog) /* got_prog */ The cost of this reuse would be a read-only cache line per-q when XDP is not enabled. Right now drivers have the ability to pack the XDP prog into a structure which is in cache already, and don't need to bring the entire RXQ structure out (which is cache line aligned so driver authors can't do anything to place it cleverly). No doubt, thought, that if we allow both to be enabled we will have to bloat the data structures. > 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 :) > [1] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp_per_rxq01.org#refactor-idea-move-xdp_rxq_info-to-net_devicenetdev_rx_queue > > [2] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp_per_rxq01.org#refactor-idea-xdpbpf_prog-into-netdev_rx_queuenet_device > > > Apart from a touch up to test_offload.py I don't think anything > > would care. netlink can already carry multiple IDs, iproute2 > > understands it, too.. > > And we did notice you added support for HW+native: > [3] https://github.com/xdp-project/xdp-project/blob/master/areas/core/xdp_per_rxq01.org
On 02/01/2019 07:47 PM, Jakub Kicinski wrote: [...] >> 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? :) 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. Thanks, Daniel
On Fri, 1 Feb 2019 22:33:22 +0100, Daniel Borkmann wrote: > On 02/01/2019 07:47 PM, Jakub Kicinski wrote: > >> 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?
On 02/01/2019 10:44 PM, Jakub Kicinski wrote: [...] > 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? Yeah that looks good to me, lets do it. At least this would allow for prototyping in combination with HW mode till full support of a feature has landed properly in native mode. Thanks, Daniel
diff --git a/net/core/dev.c b/net/core/dev.c index 8e276e0192a1..bfa4be42afff 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -7983,8 +7983,10 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, query = flags & XDP_FLAGS_HW_MODE ? 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))) + if (!bpf_op && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE))) { + NL_SET_ERR_MSG(extack, "underlying driver does not support XDP in native mode"); return -EOPNOTSUPP; + } if (!bpf_op || (flags & XDP_FLAGS_SKB_MODE)) bpf_op = generic_xdp_install; if (bpf_op == bpf_chk) @@ -7992,11 +7994,15 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, if (fd >= 0) { if (__dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG) || - __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW)) + __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG_HW)) { + NL_SET_ERR_MSG(extack, "native and generic XDP can't be active at the same time"); return -EEXIST; + } if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && - __dev_xdp_query(dev, bpf_op, query)) + __dev_xdp_query(dev, bpf_op, query)) { + NL_SET_ERR_MSG(extack, "XDP program already attached"); return -EBUSY; + } prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, bpf_op == ops->ndo_bpf);
In order to provide more meaningful messages to user when the process of loading xdp program onto network interface failed, let's add extack messages within dev_change_xdp_fd. Suggested-by: Jakub Kicinski <jakub.kicinski@netronome.com> Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- net/core/dev.c | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-)