Re: [PATCH 1/2] r8169: Add XDP support for pass and drop actions

Re: [PATCH 1/2] r8169: Add XDP support for pass and drop actions

[Heiner Kallweit]

On 14.09.2021 11:31, Guilin Tang wrote:
> This commi implements simple xdp drop and pass in the r8169 driver
> 
> Signed-off-by: Guilin Tang <tangguilin@uniontech.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 99 +++++++++++++++++++++--
>  1 file changed, 94 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index d927211f8d2c..69bc3c68e73d 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -30,6 +30,9 @@
>  #include <linux/ipv6.h>
>  #include <asm/unaligned.h>
>  #include <net/ip6_checksum.h>
> +#include <net/xdp.h>
> +#include <linux/bpf.h>
> +#include <linux/bpf_trace.h>
>  
>  #include "r8169.h"
>  #include "r8169_firmware.h"
> @@ -543,6 +546,12 @@ enum rtl_rx_desc_bit {
>  #define RTL_GSO_MAX_SIZE_V2	64000
>  #define RTL_GSO_MAX_SEGS_V2	64
>  
> +/* XDP */
> +#define R8169_XDP_PASS		0
> +#define R8169_XDP_DROP		BIT(0)
> +#define R8169_XDP_TX		BIT(1)
> +#define R8169_XDP_REDIR		BIT(2)
> +
>  struct TxDesc {
>  	__le32 opts1;
>  	__le32 opts2;
> @@ -634,6 +643,8 @@ struct rtl8169_private {
>  	struct rtl_fw *rtl_fw;
>  
>  	u32 ocp_base;
> +	/*xdp bpf*/
> +	struct bpf_prog *rtl_xdp;
>  };
>  
>  typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
> @@ -3896,6 +3907,7 @@ static void rtl8169_rx_clear(struct rtl8169_private *tp)
>  		tp->RxDescArray[i].addr = 0;
>  		tp->RxDescArray[i].opts1 = 0;
>  	}
> +	tp->rtl_xdp = NULL;
>  }
>  
>  static int rtl8169_rx_fill(struct rtl8169_private *tp)
> @@ -4501,10 +4513,44 @@ static inline void rtl8169_rx_csum(struct sk_buff *skb, u32 opts1)
>  		skb_checksum_none_assert(skb);
>  }
>  
> +static struct sk_buff *rtl8619_run_xdp(struct rtl8169_private *tp, struct bpf_prog *xdp_prog,
> +				void *rx_buf, unsigned int pkt_size)
> +{

Why return type struct sk_buff * and not a normal int / errno ?

> +	int result = R8169_XDP_PASS;
> +	struct xdp_buff xdp;
> +	u32 act;
> +
> +	xdp.data = rx_buf;
> +	xdp.data_end = xdp.data + pkt_size;
> +	xdp_set_data_meta_invalid(&xdp);
> +
> +	act = bpf_prog_run_xdp(xdp_prog, &xdp);
> +	switch (act) {
> +	case XDP_PASS:
> +		break;
> +	case XDP_TX:
> +	case XDP_REDIRECT:
> +		goto out_failure;
> +	default:
> +		bpf_warn_invalid_xdp_action(act);
> +		fallthrough;
> +	case XDP_ABORTED:
> +out_failure:
> +		trace_xdp_exception(tp->dev, xdp_prog, act);
> +		fallthrough;
> +	case XDP_DROP:
> +		result = R8169_XDP_DROP;
> +		break;
> +	}
> +
> +	return ERR_PTR(-result);

Overriding errno's with own values isn't nice. If you need an errno,
use an errno.

> +}
> +
>  static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget)
>  {
>  	struct device *d = tp_to_dev(tp);
>  	int count;
> +	struct bpf_prog *xdp_prog;
>  
>  	for (count = 0; count < budget; count++, tp->cur_rx++) {
>  		unsigned int pkt_size, entry = tp->cur_rx % NUM_RX_DESC;
> @@ -4553,17 +4599,27 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>  			goto release_descriptor;
>  		}
>  
> +		addr = le64_to_cpu(desc->addr);
> +		rx_buf = page_address(tp->Rx_databuff[entry]);
> +
> +		dma_sync_single_for_cpu(d, addr, pkt_size, DMA_FROM_DEVICE);
> +		prefetch(rx_buf);
> +		//Determine whether to execute xdp
> +		xdp_prog = READ_ONCE(tp->rtl_xdp);
> +		if (xdp_prog) {
> +			skb = rtl8619_run_xdp(tp, xdp_prog, (void *)rx_buf, pkt_size);

Why do you hijack the skb variable? In case of no error it's overwritten
a few lines later.

> +			if (IS_ERR(skb)) {
> +				dev->stats.rx_dropped++;
> +				goto release_descriptor;
> +			}
> +		}
> +
>  		skb = napi_alloc_skb(&tp->napi, pkt_size);
>  		if (unlikely(!skb)) {
>  			dev->stats.rx_dropped++;
>  			goto release_descriptor;
>  		}
>  
> -		addr = le64_to_cpu(desc->addr);
> -		rx_buf = page_address(tp->Rx_databuff[entry]);
> -
> -		dma_sync_single_for_cpu(d, addr, pkt_size, DMA_FROM_DEVICE);
> -		prefetch(rx_buf);
>  		skb_copy_to_linear_data(skb, rx_buf, pkt_size);
>  		skb->tail += pkt_size;
>  		skb->len = pkt_size;
> @@ -4999,6 +5055,38 @@ static void rtl_remove_one(struct pci_dev *pdev)
>  	rtl_rar_set(tp, tp->dev->perm_addr);
>  }
>  
> +static int r8169_xdp_set(struct net_device *netdev, struct netdev_bpf *bpf)
> +{
> +	struct rtl8169_private *tp = netdev_priv(netdev);
> +	struct bpf_prog *prog = bpf->prog, *old_prog;
> +	bool running = netif_running(netdev);
> +	bool need_reset;
> +
> +	need_reset = !!tp->rtl_xdp != !!prog;
> +

An explanation would be helpful why a reset is needed and what you
mean with reset. Using these functions outside their usual context
is at least risky.

> +	if (need_reset && running)
> +		rtl8169_close(netdev);
> +
> +	old_prog = xchg(&tp->rtl_xdp, prog);
> +	if (old_prog)
> +		bpf_prog_put(old_prog);
> +
> +	if (need_reset && running)
> +		rtl_open(netdev);
> +
> +	return 0;
> +}
> +
> +static int rtl8169_xdp(struct net_device *dev, struct netdev_bpf *xdp)
> +{
> +	switch (xdp->command) {
> +	case XDP_SETUP_PROG:
> +		return r8169_xdp_set(dev, xdp);
> +	default:
> +		return -EINVAL;
> +	}
> +}
> +
>  static const struct net_device_ops rtl_netdev_ops = {
>  	.ndo_open		= rtl_open,
>  	.ndo_stop		= rtl8169_close,
> @@ -5013,6 +5101,7 @@ static const struct net_device_ops rtl_netdev_ops = {
>  	.ndo_set_mac_address	= rtl_set_mac_address,
>  	.ndo_eth_ioctl		= phy_do_ioctl_running,
>  	.ndo_set_rx_mode	= rtl_set_rx_mode,
> +	.ndo_bpf			= rtl8169_xdp,
>  #ifdef CONFIG_NET_POLL_CONTROLLER
>  	.ndo_poll_controller	= rtl8169_netpoll,
>  #endif
> 

Re: [PATCH 2/2] r8169: ADD XDP support for redirect action

[Heiner Kallweit]

On 14.09.2021 11:31, Guilin Tang wrote:
> Implement XDP_REDIRECT based on the r8169 XDP implementation.
> Can use sample/bpf/xdpsock test.
> 
> Signed-off-by: Guilin Tang <tangguilin@uniontech.com>
> ---
>  drivers/net/ethernet/realtek/r8169_main.c | 78 +++++++++++++++++++----
>  1 file changed, 65 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 69bc3c68e73d..b56899530ed5 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -69,6 +69,8 @@
>  
>  #define R8169_REGS_SIZE		256
>  #define R8169_RX_BUF_SIZE	(SZ_16K - 1)
> +/*Reserve space for XDP frame*/
> +#define R8169_RX_XDP_BUF_SIZE	(R8169_RX_BUF_SIZE - XDP_PACKET_HEADROOM)
>  #define NUM_TX_DESC	256	/* Number of Tx descriptor registers */
>  #define NUM_RX_DESC	256	/* Number of Rx descriptor registers */
>  #define R8169_TX_RING_BYTES	(NUM_TX_DESC * sizeof(struct TxDesc))
> @@ -645,6 +647,7 @@ struct rtl8169_private {
>  	u32 ocp_base;
>  	/*xdp bpf*/
>  	struct bpf_prog *rtl_xdp;
> +	struct xdp_rxq_info rtl_rxq;
>  };
>  
>  typedef void (*rtl_generic_fct)(struct rtl8169_private *tp);
> @@ -2516,7 +2519,7 @@ static void rtl_set_tx_config_registers(struct rtl8169_private *tp)
>  static void rtl_set_rx_max_size(struct rtl8169_private *tp)
>  {
>  	/* Low hurts. Let's disable the filtering. */
> -	RTL_W16(tp, RxMaxSize, R8169_RX_BUF_SIZE + 1);
> +	RTL_W16(tp, RxMaxSize, R8169_RX_XDP_BUF_SIZE + 1);

You shouldn't have to change this value. Max mtu is 9k and this
16k buffer is used due to hw bug on an older chip versions.

>  }
>  
>  static void rtl_set_rx_tx_desc_registers(struct rtl8169_private *tp)
> @@ -3866,7 +3869,7 @@ static void rtl8169_mark_to_asic(struct RxDesc *desc)
>  	desc->opts2 = 0;
>  	/* Force memory writes to complete before releasing descriptor */
>  	dma_wmb();
> -	WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE));
> +	WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_XDP_BUF_SIZE));
>  }
>  
>  static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp,
> @@ -3881,7 +3884,8 @@ static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp,
>  	if (!data)
>  		return NULL;
>  
> -	mapping = dma_map_page(d, data, 0, R8169_RX_BUF_SIZE, DMA_FROM_DEVICE);
> +	mapping = dma_map_page(d, data, XDP_PACKET_HEADROOM,
> +			R8169_RX_XDP_BUF_SIZE, DMA_FROM_DEVICE);
>  	if (unlikely(dma_mapping_error(d, mapping))) {
>  		netdev_err(tp->dev, "Failed to map RX DMA!\n");
>  		__free_pages(data, get_order(R8169_RX_BUF_SIZE));
> @@ -3901,19 +3905,28 @@ static void rtl8169_rx_clear(struct rtl8169_private *tp)
>  	for (i = 0; i < NUM_RX_DESC && tp->Rx_databuff[i]; i++) {
>  		dma_unmap_page(tp_to_dev(tp),
>  			       le64_to_cpu(tp->RxDescArray[i].addr),
> -			       R8169_RX_BUF_SIZE, DMA_FROM_DEVICE);
> +			       R8169_RX_XDP_BUF_SIZE, DMA_FROM_DEVICE);
>  		__free_pages(tp->Rx_databuff[i], get_order(R8169_RX_BUF_SIZE));
>  		tp->Rx_databuff[i] = NULL;
>  		tp->RxDescArray[i].addr = 0;
>  		tp->RxDescArray[i].opts1 = 0;
>  	}
>  	tp->rtl_xdp = NULL;
> +	xdp_rxq_info_unreg(&tp->rtl_rxq);
>  }
>  
>  static int rtl8169_rx_fill(struct rtl8169_private *tp)
>  {
>  	int i;
>  
> +	if (xdp_rxq_info_reg(&tp->rtl_rxq, tp->dev, 0, 0))
> +		return -1;

Again, if you need an errno, use an errno.

> +
> +	if (xdp_rxq_info_reg_mem_model(&tp->rtl_rxq, MEM_TYPE_PAGE_SHARED, NULL)) {
> +		xdp_rxq_info_unreg(&tp->rtl_rxq);
> +		return -1;
> +	}
> +
>  	for (i = 0; i < NUM_RX_DESC; i++) {
>  		struct page *data;
>  
> @@ -4513,24 +4526,50 @@ static inline void rtl8169_rx_csum(struct sk_buff *skb, u32 opts1)
>  		skb_checksum_none_assert(skb);
>  }
>  
> +struct page *rtl8169_realloc_rx_data(struct rtl8169_private *tp, struct RxDesc *desc)
> +{
> +	struct page *data = NULL;
> +
> +	do {
> +		data = rtl8169_alloc_rx_data(tp, desc);
> +	} while (data == NULL);
> +
> +	return data;
> +}
> +
>  static struct sk_buff *rtl8619_run_xdp(struct rtl8169_private *tp, struct bpf_prog *xdp_prog,
> -				void *rx_buf, unsigned int pkt_size)
> +				void *rx_buf, unsigned int pkt_size, unsigned int entry)
>  {
> -	int result = R8169_XDP_PASS;
> +	int err, result = R8169_XDP_PASS;
>  	struct xdp_buff xdp;
>  	u32 act;
> +	struct RxDesc *desc = tp->RxDescArray + entry;
>  
> -	xdp.data = rx_buf;
> -	xdp.data_end = xdp.data + pkt_size;
> -	xdp_set_data_meta_invalid(&xdp);
> +	xdp_init_buff(&xdp, R8169_RX_XDP_BUF_SIZE, &tp->rtl_rxq);
> +	xdp_prepare_buff(&xdp, (unsigned char *)rx_buf, XDP_PACKET_HEADROOM, pkt_size, true);
> +	prefetchw(xdp.data_hard_start);
>  
>  	act = bpf_prog_run_xdp(xdp_prog, &xdp);
>  	switch (act) {
>  	case XDP_PASS:
>  		break;
>  	case XDP_TX:
> -	case XDP_REDIRECT:
>  		goto out_failure;
> +	case XDP_REDIRECT:
> +		/*unmap dma page*/
> +		dma_unmap_page(tp_to_dev(tp), le64_to_cpu(desc->addr),
> +			R8169_RX_XDP_BUF_SIZE, DMA_FROM_DEVICE);
> +		err = xdp_do_redirect(tp->dev, &xdp, xdp_prog);
> +		if (unlikely(err)) {
> +			/*free page*/
> +			__free_pages(tp->Rx_databuff[entry], get_order(R8169_RX_BUF_SIZE));
> +			/*realloc*/
> +			tp->Rx_databuff[entry] = rtl8169_realloc_rx_data(tp, desc);

Why this reallocation?

> +			goto out_failure;
> +		} else {
> +			result = R8169_XDP_REDIR;
> +		}
> +		break;
>  	default:
>  		bpf_warn_invalid_xdp_action(act);
>  		fallthrough;
> @@ -4551,6 +4590,7 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>  	struct device *d = tp_to_dev(tp);
>  	int count;
>  	struct bpf_prog *xdp_prog;
> +	unsigned int xdp_xmit = 0;
>  
>  	for (count = 0; count < budget; count++, tp->cur_rx++) {
>  		unsigned int pkt_size, entry = tp->cur_rx % NUM_RX_DESC;
> @@ -4607,9 +4647,18 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>  		//Determine whether to execute xdp
>  		xdp_prog = READ_ONCE(tp->rtl_xdp);
>  		if (xdp_prog) {
> -			skb = rtl8619_run_xdp(tp, xdp_prog, (void *)rx_buf, pkt_size);
> +			skb = rtl8619_run_xdp(tp, xdp_prog, (void *)rx_buf, pkt_size, entry);
>  			if (IS_ERR(skb)) {
> -				dev->stats.rx_dropped++;
> +				unsigned int xdp_res = -PTR_ERR(skb);
> +
> +				if (xdp_res & R8169_XDP_REDIR) {
> +					tp->Rx_databuff[entry] = rtl8169_realloc_rx_data(tp, desc);

Why this reallocation?

> +					xdp_xmit |= xdp_res;
> +					dev->stats.rx_packets++;

Why not use dev_sw_netstats_rx_add() like in standard path?

> +					dev->stats.rx_bytes += pkt_size;
> +				} else {
> +					dev->stats.rx_dropped++;
> +				}
>  				goto release_descriptor;
>  			}
>  		}
> @@ -4620,7 +4669,7 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>  			goto release_descriptor;
>  		}
>  
> -		skb_copy_to_linear_data(skb, rx_buf, pkt_size);
> +		skb_copy_to_linear_data(skb, rx_buf + XDP_PACKET_HEADROOM, pkt_size);
>  		skb->tail += pkt_size;
>  		skb->len = pkt_size;
>  		dma_sync_single_for_device(d, addr, pkt_size, DMA_FROM_DEVICE);
> @@ -4640,6 +4689,9 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>  		rtl8169_mark_to_asic(desc);
>  	}
>  
> +	if (xdp_xmit & R8169_XDP_REDIR)
> +		xdp_do_flush();
> +
>  	return count;
>  }
>  
> 

Re: [PATCH 0/2] Implement XDP in r8169

[Heiner Kallweit]

On 14.09.2021 11:31, Guilin Tang wrote:
> I implemented XDP on the r8169 driver so that people in need can use it.
> 
> Guilin Tang (2):
>   r8169:Add XDP support for pass and drop actions
>   r8169: ADD XDP support for redirect action
> 
>  drivers/net/ethernet/realtek/r8169_main.c | 161 ++++++++++++++++++++--
>  1 file changed, 151 insertions(+), 10 deletions(-)
> 

I don't know XDP, therefore my review comments don't cover the
functionality. As prerequisite for merging this series somebody
knowing XDP would have to review the functional part.

Some comments would be helpful on which platforms and with which 
chip versions you tested.

Last but not least: I can't support the XDP extension. Therefore I'd ask you
to provide the needed support (incl. monitoring bugzilla.kernel.org) in case
of any problem reports, even if not directly related to XDP.
The XDP changes may brake other stuff.