Message ID | 20170616235746.16337-3-jakub.kicinski@netronome.com |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On 06/17/2017 01:57 AM, Jakub Kicinski wrote: > Add an installation-time flag for requesting that the program > be installed only if it can be offloaded to HW. > > Internally new command for ndo_xdp is added, this way we avoid > putting checks into drivers since they all return -EINVAL on > an unknown command. > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> [...] > diff --git a/net/core/dev.c b/net/core/dev.c > index a04db264aa1c..05cec8e2cd82 100644 > --- a/net/core/dev.c > +++ b/net/core/dev.c > @@ -6959,7 +6959,10 @@ static int dev_xdp_install(struct net_device *dev, xdp_op_t xdp_op, > struct netdev_xdp xdp; > > memset(&xdp, 0, sizeof(xdp)); > - xdp.command = XDP_SETUP_PROG; > + if (flags & XDP_FLAGS_HW_MODE) > + xdp.command = XDP_SETUP_PROG_HW; > + else > + xdp.command = XDP_SETUP_PROG; > xdp.extack = extack; > xdp.flags = flags; > xdp.prog = prog; One thing I'm not sure I follow is that while you pass flags to the ndo in patch 1, add a new XDP_SETUP_PROG_HW command here in patch 2 based on the flags, and later on in patch 6, you don't really make use of it, but look at the flags anyway? Then, why adding separate XDP_SETUP_PROG_HW in the first place? [patch 6:] @@ -3338,6 +3341,7 @@ static int nfp_net_xdp(struct net_device *netdev, struct netdev_xdp *xdp) switch (xdp->command) { case XDP_SETUP_PROG: + case XDP_SETUP_PROG_HW: return nfp_net_xdp_setup(nn, xdp->prog, xdp->flags, xdp->extack);
On Tue, 20 Jun 2017 00:55:41 +0200, Daniel Borkmann wrote: > On 06/17/2017 01:57 AM, Jakub Kicinski wrote: > > Add an installation-time flag for requesting that the program > > be installed only if it can be offloaded to HW. > > > > Internally new command for ndo_xdp is added, this way we avoid > > putting checks into drivers since they all return -EINVAL on > > an unknown command. > > > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> > [...] > > diff --git a/net/core/dev.c b/net/core/dev.c > > index a04db264aa1c..05cec8e2cd82 100644 > > --- a/net/core/dev.c > > +++ b/net/core/dev.c > > @@ -6959,7 +6959,10 @@ static int dev_xdp_install(struct net_device *dev, xdp_op_t xdp_op, > > struct netdev_xdp xdp; > > > > memset(&xdp, 0, sizeof(xdp)); > > - xdp.command = XDP_SETUP_PROG; > > + if (flags & XDP_FLAGS_HW_MODE) > > + xdp.command = XDP_SETUP_PROG_HW; > > + else > > + xdp.command = XDP_SETUP_PROG; > > xdp.extack = extack; > > xdp.flags = flags; > > xdp.prog = prog; > > One thing I'm not sure I follow is that while you pass flags to the ndo > in patch 1, add a new XDP_SETUP_PROG_HW command here in patch 2 based on > the flags, and later on in patch 6, you don't really make use of it, but > look at the flags anyway? Then, why adding separate XDP_SETUP_PROG_HW > in the first place? > > [patch 6:] > @@ -3338,6 +3341,7 @@ static int nfp_net_xdp(struct net_device *netdev, struct netdev_xdp *xdp) > > switch (xdp->command) { > case XDP_SETUP_PROG: > + case XDP_SETUP_PROG_HW: > return nfp_net_xdp_setup(nn, xdp->prog, xdp->flags, > xdp->extack); We still need the flags to be able to differentiate between default/no flags case where we load to the driver and the HW ("both"), and when the DRV_MODE flag is set, in which case we disable the HW offload and only load to the driver. We have three cases: drv offload no flag yes attempted DRV_MODE yes no HW_MODE no yes The XDP_SETUP_PROG_HW command is purely for convenience of drivers without an offload. I felt it's not appropriate to burden all drivers with: if (xdp->flags & XDP_FLAGS_HW_MODE) return -EOPNOTSUPP; But, I do have a patch which does it, so I'm happy to drop the new command if it's preferred.
On 06/20/2017 01:24 AM, Jakub Kicinski wrote: [...] > The XDP_SETUP_PROG_HW command is purely for convenience of drivers > without an offload. I felt it's not appropriate to burden all drivers > with: > > if (xdp->flags & XDP_FLAGS_HW_MODE) > return -EOPNOTSUPP; > > But, I do have a patch which does it, so I'm happy to drop the new > command if it's preferred. Ahh, that makes sense, yep. I was only focused on reviewing this in the context of nfp driver. Lack of coffee. ;)
On 06/17/2017 01:57 AM, Jakub Kicinski wrote: > Add an installation-time flag for requesting that the program > be installed only if it can be offloaded to HW. > > Internally new command for ndo_xdp is added, this way we avoid > putting checks into drivers since they all return -EINVAL on > an unknown command. > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> Acked-by: Daniel Borkmann <daniel@iogearbox.net>
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index b194817631de..a838591aad28 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -807,6 +807,7 @@ enum xdp_netdev_command { * when it is no longer used. */ XDP_SETUP_PROG, + XDP_SETUP_PROG_HW, /* Check if a bpf program is set on the device. The callee should * return true if a program is currently attached and running. */ diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index dd88375a6580..ce777ec88e1e 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -891,9 +891,12 @@ enum { #define XDP_FLAGS_UPDATE_IF_NOEXIST (1U << 0) #define XDP_FLAGS_SKB_MODE (1U << 1) #define XDP_FLAGS_DRV_MODE (1U << 2) +#define XDP_FLAGS_HW_MODE (1U << 3) +#define XDP_FLAGS_MODES (XDP_FLAGS_SKB_MODE | \ + XDP_FLAGS_DRV_MODE | \ + XDP_FLAGS_HW_MODE) #define XDP_FLAGS_MASK (XDP_FLAGS_UPDATE_IF_NOEXIST | \ - XDP_FLAGS_SKB_MODE | \ - XDP_FLAGS_DRV_MODE) + XDP_FLAGS_MODES) /* These are stored into IFLA_XDP_ATTACHED on dump. */ enum { diff --git a/net/core/dev.c b/net/core/dev.c index a04db264aa1c..05cec8e2cd82 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6959,7 +6959,10 @@ static int dev_xdp_install(struct net_device *dev, xdp_op_t xdp_op, struct netdev_xdp xdp; memset(&xdp, 0, sizeof(xdp)); - xdp.command = XDP_SETUP_PROG; + if (flags & XDP_FLAGS_HW_MODE) + xdp.command = XDP_SETUP_PROG_HW; + else + xdp.command = XDP_SETUP_PROG; xdp.extack = extack; xdp.flags = flags; xdp.prog = prog; @@ -6987,7 +6990,7 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, ASSERT_RTNL(); xdp_op = xdp_chk = ops->ndo_xdp; - if (!xdp_op && (flags & XDP_FLAGS_DRV_MODE)) + if (!xdp_op && (flags & (XDP_FLAGS_DRV_MODE | XDP_FLAGS_HW_MODE))) return -EOPNOTSUPP; if (!xdp_op || (flags & XDP_FLAGS_SKB_MODE)) xdp_op = generic_xdp_install; diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index 3aa57848a895..daf3b39be649 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -16,6 +16,7 @@ * Vitaly E. Lavrov RTA_OK arithmetics was wrong. */ +#include <linux/bitops.h> #include <linux/errno.h> #include <linux/module.h> #include <linux/types.h> @@ -2251,8 +2252,7 @@ static int do_setlink(const struct sk_buff *skb, err = -EINVAL; goto errout; } - if ((xdp_flags & XDP_FLAGS_SKB_MODE) && - (xdp_flags & XDP_FLAGS_DRV_MODE)) { + if (hweight32(xdp_flags & XDP_FLAGS_MODES) > 1) { err = -EINVAL; goto errout; }
Add an installation-time flag for requesting that the program be installed only if it can be offloaded to HW. Internally new command for ndo_xdp is added, this way we avoid putting checks into drivers since they all return -EINVAL on an unknown command. Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com> --- include/linux/netdevice.h | 1 + include/uapi/linux/if_link.h | 7 +++++-- net/core/dev.c | 7 +++++-- net/core/rtnetlink.c | 4 ++-- 4 files changed, 13 insertions(+), 6 deletions(-)