* [PATCH net 0/2] Two generic xdp related follow-ups @ 2017-05-10 1:31 Daniel Borkmann 2017-05-10 1:31 ` [PATCH net 1/2] xdp: add flag to enforce driver mode Daniel Borkmann 2017-05-10 1:31 ` [PATCH net 2/2] xdp: disallow use of native and generic hook at once Daniel Borkmann 0 siblings, 2 replies; 9+ messages in thread From: Daniel Borkmann @ 2017-05-10 1:31 UTC (permalink / raw) To: davem; +Cc: alexei.starovoitov, john.fastabend, netdev, Daniel Borkmann Two follow-ups for the generic XDP API, would be great if both could still be considered, since the XDP API is not frozen yet. For details please see individual patches. Thanks! Daniel Borkmann (2): xdp: add flag to enforce driver mode xdp: disallow use of native and generic hook at once include/linux/netdevice.h | 15 ++++++++-- include/uapi/linux/if_link.h | 6 ++-- net/core/dev.c | 57 +++++++++++++++++++++++++------------- net/core/rtnetlink.c | 19 +++++++------ samples/bpf/xdp1_user.c | 8 ++++-- samples/bpf/xdp_tx_iptunnel_user.c | 7 ++++- 6 files changed, 77 insertions(+), 35 deletions(-) -- 1.9.3 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH net 1/2] xdp: add flag to enforce driver mode 2017-05-10 1:31 [PATCH net 0/2] Two generic xdp related follow-ups Daniel Borkmann @ 2017-05-10 1:31 ` Daniel Borkmann 2017-05-10 1:31 ` [PATCH net 2/2] xdp: disallow use of native and generic hook at once Daniel Borkmann 1 sibling, 0 replies; 9+ messages in thread From: Daniel Borkmann @ 2017-05-10 1:31 UTC (permalink / raw) To: davem; +Cc: alexei.starovoitov, john.fastabend, netdev, Daniel Borkmann After commit b5cdae3291f7 ("net: Generic XDP") we automatically fall back to a generic XDP variant if the driver does not support native XDP. Allow for an option where the user can specify that always the native XDP variant should be selected and in case it's not supported by a driver, just bail out. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Acked-by: Alexei Starovoitov <ast@kernel.org> --- include/uapi/linux/if_link.h | 6 ++++-- net/core/dev.c | 2 ++ net/core/rtnetlink.c | 5 +++++ samples/bpf/xdp1_user.c | 8 ++++++-- samples/bpf/xdp_tx_iptunnel_user.c | 7 ++++++- 5 files changed, 23 insertions(+), 5 deletions(-) diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h index 8e56ac7..549ac8a 100644 --- a/include/uapi/linux/if_link.h +++ b/include/uapi/linux/if_link.h @@ -888,9 +888,11 @@ enum { /* XDP section */ #define XDP_FLAGS_UPDATE_IF_NOEXIST (1U << 0) -#define XDP_FLAGS_SKB_MODE (2U << 0) +#define XDP_FLAGS_SKB_MODE (1U << 1) +#define XDP_FLAGS_DRV_MODE (1U << 2) #define XDP_FLAGS_MASK (XDP_FLAGS_UPDATE_IF_NOEXIST | \ - XDP_FLAGS_SKB_MODE) + XDP_FLAGS_SKB_MODE | \ + XDP_FLAGS_DRV_MODE) enum { IFLA_XDP_UNSPEC, diff --git a/net/core/dev.c b/net/core/dev.c index d07aa5f..61443f0 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6872,6 +6872,8 @@ int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, ASSERT_RTNL(); xdp_op = ops->ndo_xdp; + if (!xdp_op && (flags & XDP_FLAGS_DRV_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 bcb0f610..dda9f16 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -2199,6 +2199,11 @@ 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)) { + err = -EINVAL; + goto errout; + } } if (xdp[IFLA_XDP_FD]) { diff --git a/samples/bpf/xdp1_user.c b/samples/bpf/xdp1_user.c index 378850c..17be9ea 100644 --- a/samples/bpf/xdp1_user.c +++ b/samples/bpf/xdp1_user.c @@ -62,13 +62,14 @@ static void usage(const char *prog) fprintf(stderr, "usage: %s [OPTS] IFINDEX\n\n" "OPTS:\n" - " -S use skb-mode\n", + " -S use skb-mode\n" + " -N enforce native mode\n", prog); } int main(int argc, char **argv) { - const char *optstr = "S"; + const char *optstr = "SN"; char filename[256]; int opt; @@ -77,6 +78,9 @@ int main(int argc, char **argv) case 'S': xdp_flags |= XDP_FLAGS_SKB_MODE; break; + case 'N': + xdp_flags |= XDP_FLAGS_DRV_MODE; + break; default: usage(basename(argv[0])); return 1; diff --git a/samples/bpf/xdp_tx_iptunnel_user.c b/samples/bpf/xdp_tx_iptunnel_user.c index 92b8bde..631cdcc 100644 --- a/samples/bpf/xdp_tx_iptunnel_user.c +++ b/samples/bpf/xdp_tx_iptunnel_user.c @@ -79,6 +79,8 @@ static void usage(const char *cmd) printf(" -m <dest-MAC> Used in sending the IP Tunneled pkt\n"); printf(" -T <stop-after-X-seconds> Default: 0 (forever)\n"); printf(" -P <IP-Protocol> Default is TCP\n"); + printf(" -S use skb-mode\n"); + printf(" -N enforce native mode\n"); printf(" -h Display this help\n"); } @@ -138,7 +140,7 @@ int main(int argc, char **argv) { unsigned char opt_flags[256] = {}; unsigned int kill_after_s = 0; - const char *optstr = "i:a:p:s:d:m:T:P:Sh"; + const char *optstr = "i:a:p:s:d:m:T:P:SNh"; int min_port = 0, max_port = 0; struct iptnl_info tnl = {}; struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY}; @@ -206,6 +208,9 @@ int main(int argc, char **argv) case 'S': xdp_flags |= XDP_FLAGS_SKB_MODE; break; + case 'N': + xdp_flags |= XDP_FLAGS_DRV_MODE; + break; default: usage(argv[0]); return 1; -- 1.9.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH net 2/2] xdp: disallow use of native and generic hook at once 2017-05-10 1:31 [PATCH net 0/2] Two generic xdp related follow-ups Daniel Borkmann 2017-05-10 1:31 ` [PATCH net 1/2] xdp: add flag to enforce driver mode Daniel Borkmann @ 2017-05-10 1:31 ` Daniel Borkmann 2017-05-10 3:18 ` Jakub Kicinski 1 sibling, 1 reply; 9+ messages in thread From: Daniel Borkmann @ 2017-05-10 1:31 UTC (permalink / raw) To: davem; +Cc: alexei.starovoitov, john.fastabend, netdev, Daniel Borkmann While working on the iproute2 generic XDP frontend, I noticed that as of right now it's possible to have native *and* generic XDP programs loaded both at the same time for the case when a driver supports native XDP. The intended model for generic XDP from b5cdae3291f7 ("net: Generic XDP") is, however, that only one out of the two can be present at once which is also indicated as such in the XPD netlink dump part. The main rationale for generic XDP is to ease accessibility (in case a driver does not yet have XDP support) and to generically provide a semantical model as an example for driver developers wanting to add XDP support. The generic XDP option for an XDP aware driver can still be useful for comparing and testing both implementations. However, it is not intended to have a second XDP processing stage or layer with exactly the same functionality of the first native stage. Only reason would be to have a partial fallback for future XDP features that are not supported yet in the native implementation and we probably also shouldn't strive for such fallback and instead encourage native feature support in the first place. Given there's currently no such fallback issue or use case, lets not go there yet if we don't need to. Therefore, change semantics for loading XDP and bail out if the user tries to load a generic XDP program when a native one is present and vice versa. Another alternative to bailing out would be to handle the transition from one flavor to another gracefully, but that would require to bring the device down, exchange both types of programs, and bring it up again in order to avoid a tiny window where a packet could hit both hooks. Given this complicates the logic a bit more for just a debugging feature, I went with the simpler variant. Signed-off-by: Daniel Borkmann <daniel@iogearbox.net> Acked-by: Alexei Starovoitov <ast@kernel.org> --- include/linux/netdevice.h | 15 +++++++++++-- net/core/dev.c | 55 +++++++++++++++++++++++++++++++---------------- net/core/rtnetlink.c | 14 +++++------- 3 files changed, 54 insertions(+), 30 deletions(-) diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h index 9c23bd2..abedec7 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -3296,11 +3296,22 @@ int dev_get_phys_port_id(struct net_device *dev, int dev_get_phys_port_name(struct net_device *dev, char *name, size_t len); int dev_change_proto_down(struct net_device *dev, bool proto_down); -int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, - int fd, u32 flags); struct sk_buff *validate_xmit_skb_list(struct sk_buff *skb, struct net_device *dev); struct sk_buff *dev_hard_start_xmit(struct sk_buff *skb, struct net_device *dev, struct netdev_queue *txq, int *ret); + +typedef int (*xdp_op_t)(struct net_device *dev, struct netdev_xdp *xdp); +int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, + int fd, u32 flags); +bool __dev_xdp_attached(struct net_device *dev, xdp_op_t xdp_op); + +static inline bool dev_xdp_attached(struct net_device *dev) +{ + const struct net_device_ops *ops = dev->netdev_ops; + + return ops->ndo_xdp && __dev_xdp_attached(dev, ops->ndo_xdp); +} + int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb); int dev_forward_skb(struct net_device *dev, struct sk_buff *skb); bool is_skb_forwardable(const struct net_device *dev, diff --git a/net/core/dev.c b/net/core/dev.c index 61443f0..d25f847 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -6851,6 +6851,32 @@ int dev_change_proto_down(struct net_device *dev, bool proto_down) } EXPORT_SYMBOL(dev_change_proto_down); +bool __dev_xdp_attached(struct net_device *dev, xdp_op_t xdp_op) +{ + struct netdev_xdp xdp; + + memset(&xdp, 0, sizeof(xdp)); + xdp.command = XDP_QUERY_PROG; + + /* Query must always succeed. */ + WARN_ON(xdp_op(dev, &xdp) < 0); + return xdp.prog_attached; +} + +static int dev_xdp_install(struct net_device *dev, xdp_op_t xdp_op, + struct netlink_ext_ack *extack, + struct bpf_prog *prog) +{ + struct netdev_xdp xdp; + + memset(&xdp, 0, sizeof(xdp)); + xdp.command = XDP_SETUP_PROG; + xdp.extack = extack; + xdp.prog = prog; + + return xdp_op(dev, &xdp); +} + /** * dev_change_xdp_fd - set or clear a bpf program for a device rx path * @dev: device @@ -6863,43 +6889,34 @@ int dev_change_proto_down(struct net_device *dev, bool proto_down) int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack *extack, int fd, u32 flags) { - int (*xdp_op)(struct net_device *dev, struct netdev_xdp *xdp); const struct net_device_ops *ops = dev->netdev_ops; struct bpf_prog *prog = NULL; - struct netdev_xdp xdp; + xdp_op_t xdp_op, xdp_chk; int err; ASSERT_RTNL(); - xdp_op = ops->ndo_xdp; + xdp_op = xdp_chk = ops->ndo_xdp; if (!xdp_op && (flags & XDP_FLAGS_DRV_MODE)) return -EOPNOTSUPP; if (!xdp_op || (flags & XDP_FLAGS_SKB_MODE)) xdp_op = generic_xdp_install; + if (xdp_op == xdp_chk) + xdp_chk = generic_xdp_install; if (fd >= 0) { - if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST) { - memset(&xdp, 0, sizeof(xdp)); - xdp.command = XDP_QUERY_PROG; - - err = xdp_op(dev, &xdp); - if (err < 0) - return err; - if (xdp.prog_attached) - return -EBUSY; - } + if (xdp_chk && __dev_xdp_attached(dev, xdp_chk)) + return -EEXIST; + if ((flags & XDP_FLAGS_UPDATE_IF_NOEXIST) && + __dev_xdp_attached(dev, xdp_op)) + return -EBUSY; prog = bpf_prog_get_type(fd, BPF_PROG_TYPE_XDP); if (IS_ERR(prog)) return PTR_ERR(prog); } - memset(&xdp, 0, sizeof(xdp)); - xdp.command = XDP_SETUP_PROG; - xdp.extack = extack; - xdp.prog = prog; - - err = xdp_op(dev, &xdp); + err = dev_xdp_install(dev, xdp_op, extack, prog); if (err < 0 && prog) bpf_prog_put(prog); diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c index dda9f16..99320f0 100644 --- a/net/core/rtnetlink.c +++ b/net/core/rtnetlink.c @@ -1251,24 +1251,20 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev) { struct nlattr *xdp; u32 xdp_flags = 0; - u8 val = 0; int err; + u8 val; xdp = nla_nest_start(skb, IFLA_XDP); if (!xdp) return -EMSGSIZE; + if (rcu_access_pointer(dev->xdp_prog)) { xdp_flags = XDP_FLAGS_SKB_MODE; val = 1; - } else if (dev->netdev_ops->ndo_xdp) { - struct netdev_xdp xdp_op = {}; - - xdp_op.command = XDP_QUERY_PROG; - err = dev->netdev_ops->ndo_xdp(dev, &xdp_op); - if (err) - goto err_cancel; - val = xdp_op.prog_attached; + } else { + val = dev_xdp_attached(dev); } + err = nla_put_u8(skb, IFLA_XDP_ATTACHED, val); if (err) goto err_cancel; -- 1.9.3 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once 2017-05-10 1:31 ` [PATCH net 2/2] xdp: disallow use of native and generic hook at once Daniel Borkmann @ 2017-05-10 3:18 ` Jakub Kicinski 2017-05-10 9:36 ` Daniel Borkmann 0 siblings, 1 reply; 9+ messages in thread From: Jakub Kicinski @ 2017-05-10 3:18 UTC (permalink / raw) To: Daniel Borkmann; +Cc: davem, alexei.starovoitov, john.fastabend, netdev On Wed, 10 May 2017 03:31:31 +0200, Daniel Borkmann wrote: > While working on the iproute2 generic XDP frontend, I noticed that > as of right now it's possible to have native *and* generic XDP > programs loaded both at the same time for the case when a driver > supports native XDP. Nice improvement! A couple of absolute nitpicks below.. > The intended model for generic XDP from b5cdae3291f7 ("net: Generic > XDP") is, however, that only one out of the two can be present at > once which is also indicated as such in the XPD netlink dump part. ^^^ XDP > @@ -6851,6 +6851,32 @@ int dev_change_proto_down(struct net_device *dev, bool proto_down) > } > EXPORT_SYMBOL(dev_change_proto_down); > > +bool __dev_xdp_attached(struct net_device *dev, xdp_op_t xdp_op) Out of curiosity - the leading underscores refer to caller having to hold rtnl? I assume they are not needed in the function below because it's static? > +{ > + struct netdev_xdp xdp; > + > + memset(&xdp, 0, sizeof(xdp)); > + xdp.command = XDP_QUERY_PROG; Probably personal preference, but seems like designated struct initializer would do quite nicely here and save the memset :) > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > index dda9f16..99320f0 100644 > --- a/net/core/rtnetlink.c > +++ b/net/core/rtnetlink.c > @@ -1251,24 +1251,20 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev) > { > struct nlattr *xdp; > u32 xdp_flags = 0; > - u8 val = 0; > int err; > + u8 val; > > xdp = nla_nest_start(skb, IFLA_XDP); > if (!xdp) > return -EMSGSIZE; > + > if (rcu_access_pointer(dev->xdp_prog)) { > xdp_flags = XDP_FLAGS_SKB_MODE; > val = 1; > - } else if (dev->netdev_ops->ndo_xdp) { > - struct netdev_xdp xdp_op = {}; > - > - xdp_op.command = XDP_QUERY_PROG; > - err = dev->netdev_ops->ndo_xdp(dev, &xdp_op); > - if (err) > - goto err_cancel; > - val = xdp_op.prog_attached; > + } else { > + val = dev_xdp_attached(dev); > } Would it make sense to set xdp_flags to XDP_FLAGS_DRV_MODE here to keep things symmetrical? I know you are just preserving existing behaviour but it may seem slightly arbitrary to a new comer to report one of the very similarly named flags in the dump but not the other. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once 2017-05-10 3:18 ` Jakub Kicinski @ 2017-05-10 9:36 ` Daniel Borkmann 2017-05-10 21:07 ` Jakub Kicinski 0 siblings, 1 reply; 9+ messages in thread From: Daniel Borkmann @ 2017-05-10 9:36 UTC (permalink / raw) To: Jakub Kicinski; +Cc: davem, alexei.starovoitov, john.fastabend, netdev On 05/10/2017 05:18 AM, Jakub Kicinski wrote: > On Wed, 10 May 2017 03:31:31 +0200, Daniel Borkmann wrote: >> While working on the iproute2 generic XDP frontend, I noticed that >> as of right now it's possible to have native *and* generic XDP >> programs loaded both at the same time for the case when a driver >> supports native XDP. > > Nice improvement! A couple of absolute nitpicks below.. > >> The intended model for generic XDP from b5cdae3291f7 ("net: Generic >> XDP") is, however, that only one out of the two can be present at >> once which is also indicated as such in the XPD netlink dump part. > ^^^ > XDP Good point. >> @@ -6851,6 +6851,32 @@ int dev_change_proto_down(struct net_device *dev, bool proto_down) >> } >> EXPORT_SYMBOL(dev_change_proto_down); >> >> +bool __dev_xdp_attached(struct net_device *dev, xdp_op_t xdp_op) > > Out of curiosity - the leading underscores refer to caller having to > hold rtnl? I assume they are not needed in the function below because > it's static? I think I don't quite follow the last question, but it probably makes sense to add an ASSERT_RTNL() into dev_xdp_attached() inline helper to make it clearly visible to callers of this api. >> +{ >> + struct netdev_xdp xdp; >> + >> + memset(&xdp, 0, sizeof(xdp)); >> + xdp.command = XDP_QUERY_PROG; > > Probably personal preference, but seems like designated struct > initializer would do quite nicely here and save the memset :) I had that initially, but I recalled that gcc < 4.6 does not handle this style for the initialization of anonymous struct/union properly (e.g., we fixed that in iproute2 as well). Andrew Morton still uses gcc 4.4.4 and occasionally sends kernel fixes, so we might end up like this anyway. >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c >> index dda9f16..99320f0 100644 >> --- a/net/core/rtnetlink.c >> +++ b/net/core/rtnetlink.c >> @@ -1251,24 +1251,20 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev) >> { >> struct nlattr *xdp; >> u32 xdp_flags = 0; >> - u8 val = 0; >> int err; >> + u8 val; >> >> xdp = nla_nest_start(skb, IFLA_XDP); >> if (!xdp) >> return -EMSGSIZE; >> + >> if (rcu_access_pointer(dev->xdp_prog)) { >> xdp_flags = XDP_FLAGS_SKB_MODE; >> val = 1; >> - } else if (dev->netdev_ops->ndo_xdp) { >> - struct netdev_xdp xdp_op = {}; >> - >> - xdp_op.command = XDP_QUERY_PROG; >> - err = dev->netdev_ops->ndo_xdp(dev, &xdp_op); >> - if (err) >> - goto err_cancel; >> - val = xdp_op.prog_attached; >> + } else { >> + val = dev_xdp_attached(dev); >> } > > Would it make sense to set xdp_flags to XDP_FLAGS_DRV_MODE here to keep > things symmetrical? I know you are just preserving existing behaviour > but it may seem slightly arbitrary to a new comer to report one of the > very similarly named flags in the dump but not the other. I thought about it, it's kind of redundant information since IFLA_XDP_ATTACHED attribute w/o IFLA_XDP_FLAGS attribute today says that it's native already. It might look strange if we add also XDP_FLAGS_DRV_MODE there, since it doesn't give anything new. I rather see it similar to XDP_FLAGS_UPDATE_IF_NOEXIST flag that is for updating fd only, but I don't really have a strong opinion on this though. I could add it to the respin if preferred. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once 2017-05-10 9:36 ` Daniel Borkmann @ 2017-05-10 21:07 ` Jakub Kicinski 2017-05-10 22:24 ` Daniel Borkmann 0 siblings, 1 reply; 9+ messages in thread From: Jakub Kicinski @ 2017-05-10 21:07 UTC (permalink / raw) To: Daniel Borkmann; +Cc: davem, alexei.starovoitov, john.fastabend, netdev On Wed, 10 May 2017 11:36:22 +0200, Daniel Borkmann wrote: > On 05/10/2017 05:18 AM, Jakub Kicinski wrote: > > On Wed, 10 May 2017 03:31:31 +0200, Daniel Borkmann wrote: > >> @@ -6851,6 +6851,32 @@ int dev_change_proto_down(struct net_device *dev, bool proto_down) > >> } > >> EXPORT_SYMBOL(dev_change_proto_down); > >> > >> +bool __dev_xdp_attached(struct net_device *dev, xdp_op_t xdp_op) > > > > Out of curiosity - the leading underscores refer to caller having to > > hold rtnl? I assume they are not needed in the function below because > > it's static? > > I think I don't quite follow the last question, but it probably makes > sense to add an ASSERT_RTNL() into dev_xdp_attached() inline helper to > make it clearly visible to callers of this api. Sorry, I missed you have a dev_xdp_attached() defined in the header, hence the confusion. > >> +{ > >> + struct netdev_xdp xdp; > >> + > >> + memset(&xdp, 0, sizeof(xdp)); > >> + xdp.command = XDP_QUERY_PROG; > > > > Probably personal preference, but seems like designated struct > > initializer would do quite nicely here and save the memset :) > > I had that initially, but I recalled that gcc < 4.6 does not handle this > style for the initialization of anonymous struct/union properly (e.g., > we fixed that in iproute2 as well). Andrew Morton still uses gcc 4.4.4 > and occasionally sends kernel fixes, so we might end up like this anyway. Ah, good to know! > >> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c > >> index dda9f16..99320f0 100644 > >> --- a/net/core/rtnetlink.c > >> +++ b/net/core/rtnetlink.c > >> @@ -1251,24 +1251,20 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev) > >> { > >> struct nlattr *xdp; > >> u32 xdp_flags = 0; > >> - u8 val = 0; > >> int err; > >> + u8 val; > >> > >> xdp = nla_nest_start(skb, IFLA_XDP); > >> if (!xdp) > >> return -EMSGSIZE; > >> + > >> if (rcu_access_pointer(dev->xdp_prog)) { > >> xdp_flags = XDP_FLAGS_SKB_MODE; > >> val = 1; > >> - } else if (dev->netdev_ops->ndo_xdp) { > >> - struct netdev_xdp xdp_op = {}; > >> - > >> - xdp_op.command = XDP_QUERY_PROG; > >> - err = dev->netdev_ops->ndo_xdp(dev, &xdp_op); > >> - if (err) > >> - goto err_cancel; > >> - val = xdp_op.prog_attached; > >> + } else { > >> + val = dev_xdp_attached(dev); > >> } > > > > Would it make sense to set xdp_flags to XDP_FLAGS_DRV_MODE here to keep > > things symmetrical? I know you are just preserving existing behaviour > > but it may seem slightly arbitrary to a new comer to report one of the > > very similarly named flags in the dump but not the other. > > I thought about it, it's kind of redundant information since > IFLA_XDP_ATTACHED attribute w/o IFLA_XDP_FLAGS attribute today > says that it's native already. It might look strange if we add > also XDP_FLAGS_DRV_MODE there, since it doesn't give anything > new. I rather see it similar to XDP_FLAGS_UPDATE_IF_NOEXIST flag > that is for updating fd only, but I don't really have a strong > opinion on this though. I could add it to the respin if preferred. XDP_FLAGS_UPDATE_IF_NOEXIST is indeed the precedent which makes things a bit murky. There are no reasonably useful semantics for IF_NOEXIST on dump :( Note that meaning of SKB_MODE flag shifts slightly between set and dump IIUC. At set time it means: "force installation at the generic hook", at dump time it means: "installed at generic hook - regardless of whether the flag was set at installation time", So I would argue that DRV_MODE flag is closer to SKB_MODE not only by name but also by semantics, and it would be cool if we could keep the semantics close on dump as well as set. I understand the counter argument that from user space perspective it would make things slightly more complicated because there would be two conditions in which driver hook is used: 1) DRV_MODE set on dump; 2) flags attribute not present (old kernel). I'm concerned about number 2). We can't simply depend on SKB_MODE not being set because we may add more *_MODE flags in the future. So doing: if (flags & SKB_MODE) printf("skb-mode"); else printf("drv-mode"); is not correct. The flags attribute must not be present at all (think HW_MODE flag). But going further there can also be non-MODE flags, like, say.. NEVER_TX, and then there may be flags present in dump, and if SKB_MODE isn't be set, the mode could be some new MODE user space doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no way to tell :S ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once 2017-05-10 21:07 ` Jakub Kicinski @ 2017-05-10 22:24 ` Daniel Borkmann 2017-05-10 22:46 ` Jakub Kicinski 0 siblings, 1 reply; 9+ messages in thread From: Daniel Borkmann @ 2017-05-10 22:24 UTC (permalink / raw) To: Jakub Kicinski; +Cc: davem, alexei.starovoitov, john.fastabend, netdev On 05/10/2017 11:07 PM, Jakub Kicinski wrote: > On Wed, 10 May 2017 11:36:22 +0200, Daniel Borkmann wrote: >> On 05/10/2017 05:18 AM, Jakub Kicinski wrote: >>> On Wed, 10 May 2017 03:31:31 +0200, Daniel Borkmann wrote: [...] >>>> xdp = nla_nest_start(skb, IFLA_XDP); >>>> if (!xdp) >>>> return -EMSGSIZE; >>>> + >>>> if (rcu_access_pointer(dev->xdp_prog)) { >>>> xdp_flags = XDP_FLAGS_SKB_MODE; >>>> val = 1; >>>> - } else if (dev->netdev_ops->ndo_xdp) { >>>> - struct netdev_xdp xdp_op = {}; >>>> - >>>> - xdp_op.command = XDP_QUERY_PROG; >>>> - err = dev->netdev_ops->ndo_xdp(dev, &xdp_op); >>>> - if (err) >>>> - goto err_cancel; >>>> - val = xdp_op.prog_attached; >>>> + } else { >>>> + val = dev_xdp_attached(dev); >>>> } >>> >>> Would it make sense to set xdp_flags to XDP_FLAGS_DRV_MODE here to keep >>> things symmetrical? I know you are just preserving existing behaviour >>> but it may seem slightly arbitrary to a new comer to report one of the >>> very similarly named flags in the dump but not the other. >> >> I thought about it, it's kind of redundant information since >> IFLA_XDP_ATTACHED attribute w/o IFLA_XDP_FLAGS attribute today >> says that it's native already. It might look strange if we add >> also XDP_FLAGS_DRV_MODE there, since it doesn't give anything >> new. I rather see it similar to XDP_FLAGS_UPDATE_IF_NOEXIST flag >> that is for updating fd only, but I don't really have a strong >> opinion on this though. I could add it to the respin if preferred. > > XDP_FLAGS_UPDATE_IF_NOEXIST is indeed the precedent which makes things > a bit murky. There are no reasonably useful semantics for IF_NOEXIST > on dump :( Note that meaning of SKB_MODE flag shifts slightly between > set and dump IIUC. At set time it means: > "force installation at the generic hook", > at dump time it means: > "installed at generic hook - regardless of whether the flag was set at > installation time", > > So I would argue that DRV_MODE flag is closer to SKB_MODE not only by > name but also by semantics, and it would be cool if we could keep the > semantics close on dump as well as set. Right. > I understand the counter argument that from user space perspective it > would make things slightly more complicated because there would be two > conditions in which driver hook is used: > 1) DRV_MODE set on dump; > 2) flags attribute not present (old kernel). > > I'm concerned about number 2). We can't simply depend on SKB_MODE > not being set because we may add more *_MODE flags in the future. So > doing: > > if (flags & SKB_MODE) > printf("skb-mode"); > else > printf("drv-mode"); > > is not correct. The flags attribute must not be present at all (think > HW_MODE flag). But going further there can also be non-MODE flags, > like, say.. NEVER_TX, and then there may be flags present in dump, > and if SKB_MODE isn't be set, the mode could be some new MODE user space > doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no > way to tell :S Yep, I see your point. Additionally, if we use XDP_FLAGS_* again for dumping we're wasting bit space for flags we would never dump back such as mentioned XDP_FLAGS_UPDATE_IF_NOEXIST (or any other future flags that are only relevant for loading, but never for dumping). Given dumping IFLA_XDP_FLAGS was added due to XDP_FLAGS_SKB_MODE, we can still change this, since it's not too late. How about the following proposal: IFLA_XDP_ATTACHED we have as-is (need to keep that anyway), if that is true, it means "something is attached at XDP layer". Then, we add a new attribute IFLA_XDP_MODE (enum as type), which can contain XDP_DRV_MODE (0), XDP_SKB_MODE (1). I don't think there's a strict requirement to really dump IFLA_XDP_FLAGS back, separating both attrs would avoid this hassle of what current or future load flag fits for dump as well and which not. Wdyt? Thanks, Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once 2017-05-10 22:24 ` Daniel Borkmann @ 2017-05-10 22:46 ` Jakub Kicinski 2017-05-10 23:02 ` Daniel Borkmann 0 siblings, 1 reply; 9+ messages in thread From: Jakub Kicinski @ 2017-05-10 22:46 UTC (permalink / raw) To: Daniel Borkmann; +Cc: davem, alexei.starovoitov, john.fastabend, netdev On Thu, 11 May 2017 00:24:56 +0200, Daniel Borkmann wrote: > > I understand the counter argument that from user space perspective it > > would make things slightly more complicated because there would be two > > conditions in which driver hook is used: > > 1) DRV_MODE set on dump; > > 2) flags attribute not present (old kernel). > > > > I'm concerned about number 2). We can't simply depend on SKB_MODE > > not being set because we may add more *_MODE flags in the future. So > > doing: > > > > if (flags & SKB_MODE) > > printf("skb-mode"); > > else > > printf("drv-mode"); > > > > is not correct. The flags attribute must not be present at all (think > > HW_MODE flag). But going further there can also be non-MODE flags, > > like, say.. NEVER_TX, and then there may be flags present in dump, > > and if SKB_MODE isn't be set, the mode could be some new MODE user space > > doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no > > way to tell :S > > Yep, I see your point. Additionally, if we use XDP_FLAGS_* again for > dumping we're wasting bit space for flags we would never dump back > such as mentioned XDP_FLAGS_UPDATE_IF_NOEXIST (or any other future > flags that are only relevant for loading, but never for dumping). > Given dumping IFLA_XDP_FLAGS was added due to XDP_FLAGS_SKB_MODE, > we can still change this, since it's not too late. > > How about the following proposal: IFLA_XDP_ATTACHED we have as-is > (need to keep that anyway), if that is true, it means "something > is attached at XDP layer". Then, we add a new attribute IFLA_XDP_MODE > (enum as type), which can contain XDP_DRV_MODE (0), XDP_SKB_MODE (1). > I don't think there's a strict requirement to really dump IFLA_XDP_FLAGS > back, separating both attrs would avoid this hassle of what current > or future load flag fits for dump as well and which not. Wdyt? I really like the idea of not reusing IFLA_XDP_FLAGS for dumps! New enum sounds good, but perhaps reusing IFLA_XDP_ATTACHED isn't 100% off-limits either? 4.11 would report (0) - driver supports XDP but nothing is attached, (1) - something attached at the driver, could we make (2) mean - something attached at generic XDP? Would that be considered breaking the ABI, are we bound to boolean semantics? ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH net 2/2] xdp: disallow use of native and generic hook at once 2017-05-10 22:46 ` Jakub Kicinski @ 2017-05-10 23:02 ` Daniel Borkmann 0 siblings, 0 replies; 9+ messages in thread From: Daniel Borkmann @ 2017-05-10 23:02 UTC (permalink / raw) To: Jakub Kicinski; +Cc: davem, alexei.starovoitov, john.fastabend, netdev On 05/11/2017 12:46 AM, Jakub Kicinski wrote: > On Thu, 11 May 2017 00:24:56 +0200, Daniel Borkmann wrote: >>> I understand the counter argument that from user space perspective it >>> would make things slightly more complicated because there would be two >>> conditions in which driver hook is used: >>> 1) DRV_MODE set on dump; >>> 2) flags attribute not present (old kernel). >>> >>> I'm concerned about number 2). We can't simply depend on SKB_MODE >>> not being set because we may add more *_MODE flags in the future. So >>> doing: >>> >>> if (flags & SKB_MODE) >>> printf("skb-mode"); >>> else >>> printf("drv-mode"); >>> >>> is not correct. The flags attribute must not be present at all (think >>> HW_MODE flag). But going further there can also be non-MODE flags, >>> like, say.. NEVER_TX, and then there may be flags present in dump, >>> and if SKB_MODE isn't be set, the mode could be some new MODE user space >>> doesn't understand, or it could be DRV_MODE+a new non-MODE flag... no >>> way to tell :S >> >> Yep, I see your point. Additionally, if we use XDP_FLAGS_* again for >> dumping we're wasting bit space for flags we would never dump back >> such as mentioned XDP_FLAGS_UPDATE_IF_NOEXIST (or any other future >> flags that are only relevant for loading, but never for dumping). >> Given dumping IFLA_XDP_FLAGS was added due to XDP_FLAGS_SKB_MODE, >> we can still change this, since it's not too late. >> >> How about the following proposal: IFLA_XDP_ATTACHED we have as-is >> (need to keep that anyway), if that is true, it means "something >> is attached at XDP layer". Then, we add a new attribute IFLA_XDP_MODE >> (enum as type), which can contain XDP_DRV_MODE (0), XDP_SKB_MODE (1). >> I don't think there's a strict requirement to really dump IFLA_XDP_FLAGS >> back, separating both attrs would avoid this hassle of what current >> or future load flag fits for dump as well and which not. Wdyt? > > I really like the idea of not reusing IFLA_XDP_FLAGS for dumps! New > enum sounds good, but perhaps reusing IFLA_XDP_ATTACHED isn't 100% > off-limits either? 4.11 would report (0) - driver supports XDP but > nothing is attached, (1) - something attached at the driver, could we > make (2) mean - something attached at generic XDP? Would that be > considered breaking the ABI, are we bound to boolean semantics? I like the idea, it would also render IFLA_XDP_ATTACHED not useless or redundant then. Older binaries check !rta_getattr_u8(tb[IFLA_XDP_ATTACHED]) whether something is attached or not at XDP layer. So they won't know IFLA_XDP_FLAGS attr either to make a more fine-grained distinction about what mode. That seems actually cleaner to me, will give it a try. Thanks, Daniel ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2017-05-10 23:02 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2017-05-10 1:31 [PATCH net 0/2] Two generic xdp related follow-ups Daniel Borkmann 2017-05-10 1:31 ` [PATCH net 1/2] xdp: add flag to enforce driver mode Daniel Borkmann 2017-05-10 1:31 ` [PATCH net 2/2] xdp: disallow use of native and generic hook at once Daniel Borkmann 2017-05-10 3:18 ` Jakub Kicinski 2017-05-10 9:36 ` Daniel Borkmann 2017-05-10 21:07 ` Jakub Kicinski 2017-05-10 22:24 ` Daniel Borkmann 2017-05-10 22:46 ` Jakub Kicinski 2017-05-10 23:02 ` Daniel Borkmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).