[bpf-next,1/2] net: xdp: refactor XDP_QUERY_PROG{,_HW} to netdev

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 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@intel.com>
---
 include/linux/netdevice.h |  13 +++--
 net/core/dev.c            | 120 ++++++++++++++++++++------------------
 net/core/rtnetlink.c      |  33 ++---------
 3 files changed, 76 insertions(+), 90 deletions(-)

Björn Töpel <bjorn.topel@gmail.com> writes:

> 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 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@intel.com>

Nice work!

Reviewed-by: Toke Høiland-Jørgensen <toke@redhat.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;
> +
>  /*
>   * 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);
> +	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;
>  	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;
>  
> -	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));
> +	}
>  }
>  
>  /**
> @@ -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;
>  
> -	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;
> +
> +	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;
>  
> -- 
> 2.20.1

On Wed, 22 May 2019 14:53:51 +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 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.

I'm not clear why this change is necessary? It is a change in
behaviour, and if anything returning ENOENT would seem cleaner 
in this case.

> Signed-off-by: Björn Töpel <bjorn.topel@intel.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;

Do we really need 3 xdp flags variables?  Offloaded programs
realistically must have only the HW mode flag set (not sure if 
netdevsim still pretends we can do offload of code after rewrites,
but if it does it can be changed :)).  Non-offloaded programs need
flags, but we don't need the "all ORed" flags either, AFAICT.  No?

> +
>  /*
>   * 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)

You only pass the mode here, so perhaps rename the flags argument to
mode?

>  {
> -	struct netdev_bpf xdp;
> +	struct bpf_prog *prog = NULL;
>  
> -	if (!bpf_op)
> +	if (hweight32(flags) != 1)
>  		return 0;

This looks superfluous, given callers will always pass mode, right?

> -	memset(&xdp, 0, sizeof(xdp));
> -	xdp.command = cmd;
> -
> -	/* Query must always succeed. */
> -	WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> +	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;

Perhaps use a switch statement here?

> -	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,

Could you say mode instead of target everywhere?

>  			   struct netlink_ext_ack *extack, u32 flags,
>  			   struct bpf_prog *prog)
>  {
> +	bpf_op_t bpf_op = dev->netdev_ops->ndo_bpf;

The point of passing bpf_op around is to have the right one (generic or
driver) this is lost with the ternary statement below :S

>  	struct netdev_bpf xdp;
> +	int err;
>  
>  	memset(&xdp, 0, sizeof(xdp));
> -	if (flags & XDP_FLAGS_HW_MODE)
> +	if (target == XDP_FLAGS_HW_MODE)

Is this change necessary?

>  		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;
>  
> -	return bpf_op(dev, &xdp);
> +	err = (target == XDP_FLAGS_SKB_MODE) ?

Brackets unnecessary.

> +	      generic_xdp_install(dev, &xdp) :
> +	      bpf_op(dev, &xdp);
> +	if (!err) {

Keep success path unindented, save indentation.

	if (err)
		return err;

	bla bla

	return 0;

> +		if (prog)
> +			dev->xdp_flags |= target;
> +		else
> +			dev->xdp_flags &= ~target;

These "all ORed" flags are not needed, AFAICT, as mentioned above.

> +		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));
> +	}

Brackets unnecessary.

> +	if (dev->xdp_flags & XDP_FLAGS_DRV_MODE) {
> +		WARN_ON(dev_xdp_install(dev, XDP_FLAGS_DRV_MODE,
> +					NULL, dev->xdp_prog_flags, NULL));
> +	}

You should be able to just call install with the original flags, and
install handler should do the right maths again to direct it either to
drv or generic, no?

> +	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));
> +	}
>  }
>  
>  /**
> @@ -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;

Please avoid calculations in init.  If the function gets complicated
this just ends up as a weirdly indented code, which is hard to read.

>  	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;
>  
> -	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))) {

Here you have a bracket for & inside an &&..

>  		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;
> +
> +	if ((mode == XDP_FLAGS_SKB_MODE &&
> +	     dev->xdp_flags & XDP_FLAGS_DRV_MODE) ||

.. and here you don't :)

> +	    (target == XDP_FLAGS_DRV_MODE &&
> +	     dev->xdp_flags & XDP_FLAGS_SKB_MODE)) {

I think this condition can be shortened.  You can't get a conflict if
the driver has no support.  So the only conflicting case is:

	rcu_access_pointer(dev->xdp_prog) && drv_supp &&
	(flags ^ 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);
> +

Extra new line.

>  		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);
>  

I think apart from returning the new error it looks functionally
correct :)

On Wed, 22 May 2019 at 20:32, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 22 May 2019 14:53:51 +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 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.
>
> I'm not clear why this change is necessary? It is a change in
> behaviour, and if anything returning ENOENT would seem cleaner
> in this case.
>

To me, the existing behavior was non-intuitive. If most people *don't*
agree, I'll remove this change. So, what do people think about this?
:-)

ENOENT does make more sense.

> > Signed-off-by: Björn Töpel <bjorn.topel@intel.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;
>
> Do we really need 3 xdp flags variables?  Offloaded programs
> realistically must have only the HW mode flag set (not sure if
> netdevsim still pretends we can do offload of code after rewrites,
> but if it does it can be changed :)).  Non-offloaded programs need
> flags, but we don't need the "all ORed" flags either, AFAICT.  No?
>

Good point, I'll try to reduce the netdev bloat.

> > +
> >  /*
> >   * 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)
>
> You only pass the mode here, so perhaps rename the flags argument to
> mode?
>
> >  {
> > -     struct netdev_bpf xdp;
> > +     struct bpf_prog *prog = NULL;
> >
> > -     if (!bpf_op)
> > +     if (hweight32(flags) != 1)
> >               return 0;
>
> This looks superfluous, given callers will always pass mode, right?
>
> > -     memset(&xdp, 0, sizeof(xdp));
> > -     xdp.command = cmd;
> > -
> > -     /* Query must always succeed. */
> > -     WARN_ON(bpf_op(dev, &xdp) < 0 && cmd == XDP_QUERY_PROG);
> > +     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;
>
> Perhaps use a switch statement here?
>
> > -     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,
>
> Could you say mode instead of target everywhere?
>
> >                          struct netlink_ext_ack *extack, u32 flags,
> >                          struct bpf_prog *prog)
> >  {
> > +     bpf_op_t bpf_op = dev->netdev_ops->ndo_bpf;
>
> The point of passing bpf_op around is to have the right one (generic or
> driver) this is lost with the ternary statement below :S
>
> >       struct netdev_bpf xdp;
> > +     int err;
> >
> >       memset(&xdp, 0, sizeof(xdp));
> > -     if (flags & XDP_FLAGS_HW_MODE)
> > +     if (target == XDP_FLAGS_HW_MODE)
>
> Is this change necessary?
>
> >               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;
> >
> > -     return bpf_op(dev, &xdp);
> > +     err = (target == XDP_FLAGS_SKB_MODE) ?
>
> Brackets unnecessary.
>
> > +           generic_xdp_install(dev, &xdp) :
> > +           bpf_op(dev, &xdp);
> > +     if (!err) {
>
> Keep success path unindented, save indentation.
>
>         if (err)
>                 return err;
>
>         bla bla
>
>         return 0;
>
> > +             if (prog)
> > +                     dev->xdp_flags |= target;
> > +             else
> > +                     dev->xdp_flags &= ~target;
>
> These "all ORed" flags are not needed, AFAICT, as mentioned above.
>
> > +             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));
> > +     }
>
> Brackets unnecessary.
>
> > +     if (dev->xdp_flags & XDP_FLAGS_DRV_MODE) {
> > +             WARN_ON(dev_xdp_install(dev, XDP_FLAGS_DRV_MODE,
> > +                                     NULL, dev->xdp_prog_flags, NULL));
> > +     }
>
> You should be able to just call install with the original flags, and
> install handler should do the right maths again to direct it either to
> drv or generic, no?
>
> > +     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));
> > +     }
> >  }
> >
> >  /**
> > @@ -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;
>
> Please avoid calculations in init.  If the function gets complicated
> this just ends up as a weirdly indented code, which is hard to read.
>
> >       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;
> >
> > -     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))) {
>
> Here you have a bracket for & inside an &&..
>
> >               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;
> > +
> > +     if ((mode == XDP_FLAGS_SKB_MODE &&
> > +          dev->xdp_flags & XDP_FLAGS_DRV_MODE) ||
>
> .. and here you don't :)
>
> > +         (target == XDP_FLAGS_DRV_MODE &&
> > +          dev->xdp_flags & XDP_FLAGS_SKB_MODE)) {
>
> I think this condition can be shortened.  You can't get a conflict if
> the driver has no support.  So the only conflicting case is:
>
>         rcu_access_pointer(dev->xdp_prog) && drv_supp &&
>         (flags ^ 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);
> > +
>
> Extra new line.
>
> >               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);
> >
>
> I think apart from returning the new error it looks functionally
> correct :)

Thanks for the review, Jakub. Very valuable. I'll address all your
comments in the v3!


Cheers,
Björn

On Wed, 22 May 2019 22:54:44 +0200, Björn Töpel wrote:
> > > 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.  
> >
> > I'm not clear why this change is necessary? It is a change in
> > behaviour, and if anything returning ENOENT would seem cleaner
> > in this case.
> 
> To me, the existing behavior was non-intuitive. If most people *don't*
> agree, I'll remove this change. So, what do people think about this?
> :-)

Having things start to fail after they were successful/ignored
is one of those ABI breakage types Linux and netdev usually takes
pretty seriously, unfortunately.  Especially when motivation is 
"it's more intuitive" :)

If nobody chimes in please break out this behaviour change into 
a commit of its own.

> ENOENT does make more sense.

On Wed, 22 May 2019 at 23:04, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 22 May 2019 22:54:44 +0200, Björn Töpel wrote:
> > > > 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.
> > >
> > > I'm not clear why this change is necessary? It is a change in
> > > behaviour, and if anything returning ENOENT would seem cleaner
> > > in this case.
> >
> > To me, the existing behavior was non-intuitive. If most people *don't*
> > agree, I'll remove this change. So, what do people think about this?
> > :-)
>
> Having things start to fail after they were successful/ignored
> is one of those ABI breakage types Linux and netdev usually takes
> pretty seriously, unfortunately.  Especially when motivation is
> "it's more intuitive" :)
>

Hey, intuition is a great thing! :-D

> If nobody chimes in please break out this behaviour change into
> a commit of its own.
>

Will do.


Björn


> > ENOENT does make more sense.

On Wed, 2019-05-22 at 14:53 +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 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@intel.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;
>

On Thu, 23 May 2019 at 07:47, Saeed Mahameed <saeedm@mellanox.com> wrote:
>
> On Wed, 2019-05-22 at 14:53 +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 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@intel.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;
> >

Saeed, thanks a lot for your comments! All good stuff. I'll address
them in the next version!


Thanks,
Björn

Jakub Kicinski <jakub.kicinski@netronome.com> writes:

> On Wed, 22 May 2019 22:54:44 +0200, Björn Töpel wrote:
>> > > 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.  
>> >
>> > I'm not clear why this change is necessary? It is a change in
>> > behaviour, and if anything returning ENOENT would seem cleaner
>> > in this case.
>> 
>> To me, the existing behavior was non-intuitive. If most people *don't*
>> agree, I'll remove this change. So, what do people think about this?
>> :-)
>
> Having things start to fail after they were successful/ignored
> is one of those ABI breakage types Linux and netdev usually takes
> pretty seriously, unfortunately.  Especially when motivation is 
> "it's more intuitive" :)
>
> If nobody chimes in please break out this behaviour change into 
> a commit of its own.

Björn and I already had this discussion off-list. I think we ended up
concluding that since it's not changing kernel *behaviour*, but just
making an existing error explicit, it might be acceptable from an ABI
breakage PoV. And I'd generally prefer explicit errors over silent
failures.

But yeah, can totally see why it could also be considered a breaking
change...

-Toke

On Wed, 22 May 2019 at 20:32, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
[...]
>
> You should be able to just call install with the original flags, and
> install handler should do the right maths again to direct it either to
> drv or generic, no?
>

On a related note: I ran the test_offload.py test (thanks for pointing
that out!), and realized that my view of load flags was incorrect. To
double-check:

Given an XDP DRV capable netdev "eth0".

# ip link set dev eth0 xdp obj foo.o sec .text
# ip link set dev eth0 xdpdrv off

and

# ip link set dev eth0 xdpdrv obj foo.o sec .text
# ip link set dev eth0 xdp off

and

# ip link set dev eth0 xdpdrv obj foo.o sec .text
# ip link -force set dev eth0 xdp obj foo.o sec .text

and

# ip link set dev eth0 xdp obj foo.o sec .text
# ip link -force set dev eth0 xdpdrv obj foo.o sec .text

Should all fail. IOW, there's a distinction between explicit DRV and
auto-detected DRV? It's considered to be different flags.

Correct?

This was *not* my view. :-)


Thanks,
Björn

On Tue, 28 May 2019 19:06:21 +0200, Björn Töpel wrote:
> On Wed, 22 May 2019 at 20:32, Jakub Kicinski wrote:
> > You should be able to just call install with the original flags, and
> > install handler should do the right maths again to direct it either to
> > drv or generic, no?
> >  
> 
> On a related note: I ran the test_offload.py test (thanks for pointing
> that out!), and realized that my view of load flags was incorrect. To
> double-check:
> 
> Given an XDP DRV capable netdev "eth0".
> 
> # ip link set dev eth0 xdp obj foo.o sec .text
> # ip link set dev eth0 xdpdrv off
> 
> and
> 
> # ip link set dev eth0 xdpdrv obj foo.o sec .text
> # ip link set dev eth0 xdp off
> 
> and
> 
> # ip link set dev eth0 xdpdrv obj foo.o sec .text
> # ip link -force set dev eth0 xdp obj foo.o sec .text
> 
> and
> 
> # ip link set dev eth0 xdp obj foo.o sec .text
> # ip link -force set dev eth0 xdpdrv obj foo.o sec .text
> 
> Should all fail. IOW, there's a distinction between explicit DRV and
> auto-detected DRV? It's considered to be different flags.
> 
> Correct?

I think so.  That's the way drivers which implement offloads work
(netdevsim and nfp).

However:

ip link set dev eth0 xdpdrv obj foo.o sec .text
ip link set dev eth0 xdpoffload off
ip link set dev eth0 xdpgeneric off

are fine.  It's just the no flag case that's special, to avoid
confusion.  If one always uses the flags there should be no errors.

> This was *not* my view. :-)