Message-ID: <b0a9c3b198bdefd145c34e52aa89d33aa502aaf5.camel@mellanox.com>
Date:   Fri, 31 May 2019 19:18:17 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "toke@...hat.com" <toke@...hat.com>,
        "daniel@...earbox.net" <daniel@...earbox.net>,
        "ast@...nel.org" <ast@...nel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "bjorn.topel@...il.com" <bjorn.topel@...il.com>
CC:     "magnus.karlsson@...el.com" <magnus.karlsson@...el.com>,
        "bjorn.topel@...el.com" <bjorn.topel@...el.com>,
        "jakub.kicinski@...ronome.com" <jakub.kicinski@...ronome.com>,
        "brouer@...hat.com" <brouer@...hat.com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>
Subject: Re: [PATCH bpf-next v2 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW}
 to netdev

On Fri, 2019-05-31 at 11:42 +0200, Björn Töpel wrote:
> From: Björn Töpel <bjorn.topel@...el.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@...el.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,

Powered by blists - more mailing lists