* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
@ 2019-05-31 19:18 ` Saeed Mahameed
2019-06-01 19:42 ` Jakub Kicinski
2019-06-01 19:57 ` Jakub Kicinski
2019-06-01 9:50 ` kbuild test robot
` (2 subsequent siblings)
3 siblings, 2 replies; 23+ messages in thread
From: Saeed Mahameed @ 2019-05-31 19:18 UTC (permalink / raw)
To: toke@redhat.com, daniel@iogearbox.net, ast@kernel.org,
netdev@vger.kernel.org, bjorn.topel@gmail.com
Cc: magnus.karlsson@intel.com, bjorn.topel@intel.com,
jakub.kicinski@netronome.com, brouer@redhat.com,
bpf@vger.kernel.org
On Fri, 2019-05-31 at 11:42 +0200, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
> command of ndo_bpf. The query code is fairly generic. This commit
> refactors the query code up from the drivers to the netdev level.
>
> The struct net_device has gained two new members: xdp_prog_hw and
> xdp_flags. The former is the offloaded XDP program, if any, and the
> latter tracks the flags that the supplied when attaching the XDP
> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
>
> The xdp_prog member, previously only used for SKB_MODE, is shared
> with
> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> mutually exclusive. To differentiate between the two modes, a new
> internal flag is introduced as well.
Just thinking out loud, why can't we allow any combination of
HW/DRV/SKB modes? they are totally different attach points in a totally
different checkpoints in a frame life cycle.
Down the road i think we will utilize this fact and start introducing
SKB helpers for SKB mode and driver helpers for DRV mode..
>
> The program query operations is all done under the rtnl lock.
> However,
> the xdp_prog member is __rcu annotated, and used in a lock-less
> manner
> for the SKB_MODE. Now that xdp_prog member is shared from a query
> program perspective, RCU read and assignments functions are still
> used
> when altering xdp_prog, even when only for queries in DRV_MODE. This
> is for sparse/lockdep correctness.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> include/linux/netdevice.h | 15 +++--
> include/net/xdp.h | 4 ++
> net/core/dev.c | 138 ++++++++++++++++++++++++----------
> ----
> net/core/rtnetlink.c | 53 +++++++--------
> net/core/xdp.c | 17 +++--
> 5 files changed, 139 insertions(+), 88 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 44b47e9df94a..f3a875a52c6c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1940,6 +1940,9 @@ struct net_device {
> #endif
> struct hlist_node index_hlist;
>
> + struct bpf_prog *xdp_prog_hw;
> + u32 xdp_flags;
> +
> /*
> * Cache lines mostly used on transmit path
> */
> @@ -2043,11 +2046,14 @@ struct net_device {
> };
> #define to_net_dev(d) container_of(d, struct net_device, dev)
>
> +/* Reusing the XDP flags space for kernel internal flag */
> +#define XDP_FLAGS_KERN_GENERIC (1U << 4)
> +static_assert(!(XDP_FLAGS_KERN_GENERIC & XDP_FLAGS_MASK));
> +
> static inline bool netif_elide_gro(const struct net_device *dev)
> {
> - if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
> - return true;
> - return false;
> + return !(dev->features & NETIF_F_GRO) ||
> + dev->xdp_flags & XDP_FLAGS_KERN_GENERIC;
> }
>
> #define NETDEV_ALIGN 32
> @@ -3684,8 +3690,7 @@ struct sk_buff *dev_hard_start_xmit(struct
> sk_buff *skb, struct net_device *dev,
> typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf
> *bpf);
> int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack
> *extack,
> int fd, u32 flags);
> -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
> - enum bpf_netdev_command cmd);
> +u32 dev_xdp_query(struct net_device *dev, unsigned int mode);
> int xdp_umem_query(struct net_device *dev, u16 queue_id);
>
> int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 0f25b3675c5c..3691280c8fc1 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -51,6 +51,7 @@ struct xdp_mem_info {
> };
>
> struct page_pool;
> +struct netlink_ext_ack;
>
> struct zero_copy_allocator {
> void (*free)(struct zero_copy_allocator *zca, unsigned long
> handle);
> @@ -166,4 +167,7 @@ bool xdp_attachment_flags_ok(struct
> xdp_attachment_info *info,
> void xdp_attachment_setup(struct xdp_attachment_info *info,
> struct netdev_bpf *bpf);
>
> +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> + struct netlink_ext_ack *extack);
> +
> #endif /* __LINUX_NET_XDP_H__ */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b6b8505cfb3e..1a9da508149a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8005,21 +8005,43 @@ int dev_change_proto_down_generic(struct
> net_device *dev, bool proto_down)
> }
> EXPORT_SYMBOL(dev_change_proto_down_generic);
>
> -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t bpf_op,
> - enum bpf_netdev_command cmd)
> +static u32 dev_xdp_query_generic(struct net_device *dev)
> {
> - struct netdev_bpf xdp;
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
>
> - if (!bpf_op)
> - return 0;
> + return prog && dev->xdp_flags & XDP_FLAGS_KERN_GENERIC ?
> + prog->aux->id : 0;
> +}
>
> - memset(&xdp, 0, sizeof(xdp));
> - xdp.command = cmd;
> +static u32 dev_xdp_query_drv(struct net_device *dev)
> +{
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> +
> + return prog && !(dev->xdp_flags & XDP_FLAGS_KERN_GENERIC) ?
> + prog->aux->id : 0;
> +}
> +
> +static u32 dev_xdp_current_mode(struct net_device *dev)
> +{
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
>
> - /* Query must always succeed. */
> - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> + if (prog)
> + return dev_xdp_query_generic(dev) ? XDP_FLAGS_SKB_MODE
> :
> + XDP_FLAGS_DRV_MODE;
> + return 0;
> +}
>
> - return xdp.prog_id;
> +u32 dev_xdp_query(struct net_device *dev, unsigned int mode)
> +{
> + switch (mode) {
> + case XDP_FLAGS_DRV_MODE:
> + return dev_xdp_query_drv(dev);
> + case XDP_FLAGS_SKB_MODE:
> + return dev_xdp_query_generic(dev);
> + case XDP_FLAGS_HW_MODE:
> + return dev->xdp_prog_hw ? dev->xdp_prog_hw->aux->id :
> 0;
> + }
> + return 0;
> }
>
> static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> @@ -8027,45 +8049,47 @@ static int dev_xdp_install(struct net_device
> *dev, bpf_op_t bpf_op,
> struct bpf_prog *prog)
> {
> struct netdev_bpf xdp;
> + int err;
>
> memset(&xdp, 0, sizeof(xdp));
> - if (flags & XDP_FLAGS_HW_MODE)
> + if (flags & XDP_FLAGS_HW_MODE) {
> xdp.command = XDP_SETUP_PROG_HW;
> - else
> + xdp.flags = XDP_FLAGS_HW_MODE;
> + } else {
> xdp.command = XDP_SETUP_PROG;
> + xdp.flags = flags;
> + }
> xdp.extack = extack;
> - xdp.flags = flags;
> xdp.prog = prog;
>
> - return bpf_op(dev, &xdp);
> + err = bpf_op(dev, &xdp);
> + if (err)
> + return err;
> +
> + if (flags & XDP_FLAGS_HW_MODE) {
> + dev->xdp_prog_hw = prog;
> + return 0;
> + }
> +
> + rcu_assign_pointer(dev->xdp_prog, prog);
> + dev->xdp_flags = prog ? flags : 0;
> + return 0;
> }
>
> static void dev_xdp_uninstall(struct net_device *dev)
> {
> - struct netdev_bpf xdp;
> - bpf_op_t ndo_bpf;
> -
> - /* Remove generic XDP */
> - WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0,
> NULL));
> -
> - /* Remove from the driver */
> - ndo_bpf = dev->netdev_ops->ndo_bpf;
> - if (!ndo_bpf)
> - return;
> -
> - memset(&xdp, 0, sizeof(xdp));
> - xdp.command = XDP_QUERY_PROG;
> - WARN_ON(ndo_bpf(dev, &xdp));
> - if (xdp.prog_id)
> - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL,
> xdp.prog_flags,
> - NULL));
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> + bpf_op_t bpf_op;
>
> - /* Remove HW offload */
> - memset(&xdp, 0, sizeof(xdp));
> - xdp.command = XDP_QUERY_PROG_HW;
> - if (!ndo_bpf(dev, &xdp) && xdp.prog_id)
> - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL,
> xdp.prog_flags,
> + if (prog) {
> + bpf_op = dev_xdp_query_generic(dev) ?
> + generic_xdp_install : dev->netdev_ops-
> >ndo_bpf;
> + WARN_ON(dev_xdp_install(dev, bpf_op, NULL, dev-
> >xdp_flags,
> NULL));
> + }
> + if (dev_xdp_query(dev, XDP_FLAGS_HW_MODE))
> + WARN_ON(dev_xdp_install(dev, dev->netdev_ops->ndo_bpf,
> + NULL, XDP_FLAGS_HW_MODE,
> NULL));
> }
>
> /**
> @@ -8080,41 +8104,49 @@ static void dev_xdp_uninstall(struct
> net_device *dev)
> int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack
> *extack,
> int fd, u32 flags)
> {
> - const struct net_device_ops *ops = dev->netdev_ops;
> - enum bpf_netdev_command query;
> struct bpf_prog *prog = NULL;
> - bpf_op_t bpf_op, bpf_chk;
> + u32 mode, curr_mode;
> + bpf_op_t bpf_op;
> bool offload;
> int err;
>
> ASSERT_RTNL();
>
> offload = flags & XDP_FLAGS_HW_MODE;
> - query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> + mode = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
>
> - bpf_op = bpf_chk = ops->ndo_bpf;
> + bpf_op = dev->netdev_ops->ndo_bpf;
> 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)
> - bpf_chk = generic_xdp_install;
>
> - if (fd >= 0) {
> - if (!offload && __dev_xdp_query(dev, bpf_chk,
> XDP_QUERY_PROG)) {
> + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> + mode = XDP_FLAGS_SKB_MODE;
> +
> + curr_mode = dev_xdp_current_mode(dev);
> +
> + if (!offload && curr_mode && (mode ^ curr_mode) &
> + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
if i am reading this correctly this is equivalent to :
if (!offload && (curre_mode != mode))
offlad is false then curr_mode and mode must be DRV or GENERIC ..
better if you keep bitwise operations for actual bitmasks, mode and
curr_mode are not bitmask, they can hold one value each .. according to
your logic..
> + if (fd >= 0) {
> 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)) {
> + return 0;
> + }
> +
> + if (!offload && dev_xdp_query(dev, mode) &&
> + !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
> + return -EBUSY;
> +
> + if (fd >= 0) {
> + if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
> + dev_xdp_query(dev, mode)) {
> 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);
> + prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP,
> !!bpf_op);
> if (IS_ERR(prog))
> return PTR_ERR(prog);
>
> @@ -8125,6 +8157,10 @@ int dev_change_xdp_fd(struct net_device *dev,
> struct netlink_ext_ack *extack,
> }
> }
>
> + if (mode == XDP_FLAGS_SKB_MODE) {
> + bpf_op = generic_xdp_install;
> + flags |= XDP_FLAGS_KERN_GENERIC;
> + }
> err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> if (err < 0 && prog)
> bpf_prog_put(prog);
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index adcc045952c2..5e396fd01d8b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,42 +1360,44 @@ static int rtnl_fill_link_ifmap(struct
> sk_buff *skb, struct net_device *dev)
> return 0;
> }
>
> -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> {
> - const struct bpf_prog *generic_xdp_prog;
> -
> - ASSERT_RTNL();
> -
> - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> - if (!generic_xdp_prog)
> - return 0;
> - return generic_xdp_prog->aux->id;
> -}
> -
> -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> -{
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> XDP_QUERY_PROG);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return XDP_FLAGS_DRV_MODE;
> + case XDP_ATTACHED_SKB:
> + return XDP_FLAGS_SKB_MODE;
> + case XDP_ATTACHED_HW:
> + return XDP_FLAGS_HW_MODE;
> + }
> + return 0;
> }
>
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> {
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> - XDP_QUERY_PROG_HW);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return IFLA_XDP_DRV_PROG_ID;
> + case XDP_ATTACHED_SKB:
> + return IFLA_XDP_SKB_PROG_ID;
> + case XDP_ATTACHED_HW:
> + return IFLA_XDP_HW_PROG_ID;
> + }
> + return 0;
> }
>
> static int rtnl_xdp_report_one(struct sk_buff *skb, struct
> net_device *dev,
> - u32 *prog_id, u8 *mode, u8 tgt_mode, u32
> attr,
> - u32 (*get_prog_id)(struct net_device
> *dev))
> + u32 *prog_id, u8 *mode, u8 tgt_mode)
> {
> u32 curr_id;
> int err;
>
> - curr_id = get_prog_id(dev);
> + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> if (!curr_id)
> return 0;
>
> *prog_id = curr_id;
> - err = nla_put_u32(skb, attr, curr_id);
> + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode),
> curr_id);
> if (err)
> return err;
>
> @@ -1420,16 +1422,13 @@ static int rtnl_xdp_fill(struct sk_buff *skb,
> struct net_device *dev)
>
> prog_id = 0;
> mode = XDP_ATTACHED_NONE;
> - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_SKB,
> - IFLA_XDP_SKB_PROG_ID,
> rtnl_xdp_prog_skb);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_SKB);
> if (err)
> goto err_cancel;
> - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_DRV,
> - IFLA_XDP_DRV_PROG_ID,
> rtnl_xdp_prog_drv);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_DRV);
> if (err)
> goto err_cancel;
> - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_HW,
> - IFLA_XDP_HW_PROG_ID,
> rtnl_xdp_prog_hw);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_HW);
> if (err)
> goto err_cancel;
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 4b2b194f4f1f..af92c04a58d8 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -388,16 +388,23 @@ int xdp_attachment_query(struct
> xdp_attachment_info *info,
> }
> EXPORT_SYMBOL_GPL(xdp_attachment_query);
>
> -bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> - struct netdev_bpf *bpf)
> +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> + struct netlink_ext_ack *extack)
> {
> - if (info->prog && (bpf->flags ^ info->flags) & XDP_FLAGS_MODES)
> {
> - NL_SET_ERR_MSG(bpf->extack,
> - "program loaded with different flags");
> + if ((new_flags ^ old_flags) & XDP_FLAGS_MODES) {
> + NL_SET_ERR_MSG(extack, "program loaded with different
> flags");
> return false;
> }
> return true;
> }
> +
> +bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> + struct netdev_bpf *bpf)
> +{
> + if (info->prog)
> + return xdp_prog_flags_ok(bpf->flags, info->flags, bpf-
> >extack);
> + return true;
> +}
> EXPORT_SYMBOL_GPL(xdp_attachment_flags_ok);
>
> void xdp_attachment_setup(struct xdp_attachment_info *info,
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 19:18 ` Saeed Mahameed
@ 2019-06-01 19:42 ` Jakub Kicinski
2019-06-03 9:04 ` Björn Töpel
2019-06-01 19:57 ` Jakub Kicinski
1 sibling, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2019-06-01 19:42 UTC (permalink / raw)
To: Saeed Mahameed
Cc: toke@redhat.com, daniel@iogearbox.net, ast@kernel.org,
netdev@vger.kernel.org, bjorn.topel@gmail.com,
magnus.karlsson@intel.com, bjorn.topel@intel.com,
brouer@redhat.com, bpf@vger.kernel.org
On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> On Fri, 2019-05-31 at 11:42 +0200, Björn Töpel wrote:
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
> > command of ndo_bpf. The query code is fairly generic. This commit
> > refactors the query code up from the drivers to the netdev level.
> >
> > The struct net_device has gained two new members: xdp_prog_hw and
> > xdp_flags. The former is the offloaded XDP program, if any, and the
> > latter tracks the flags that the supplied when attaching the XDP
> > program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
> >
> > The xdp_prog member, previously only used for SKB_MODE, is shared
> > with
> > DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> > mutually exclusive. To differentiate between the two modes, a new
> > internal flag is introduced as well.
>
> Just thinking out loud, why can't we allow any combination of
> HW/DRV/SKB modes? they are totally different attach points in a totally
> different checkpoints in a frame life cycle.
FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
> Down the road i think we will utilize this fact and start introducing
> SKB helpers for SKB mode and driver helpers for DRV mode..
Any reason why we would want the extra complexity? There is cls_bpf
if someone wants skb features after all..
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-01 19:42 ` Jakub Kicinski
@ 2019-06-03 9:04 ` Björn Töpel
2019-06-03 21:20 ` Saeed Mahameed
0 siblings, 1 reply; 23+ messages in thread
From: Björn Töpel @ 2019-06-03 9:04 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Saeed Mahameed, toke@redhat.com, daniel@iogearbox.net,
ast@kernel.org, netdev@vger.kernel.org, magnus.karlsson@intel.com,
bjorn.topel@intel.com, brouer@redhat.com, bpf@vger.kernel.org
On Sat, 1 Jun 2019 at 21:42, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > On Fri, 2019-05-31 at 11:42 +0200, Björn Töpel wrote:
> > > From: Björn Töpel <bjorn.topel@intel.com>
> > >
> > > All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
> > > command of ndo_bpf. The query code is fairly generic. This commit
> > > refactors the query code up from the drivers to the netdev level.
> > >
> > > The struct net_device has gained two new members: xdp_prog_hw and
> > > xdp_flags. The former is the offloaded XDP program, if any, and the
> > > latter tracks the flags that the supplied when attaching the XDP
> > > program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
> > >
> > > The xdp_prog member, previously only used for SKB_MODE, is shared
> > > with
> > > DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> > > mutually exclusive. To differentiate between the two modes, a new
> > > internal flag is introduced as well.
> >
> > Just thinking out loud, why can't we allow any combination of
> > HW/DRV/SKB modes? they are totally different attach points in a totally
> > different checkpoints in a frame life cycle.
>
> FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
>
I've always seen the SKB-mode as something that will eventually be removed.
Clickable link:
https://lore.kernel.org/netdev/20190201080236.446d84d4@redhat.com/ :-P
> > Down the road i think we will utilize this fact and start introducing
> > SKB helpers for SKB mode and driver helpers for DRV mode..
>
> Any reason why we would want the extra complexity? There is cls_bpf
> if someone wants skb features after all..
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 9:04 ` Björn Töpel
@ 2019-06-03 21:20 ` Saeed Mahameed
2019-06-04 5:18 ` Björn Töpel
2019-06-04 10:17 ` Jesper Dangaard Brouer
0 siblings, 2 replies; 23+ messages in thread
From: Saeed Mahameed @ 2019-06-03 21:20 UTC (permalink / raw)
To: jakub.kicinski@netronome.com, bjorn.topel@gmail.com
Cc: toke@redhat.com, magnus.karlsson@intel.com, daniel@iogearbox.net,
brouer@redhat.com, ast@kernel.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, bjorn.topel@intel.com
On Mon, 2019-06-03 at 11:04 +0200, Björn Töpel wrote:
> On Sat, 1 Jun 2019 at 21:42, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > On Fri, 2019-05-31 at 11:42 +0200, Björn Töpel wrote:
> > > > From: Björn Töpel <bjorn.topel@intel.com>
> > > >
> > > > All XDP capable drivers need to implement the
> > > > XDP_QUERY_PROG{,_HW}
> > > > command of ndo_bpf. The query code is fairly generic. This
> > > > commit
> > > > refactors the query code up from the drivers to the netdev
> > > > level.
> > > >
> > > > The struct net_device has gained two new members: xdp_prog_hw
> > > > and
> > > > xdp_flags. The former is the offloaded XDP program, if any, and
> > > > the
> > > > latter tracks the flags that the supplied when attaching the
> > > > XDP
> > > > program. The flags only apply to SKB_MODE or DRV_MODE, not
> > > > HW_MODE.
> > > >
> > > > The xdp_prog member, previously only used for SKB_MODE, is
> > > > shared
> > > > with
> > > > DRV_MODE. This is OK, due to the fact that SKB_MODE and
> > > > DRV_MODE are
> > > > mutually exclusive. To differentiate between the two modes, a
> > > > new
> > > > internal flag is introduced as well.
> > >
> > > Just thinking out loud, why can't we allow any combination of
> > > HW/DRV/SKB modes? they are totally different attach points in a
> > > totally
> > > different checkpoints in a frame life cycle.
> >
> > FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
> >
>
> I've always seen the SKB-mode as something that will eventually be
> removed.
>
I don't think so, we are too deep into SKB-mode.
> Clickable link:
> https://lore.kernel.org/netdev/20190201080236.446d84d4@redhat.com/ :-
> P
>
So we are all hanging on Jesper's refactoring ideas that are not
getting any priority for now :).
> > > Down the road i think we will utilize this fact and start
> > > introducing
> > > SKB helpers for SKB mode and driver helpers for DRV mode..
> >
> > Any reason why we would want the extra complexity? There is
> > cls_bpf
> > if someone wants skb features after all..
Donno, SKB mode is earlier in the stack maybe ..
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 21:20 ` Saeed Mahameed
@ 2019-06-04 5:18 ` Björn Töpel
2019-06-04 10:17 ` Jesper Dangaard Brouer
1 sibling, 0 replies; 23+ messages in thread
From: Björn Töpel @ 2019-06-04 5:18 UTC (permalink / raw)
To: Saeed Mahameed
Cc: jakub.kicinski@netronome.com, toke@redhat.com,
magnus.karlsson@intel.com, daniel@iogearbox.net,
brouer@redhat.com, ast@kernel.org, netdev@vger.kernel.org,
bpf@vger.kernel.org, bjorn.topel@intel.com
On Mon, 3 Jun 2019 at 23:20, Saeed Mahameed <saeedm@mellanox.com> wrote:
>
> On Mon, 2019-06-03 at 11:04 +0200, Björn Töpel wrote:
> > On Sat, 1 Jun 2019 at 21:42, Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > > On Fri, 2019-05-31 at 11:42 +0200, Björn Töpel wrote:
> > > > > From: Björn Töpel <bjorn.topel@intel.com>
> > > > >
> > > > > All XDP capable drivers need to implement the
> > > > > XDP_QUERY_PROG{,_HW}
> > > > > command of ndo_bpf. The query code is fairly generic. This
> > > > > commit
> > > > > refactors the query code up from the drivers to the netdev
> > > > > level.
> > > > >
> > > > > The struct net_device has gained two new members: xdp_prog_hw
> > > > > and
> > > > > xdp_flags. The former is the offloaded XDP program, if any, and
> > > > > the
> > > > > latter tracks the flags that the supplied when attaching the
> > > > > XDP
> > > > > program. The flags only apply to SKB_MODE or DRV_MODE, not
> > > > > HW_MODE.
> > > > >
> > > > > The xdp_prog member, previously only used for SKB_MODE, is
> > > > > shared
> > > > > with
> > > > > DRV_MODE. This is OK, due to the fact that SKB_MODE and
> > > > > DRV_MODE are
> > > > > mutually exclusive. To differentiate between the two modes, a
> > > > > new
> > > > > internal flag is introduced as well.
> > > >
> > > > Just thinking out loud, why can't we allow any combination of
> > > > HW/DRV/SKB modes? they are totally different attach points in a
> > > > totally
> > > > different checkpoints in a frame life cycle.
> > >
> > > FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
> > >
> >
> > I've always seen the SKB-mode as something that will eventually be
> > removed.
> >
>
> I don't think so, we are too deep into SKB-mode.
>
You might be right. :-(
Are you envisioning sk_buff specfic XDP_SKB functions, that only apply
for XDP_SKB? Ick.
> > Clickable link:
> > https://lore.kernel.org/netdev/20190201080236.446d84d4@redhat.com/ :-
> > P
> >
>
> So we are all hanging on Jesper's refactoring ideas that are not
> getting any priority for now :).
>
>
> > > > Down the road i think we will utilize this fact and start
> > > > introducing
> > > > SKB helpers for SKB mode and driver helpers for DRV mode..
> > >
> > > Any reason why we would want the extra complexity? There is
> > > cls_bpf
> > > if someone wants skb features after all..
>
> Donno, SKB mode is earlier in the stack maybe ..
>
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 21:20 ` Saeed Mahameed
2019-06-04 5:18 ` Björn Töpel
@ 2019-06-04 10:17 ` Jesper Dangaard Brouer
1 sibling, 0 replies; 23+ messages in thread
From: Jesper Dangaard Brouer @ 2019-06-04 10:17 UTC (permalink / raw)
To: Saeed Mahameed
Cc: jakub.kicinski@netronome.com, bjorn.topel@gmail.com,
toke@redhat.com, magnus.karlsson@intel.com, daniel@iogearbox.net,
ast@kernel.org, netdev@vger.kernel.org, bpf@vger.kernel.org,
bjorn.topel@intel.com, brouer, John Fastabend
On Mon, 3 Jun 2019 21:20:30 +0000
Saeed Mahameed <saeedm@mellanox.com> wrote:
> On Mon, 2019-06-03 at 11:04 +0200, Björn Töpel wrote:
> > On Sat, 1 Jun 2019 at 21:42, Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > > On Fri, 2019-05-31 at 11:42 +0200, Björn Töpel wrote:
> > > > > From: Björn Töpel <bjorn.topel@intel.com>
> > > > >
> > > > > All XDP capable drivers need to implement the
> > > > > XDP_QUERY_PROG{,_HW} command of ndo_bpf. The query code is
> > > > > fairly generic. This commit refactors the query code up from
> > > > > the drivers to the netdev level.
> > > > >
> > > > > The struct net_device has gained two new members: xdp_prog_hw
> > > > > and xdp_flags. The former is the offloaded XDP program, if
> > > > > any, and the latter tracks the flags that the supplied when
> > > > > attaching the XDP program. The flags only apply to SKB_MODE
> > > > > or DRV_MODE, not HW_MODE.
> > > > >
> > > > > The xdp_prog member, previously only used for SKB_MODE, is
> > > > > shared with DRV_MODE. This is OK, due to the fact that
> > > > > SKB_MODE and DRV_MODE are mutually exclusive. To
> > > > > differentiate between the two modes, a new internal flag is
> > > > > introduced as well.
> > > >
> > > > Just thinking out loud, why can't we allow any combination of
> > > > HW/DRV/SKB modes? they are totally different attach points in a
> > > > totally different checkpoints in a frame life cycle.
The DRV_MODE and SKB_MODE is tricky to run concurrently. The XDP-redirect
scheme (designed by John) use a BPF helper (bpf_xdp_redirect_map) that
set global per-CPU state (this_cpu_ptr(&bpf_redirect_info)).
The tricky part (which I warned about, and we already have some fixes
for) is that the XDP/BPF-prog can call bpf_redirect_map, which update
the per-CPU state, but it can still choose not to return XDP_REDIRECT,
which then miss an invocation of xdp_do_redirect().
Later, your SKB_MODE XDP/BPF-prog can return XDP_REDIRECT without
calling the helper, and then use/reach to the per-CPU info set by the
DRV_MODE program, which is NOT something I want us to "support".
> > > FWIW see Message-ID: <20190201080236.446d84d4@redhat.com>
> >
> > I've always seen the SKB-mode as something that will eventually be
> > removed.
>
> I don't think so, we are too deep into SKB-mode.
I wish we could remove it. After we got native XDP support in veth,
then its original purpose is gone, which were making it easier for
developers to get something working on their laptop, without having to
deploy it to the server with physical hardware all the time.
> > Clickable link:
> > https://lore.kernel.org/netdev/20190201080236.446d84d4@redhat.com/
> >
>
> So we are all hanging on Jesper's refactoring ideas that are not
> getting any priority for now :).
Well, that is not true. This patchset _is_ the refactor idea that
Bjørn is taking over and working on. Specifically [2] from above link.
[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
> > > > Down the road i think we will utilize this fact and start
> > > > introducing SKB helpers for SKB mode and driver helpers for DRV
> > > > mode..
> > >
> > > Any reason why we would want the extra complexity? There is
> > > cls_bpf if someone wants skb features after all..
>
> Donno, SKB mode is earlier in the stack maybe ..
From a BPF perspective you cannot introduce SKB helpers for SKB mode.
An XDP-prog have bpf prog type XDP (BPF_PROG_TYPE_XDP), and the program
itself is identical regardless of attaching for DRV_MODE or SKB_MODE.
You cannot detect this at attach time, due to tail-calls (which have
painted us into a corner).
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 19:18 ` Saeed Mahameed
2019-06-01 19:42 ` Jakub Kicinski
@ 2019-06-01 19:57 ` Jakub Kicinski
2019-06-03 9:04 ` Björn Töpel
1 sibling, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2019-06-01 19:57 UTC (permalink / raw)
To: Saeed Mahameed
Cc: toke@redhat.com, daniel@iogearbox.net, ast@kernel.org,
netdev@vger.kernel.org, bjorn.topel@gmail.com,
magnus.karlsson@intel.com, bjorn.topel@intel.com,
brouer@redhat.com, bpf@vger.kernel.org
On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > + mode = XDP_FLAGS_SKB_MODE;
> > +
> > + curr_mode = dev_xdp_current_mode(dev);
> > +
> > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
>
> if i am reading this correctly this is equivalent to :
>
> if (!offload && (curre_mode != mode))
> offlad is false then curr_mode and mode must be DRV or GENERIC ..
Naw, if curr_mode is not set, i.e. nothing installed now, we don't care
about the diff.
> better if you keep bitwise operations for actual bitmasks, mode and
> curr_mode are not bitmask, they can hold one value each .. according to
> your logic..
Well, they hold one bit each, whether one bit is a bitmap perhaps is
disputable? :)
I think the logic is fine.
What happened to my request to move the change in behaviour for
disabling to a separate patch, tho, Bjorn? :)
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-01 19:57 ` Jakub Kicinski
@ 2019-06-03 9:04 ` Björn Töpel
2019-06-03 17:03 ` Jakub Kicinski
0 siblings, 1 reply; 23+ messages in thread
From: Björn Töpel @ 2019-06-03 9:04 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Saeed Mahameed, toke@redhat.com, daniel@iogearbox.net,
ast@kernel.org, netdev@vger.kernel.org, magnus.karlsson@intel.com,
bjorn.topel@intel.com, brouer@redhat.com, bpf@vger.kernel.org
On Sat, 1 Jun 2019 at 21:57, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > > + mode = XDP_FLAGS_SKB_MODE;
> > > +
> > > + curr_mode = dev_xdp_current_mode(dev);
> > > +
> > > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> >
> > if i am reading this correctly this is equivalent to :
> >
> > if (!offload && (curre_mode != mode))
> > offlad is false then curr_mode and mode must be DRV or GENERIC ..
>
> Naw, if curr_mode is not set, i.e. nothing installed now, we don't care
> about the diff.
>
> > better if you keep bitwise operations for actual bitmasks, mode and
> > curr_mode are not bitmask, they can hold one value each .. according to
> > your logic..
>
> Well, they hold one bit each, whether one bit is a bitmap perhaps is
> disputable? :)
>
> I think the logic is fine.
>
Hmm, but changing to:
if (!offload && curr_mode && mode != curr_mode)
is equal, and to Saeed's point, clearer. I'll go that route in a v3.
> What happened to my request to move the change in behaviour for
> disabling to a separate patch, tho, Bjorn? :)
Actually, I left that out completely. This patch doesn't change the
behavior. After I realized how the flags *should* be used, I don't
think my v1 change makes sense anymore. My v1 patch was to give an
error if you tried to disable, say generic if drv was enabled via
"auto detect/no flags". But this is catched by looking at the flags.
What I did, however, was moving the flags check into change_fd so that
the driver doesn't have to do the check. E.g. the Intel drivers didn't
do correct checking of flags.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 9:04 ` Björn Töpel
@ 2019-06-03 17:03 ` Jakub Kicinski
2019-06-04 5:16 ` Björn Töpel
0 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2019-06-03 17:03 UTC (permalink / raw)
To: Björn Töpel
Cc: Saeed Mahameed, toke@redhat.com, daniel@iogearbox.net,
ast@kernel.org, netdev@vger.kernel.org, magnus.karlsson@intel.com,
bjorn.topel@intel.com, brouer@redhat.com, bpf@vger.kernel.org
On Mon, 3 Jun 2019 11:04:36 +0200, Björn Töpel wrote:
> On Sat, 1 Jun 2019 at 21:57, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> >
> > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > > > + mode = XDP_FLAGS_SKB_MODE;
> > > > +
> > > > + curr_mode = dev_xdp_current_mode(dev);
> > > > +
> > > > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > > > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> > >
> > > if i am reading this correctly this is equivalent to :
> > >
> > > if (!offload && (curre_mode != mode))
> > > offlad is false then curr_mode and mode must be DRV or GENERIC ..
> >
> > Naw, if curr_mode is not set, i.e. nothing installed now, we don't care
> > about the diff.
> >
> > > better if you keep bitwise operations for actual bitmasks, mode and
> > > curr_mode are not bitmask, they can hold one value each .. according to
> > > your logic..
> >
> > Well, they hold one bit each, whether one bit is a bitmap perhaps is
> > disputable? :)
> >
> > I think the logic is fine.
> >
>
> Hmm, but changing to:
>
> if (!offload && curr_mode && mode != curr_mode)
>
> is equal, and to Saeed's point, clearer. I'll go that route in a v3.
Sorry, you're right, the flags get mangled before they get here, so
yeah, this condition should work. Confusingly.
> > What happened to my request to move the change in behaviour for
> > disabling to a separate patch, tho, Bjorn? :)
>
> Actually, I left that out completely. This patch doesn't change the
> behavior. After I realized how the flags *should* be used, I don't
> think my v1 change makes sense anymore. My v1 patch was to give an
> error if you tried to disable, say generic if drv was enabled via
> "auto detect/no flags". But this is catched by looking at the flags.
>
> What I did, however, was moving the flags check into change_fd so that
> the driver doesn't have to do the check. E.g. the Intel drivers didn't
> do correct checking of flags.
Ugh. Could you please rewrite the conditions to make the fd >= check
consistently the outside if? Also could you add extack to this:
+ if (!offload && dev_xdp_query(dev, mode) &&
+ !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
+ return -EBUSY;
It's unclear what it's doing.
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 17:03 ` Jakub Kicinski
@ 2019-06-04 5:16 ` Björn Töpel
0 siblings, 0 replies; 23+ messages in thread
From: Björn Töpel @ 2019-06-04 5:16 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Saeed Mahameed, toke@redhat.com, daniel@iogearbox.net,
ast@kernel.org, netdev@vger.kernel.org, magnus.karlsson@intel.com,
bjorn.topel@intel.com, brouer@redhat.com, bpf@vger.kernel.org
On Mon, 3 Jun 2019 at 19:03, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Mon, 3 Jun 2019 11:04:36 +0200, Björn Töpel wrote:
> > On Sat, 1 Jun 2019 at 21:57, Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > >
> > > On Fri, 31 May 2019 19:18:17 +0000, Saeed Mahameed wrote:
> > > > > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > > > > + mode = XDP_FLAGS_SKB_MODE;
> > > > > +
> > > > > + curr_mode = dev_xdp_current_mode(dev);
> > > > > +
> > > > > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > > > > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> > > >
> > > > if i am reading this correctly this is equivalent to :
> > > >
> > > > if (!offload && (curre_mode != mode))
> > > > offlad is false then curr_mode and mode must be DRV or GENERIC ..
> > >
> > > Naw, if curr_mode is not set, i.e. nothing installed now, we don't care
> > > about the diff.
> > >
> > > > better if you keep bitwise operations for actual bitmasks, mode and
> > > > curr_mode are not bitmask, they can hold one value each .. according to
> > > > your logic..
> > >
> > > Well, they hold one bit each, whether one bit is a bitmap perhaps is
> > > disputable? :)
> > >
> > > I think the logic is fine.
> > >
> >
> > Hmm, but changing to:
> >
> > if (!offload && curr_mode && mode != curr_mode)
> >
> > is equal, and to Saeed's point, clearer. I'll go that route in a v3.
>
> Sorry, you're right, the flags get mangled before they get here, so
> yeah, this condition should work. Confusingly.
>
> > > What happened to my request to move the change in behaviour for
> > > disabling to a separate patch, tho, Bjorn? :)
> >
> > Actually, I left that out completely. This patch doesn't change the
> > behavior. After I realized how the flags *should* be used, I don't
> > think my v1 change makes sense anymore. My v1 patch was to give an
> > error if you tried to disable, say generic if drv was enabled via
> > "auto detect/no flags". But this is catched by looking at the flags.
> >
> > What I did, however, was moving the flags check into change_fd so that
> > the driver doesn't have to do the check. E.g. the Intel drivers didn't
> > do correct checking of flags.
>
> Ugh. Could you please rewrite the conditions to make the fd >= check
> consistently the outside if? Also could you add extack to this:
>
The reason I moved the if-statement (checking if we're mixing
drv/skb), is because I'd like to catch the no-op (e.g. xdpdrv active
and calling xdpgeneric off) early (the return 0, under the if (fd >=
check).
> + if (!offload && dev_xdp_query(dev, mode) &&
> + !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
> + return -EBUSY;
>
> It's unclear what it's doing.
This checks whether the flags have changed, pulling out the logic from
the drivers. xdp_prog_flags_ok adds to extack, resuing the flags_ok
function. The xdp_attachment_flags_ok OTOH is not necessary anymore,
and should be further cleaned up. I'll address this and make the this
clause more clear.
Björn
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-31 19:18 ` Saeed Mahameed
@ 2019-06-01 9:50 ` kbuild test robot
2019-06-01 18:12 ` Jonathan Lemon
2019-06-01 20:02 ` Jakub Kicinski
3 siblings, 0 replies; 23+ messages in thread
From: kbuild test robot @ 2019-06-01 9:50 UTC (permalink / raw)
To: Björn Töpel
Cc: kbuild-all, toke, ast, daniel, netdev, Björn Töpel,
magnus.karlsson, brouer, bpf, jakub.kicinski, saeedm
[-- Attachment #1: Type: text/plain, Size: 24325 bytes --]
Hi "Björn,
I love your patch! Perhaps something to improve:
[auto build test WARNING on bpf-next/master]
url: https://github.com/0day-ci/linux/commits/Bj-rn-T-pel/net-xdp-refactor-XDP_QUERY_PROG-_HW-to-netdev/20190601-053952
base: https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf-next.git master
reproduce: make htmldocs
If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:344: warning: Function parameter or member 'range' not described in 'amdgpu_mn_invalidate_range_end'
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:344: warning: Excess function parameter 'mm' description in 'amdgpu_mn_invalidate_range_end'
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:344: warning: Excess function parameter 'start' description in 'amdgpu_mn_invalidate_range_end'
drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:344: warning: Excess function parameter 'end' description in 'amdgpu_mn_invalidate_range_end'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:347: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:348: warning: cannot understand function prototype: 'struct amdgpu_vm_pt_cursor '
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:494: warning: Function parameter or member 'start' not described in 'amdgpu_vm_pt_first_dfs'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'adev' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'vm' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'start' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'cursor' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:546: warning: Function parameter or member 'entry' not described in 'for_each_amdgpu_vm_pt_dfs_safe'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:823: warning: Function parameter or member 'level' not described in 'amdgpu_vm_bo_param'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'params' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'bo' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'level' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'pe' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'addr' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'count' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'incr' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1285: warning: Function parameter or member 'flags' not described in 'amdgpu_vm_update_flags'
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2812: warning: Function parameter or member 'pasid' not described in 'amdgpu_vm_make_compute'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:375: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:376: warning: Function parameter or member 'ih' not described in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_irq.c:376: warning: Excess function parameter 'entry' description in 'amdgpu_irq_dispatch'
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:1: warning: no structured comments found
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:128: warning: Incorrect use of kernel-doc format: Documentation Makefile include scripts source @atomic_obj
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'atomic_obj' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'backlight_link' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'backlight_caps' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'freesync_module' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'fw_dmcu' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h:210: warning: Function parameter or member 'dmcu_fw_version' not described in 'amdgpu_display_manager'
drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c:1: warning: no structured comments found
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
include/drm/drm_drv.h:722: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
include/drm/drm_modeset_helper_vtables.h:1004: warning: Function parameter or member 'prepare_writeback_job' not described in 'drm_connector_helper_funcs'
include/drm/drm_modeset_helper_vtables.h:1004: warning: Function parameter or member 'cleanup_writeback_job' not described in 'drm_connector_helper_funcs'
include/drm/drm_atomic_state_helper.h:1: warning: no structured comments found
drivers/gpu/drm/scheduler/sched_main.c:419: warning: Function parameter or member 'full_recovery' not described in 'drm_sched_start'
drivers/gpu/drm/i915/i915_vma.h:50: warning: cannot understand function prototype: 'struct i915_vma '
drivers/gpu/drm/i915/i915_vma.h:1: warning: no structured comments found
drivers/gpu/drm/i915/intel_guc_fwif.h:536: warning: cannot understand function prototype: 'struct guc_log_buffer_state '
drivers/gpu/drm/i915/i915_trace.h:1: warning: no structured comments found
drivers/gpu/drm/i915/i915_reg.h:156: warning: bad line:
include/linux/interconnect.h:1: warning: no structured comments found
include/linux/skbuff.h:897: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'list' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'skb_mstamp_ns' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member '__pkt_vlan_present_offset' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'vlan_present' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
include/linux/skbuff.h:897: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
include/net/sock.h:238: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
include/net/sock.h:238: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
include/net/sock.h:520: warning: Function parameter or member 'sk_rx_skb_cache' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_tx_skb_cache' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
include/net/sock.h:520: warning: Function parameter or member 'sk_bpf_storage' not described in 'sock'
include/linux/netdevice.h:2047: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
>> include/linux/netdevice.h:2047: warning: Function parameter or member 'xdp_prog_hw' not described in 'net_device'
>> include/linux/netdevice.h:2047: warning: Function parameter or member 'xdp_flags' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
include/linux/netdevice.h:2047: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quotactl' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'quota_on' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_free_mnt_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_eat_lsm_opts' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_kern_mount' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_show_options' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'sb_add_mnt_opt' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'd_instantiate' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'getprocattr' not described in 'security_list_options'
include/linux/lsm_hooks.h:1811: warning: Function parameter or member 'setprocattr' not described in 'security_list_options'
Documentation/admin-guide/mm/numaperf.rst:24: ERROR: Unexpected indentation.
Documentation/admin-guide/mm/numaperf.rst:24: WARNING: Inline substitution_reference start-string without end-string.
Documentation/admin-guide/mm/numaperf.rst:25: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/bpf/btf.rst:154: ERROR: Unexpected indentation.
Documentation/bpf/btf.rst:163: ERROR: Unexpected indentation.
lib/list_sort.c:128: WARNING: Definition list ends without a blank line; unexpected unindent.
lib/list_sort.c:161: ERROR: Unexpected indentation.
lib/list_sort.c:162: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/xarray.h:232: ERROR: Unexpected indentation.
kernel/time/hrtimer.c:1120: WARNING: Block quote ends without a blank line; unexpected unindent.
kernel/signal.c:349: WARNING: Inline literal start-string without end-string.
include/uapi/linux/firewire-cdev.h:312: WARNING: Inline literal start-string without end-string.
Documentation/driver-api/gpio/driver.rst:419: ERROR: Unknown target name: "devm".
include/linux/i2c.h:510: WARNING: Inline strong start-string without end-string.
drivers/ata/libata-core.c:5944: ERROR: Unknown target name: "hw".
drivers/message/fusion/mptbase.c:5057: WARNING: Definition list ends without a blank line; unexpected unindent.
drivers/tty/serial/serial_core.c:1959: WARNING: Definition list ends without a blank line; unexpected unindent.
include/linux/regulator/driver.h:289: ERROR: Unknown target name: "regulator_regmap_x_voltage".
Documentation/driver-api/soundwire/locking.rst:50: ERROR: Inconsistent literal block quoting.
Documentation/driver-api/soundwire/locking.rst:51: WARNING: Line block ends without a blank line.
Documentation/driver-api/soundwire/locking.rst:55: WARNING: Inline substitution_reference start-string without end-string.
Documentation/driver-api/soundwire/locking.rst:56: WARNING: Line block ends without a blank line.
include/linux/spi/spi.h:380: ERROR: Unexpected indentation.
fs/posix_acl.c:636: WARNING: Inline emphasis start-string without end-string.
fs/debugfs/inode.c:385: WARNING: Inline literal start-string without end-string.
fs/debugfs/inode.c:464: WARNING: Inline literal start-string without end-string.
fs/debugfs/inode.c:496: WARNING: Inline literal start-string without end-string.
fs/debugfs/inode.c:583: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:394: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:400: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:439: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:445: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:484: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:490: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:530: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:536: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:578: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:584: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:845: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:851: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:898: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:904: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1001: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1001: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1096: WARNING: Inline literal start-string without end-string.
fs/debugfs/file.c:1102: WARNING: Inline literal start-string without end-string.
Documentation/firmware-guide/acpi/enumeration.rst:430: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:432: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:434: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:435: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/firmware-guide/acpi/enumeration.rst:436: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/firmware-guide/acpi/enumeration.rst:439: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:441: ERROR: Unexpected indentation.
Documentation/firmware-guide/acpi/enumeration.rst:445: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/firmware-guide/acpi/enumeration.rst:447: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/firmware-guide/acpi/enumeration.rst:448: WARNING: Block quote ends without a blank line; unexpected unindent.
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:2024: WARNING: Inline emphasis start-string without end-string.
drivers/gpu/drm/amd/amdgpu/amdgpu_pm.c:2026: WARNING: Inline emphasis start-string without end-string.
Documentation/networking/af_xdp.rst:319: WARNING: Literal block expected; none found.
Documentation/networking/af_xdp.rst:326: WARNING: Literal block expected; none found.
Documentation/networking/device_drivers/freescale/dpaa2/dpio-driver.rst:43: WARNING: Definition list ends without a blank line; unexpected unindent.
Documentation/networking/device_drivers/freescale/dpaa2/dpio-driver.rst:63: ERROR: Unexpected indentation.
Documentation/networking/dsa/sja1105.rst:91: WARNING: Block quote ends without a blank line; unexpected unindent.
Documentation/networking/dsa/sja1105.rst:91: WARNING: Block quote ends without a blank line; unexpected unindent.
include/linux/netdevice.h:3492: WARNING: Inline emphasis start-string without end-string.
include/linux/netdevice.h:3492: WARNING: Inline emphasis start-string without end-string.
net/core/dev.c:4999: ERROR: Unknown target name: "page_is".
Documentation/powerpc/isa-versions.rst:15: ERROR: Malformed table.
Column span alignment problem in table line 10.
vim +2047 include/linux/netdevice.h
^1da177e4 Linus Torvalds 2005-04-16 2014
43cb76d91 Greg Kroah-Hartman 2002-04-09 2015 struct device dev;
0c509a6c9 Eric W. Biederman 2009-10-29 2016 const struct attribute_group *sysfs_groups[4];
a953be53c Michael Dalton 2014-01-16 2017 const struct attribute_group *sysfs_rx_queue_group;
38f7b870d Patrick McHardy 2007-06-13 2018
38f7b870d Patrick McHardy 2007-06-13 2019 const struct rtnl_link_ops *rtnl_link_ops;
f25f4e448 Peter P Waskiewicz Jr 2007-07-06 2020
82cc1a7a5 Peter P Waskiewicz Jr 2008-03-21 2021 /* for setting kernel sock attribute on TCP connection setup */
82cc1a7a5 Peter P Waskiewicz Jr 2008-03-21 2022 #define GSO_MAX_SIZE 65536
82cc1a7a5 Peter P Waskiewicz Jr 2008-03-21 2023 unsigned int gso_max_size;
30b678d84 Ben Hutchings 2012-07-30 2024 #define GSO_MAX_SEGS 65535
30b678d84 Ben Hutchings 2012-07-30 2025 u16 gso_max_segs;
743b03a83 Eric Dumazet 2016-04-09 2026
7a6b6f515 Jeff Kirsher 2008-11-25 2027 #ifdef CONFIG_DCB
329535432 Stephen Hemminger 2009-10-05 2028 const struct dcbnl_rtnl_ops *dcbnl_ops;
2f90b8657 Alexander Duyck 2008-11-20 2029 #endif
ffcfe25bb Alexander Duyck 2018-07-09 2030 s16 num_tc;
4f57c087d John Fastabend 2011-01-17 2031 struct netdev_tc_txq tc_to_txq[TC_MAX_QUEUE];
4f57c087d John Fastabend 2011-01-17 2032 u8 prio_tc_map[TC_BITMASK + 1];
2f90b8657 Alexander Duyck 2008-11-20 2033
d11ead756 Ben Hutchings 2011-11-25 2034 #if IS_ENABLED(CONFIG_FCOE)
4d288d576 Yi Zou 2009-02-27 2035 unsigned int fcoe_ddp_xid;
4d288d576 Yi Zou 2009-02-27 2036 #endif
86f8515f9 Daniel Borkmann 2013-12-29 2037 #if IS_ENABLED(CONFIG_CGROUP_NET_PRIO)
5bc1421e3 Neil Horman 2011-11-22 2038 struct netprio_map __rcu *priomap;
5bc1421e3 Neil Horman 2011-11-22 2039 #endif
c1f19b51d Richard Cochran 2010-07-17 2040 struct phy_device *phydev;
e679c9c1d Russell King 2018-03-28 2041 struct sfp_bus *sfp_bus;
23d3b8bfb Eric Dumazet 2012-09-05 2042 struct lock_class_key *qdisc_tx_busylock;
f9eb8aea2 Eric Dumazet 2016-06-06 2043 struct lock_class_key *qdisc_running_key;
d746d707a Anuradha Karuppiah 2015-07-14 2044 bool proto_down;
619411432 Heiner Kallweit 2018-09-24 2045 unsigned wol_enabled:1;
^1da177e4 Linus Torvalds 2005-04-16 2046 };
43cb76d91 Greg Kroah-Hartman 2002-04-09 @2047 #define to_net_dev(d) container_of(d, struct net_device, dev)
^1da177e4 Linus Torvalds 2005-04-16 2048
:::::: The code at line 2047 was first introduced by commit
:::::: 43cb76d91ee85f579a69d42bc8efc08bac560278 Network: convert network devices to use struct device instead of class_device
:::::: TO: Greg Kroah-Hartman <gregkh@suse.de>
:::::: CC: Greg Kroah-Hartman <gregkh@suse.de>
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7240 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
2019-05-31 19:18 ` Saeed Mahameed
2019-06-01 9:50 ` kbuild test robot
@ 2019-06-01 18:12 ` Jonathan Lemon
2019-06-03 8:39 ` Björn Töpel
2019-06-01 20:02 ` Jakub Kicinski
3 siblings, 1 reply; 23+ messages in thread
From: Jonathan Lemon @ 2019-06-01 18:12 UTC (permalink / raw)
To: Björn Töpel
Cc: toke, ast, daniel, netdev, Björn Töpel, magnus.karlsson,
brouer, bpf, jakub.kicinski, saeedm
On 31 May 2019, at 2:42, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
> command of ndo_bpf. The query code is fairly generic. This commit
> refactors the query code up from the drivers to the netdev level.
>
> The struct net_device has gained two new members: xdp_prog_hw and
> xdp_flags. The former is the offloaded XDP program, if any, and the
> latter tracks the flags that the supplied when attaching the XDP
> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
>
> The xdp_prog member, previously only used for SKB_MODE, is shared with
> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> mutually exclusive. To differentiate between the two modes, a new
> internal flag is introduced as well.
I'm not entirely clear why this new flag is needed - GENERIC seems to
be an alias for SKB_MODE, so why just use SKB_MODE directly?
If the user does not explicitly specify a type (skb|drv|hw), then the
command should choose the correct type and then behave as if this type
was specified.
The logic in dev_change_xdp_fd() is too complicated. It disallows
setting (drv|skb), but allows (hw|skb), which I'm not sure is
intentional.
It should be clearer as to which combinations are allowed.
--
Jonathan
>
> The program query operations is all done under the rtnl lock. However,
> the xdp_prog member is __rcu annotated, and used in a lock-less manner
> for the SKB_MODE. Now that xdp_prog member is shared from a query
> program perspective, RCU read and assignments functions are still used
> when altering xdp_prog, even when only for queries in DRV_MODE. This
> is for sparse/lockdep correctness.
>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> ---
> include/linux/netdevice.h | 15 +++--
> include/net/xdp.h | 4 ++
> net/core/dev.c | 138
> ++++++++++++++++++++++++--------------
> net/core/rtnetlink.c | 53 +++++++--------
> net/core/xdp.c | 17 +++--
> 5 files changed, 139 insertions(+), 88 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 44b47e9df94a..f3a875a52c6c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1940,6 +1940,9 @@ struct net_device {
> #endif
> struct hlist_node index_hlist;
>
> + struct bpf_prog *xdp_prog_hw;
> + u32 xdp_flags;
> +
> /*
> * Cache lines mostly used on transmit path
> */
> @@ -2043,11 +2046,14 @@ struct net_device {
> };
> #define to_net_dev(d) container_of(d, struct net_device, dev)
>
> +/* Reusing the XDP flags space for kernel internal flag */
> +#define XDP_FLAGS_KERN_GENERIC (1U << 4)
> +static_assert(!(XDP_FLAGS_KERN_GENERIC & XDP_FLAGS_MASK));
> +
> static inline bool netif_elide_gro(const struct net_device *dev)
> {
> - if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
> - return true;
> - return false;
> + return !(dev->features & NETIF_F_GRO) ||
> + dev->xdp_flags & XDP_FLAGS_KERN_GENERIC;
> }
>
> #define NETDEV_ALIGN 32
> @@ -3684,8 +3690,7 @@ struct sk_buff *dev_hard_start_xmit(struct
> sk_buff *skb, struct net_device *dev,
> typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf
> *bpf);
> int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack
> *extack,
> int fd, u32 flags);
> -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
> - enum bpf_netdev_command cmd);
> +u32 dev_xdp_query(struct net_device *dev, unsigned int mode);
> int xdp_umem_query(struct net_device *dev, u16 queue_id);
>
> int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index 0f25b3675c5c..3691280c8fc1 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h
> @@ -51,6 +51,7 @@ struct xdp_mem_info {
> };
>
> struct page_pool;
> +struct netlink_ext_ack;
>
> struct zero_copy_allocator {
> void (*free)(struct zero_copy_allocator *zca, unsigned long handle);
> @@ -166,4 +167,7 @@ bool xdp_attachment_flags_ok(struct
> xdp_attachment_info *info,
> void xdp_attachment_setup(struct xdp_attachment_info *info,
> struct netdev_bpf *bpf);
>
> +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> + struct netlink_ext_ack *extack);
> +
> #endif /* __LINUX_NET_XDP_H__ */
> diff --git a/net/core/dev.c b/net/core/dev.c
> index b6b8505cfb3e..1a9da508149a 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8005,21 +8005,43 @@ int dev_change_proto_down_generic(struct
> net_device *dev, bool proto_down)
> }
> EXPORT_SYMBOL(dev_change_proto_down_generic);
>
> -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t bpf_op,
> - enum bpf_netdev_command cmd)
> +static u32 dev_xdp_query_generic(struct net_device *dev)
> {
> - struct netdev_bpf xdp;
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
>
> - if (!bpf_op)
> - return 0;
> + return prog && dev->xdp_flags & XDP_FLAGS_KERN_GENERIC ?
> + prog->aux->id : 0;
> +}
>
> - memset(&xdp, 0, sizeof(xdp));
> - xdp.command = cmd;
> +static u32 dev_xdp_query_drv(struct net_device *dev)
> +{
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> +
> + return prog && !(dev->xdp_flags & XDP_FLAGS_KERN_GENERIC) ?
> + prog->aux->id : 0;
> +}
> +
> +static u32 dev_xdp_current_mode(struct net_device *dev)
> +{
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
>
> - /* Query must always succeed. */
> - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> + if (prog)
> + return dev_xdp_query_generic(dev) ? XDP_FLAGS_SKB_MODE :
> + XDP_FLAGS_DRV_MODE;
return xdp->flags & XDP_FLAGS_MODE;
> + return 0;
> +}
>
> - return xdp.prog_id;
> +u32 dev_xdp_query(struct net_device *dev, unsigned int mode)
> +{
> + switch (mode) {
> + case XDP_FLAGS_DRV_MODE:
> + return dev_xdp_query_drv(dev);
> + case XDP_FLAGS_SKB_MODE:
> + return dev_xdp_query_generic(dev);
> + case XDP_FLAGS_HW_MODE:
> + return dev->xdp_prog_hw ? dev->xdp_prog_hw->aux->id : 0;
> + }
> + return 0;
> }
>
> static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> @@ -8027,45 +8049,47 @@ static int dev_xdp_install(struct net_device
> *dev, bpf_op_t bpf_op,
> struct bpf_prog *prog)
> {
> struct netdev_bpf xdp;
> + int err;
>
> memset(&xdp, 0, sizeof(xdp));
> - if (flags & XDP_FLAGS_HW_MODE)
> + if (flags & XDP_FLAGS_HW_MODE) {
> xdp.command = XDP_SETUP_PROG_HW;
> - else
> + xdp.flags = XDP_FLAGS_HW_MODE;
> + } else {
> xdp.command = XDP_SETUP_PROG;
> + xdp.flags = flags;
> + }
> xdp.extack = extack;
> - xdp.flags = flags;
> xdp.prog = prog;
>
> - return bpf_op(dev, &xdp);
> + err = bpf_op(dev, &xdp);
> + if (err)
> + return err;
> +
> + if (flags & XDP_FLAGS_HW_MODE) {
> + dev->xdp_prog_hw = prog;
> + return 0;
> + }
> +
> + rcu_assign_pointer(dev->xdp_prog, prog);
> + dev->xdp_flags = prog ? flags : 0;
> + return 0;
> }
>
> static void dev_xdp_uninstall(struct net_device *dev)
> {
> - struct netdev_bpf xdp;
> - bpf_op_t ndo_bpf;
> -
> - /* Remove generic XDP */
> - WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
> -
> - /* Remove from the driver */
> - ndo_bpf = dev->netdev_ops->ndo_bpf;
> - if (!ndo_bpf)
> - return;
> -
> - memset(&xdp, 0, sizeof(xdp));
> - xdp.command = XDP_QUERY_PROG;
> - WARN_ON(ndo_bpf(dev, &xdp));
> - if (xdp.prog_id)
> - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> - NULL));
> + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> + bpf_op_t bpf_op;
>
> - /* Remove HW offload */
> - memset(&xdp, 0, sizeof(xdp));
> - xdp.command = XDP_QUERY_PROG_HW;
> - if (!ndo_bpf(dev, &xdp) && xdp.prog_id)
> - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> + if (prog) {
> + bpf_op = dev_xdp_query_generic(dev) ?
> + generic_xdp_install : dev->netdev_ops->ndo_bpf;
> + WARN_ON(dev_xdp_install(dev, bpf_op, NULL, dev->xdp_flags,
> NULL));
> + }
> + if (dev_xdp_query(dev, XDP_FLAGS_HW_MODE))
> + WARN_ON(dev_xdp_install(dev, dev->netdev_ops->ndo_bpf,
> + NULL, XDP_FLAGS_HW_MODE, NULL));
> }
>
> /**
> @@ -8080,41 +8104,49 @@ static void dev_xdp_uninstall(struct
> net_device *dev)
> int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack
> *extack,
> int fd, u32 flags)
> {
> - const struct net_device_ops *ops = dev->netdev_ops;
> - enum bpf_netdev_command query;
> struct bpf_prog *prog = NULL;
> - bpf_op_t bpf_op, bpf_chk;
> + u32 mode, curr_mode;
> + bpf_op_t bpf_op;
> bool offload;
> int err;
>
> ASSERT_RTNL();
>
> offload = flags & XDP_FLAGS_HW_MODE;
> - query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> + mode = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
>
> - bpf_op = bpf_chk = ops->ndo_bpf;
> + bpf_op = dev->netdev_ops->ndo_bpf;
> 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)
> - bpf_chk = generic_xdp_install;
>
> - if (fd >= 0) {
> - if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
> + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> + mode = XDP_FLAGS_SKB_MODE;
> +
> + curr_mode = dev_xdp_current_mode(dev);
> +
> + if (!offload && curr_mode && (mode ^ curr_mode) &
> + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> + if (fd >= 0) {
> 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)) {
> + return 0;
> + }
> +
> + if (!offload && dev_xdp_query(dev, mode) &&
> + !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
> + return -EBUSY;
> +
> + if (fd >= 0) {
> + if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
> + dev_xdp_query(dev, mode)) {
> 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);
> + prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, !!bpf_op);
> if (IS_ERR(prog))
> return PTR_ERR(prog);
>
> @@ -8125,6 +8157,10 @@ int dev_change_xdp_fd(struct net_device *dev,
> struct netlink_ext_ack *extack,
> }
> }
>
> + if (mode == XDP_FLAGS_SKB_MODE) {
> + bpf_op = generic_xdp_install;
> + flags |= XDP_FLAGS_KERN_GENERIC;
> + }
> err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> if (err < 0 && prog)
> bpf_prog_put(prog);
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index adcc045952c2..5e396fd01d8b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,42 +1360,44 @@ static int rtnl_fill_link_ifmap(struct sk_buff
> *skb, struct net_device *dev)
> return 0;
> }
>
> -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> {
> - const struct bpf_prog *generic_xdp_prog;
> -
> - ASSERT_RTNL();
> -
> - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> - if (!generic_xdp_prog)
> - return 0;
> - return generic_xdp_prog->aux->id;
> -}
> -
> -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> -{
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> XDP_QUERY_PROG);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return XDP_FLAGS_DRV_MODE;
> + case XDP_ATTACHED_SKB:
> + return XDP_FLAGS_SKB_MODE;
> + case XDP_ATTACHED_HW:
> + return XDP_FLAGS_HW_MODE;
> + }
> + return 0;
> }
>
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> {
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> - XDP_QUERY_PROG_HW);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return IFLA_XDP_DRV_PROG_ID;
> + case XDP_ATTACHED_SKB:
> + return IFLA_XDP_SKB_PROG_ID;
> + case XDP_ATTACHED_HW:
> + return IFLA_XDP_HW_PROG_ID;
> + }
> + return 0;
> }
>
> static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device
> *dev,
> - u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
> - u32 (*get_prog_id)(struct net_device *dev))
> + u32 *prog_id, u8 *mode, u8 tgt_mode)
> {
> u32 curr_id;
> int err;
>
> - curr_id = get_prog_id(dev);
> + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> if (!curr_id)
> return 0;
>
> *prog_id = curr_id;
> - err = nla_put_u32(skb, attr, curr_id);
> + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
> if (err)
> return err;
>
> @@ -1420,16 +1422,13 @@ static int rtnl_xdp_fill(struct sk_buff *skb,
> struct net_device *dev)
>
> prog_id = 0;
> mode = XDP_ATTACHED_NONE;
> - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_SKB,
> - IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_SKB);
> if (err)
> goto err_cancel;
> - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_DRV,
> - IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_DRV);
> if (err)
> goto err_cancel;
> - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_HW,
> - IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> XDP_ATTACHED_HW);
> if (err)
> goto err_cancel;
>
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 4b2b194f4f1f..af92c04a58d8 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -388,16 +388,23 @@ int xdp_attachment_query(struct
> xdp_attachment_info *info,
> }
> EXPORT_SYMBOL_GPL(xdp_attachment_query);
>
> -bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> - struct netdev_bpf *bpf)
> +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> + struct netlink_ext_ack *extack)
> {
> - if (info->prog && (bpf->flags ^ info->flags) & XDP_FLAGS_MODES) {
> - NL_SET_ERR_MSG(bpf->extack,
> - "program loaded with different flags");
> + if ((new_flags ^ old_flags) & XDP_FLAGS_MODES) {
> + NL_SET_ERR_MSG(extack, "program loaded with different flags");
> return false;
> }
> return true;
> }
> +
> +bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> + struct netdev_bpf *bpf)
> +{
> + if (info->prog)
> + return xdp_prog_flags_ok(bpf->flags, info->flags, bpf->extack);
> + return true;
> +}
> EXPORT_SYMBOL_GPL(xdp_attachment_flags_ok);
>
> void xdp_attachment_setup(struct xdp_attachment_info *info,
> --
> 2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-01 18:12 ` Jonathan Lemon
@ 2019-06-03 8:39 ` Björn Töpel
2019-06-03 14:58 ` Jonathan Lemon
2019-06-03 23:11 ` Daniel Borkmann
0 siblings, 2 replies; 23+ messages in thread
From: Björn Töpel @ 2019-06-03 8:39 UTC (permalink / raw)
To: Jonathan Lemon
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus,
Jesper Dangaard Brouer, bpf, Jakub Kicinski, Saeed Mahameed
On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@flugsvamp.com> wrote:
>
> On 31 May 2019, at 2:42, Björn Töpel wrote:
>
> > From: Björn Töpel <bjorn.topel@intel.com>
> >
> > All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
> > command of ndo_bpf. The query code is fairly generic. This commit
> > refactors the query code up from the drivers to the netdev level.
> >
> > The struct net_device has gained two new members: xdp_prog_hw and
> > xdp_flags. The former is the offloaded XDP program, if any, and the
> > latter tracks the flags that the supplied when attaching the XDP
> > program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
> >
> > The xdp_prog member, previously only used for SKB_MODE, is shared with
> > DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> > mutually exclusive. To differentiate between the two modes, a new
> > internal flag is introduced as well.
>
> I'm not entirely clear why this new flag is needed - GENERIC seems to
> be an alias for SKB_MODE, so why just use SKB_MODE directly?
>
> If the user does not explicitly specify a type (skb|drv|hw), then the
> command should choose the correct type and then behave as if this type
> was specified.
>
Yes, this is kind of hairy.
SKB and DRV are mutually exclusive, but HW is not. IOW, valid options are:
SKB, DRV, HW, SKB+HW DRV+HW.
What complicates things further, is that SKB and DRV can be implicitly
(auto/no flags) or explicitly enabled (flags).
If a user doesn't pass any flags, the "best supported mode" should be
selected. If this "auto mode" is used, it should be seen as a third
mode. E.g.
ip link set dev eth0 xdp on -- OK
ip link set dev eth0 xdp off -- OK
ip link set dev eth0 xdp on -- OK # generic auto selected
ip link set dev eth0 xdpgeneric off -- NOK, bad flags
ip link set dev eth0 xdp on -- OK # drv auto selected
ip link set dev eth0 xdpdrv off -- NOK, bad flags
...and so on. The idea is that a user should use the same set of flags always.
The internal "GENERIC" flag is only to determine if the xdp_prog
represents a DRV version or SKB version. Maybe it would be clearer
just to add an additional xdp_prog_drv to the net_device, instead?
> The logic in dev_change_xdp_fd() is too complicated. It disallows
> setting (drv|skb), but allows (hw|skb), which I'm not sure is
> intentional.
>
> It should be clearer as to which combinations are allowed.
Fair point. I'll try to clean it up further.
Björn
> --
> Jonathan
>
>
>
> >
> > The program query operations is all done under the rtnl lock. However,
> > the xdp_prog member is __rcu annotated, and used in a lock-less manner
> > for the SKB_MODE. Now that xdp_prog member is shared from a query
> > program perspective, RCU read and assignments functions are still used
> > when altering xdp_prog, even when only for queries in DRV_MODE. This
> > is for sparse/lockdep correctness.
> >
> > Signed-off-by: Björn Töpel <bjorn.topel@intel.com>
> > ---
> > include/linux/netdevice.h | 15 +++--
> > include/net/xdp.h | 4 ++
> > net/core/dev.c | 138
> > ++++++++++++++++++++++++--------------
> > net/core/rtnetlink.c | 53 +++++++--------
> > net/core/xdp.c | 17 +++--
> > 5 files changed, 139 insertions(+), 88 deletions(-)
> >
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 44b47e9df94a..f3a875a52c6c 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1940,6 +1940,9 @@ struct net_device {
> > #endif
> > struct hlist_node index_hlist;
> >
> > + struct bpf_prog *xdp_prog_hw;
> > + u32 xdp_flags;
> > +
> > /*
> > * Cache lines mostly used on transmit path
> > */
> > @@ -2043,11 +2046,14 @@ struct net_device {
> > };
> > #define to_net_dev(d) container_of(d, struct net_device, dev)
> >
> > +/* Reusing the XDP flags space for kernel internal flag */
> > +#define XDP_FLAGS_KERN_GENERIC (1U << 4)
> > +static_assert(!(XDP_FLAGS_KERN_GENERIC & XDP_FLAGS_MASK));
> > +
> > static inline bool netif_elide_gro(const struct net_device *dev)
> > {
> > - if (!(dev->features & NETIF_F_GRO) || dev->xdp_prog)
> > - return true;
> > - return false;
> > + return !(dev->features & NETIF_F_GRO) ||
> > + dev->xdp_flags & XDP_FLAGS_KERN_GENERIC;
> > }
> >
> > #define NETDEV_ALIGN 32
> > @@ -3684,8 +3690,7 @@ struct sk_buff *dev_hard_start_xmit(struct
> > sk_buff *skb, struct net_device *dev,
> > typedef int (*bpf_op_t)(struct net_device *dev, struct netdev_bpf
> > *bpf);
> > int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack
> > *extack,
> > int fd, u32 flags);
> > -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t xdp_op,
> > - enum bpf_netdev_command cmd);
> > +u32 dev_xdp_query(struct net_device *dev, unsigned int mode);
> > int xdp_umem_query(struct net_device *dev, u16 queue_id);
> >
> > int __dev_forward_skb(struct net_device *dev, struct sk_buff *skb);
> > diff --git a/include/net/xdp.h b/include/net/xdp.h
> > index 0f25b3675c5c..3691280c8fc1 100644
> > --- a/include/net/xdp.h
> > +++ b/include/net/xdp.h
> > @@ -51,6 +51,7 @@ struct xdp_mem_info {
> > };
> >
> > struct page_pool;
> > +struct netlink_ext_ack;
> >
> > struct zero_copy_allocator {
> > void (*free)(struct zero_copy_allocator *zca, unsigned long handle);
> > @@ -166,4 +167,7 @@ bool xdp_attachment_flags_ok(struct
> > xdp_attachment_info *info,
> > void xdp_attachment_setup(struct xdp_attachment_info *info,
> > struct netdev_bpf *bpf);
> >
> > +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> > + struct netlink_ext_ack *extack);
> > +
> > #endif /* __LINUX_NET_XDP_H__ */
> > diff --git a/net/core/dev.c b/net/core/dev.c
> > index b6b8505cfb3e..1a9da508149a 100644
> > --- a/net/core/dev.c
> > +++ b/net/core/dev.c
> > @@ -8005,21 +8005,43 @@ int dev_change_proto_down_generic(struct
> > net_device *dev, bool proto_down)
> > }
> > EXPORT_SYMBOL(dev_change_proto_down_generic);
> >
> > -u32 __dev_xdp_query(struct net_device *dev, bpf_op_t bpf_op,
> > - enum bpf_netdev_command cmd)
> > +static u32 dev_xdp_query_generic(struct net_device *dev)
> > {
> > - struct netdev_bpf xdp;
> > + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> >
> > - if (!bpf_op)
> > - return 0;
> > + return prog && dev->xdp_flags & XDP_FLAGS_KERN_GENERIC ?
> > + prog->aux->id : 0;
> > +}
> >
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = cmd;
> > +static u32 dev_xdp_query_drv(struct net_device *dev)
> > +{
> > + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> > +
> > + return prog && !(dev->xdp_flags & XDP_FLAGS_KERN_GENERIC) ?
> > + prog->aux->id : 0;
> > +}
> > +
> > +static u32 dev_xdp_current_mode(struct net_device *dev)
> > +{
> > + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> >
> > - /* Query must always succeed. */
> > - WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> > + if (prog)
> > + return dev_xdp_query_generic(dev) ? XDP_FLAGS_SKB_MODE :
> > + XDP_FLAGS_DRV_MODE;
>
> return xdp->flags & XDP_FLAGS_MODE;
>
>
> > + return 0;
> > +}
> >
> > - return xdp.prog_id;
> > +u32 dev_xdp_query(struct net_device *dev, unsigned int mode)
> > +{
> > + switch (mode) {
> > + case XDP_FLAGS_DRV_MODE:
> > + return dev_xdp_query_drv(dev);
> > + case XDP_FLAGS_SKB_MODE:
> > + return dev_xdp_query_generic(dev);
> > + case XDP_FLAGS_HW_MODE:
> > + return dev->xdp_prog_hw ? dev->xdp_prog_hw->aux->id : 0;
> > + }
> > + return 0;
> > }
> >
> > static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> > @@ -8027,45 +8049,47 @@ static int dev_xdp_install(struct net_device
> > *dev, bpf_op_t bpf_op,
> > struct bpf_prog *prog)
> > {
> > struct netdev_bpf xdp;
> > + int err;
> >
> > memset(&xdp, 0, sizeof(xdp));
> > - if (flags & XDP_FLAGS_HW_MODE)
> > + if (flags & XDP_FLAGS_HW_MODE) {
> > xdp.command = XDP_SETUP_PROG_HW;
> > - else
> > + xdp.flags = XDP_FLAGS_HW_MODE;
> > + } else {
> > xdp.command = XDP_SETUP_PROG;
> > + xdp.flags = flags;
> > + }
> > xdp.extack = extack;
> > - xdp.flags = flags;
> > xdp.prog = prog;
> >
> > - return bpf_op(dev, &xdp);
> > + err = bpf_op(dev, &xdp);
> > + if (err)
> > + return err;
> > +
> > + if (flags & XDP_FLAGS_HW_MODE) {
> > + dev->xdp_prog_hw = prog;
> > + return 0;
> > + }
> > +
> > + rcu_assign_pointer(dev->xdp_prog, prog);
> > + dev->xdp_flags = prog ? flags : 0;
> > + return 0;
> > }
> >
> > static void dev_xdp_uninstall(struct net_device *dev)
> > {
> > - struct netdev_bpf xdp;
> > - bpf_op_t ndo_bpf;
> > -
> > - /* Remove generic XDP */
> > - WARN_ON(dev_xdp_install(dev, generic_xdp_install, NULL, 0, NULL));
> > -
> > - /* Remove from the driver */
> > - ndo_bpf = dev->netdev_ops->ndo_bpf;
> > - if (!ndo_bpf)
> > - return;
> > -
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = XDP_QUERY_PROG;
> > - WARN_ON(ndo_bpf(dev, &xdp));
> > - if (xdp.prog_id)
> > - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> > - NULL));
> > + struct bpf_prog *prog = rtnl_dereference(dev->xdp_prog);
> > + bpf_op_t bpf_op;
> >
> > - /* Remove HW offload */
> > - memset(&xdp, 0, sizeof(xdp));
> > - xdp.command = XDP_QUERY_PROG_HW;
> > - if (!ndo_bpf(dev, &xdp) && xdp.prog_id)
> > - WARN_ON(dev_xdp_install(dev, ndo_bpf, NULL, xdp.prog_flags,
> > + if (prog) {
> > + bpf_op = dev_xdp_query_generic(dev) ?
> > + generic_xdp_install : dev->netdev_ops->ndo_bpf;
> > + WARN_ON(dev_xdp_install(dev, bpf_op, NULL, dev->xdp_flags,
> > NULL));
> > + }
> > + if (dev_xdp_query(dev, XDP_FLAGS_HW_MODE))
> > + WARN_ON(dev_xdp_install(dev, dev->netdev_ops->ndo_bpf,
> > + NULL, XDP_FLAGS_HW_MODE, NULL));
> > }
> >
> > /**
> > @@ -8080,41 +8104,49 @@ static void dev_xdp_uninstall(struct
> > net_device *dev)
> > int dev_change_xdp_fd(struct net_device *dev, struct netlink_ext_ack
> > *extack,
> > int fd, u32 flags)
> > {
> > - const struct net_device_ops *ops = dev->netdev_ops;
> > - enum bpf_netdev_command query;
> > struct bpf_prog *prog = NULL;
> > - bpf_op_t bpf_op, bpf_chk;
> > + u32 mode, curr_mode;
> > + bpf_op_t bpf_op;
> > bool offload;
> > int err;
> >
> > ASSERT_RTNL();
> >
> > offload = flags & XDP_FLAGS_HW_MODE;
> > - query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> > + mode = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;
> >
> > - bpf_op = bpf_chk = ops->ndo_bpf;
> > + bpf_op = dev->netdev_ops->ndo_bpf;
> > 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)
> > - bpf_chk = generic_xdp_install;
> >
> > - if (fd >= 0) {
> > - if (!offload && __dev_xdp_query(dev, bpf_chk, XDP_QUERY_PROG)) {
> > + if (!bpf_op || flags & XDP_FLAGS_SKB_MODE)
> > + mode = XDP_FLAGS_SKB_MODE;
> > +
> > + curr_mode = dev_xdp_current_mode(dev);
> > +
> > + if (!offload && curr_mode && (mode ^ curr_mode) &
> > + (XDP_FLAGS_DRV_MODE | XDP_FLAGS_SKB_MODE)) {
> > + if (fd >= 0) {
> > 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)) {
> > + return 0;
> > + }
> > +
> > + if (!offload && dev_xdp_query(dev, mode) &&
> > + !xdp_prog_flags_ok(dev->xdp_flags, flags, extack))
> > + return -EBUSY;
> > +
> > + if (fd >= 0) {
> > + if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
> > + dev_xdp_query(dev, mode)) {
> > 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);
> > + prog = bpf_prog_get_type_dev(fd, BPF_PROG_TYPE_XDP, !!bpf_op);
> > if (IS_ERR(prog))
> > return PTR_ERR(prog);
> >
> > @@ -8125,6 +8157,10 @@ int dev_change_xdp_fd(struct net_device *dev,
> > struct netlink_ext_ack *extack,
> > }
> > }
> >
> > + if (mode == XDP_FLAGS_SKB_MODE) {
> > + bpf_op = generic_xdp_install;
> > + flags |= XDP_FLAGS_KERN_GENERIC;
> > + }
> > err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> > if (err < 0 && prog)
> > bpf_prog_put(prog);
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index adcc045952c2..5e396fd01d8b 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1360,42 +1360,44 @@ static int rtnl_fill_link_ifmap(struct sk_buff
> > *skb, struct net_device *dev)
> > return 0;
> > }
> >
> > -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> > +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> > {
> > - const struct bpf_prog *generic_xdp_prog;
> > -
> > - ASSERT_RTNL();
> > -
> > - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> > - if (!generic_xdp_prog)
> > - return 0;
> > - return generic_xdp_prog->aux->id;
> > -}
> > -
> > -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> > -{
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> > XDP_QUERY_PROG);
> > + switch (tgt_mode) {
> > + case XDP_ATTACHED_DRV:
> > + return XDP_FLAGS_DRV_MODE;
> > + case XDP_ATTACHED_SKB:
> > + return XDP_FLAGS_SKB_MODE;
> > + case XDP_ATTACHED_HW:
> > + return XDP_FLAGS_HW_MODE;
> > + }
> > + return 0;
> > }
> >
> > -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> > +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> > {
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> > - XDP_QUERY_PROG_HW);
> > + switch (tgt_mode) {
> > + case XDP_ATTACHED_DRV:
> > + return IFLA_XDP_DRV_PROG_ID;
> > + case XDP_ATTACHED_SKB:
> > + return IFLA_XDP_SKB_PROG_ID;
> > + case XDP_ATTACHED_HW:
> > + return IFLA_XDP_HW_PROG_ID;
> > + }
> > + return 0;
> > }
> >
> > static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device
> > *dev,
> > - u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
> > - u32 (*get_prog_id)(struct net_device *dev))
> > + u32 *prog_id, u8 *mode, u8 tgt_mode)
> > {
> > u32 curr_id;
> > int err;
> >
> > - curr_id = get_prog_id(dev);
> > + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> > if (!curr_id)
> > return 0;
> >
> > *prog_id = curr_id;
> > - err = nla_put_u32(skb, attr, curr_id);
> > + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
> > if (err)
> > return err;
> >
> > @@ -1420,16 +1422,13 @@ static int rtnl_xdp_fill(struct sk_buff *skb,
> > struct net_device *dev)
> >
> > prog_id = 0;
> > mode = XDP_ATTACHED_NONE;
> > - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_SKB,
> > - IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_SKB);
> > if (err)
> > goto err_cancel;
> > - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_DRV,
> > - IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_DRV);
> > if (err)
> > goto err_cancel;
> > - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_HW,
> > - IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode,
> > XDP_ATTACHED_HW);
> > if (err)
> > goto err_cancel;
> >
> > diff --git a/net/core/xdp.c b/net/core/xdp.c
> > index 4b2b194f4f1f..af92c04a58d8 100644
> > --- a/net/core/xdp.c
> > +++ b/net/core/xdp.c
> > @@ -388,16 +388,23 @@ int xdp_attachment_query(struct
> > xdp_attachment_info *info,
> > }
> > EXPORT_SYMBOL_GPL(xdp_attachment_query);
> >
> > -bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> > - struct netdev_bpf *bpf)
> > +bool xdp_prog_flags_ok(u32 old_flags, u32 new_flags,
> > + struct netlink_ext_ack *extack)
> > {
> > - if (info->prog && (bpf->flags ^ info->flags) & XDP_FLAGS_MODES) {
> > - NL_SET_ERR_MSG(bpf->extack,
> > - "program loaded with different flags");
> > + if ((new_flags ^ old_flags) & XDP_FLAGS_MODES) {
> > + NL_SET_ERR_MSG(extack, "program loaded with different flags");
> > return false;
> > }
> > return true;
> > }
> > +
> > +bool xdp_attachment_flags_ok(struct xdp_attachment_info *info,
> > + struct netdev_bpf *bpf)
> > +{
> > + if (info->prog)
> > + return xdp_prog_flags_ok(bpf->flags, info->flags, bpf->extack);
> > + return true;
> > +}
> > EXPORT_SYMBOL_GPL(xdp_attachment_flags_ok);
> >
> > void xdp_attachment_setup(struct xdp_attachment_info *info,
> > --
> > 2.20.1
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 8:39 ` Björn Töpel
@ 2019-06-03 14:58 ` Jonathan Lemon
2019-06-03 23:11 ` Daniel Borkmann
1 sibling, 0 replies; 23+ messages in thread
From: Jonathan Lemon @ 2019-06-03 14:58 UTC (permalink / raw)
To: Björn Töpel
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus,
Jesper Dangaard Brouer, bpf, Jakub Kicinski, Saeed Mahameed
On 3 Jun 2019, at 1:39, Björn Töpel wrote:
> On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@flugsvamp.com>
> wrote:
>>
>> On 31 May 2019, at 2:42, Björn Töpel wrote:
>>
>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>
>>> All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
>>> command of ndo_bpf. The query code is fairly generic. This commit
>>> refactors the query code up from the drivers to the netdev level.
>>>
>>> The struct net_device has gained two new members: xdp_prog_hw and
>>> xdp_flags. The former is the offloaded XDP program, if any, and the
>>> latter tracks the flags that the supplied when attaching the XDP
>>> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
>>>
>>> The xdp_prog member, previously only used for SKB_MODE, is shared
>>> with
>>> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
>>> mutually exclusive. To differentiate between the two modes, a new
>>> internal flag is introduced as well.
>>
>> I'm not entirely clear why this new flag is needed - GENERIC seems to
>> be an alias for SKB_MODE, so why just use SKB_MODE directly?
>>
>> If the user does not explicitly specify a type (skb|drv|hw), then the
>> command should choose the correct type and then behave as if this
>> type
>> was specified.
>>
>
> Yes, this is kind of hairy.
>
> SKB and DRV are mutually exclusive, but HW is not. IOW, valid options
> are:
> SKB, DRV, HW, SKB+HW DRV+HW.
Fair enough, that was the understanding that I had from the code,
although I'm not sure about the usage of SKB+HW mode.
>
> What complicates things further, is that SKB and DRV can be implicitly
> (auto/no flags) or explicitly enabled (flags).
>
> If a user doesn't pass any flags, the "best supported mode" should be
> selected. If this "auto mode" is used, it should be seen as a third
> mode. E.g.
>
> ip link set dev eth0 xdp on -- OK
> ip link set dev eth0 xdp off -- OK
>
> ip link set dev eth0 xdp on -- OK # generic auto selected
> ip link set dev eth0 xdpgeneric off -- NOK, bad flags
>
> ip link set dev eth0 xdp on -- OK # drv auto selected
> ip link set dev eth0 xdpdrv off -- NOK, bad flags
>
> ...and so on. The idea is that a user should use the same set of flags
> always.
I'm not sure about this. The "xdp" mode shouldn't be treated as a
separate mode, it should be "best supported mode", as indicated above.
From my view, it should select the appropriate mode, and then proceed
as if the user had specified that mode, rather than being treated as an
independent mode.
ip link set dev eth0 xdp on - OK
ip link set dev eth0 xdp off - OK
ip link set dev eth0 xdp on - OK, selected dev
ip link set dev eth0 xdpgeneric off - NOK, not running
ip link set dev eth0 xdpdrv off - OK
> The internal "GENERIC" flag is only to determine if the xdp_prog
> represents a DRV version or SKB version. Maybe it would be clearer
> just to add an additional xdp_prog_drv to the net_device, instead?
I'd go the other way, and remove GENERIC, leaving only SKB, DRV, and HW.
The appropriate mode flag (SKB|DRV) is enough to indicate the type of
xdp_prog.
--
Jonathan
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 8:39 ` Björn Töpel
2019-06-03 14:58 ` Jonathan Lemon
@ 2019-06-03 23:11 ` Daniel Borkmann
2019-06-04 5:37 ` Björn Töpel
1 sibling, 1 reply; 23+ messages in thread
From: Daniel Borkmann @ 2019-06-03 23:11 UTC (permalink / raw)
To: Björn Töpel, Jonathan Lemon
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov, Netdev,
Björn Töpel, Karlsson, Magnus, Jesper Dangaard Brouer,
bpf, Jakub Kicinski, Saeed Mahameed
On 06/03/2019 10:39 AM, Björn Töpel wrote:
> On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@flugsvamp.com> wrote:
>> On 31 May 2019, at 2:42, Björn Töpel wrote:
>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>
>>> All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
>>> command of ndo_bpf. The query code is fairly generic. This commit
>>> refactors the query code up from the drivers to the netdev level.
>>>
>>> The struct net_device has gained two new members: xdp_prog_hw and
>>> xdp_flags. The former is the offloaded XDP program, if any, and the
>>> latter tracks the flags that the supplied when attaching the XDP
>>> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
>>>
>>> The xdp_prog member, previously only used for SKB_MODE, is shared with
>>> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
>>> mutually exclusive. To differentiate between the two modes, a new
>>> internal flag is introduced as well.
>>
>> I'm not entirely clear why this new flag is needed - GENERIC seems to
>> be an alias for SKB_MODE, so why just use SKB_MODE directly?
>>
>> If the user does not explicitly specify a type (skb|drv|hw), then the
>> command should choose the correct type and then behave as if this type
>> was specified.
>
> Yes, this is kind of hairy.
>
> SKB and DRV are mutually exclusive, but HW is not. IOW, valid options are:
> SKB, DRV, HW, SKB+HW DRV+HW.
Correct, HW is a bit special here in that it helps offloading parts of
the DRV XDP program to NIC, but also do RSS steering in BPF etc, hence
this combo is intentionally allowed (see also git log).
> What complicates things further, is that SKB and DRV can be implicitly
> (auto/no flags) or explicitly enabled (flags).
Mainly out of historic context: originally the fallback to SKB mode was
implicit if the ndo_bpf was missing. But there are use cases where we
want to fail if the driver does not support native XDP to avoid surprises.
> If a user doesn't pass any flags, the "best supported mode" should be
> selected. If this "auto mode" is used, it should be seen as a third
> mode. E.g.
>
> ip link set dev eth0 xdp on -- OK
> ip link set dev eth0 xdp off -- OK
>
> ip link set dev eth0 xdp on -- OK # generic auto selected
> ip link set dev eth0 xdpgeneric off -- NOK, bad flags
This would work if the auto selection would have selected XDP generic.
> ip link set dev eth0 xdp on -- OK # drv auto selected
> ip link set dev eth0 xdpdrv off -- NOK, bad flags
This would work if the auto selection chose native XDP previously. Are
you saying it's not the case?
Also, what is the use case in mixing these commands? It should be xdp
on+off, xdpdrv on+off, and so on. Are you saying you would prefer a
xdp{,any} off that uninstalls everything? Isn't this mostly a user space
issue to whatever orchestrates XDP?
> ...and so on. The idea is that a user should use the same set of flags always.
>
> The internal "GENERIC" flag is only to determine if the xdp_prog
> represents a DRV version or SKB version. Maybe it would be clearer
> just to add an additional xdp_prog_drv to the net_device, instead?
>
>> The logic in dev_change_xdp_fd() is too complicated. It disallows
>> setting (drv|skb), but allows (hw|skb), which I'm not sure is
>> intentional.
>>
>> It should be clearer as to which combinations are allowed.
>
> Fair point. I'll try to clean it up further.
>
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 23:11 ` Daniel Borkmann
@ 2019-06-04 5:37 ` Björn Töpel
0 siblings, 0 replies; 23+ messages in thread
From: Björn Töpel @ 2019-06-04 5:37 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jonathan Lemon, Toke Høiland-Jørgensen,
Alexei Starovoitov, Netdev, Björn Töpel,
Karlsson, Magnus, Jesper Dangaard Brouer, bpf, Jakub Kicinski,
Saeed Mahameed
On Tue, 4 Jun 2019 at 01:11, Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 06/03/2019 10:39 AM, Björn Töpel wrote:
> > On Sat, 1 Jun 2019 at 20:12, Jonathan Lemon <jlemon@flugsvamp.com> wrote:
> >> On 31 May 2019, at 2:42, Björn Töpel wrote:
> >>> From: Björn Töpel <bjorn.topel@intel.com>
> >>>
> >>> All XDP capable drivers need to implement the XDP_QUERY_PROG{,_HW}
> >>> command of ndo_bpf. The query code is fairly generic. This commit
> >>> refactors the query code up from the drivers to the netdev level.
> >>>
> >>> The struct net_device has gained two new members: xdp_prog_hw and
> >>> xdp_flags. The former is the offloaded XDP program, if any, and the
> >>> latter tracks the flags that the supplied when attaching the XDP
> >>> program. The flags only apply to SKB_MODE or DRV_MODE, not HW_MODE.
> >>>
> >>> The xdp_prog member, previously only used for SKB_MODE, is shared with
> >>> DRV_MODE. This is OK, due to the fact that SKB_MODE and DRV_MODE are
> >>> mutually exclusive. To differentiate between the two modes, a new
> >>> internal flag is introduced as well.
> >>
> >> I'm not entirely clear why this new flag is needed - GENERIC seems to
> >> be an alias for SKB_MODE, so why just use SKB_MODE directly?
> >>
> >> If the user does not explicitly specify a type (skb|drv|hw), then the
> >> command should choose the correct type and then behave as if this type
> >> was specified.
> >
> > Yes, this is kind of hairy.
> >
> > SKB and DRV are mutually exclusive, but HW is not. IOW, valid options are:
> > SKB, DRV, HW, SKB+HW DRV+HW.
>
> Correct, HW is a bit special here in that it helps offloading parts of
> the DRV XDP program to NIC, but also do RSS steering in BPF etc, hence
> this combo is intentionally allowed (see also git log).
>
> > What complicates things further, is that SKB and DRV can be implicitly
> > (auto/no flags) or explicitly enabled (flags).
>
> Mainly out of historic context: originally the fallback to SKB mode was
> implicit if the ndo_bpf was missing. But there are use cases where we
> want to fail if the driver does not support native XDP to avoid surprises.
>
> > If a user doesn't pass any flags, the "best supported mode" should be
> > selected. If this "auto mode" is used, it should be seen as a third
> > mode. E.g.
> >
> > ip link set dev eth0 xdp on -- OK
> > ip link set dev eth0 xdp off -- OK
> >
> > ip link set dev eth0 xdp on -- OK # generic auto selected
> > ip link set dev eth0 xdpgeneric off -- NOK, bad flags
>
> This would work if the auto selection would have selected XDP generic.
>
> > ip link set dev eth0 xdp on -- OK # drv auto selected
> > ip link set dev eth0 xdpdrv off -- NOK, bad flags
>
> This would work if the auto selection chose native XDP previously. Are
> you saying it's not the case?
>
Yes, that is *not* the case for some drivers. With the Intel drivers
we didn't check the flags at all at XDP attachment (check out the
usage of xdp_attachment_flags_ok), but e.g. nfp and netdevsim does.
Grep for 'program loaded with different flags' in the test_offload.py
selftest. I like this approach, and my patch does this flag check in
dev_change_xdp_fd.
> Also, what is the use case in mixing these commands? It should be xdp
> on+off, xdpdrv on+off, and so on. Are you saying you would prefer a
> xdp{,any} off that uninstalls everything? Isn't this mostly a user space
> issue to whatever orchestrates XDP?
>
No, I'm not suggesting a change. There is no use-case mixing them.
What the flags ok checks do is returning an error (like nfp and
netdevsim does) if a user tries to mix, say, "xdp" and explicit
xdpdrv/xdpgeneric". This patch moves this check to the generic
function dev_change_xdp_fd.
There seems to be a confusion about how this is supposed to be used.
It was for me, e.g. I though using "enable with xdp and disable with
xdpdrv" was OK. This was the reason why I added an error on "disable
with xdpgeneric off, if xdpdrv is active" in my first revision of the
series. I removed this in v2, after Jakub pointed out the
test_offload.py test, which is a great showcase/test of what should be
allowed and what shouldn't in terms of flags.
TL;DR: Let's stick to what test_offload.py asserts, for all XDP.
> > ...and so on. The idea is that a user should use the same set of flags always.
> >
> > The internal "GENERIC" flag is only to determine if the xdp_prog
> > represents a DRV version or SKB version. Maybe it would be clearer
> > just to add an additional xdp_prog_drv to the net_device, instead?
> >
> >> The logic in dev_change_xdp_fd() is too complicated. It disallows
> >> setting (drv|skb), but allows (hw|skb), which I'm not sure is
> >> intentional.
> >>
> >> It should be clearer as to which combinations are allowed.
> >
> > Fair point. I'll try to clean it up further.
> >
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-05-31 9:42 ` [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev Björn Töpel
` (2 preceding siblings ...)
2019-06-01 18:12 ` Jonathan Lemon
@ 2019-06-01 20:02 ` Jakub Kicinski
2019-06-03 9:07 ` Björn Töpel
3 siblings, 1 reply; 23+ messages in thread
From: Jakub Kicinski @ 2019-06-01 20:02 UTC (permalink / raw)
To: Björn Töpel
Cc: toke, ast, daniel, netdev, Björn Töpel, magnus.karlsson,
brouer, bpf, saeedm
On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 44b47e9df94a..f3a875a52c6c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1940,6 +1940,9 @@ struct net_device {
> #endif
> struct hlist_node index_hlist;
>
> + struct bpf_prog *xdp_prog_hw;
IDK if we should pay the cost of this pointer for every netdev on the
system just for the single production driver out there that implements
HW offload :( I'm on the fence about this..
> + u32 xdp_flags;
> +
> /*
> * Cache lines mostly used on transmit path
> */
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index adcc045952c2..5e396fd01d8b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,42 +1360,44 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
> return 0;
> }
>
> -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> {
> - const struct bpf_prog *generic_xdp_prog;
> -
> - ASSERT_RTNL();
> -
> - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> - if (!generic_xdp_prog)
> - return 0;
> - return generic_xdp_prog->aux->id;
> -}
> -
> -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> -{
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf, XDP_QUERY_PROG);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return XDP_FLAGS_DRV_MODE;
> + case XDP_ATTACHED_SKB:
> + return XDP_FLAGS_SKB_MODE;
> + case XDP_ATTACHED_HW:
> + return XDP_FLAGS_HW_MODE;
> + }
> + return 0;
> }
>
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> {
> - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> - XDP_QUERY_PROG_HW);
> + switch (tgt_mode) {
> + case XDP_ATTACHED_DRV:
> + return IFLA_XDP_DRV_PROG_ID;
> + case XDP_ATTACHED_SKB:
> + return IFLA_XDP_SKB_PROG_ID;
> + case XDP_ATTACHED_HW:
> + return IFLA_XDP_HW_PROG_ID;
> + }
> + return 0;
> }
>
> static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
> - u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
> - u32 (*get_prog_id)(struct net_device *dev))
> + u32 *prog_id, u8 *mode, u8 tgt_mode)
> {
> u32 curr_id;
> int err;
>
> - curr_id = get_prog_id(dev);
> + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> if (!curr_id)
> return 0;
>
> *prog_id = curr_id;
> - err = nla_put_u32(skb, attr, curr_id);
> + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
> if (err)
> return err;
>
> @@ -1420,16 +1422,13 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>
> prog_id = 0;
> mode = XDP_ATTACHED_NONE;
> - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB,
> - IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB);
> if (err)
> goto err_cancel;
> - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV,
> - IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV);
> if (err)
> goto err_cancel;
> - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW,
> - IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
> + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW);
> if (err)
> goto err_cancel;
>
So you remove all the attr and flag params just to add a conversion
helpers to get them based on mode? Why? Seems like unnecessary churn,
and questionable change :S
Otherwise looks good to me!
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-01 20:02 ` Jakub Kicinski
@ 2019-06-03 9:07 ` Björn Töpel
2019-06-03 10:56 ` Toke Høiland-Jørgensen
0 siblings, 1 reply; 23+ messages in thread
From: Björn Töpel @ 2019-06-03 9:07 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Toke Høiland-Jørgensen, Alexei Starovoitov,
Daniel Borkmann, Netdev, Björn Töpel, Karlsson, Magnus,
Jesper Dangaard Brouer, bpf, Saeed Mahameed
On Sat, 1 Jun 2019 at 22:02, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index 44b47e9df94a..f3a875a52c6c 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -1940,6 +1940,9 @@ struct net_device {
> > #endif
> > struct hlist_node index_hlist;
> >
> > + struct bpf_prog *xdp_prog_hw;
>
> IDK if we should pay the cost of this pointer for every netdev on the
> system just for the single production driver out there that implements
> HW offload :( I'm on the fence about this..
>
Hmm. Adding a config option? Keep the QUERY_PROG_HW?
> > + u32 xdp_flags;
> > +
> > /*
> > * Cache lines mostly used on transmit path
> > */
>
> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> > index adcc045952c2..5e396fd01d8b 100644
> > --- a/net/core/rtnetlink.c
> > +++ b/net/core/rtnetlink.c
> > @@ -1360,42 +1360,44 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
> > return 0;
> > }
> >
> > -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> > +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
> > {
> > - const struct bpf_prog *generic_xdp_prog;
> > -
> > - ASSERT_RTNL();
> > -
> > - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> > - if (!generic_xdp_prog)
> > - return 0;
> > - return generic_xdp_prog->aux->id;
> > -}
> > -
> > -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> > -{
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf, XDP_QUERY_PROG);
> > + switch (tgt_mode) {
> > + case XDP_ATTACHED_DRV:
> > + return XDP_FLAGS_DRV_MODE;
> > + case XDP_ATTACHED_SKB:
> > + return XDP_FLAGS_SKB_MODE;
> > + case XDP_ATTACHED_HW:
> > + return XDP_FLAGS_HW_MODE;
> > + }
> > + return 0;
> > }
> >
> > -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> > +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
> > {
> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> > - XDP_QUERY_PROG_HW);
> > + switch (tgt_mode) {
> > + case XDP_ATTACHED_DRV:
> > + return IFLA_XDP_DRV_PROG_ID;
> > + case XDP_ATTACHED_SKB:
> > + return IFLA_XDP_SKB_PROG_ID;
> > + case XDP_ATTACHED_HW:
> > + return IFLA_XDP_HW_PROG_ID;
> > + }
> > + return 0;
> > }
> >
> > static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
> > - u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
> > - u32 (*get_prog_id)(struct net_device *dev))
> > + u32 *prog_id, u8 *mode, u8 tgt_mode)
> > {
> > u32 curr_id;
> > int err;
> >
> > - curr_id = get_prog_id(dev);
> > + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
> > if (!curr_id)
> > return 0;
> >
> > *prog_id = curr_id;
> > - err = nla_put_u32(skb, attr, curr_id);
> > + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
> > if (err)
> > return err;
> >
> > @@ -1420,16 +1422,13 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
> >
> > prog_id = 0;
> > mode = XDP_ATTACHED_NONE;
> > - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB,
> > - IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB);
> > if (err)
> > goto err_cancel;
> > - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV,
> > - IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV);
> > if (err)
> > goto err_cancel;
> > - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW,
> > - IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW);
> > if (err)
> > goto err_cancel;
> >
>
> So you remove all the attr and flag params just to add a conversion
> helpers to get them based on mode? Why? Seems like unnecessary churn,
> and questionable change :S
>
Fair enough. I'll address this!
> Otherwise looks good to me!
^ permalink raw reply [flat|nested] 23+ messages in thread* Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev
2019-06-03 9:07 ` Björn Töpel
@ 2019-06-03 10:56 ` Toke Høiland-Jørgensen
0 siblings, 0 replies; 23+ messages in thread
From: Toke Høiland-Jørgensen @ 2019-06-03 10:56 UTC (permalink / raw)
To: Björn Töpel, Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, Netdev,
Björn Töpel, Karlsson, Magnus, Jesper Dangaard Brouer,
bpf, Saeed Mahameed
Björn Töpel <bjorn.topel@gmail.com> writes:
> On Sat, 1 Jun 2019 at 22:02, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
>>
>> On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:
>> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> > index 44b47e9df94a..f3a875a52c6c 100644
>> > --- a/include/linux/netdevice.h
>> > +++ b/include/linux/netdevice.h
>> > @@ -1940,6 +1940,9 @@ struct net_device {
>> > #endif
>> > struct hlist_node index_hlist;
>> >
>> > + struct bpf_prog *xdp_prog_hw;
>>
>> IDK if we should pay the cost of this pointer for every netdev on the
>> system just for the single production driver out there that implements
>> HW offload :( I'm on the fence about this..
>>
>
> Hmm. Adding a config option? Keep the QUERY_PROG_HW?
>
>> > + u32 xdp_flags;
>> > +
>> > /*
>> > * Cache lines mostly used on transmit path
>> > */
>>
>> > diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> > index adcc045952c2..5e396fd01d8b 100644
>> > --- a/net/core/rtnetlink.c
>> > +++ b/net/core/rtnetlink.c
>> > @@ -1360,42 +1360,44 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
>> > return 0;
>> > }
>> >
>> > -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
>> > +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
>> > {
>> > - const struct bpf_prog *generic_xdp_prog;
>> > -
>> > - ASSERT_RTNL();
>> > -
>> > - generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
>> > - if (!generic_xdp_prog)
>> > - return 0;
>> > - return generic_xdp_prog->aux->id;
>> > -}
>> > -
>> > -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
>> > -{
>> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf, XDP_QUERY_PROG);
>> > + switch (tgt_mode) {
>> > + case XDP_ATTACHED_DRV:
>> > + return XDP_FLAGS_DRV_MODE;
>> > + case XDP_ATTACHED_SKB:
>> > + return XDP_FLAGS_SKB_MODE;
>> > + case XDP_ATTACHED_HW:
>> > + return XDP_FLAGS_HW_MODE;
>> > + }
>> > + return 0;
>> > }
>> >
>> > -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
>> > +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
>> > {
>> > - return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
>> > - XDP_QUERY_PROG_HW);
>> > + switch (tgt_mode) {
>> > + case XDP_ATTACHED_DRV:
>> > + return IFLA_XDP_DRV_PROG_ID;
>> > + case XDP_ATTACHED_SKB:
>> > + return IFLA_XDP_SKB_PROG_ID;
>> > + case XDP_ATTACHED_HW:
>> > + return IFLA_XDP_HW_PROG_ID;
>> > + }
>> > + return 0;
>> > }
>> >
>> > static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
>> > - u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
>> > - u32 (*get_prog_id)(struct net_device *dev))
>> > + u32 *prog_id, u8 *mode, u8 tgt_mode)
>> > {
>> > u32 curr_id;
>> > int err;
>> >
>> > - curr_id = get_prog_id(dev);
>> > + curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
>> > if (!curr_id)
>> > return 0;
>> >
>> > *prog_id = curr_id;
>> > - err = nla_put_u32(skb, attr, curr_id);
>> > + err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
>> > if (err)
>> > return err;
>> >
>> > @@ -1420,16 +1422,13 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>> >
>> > prog_id = 0;
>> > mode = XDP_ATTACHED_NONE;
>> > - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB,
>> > - IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
>> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB);
>> > if (err)
>> > goto err_cancel;
>> > - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV,
>> > - IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
>> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV);
>> > if (err)
>> > goto err_cancel;
>> > - err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW,
>> > - IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
>> > + err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW);
>> > if (err)
>> > goto err_cancel;
>> >
>>
>> So you remove all the attr and flag params just to add a conversion
>> helpers to get them based on mode? Why? Seems like unnecessary churn,
>> and questionable change :S
>>
>
> Fair enough. I'll address this!
I think this was actually my idea, wasn't it? :)
My thought being that if you just do the minimal change here, we'll end
up with three empty wrapper functions, which we might as well just fold
into the caller...
-Toke
^ permalink raw reply [flat|nested] 23+ messages in thread