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

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Fri, 31 May 2019 11:42:14 +0200, Björn Töpel wrote:
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 44b47e9df94a..f3a875a52c6c 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -1940,6 +1940,9 @@ struct net_device {
>  #endif
>  	struct hlist_node	index_hlist;
>  
> +	struct bpf_prog		*xdp_prog_hw;

IDK if we should pay the cost of this pointer for every netdev on the
system just for the single production driver out there that implements
HW offload :(  I'm on the fence about this..

> +	u32			xdp_flags;
> +
>  /*
>   * Cache lines mostly used on transmit path
>   */

> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index adcc045952c2..5e396fd01d8b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -1360,42 +1360,44 @@ static int rtnl_fill_link_ifmap(struct sk_buff *skb, struct net_device *dev)
>  	return 0;
>  }
>  
> -static u32 rtnl_xdp_prog_skb(struct net_device *dev)
> +static unsigned int rtnl_xdp_mode_to_flag(u8 tgt_mode)
>  {
> -	const struct bpf_prog *generic_xdp_prog;
> -
> -	ASSERT_RTNL();
> -
> -	generic_xdp_prog = rtnl_dereference(dev->xdp_prog);
> -	if (!generic_xdp_prog)
> -		return 0;
> -	return generic_xdp_prog->aux->id;
> -}
> -
> -static u32 rtnl_xdp_prog_drv(struct net_device *dev)
> -{
> -	return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf, XDP_QUERY_PROG);
> +	switch (tgt_mode) {
> +	case XDP_ATTACHED_DRV:
> +		return XDP_FLAGS_DRV_MODE;
> +	case XDP_ATTACHED_SKB:
> +		return XDP_FLAGS_SKB_MODE;
> +	case XDP_ATTACHED_HW:
> +		return XDP_FLAGS_HW_MODE;
> +	}
> +	return 0;
>  }
>  
> -static u32 rtnl_xdp_prog_hw(struct net_device *dev)
> +static u32 rtnl_xdp_mode_to_attr(u8 tgt_mode)
>  {
> -	return __dev_xdp_query(dev, dev->netdev_ops->ndo_bpf,
> -			       XDP_QUERY_PROG_HW);
> +	switch (tgt_mode) {
> +	case XDP_ATTACHED_DRV:
> +		return IFLA_XDP_DRV_PROG_ID;
> +	case XDP_ATTACHED_SKB:
> +		return IFLA_XDP_SKB_PROG_ID;
> +	case XDP_ATTACHED_HW:
> +		return IFLA_XDP_HW_PROG_ID;
> +	}
> +	return 0;
>  }
>  
>  static int rtnl_xdp_report_one(struct sk_buff *skb, struct net_device *dev,
> -			       u32 *prog_id, u8 *mode, u8 tgt_mode, u32 attr,
> -			       u32 (*get_prog_id)(struct net_device *dev))
> +			       u32 *prog_id, u8 *mode, u8 tgt_mode)
>  {
>  	u32 curr_id;
>  	int err;
>  
> -	curr_id = get_prog_id(dev);
> +	curr_id = dev_xdp_query(dev, rtnl_xdp_mode_to_flag(tgt_mode));
>  	if (!curr_id)
>  		return 0;
>  
>  	*prog_id = curr_id;
> -	err = nla_put_u32(skb, attr, curr_id);
> +	err = nla_put_u32(skb, rtnl_xdp_mode_to_attr(tgt_mode), curr_id);
>  	if (err)
>  		return err;
>  
> @@ -1420,16 +1422,13 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
>  
>  	prog_id = 0;
>  	mode = XDP_ATTACHED_NONE;
> -	err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB,
> -				  IFLA_XDP_SKB_PROG_ID, rtnl_xdp_prog_skb);
> +	err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_SKB);
>  	if (err)
>  		goto err_cancel;
> -	err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV,
> -				  IFLA_XDP_DRV_PROG_ID, rtnl_xdp_prog_drv);
> +	err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_DRV);
>  	if (err)
>  		goto err_cancel;
> -	err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW,
> -				  IFLA_XDP_HW_PROG_ID, rtnl_xdp_prog_hw);
> +	err = rtnl_xdp_report_one(skb, dev, &prog_id, &mode, XDP_ATTACHED_HW);
>  	if (err)
>  		goto err_cancel;
>  

So you remove all the attr and flag params just to add a conversion
helpers to get them based on mode?  Why?  Seems like unnecessary churn,
and questionable change :S

Otherwise looks good to me!