Message-ID: <883964c3ad30ec49db599241857ceb41d8b35567.camel@mellanox.com>
Date:   Thu, 23 May 2019 05:47:19 +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 1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to
 netdev

On Wed, 2019-05-22 at 14:53 +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 four new members tracking the XDP
> program, the corresponding program flags, and which programs
> (SKB_MODE, DRV_MODE or HW_MODE) that are activated.
> 
> The xdp_prog member, previously only used for SKB_MODE, is shared
> with
> DRV_MODE, since they are mutually exclusive.
> 
> 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. This is because the xdp_prog member is shared from
> a
> query program perspective (again, SKB and DRV are mutual exclusive),
> 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.
> 
> A minor behavioral change was done during this refactorization; When
> passing a negative file descriptor to a netdev to disable XDP, the
> same program flag as the running program is required. An example.
> 
> The eth0 is DRV_DRV capable. Previously, this was OK, but confusing:
> 
>   # ip link set dev eth0 xdp obj foo.o sec main
>   # ip link set dev eth0 xdpgeneric off
> 
> Now, the same commands give:
> 
>   # ip link set dev eth0 xdp obj foo.o sec main
>   # ip link set dev eth0 xdpgeneric off
>   Error: native and generic XDP can't be active at the same time.
> 
> Signed-off-by: Björn Töpel <bjorn.topel@...el.com>
> ---
>  include/linux/netdevice.h |  13 +++--
>  net/core/dev.c            | 120 ++++++++++++++++++++--------------
> ----
>  net/core/rtnetlink.c      |  33 ++---------
>  3 files changed, 76 insertions(+), 90 deletions(-)
> 
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 44b47e9df94a..bdcb20a3946c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1940,6 +1940,11 @@ struct net_device {
>  #endif
>  	struct hlist_node	index_hlist;
>  
> +	struct bpf_prog		*xdp_prog_hw;
> +	unsigned int		xdp_flags;
> +	u32			xdp_prog_flags;
> +	u32			xdp_prog_hw_flags;
> +


Maybe it will be nicer if you wrap all xdp related data in one struct, 
This is good as a weak enforcement so future xdp extensions will go to
the same place and not spread all across the net_device struct.

this is going to be handy when we start introducing XDP offloads.

>  /*
>   * Cache lines mostly used on transmit path
>   */
> @@ -2045,9 +2050,8 @@ struct net_device {
>  
>  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_SKB_MODE;
>  }
>  
>  #define	NETDEV_ALIGN		32
> @@ -3684,8 +3688,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 flags);
>  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/net/core/dev.c b/net/core/dev.c
> index b6b8505cfb3e..ead16c3f955c 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -8005,31 +8005,31 @@ 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)
> +u32 dev_xdp_query(struct net_device *dev, unsigned int flags)
>  {
> -	struct netdev_bpf xdp;
> +	struct bpf_prog *prog = NULL;
>  
> -	if (!bpf_op)
> +	if (hweight32(flags) != 1)
>  		return 0;
>  
> -	memset(&xdp, 0, sizeof(xdp));
> -	xdp.command = cmd;
> -
> -	/* Query must always succeed. */
> -	WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);

since now this function can only query PROG or PROG_HW it is a good
time to reflect that in the function name, maybe dev_xdp_query_prog is
a better name ? 

> +	if (flags & (XDP_FLAGS_SKB_MODE | XDP_FLAGS_DRV_MODE))
> +		prog = rtnl_dereference(dev->xdp_prog);
> +	else if (flags & XDP_FLAGS_HW_MODE)
> +		prog = dev->xdp_prog_hw;
>  
> -	return xdp.prog_id;
> +	return prog ? prog->aux->id : 0;
>  }
>  
> -static int dev_xdp_install(struct net_device *dev, bpf_op_t bpf_op,
> +static int dev_xdp_install(struct net_device *dev, unsigned int
> target,
>  			   struct netlink_ext_ack *extack, u32 flags,
>  			   struct bpf_prog *prog)
>  {
> +	bpf_op_t bpf_op = dev->netdev_ops->ndo_bpf;

I would assing bpf_op below :
bpf_op = (target == XDP_FLAGS_SKB_MODE) ?
         generic_xdp_install :
         dev->netdev_ops->ndo_bpf;

or just drop bpf_op and call dev->netdev_ops->ndo_bpf in the one place
it bpf_op is being called.

>  	struct netdev_bpf xdp;
> +	int err;
>  
>  	memset(&xdp, 0, sizeof(xdp));
> -	if (flags & XDP_FLAGS_HW_MODE)
> +	if (target == XDP_FLAGS_HW_MODE)
>  		xdp.command = XDP_SETUP_PROG_HW;
>  	else
>  		xdp.command = XDP_SETUP_PROG;
> @@ -8037,35 +8037,41 @@ static int dev_xdp_install(struct net_device
> *dev, bpf_op_t bpf_op,
>  	xdp.flags = flags;
>  	xdp.prog = prog;

Are you sure no one is using flags 
>  
> -	return bpf_op(dev, &xdp);
> +	err = (target == XDP_FLAGS_SKB_MODE) ?
> +	      generic_xdp_install(dev, &xdp) :
> +	      bpf_op(dev, &xdp);
> +	if (!err) {
> +		if (prog)
> +			dev->xdp_flags |= target;
> +		else
> +			dev->xdp_flags &= ~target;
> +
> +		if (target == XDP_FLAGS_HW_MODE) {
> +			dev->xdp_prog_hw = prog;
> +			dev->xdp_prog_hw_flags = flags;
> +		} else if (target == XDP_FLAGS_DRV_MODE) {
> +			rcu_assign_pointer(dev->xdp_prog, prog);
> +			dev->xdp_prog_flags = flags;
> +		}
> +	}
> +
> +	return err;
>  }
>  
>  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));
> -
> -	/* 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,
> -					NULL));
> +	if (dev->xdp_flags & XDP_FLAGS_SKB_MODE) {
> +		WARN_ON(dev_xdp_install(dev, XDP_FLAGS_SKB_MODE,
> +					NULL, 0, NULL));
> +	}
> +	if (dev->xdp_flags & XDP_FLAGS_DRV_MODE) {
> +		WARN_ON(dev_xdp_install(dev, XDP_FLAGS_DRV_MODE,
> +					NULL, dev->xdp_prog_flags,
> NULL));
> +	}
> +	if (dev->xdp_flags & XDP_FLAGS_HW_MODE) {
> +		WARN_ON(dev_xdp_install(dev, XDP_FLAGS_HW_MODE,
> +					NULL, dev->xdp_prog_hw_flags,
> NULL));
> +	}

instead this long "if" dance, why don't you just use xdp->flags inside
dev_xdp_install when prog is NULL ? 
I would even consider introducing an uninstall function, which doesn't
receive flags, and will use xdp->flags directly.. 

uninstall can be just a two liner function

dev_xdp_uninstall() {
xdp.prog = NULL;
(dev->xdp_flags == XDP_FLAGS_SKB_MODE) ?
      generic_xdp_install(dev, &xdp) :
      dev->netdev_ops->ndo_bpf(dev, &xdp);
dev->xdp_flags &= ~(XDP_FLAGS_SKB_MODE | XDP_FLAGS_HW_MODE |
XDP_FLAGS_DRV_MODE);
}

>  }
>  
>  /**
> @@ -8080,41 +8086,41 @@ 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;
> +	bool offload, drv_supp = !!dev->netdev_ops->ndo_bpf;
>  	struct bpf_prog *prog = NULL;
> -	bpf_op_t bpf_op, bpf_chk;
> -	bool offload;
> +	unsigned int target;
>  	int err;
>  
>  	ASSERT_RTNL();
>  
>  	offload = flags & XDP_FLAGS_HW_MODE;
> -	query = offload ? XDP_QUERY_PROG_HW : XDP_QUERY_PROG;
> +	target = offload ? XDP_FLAGS_HW_MODE : XDP_FLAGS_DRV_MODE;

I don't think you really need this target parameter it only complicates
things for no reason, all information you need are already in flags and
dev->xdp_flags.

>  
> -	bpf_op = bpf_chk = ops->ndo_bpf;
> -	if (!bpf_op && (flags & (XDP_FLAGS_DRV_MODE |
> XDP_FLAGS_HW_MODE))) {
> +	if (!drv_supp && (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 (!drv_supp || (flags & XDP_FLAGS_SKB_MODE))
> +		target = XDP_FLAGS_SKB_MODE;
> +

since dev_xdp_install handles all cases now, you don't need this logic
here, I would move this into dev_xdp_install, maybe introduce a helper
dedicated function to choose the mode to be used.

> +	if ((target == XDP_FLAGS_SKB_MODE &&
> +	     dev->xdp_flags & XDP_FLAGS_DRV_MODE) ||
> +	    (target == XDP_FLAGS_DRV_MODE &&
> +	     dev->xdp_flags & XDP_FLAGS_SKB_MODE)) {
> +		NL_SET_ERR_MSG(extack, "native and generic XDP can't be
> active at the same time");
> +		return -EEXIST;
> +	}
>  
>  	if (fd >= 0) {
> -		if (!offload && __dev_xdp_query(dev, bpf_chk,
> XDP_QUERY_PROG)) {
> -			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)) {
> +		if (flags & XDP_FLAGS_UPDATE_IF_NOEXIST &&
> +		    dev_xdp_query(dev, target)) {
>  			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,
> drv_supp);
> +
>  		if (IS_ERR(prog))
>  			return PTR_ERR(prog);
>  
> @@ -8125,7 +8131,7 @@ int dev_change_xdp_fd(struct net_device *dev,
> struct netlink_ext_ack *extack,
>  		}
>  	}
>  
> -	err = dev_xdp_install(dev, bpf_op, extack, flags, prog);
> +	err = dev_xdp_install(dev, target, 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..99ca58db297f 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,37 +1360,14 @@ 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)
> -{
> -	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);
> -}
> -
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> -{
> -	return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> -			       XDP_QUERY_PROG_HW);
> -}
> -
>  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))
> +			       unsigned int tgt_flags)
>  {
>  	u32 curr_id;
>  	int err;
>  
> -	curr_id = get_prog_id(dev);
> +	curr_id = dev_xdp_query(dev, tgt_flags);
>  	if (!curr_id)
>  		return 0;
>  
> @@ -1421,15 +1398,15 @@ 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);
> +				  IFLA_XDP_SKB_PROG_ID,
> XDP_FLAGS_SKB_MODE);
>  	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);
> +				  IFLA_XDP_DRV_PROG_ID,
> XDP_FLAGS_DRV_MODE);
>  	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);
> +				  IFLA_XDP_HW_PROG_ID,
> XDP_FLAGS_HW_MODE);
>  	if (err)
>  		goto err_cancel;
>  

Powered by blists - more mailing lists