[PATCH 0/7] igb: xdp patches followup

[PATCH 0/7] igb: xdp patches followup

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

This patch series addresses some of the comments that came back
after the igb XDP patch was accepted.
Most of it is code cleanup.
The last patch contains a fix for a tx queue timeout
that can occur when using xdp.

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>

Sven Auhagen (7):
  igb: XDP xmit back fix error code
  igb: take vlan double header into account
  igb: XDP extack message on error
  igb: skb add metasize for xdp
  igb: use igb_rx_buffer_flip
  igb: use xdp_do_flush
  igb: avoid transmit queue timeout in xdp path

 drivers/net/ethernet/intel/igb/igb.h      |   5 +
 drivers/net/ethernet/intel/igb/igb_main.c | 119 ++++++++++------------
 2 files changed, 61 insertions(+), 63 deletions(-)

-- 
2.20.1

[PATCH 1/7] igb: XDP xmit back fix error code

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

The igb XDP xmit back function should only return
defined error codes.

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 5fc2c381da55..08cc6f59aa2e 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2910,7 +2910,7 @@ static int igb_xdp_xmit_back(struct igb_adapter *adapter, struct xdp_buff *xdp)
 	 */
 	tx_ring = adapter->xdp_prog ? igb_xdp_tx_queue_mapping(adapter) : NULL;
 	if (unlikely(!tx_ring))
-		return -ENXIO;
+		return IGB_XDP_CONSUMED;
 
 	nq = txring_txq(tx_ring);
 	__netif_tx_lock(nq, cpu);
-- 
2.20.1

[PATCH 2/7] igb: take vlan double header into account

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

Increase the packet header padding to include double VLAN tagging.
This patch uses a macro for this.

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
---
 drivers/net/ethernet/intel/igb/igb.h      | 5 +++++
 drivers/net/ethernet/intel/igb/igb_main.c | 7 +++----
 2 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
index 0286d2fceee4..7afb67cf9b94 100644
--- a/drivers/net/ethernet/intel/igb/igb.h
+++ b/drivers/net/ethernet/intel/igb/igb.h
@@ -138,6 +138,8 @@ struct vf_mac_filter {
 /* this is the size past which hardware will drop packets when setting LPE=0 */
 #define MAXIMUM_ETHERNET_VLAN_SIZE 1522
 
+#define IGB_ETH_PKT_HDR_PAD	(ETH_HLEN + ETH_FCS_LEN + (VLAN_HLEN * 2))
+
 /* Supported Rx Buffer Sizes */
 #define IGB_RXBUFFER_256	256
 #define IGB_RXBUFFER_1536	1536
@@ -247,6 +249,9 @@ enum igb_tx_flags {
 #define IGB_SFF_ADDRESSING_MODE		0x4
 #define IGB_SFF_8472_UNSUP		0x00
 
+/* TX ressources are shared between XDP and netstack
+ * and we need to tag the buffer type to distinguish them
+ */
 enum igb_tx_buf_type {
 	IGB_TYPE_SKB = 0,
 	IGB_TYPE_XDP,
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 08cc6f59aa2e..0a9198037b98 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2826,7 +2826,7 @@ static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
 
 static int igb_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
 {
-	int i, frame_size = dev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+	int i, frame_size = dev->mtu + IGB_ETH_PKT_HDR_PAD;
 	struct igb_adapter *adapter = netdev_priv(dev);
 	bool running = netif_running(dev);
 	struct bpf_prog *old_prog;
@@ -3950,8 +3950,7 @@ static int igb_sw_init(struct igb_adapter *adapter)
 	/* set default work limits */
 	adapter->tx_work_limit = IGB_DEFAULT_TX_WORK;
 
-	adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN +
-				  VLAN_HLEN;
+	adapter->max_frame_size = netdev->mtu + IGB_ETH_PKT_HDR_PAD;
 	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
 
 	spin_lock_init(&adapter->nfc_lock);
@@ -6491,7 +6490,7 @@ static void igb_get_stats64(struct net_device *netdev,
 static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 {
 	struct igb_adapter *adapter = netdev_priv(netdev);
-	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+	int max_frame = new_mtu + IGB_ETH_PKT_HDR_PAD;
 
 	if (adapter->xdp_prog) {
 		int i;
-- 
2.20.1

[PATCH 3/7] igb: XDP extack message on error

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

Add an extack error message when the RX buffer size is too small
for the frame size.

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 0a9198037b98..088f9ddb0093 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2824,20 +2824,22 @@ static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
 	}
 }
 
-static int igb_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
+static int igb_xdp_setup(struct net_device *dev, struct netdev_bpf *bpf)
 {
 	int i, frame_size = dev->mtu + IGB_ETH_PKT_HDR_PAD;
 	struct igb_adapter *adapter = netdev_priv(dev);
+	struct bpf_prog *prog = bpf->prog, *old_prog;
 	bool running = netif_running(dev);
-	struct bpf_prog *old_prog;
 	bool need_reset;
 
 	/* verify igb ring attributes are sufficient for XDP */
 	for (i = 0; i < adapter->num_rx_queues; i++) {
 		struct igb_ring *ring = adapter->rx_ring[i];
 
-		if (frame_size > igb_rx_bufsz(ring))
+		if (frame_size > igb_rx_bufsz(ring)) {
+			NL_SET_ERR_MSG_MOD(bpf->extack, "The RX buffer size is too small for the frame size");
 			return -EINVAL;
+		}
 	}
 
 	old_prog = xchg(&adapter->xdp_prog, prog);
@@ -2869,7 +2871,7 @@ static int igb_xdp(struct net_device *dev, struct netdev_bpf *xdp)
 {
 	switch (xdp->command) {
 	case XDP_SETUP_PROG:
-		return igb_xdp_setup(dev, xdp->prog);
+		return igb_xdp_setup(dev, xdp);
 	default:
 		return -EINVAL;
 	}
@@ -6499,7 +6501,7 @@ static int igb_change_mtu(struct net_device *netdev, int new_mtu)
 			struct igb_ring *ring = adapter->rx_ring[i];
 
 			if (max_frame > igb_rx_bufsz(ring)) {
-				netdev_warn(adapter->netdev, "Requested MTU size is not supported with XDP\n");
+				netdev_warn(adapter->netdev, "Requested MTU size is not supported with XDP. Max frame size is %d\n", max_frame);
 				return -EINVAL;
 			}
 		}
-- 
2.20.1

[PATCH 4/7] igb: skb add metasize for xdp

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

add metasize if it is set in xdp

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 088f9ddb0093..36ff8725fdaf 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8345,6 +8345,7 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
 				     struct xdp_buff *xdp,
 				     union e1000_adv_rx_desc *rx_desc)
 {
+	unsigned int metasize = xdp->data - xdp->data_meta;
 #if (PAGE_SIZE < 8192)
 	unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
 #else
@@ -8366,6 +8367,9 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
 	skb_reserve(skb, xdp->data - xdp->data_hard_start);
 	__skb_put(skb, xdp->data_end - xdp->data);
 
+	if (metasize)
+		skb_metadata_set(skb, metasize);
+
 	/* pull timestamp out of packet data */
 	if (igb_test_staterr(rx_desc, E1000_RXDADV_STAT_TSIP)) {
 		igb_ptp_rx_pktstamp(rx_ring->q_vector, skb->data, skb);
-- 
2.20.1

[PATCH 5/7] igb: use igb_rx_buffer_flip

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

Also use the new helper function igb_rx_buffer_flip in
igb_build_skb/igb_add_rx_frag.

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 87 +++++++++--------------
 1 file changed, 35 insertions(+), 52 deletions(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 36ff8725fdaf..f34faf24190a 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8255,6 +8255,34 @@ static bool igb_can_reuse_rx_page(struct igb_rx_buffer *rx_buffer)
 	return true;
 }
 
+static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
+					  unsigned int size)
+{
+	unsigned int truesize;
+
+#if (PAGE_SIZE < 8192)
+	truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
+#else
+	truesize = ring_uses_build_skb(rx_ring) ?
+		SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
+		SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
+		SKB_DATA_ALIGN(size);
+#endif
+	return truesize;
+}
+
+static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
+			       struct igb_rx_buffer *rx_buffer,
+			       unsigned int size)
+{
+	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
+#if (PAGE_SIZE < 8192)
+	rx_buffer->page_offset ^= truesize;
+#else
+	rx_buffer->page_offset += truesize;
+#endif
+}
+
 /**
  *  igb_add_rx_frag - Add contents of Rx buffer to sk_buff
  *  @rx_ring: rx descriptor ring to transact packets on
@@ -8269,20 +8297,12 @@ static void igb_add_rx_frag(struct igb_ring *rx_ring,
 			    struct sk_buff *skb,
 			    unsigned int size)
 {
-#if (PAGE_SIZE < 8192)
-	unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
-#else
-	unsigned int truesize = ring_uses_build_skb(rx_ring) ?
-				SKB_DATA_ALIGN(IGB_SKB_PAD + size) :
-				SKB_DATA_ALIGN(size);
-#endif
+	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
+
 	skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buffer->page,
 			rx_buffer->page_offset, size, truesize);
-#if (PAGE_SIZE < 8192)
-	rx_buffer->page_offset ^= truesize;
-#else
-	rx_buffer->page_offset += truesize;
-#endif
+
+	igb_rx_buffer_flip(rx_ring, rx_buffer, size);
 }
 
 static struct sk_buff *igb_construct_skb(struct igb_ring *rx_ring,
@@ -8345,14 +8365,9 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
 				     struct xdp_buff *xdp,
 				     union e1000_adv_rx_desc *rx_desc)
 {
+	unsigned int size = xdp->data_end - xdp->data_hard_start;
+	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
 	unsigned int metasize = xdp->data - xdp->data_meta;
-#if (PAGE_SIZE < 8192)
-	unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
-#else
-	unsigned int truesize = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
-				SKB_DATA_ALIGN(xdp->data_end -
-					       xdp->data_hard_start);
-#endif
 	struct sk_buff *skb;
 
 	/* prefetch first cache line of first page */
@@ -8377,11 +8392,7 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
 	}
 
 	/* update buffer offset */
-#if (PAGE_SIZE < 8192)
-	rx_buffer->page_offset ^= truesize;
-#else
-	rx_buffer->page_offset += truesize;
-#endif
+	igb_rx_buffer_flip(rx_ring, rx_buffer, size);
 
 	return skb;
 }
@@ -8431,34 +8442,6 @@ static struct sk_buff *igb_run_xdp(struct igb_adapter *adapter,
 	return ERR_PTR(-result);
 }
 
-static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
-					  unsigned int size)
-{
-	unsigned int truesize;
-
-#if (PAGE_SIZE < 8192)
-	truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
-#else
-	truesize = ring_uses_build_skb(rx_ring) ?
-		SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
-		SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
-		SKB_DATA_ALIGN(size);
-#endif
-	return truesize;
-}
-
-static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
-			       struct igb_rx_buffer *rx_buffer,
-			       unsigned int size)
-{
-	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
-#if (PAGE_SIZE < 8192)
-	rx_buffer->page_offset ^= truesize;
-#else
-	rx_buffer->page_offset += truesize;
-#endif
-}
-
 static inline void igb_rx_checksum(struct igb_ring *ring,
 				   union e1000_adv_rx_desc *rx_desc,
 				   struct sk_buff *skb)
-- 
2.20.1

[PATCH 6/7] igb: use xdp_do_flush

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

Since it is a new XDP implementation change xdp_do_flush_map
to xdp_do_flush.

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index f34faf24190a..6a2828b96eef 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8759,7 +8759,7 @@ static int igb_clean_rx_irq(struct igb_q_vector *q_vector, const int budget)
 	rx_ring->skb = skb;
 
 	if (xdp_xmit & IGB_XDP_REDIR)
-		xdp_do_flush_map();
+		xdp_do_flush();
 
 	if (xdp_xmit & IGB_XDP_TX) {
 		struct igb_ring *tx_ring = igb_xdp_tx_queue_mapping(adapter);
-- 
2.20.1

[PATCH 7/7] igb: avoid transmit queue timeout in xdp path

[sven.auhagen]

From: Sven Auhagen <sven.auhagen@voleatech.de>

Since we share the transmit queue with the slow path,
it is possible that we run into a transmit queue timeout.
This will reset the queue.
This happens under high load when the fast path is using the
transmit queue pretty much exclusively.

Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
---
 drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
index 6a2828b96eef..d84a99359e95 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2916,6 +2916,8 @@ static int igb_xdp_xmit_back(struct igb_adapter *adapter, struct xdp_buff *xdp)
 
 	nq = txring_txq(tx_ring);
 	__netif_tx_lock(nq, cpu);
+	/* Avoid transmit queue timeout since we share it with the slow path */
+	nq->trans_start = jiffies;
 	ret = igb_xmit_xdp_ring(adapter, tx_ring, xdpf);
 	__netif_tx_unlock(nq);
 
@@ -2948,6 +2950,9 @@ static int igb_xdp_xmit(struct net_device *dev, int n,
 	nq = txring_txq(tx_ring);
 	__netif_tx_lock(nq, cpu);
 
+	/* Avoid transmit queue timeout since we share it with the slow path */
+	nq->trans_start = jiffies;
+
 	for (i = 0; i < n; i++) {
 		struct xdp_frame *xdpf = frames[i];
 		int err;
-- 
2.20.1

Re: [PATCH 2/7] igb: take vlan double header into account

[Maciej Fijalkowski]

On Wed, Oct 07, 2020 at 05:25:01PM +0200, sven.auhagen@voleatech.de wrote:
> From: Sven Auhagen <sven.auhagen@voleatech.de>
> 
> Increase the packet header padding to include double VLAN tagging.
> This patch uses a macro for this.
> 
> Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
> ---
>  drivers/net/ethernet/intel/igb/igb.h      | 5 +++++
>  drivers/net/ethernet/intel/igb/igb_main.c | 7 +++----
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> index 0286d2fceee4..7afb67cf9b94 100644
> --- a/drivers/net/ethernet/intel/igb/igb.h
> +++ b/drivers/net/ethernet/intel/igb/igb.h
> @@ -138,6 +138,8 @@ struct vf_mac_filter {
>  /* this is the size past which hardware will drop packets when setting LPE=0 */
>  #define MAXIMUM_ETHERNET_VLAN_SIZE 1522
>  
> +#define IGB_ETH_PKT_HDR_PAD	(ETH_HLEN + ETH_FCS_LEN + (VLAN_HLEN * 2))
> +
>  /* Supported Rx Buffer Sizes */
>  #define IGB_RXBUFFER_256	256
>  #define IGB_RXBUFFER_1536	1536
> @@ -247,6 +249,9 @@ enum igb_tx_flags {
>  #define IGB_SFF_ADDRESSING_MODE		0x4
>  #define IGB_SFF_8472_UNSUP		0x00
>  
> +/* TX ressources are shared between XDP and netstack
> + * and we need to tag the buffer type to distinguish them
> + */

s/ressources/resources/

This comment sort of does not belong to this commit but I'm not sure what
place would be better.

>  enum igb_tx_buf_type {
>  	IGB_TYPE_SKB = 0,
>  	IGB_TYPE_XDP,
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 08cc6f59aa2e..0a9198037b98 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2826,7 +2826,7 @@ static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
>  
>  static int igb_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
>  {
> -	int i, frame_size = dev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> +	int i, frame_size = dev->mtu + IGB_ETH_PKT_HDR_PAD;
>  	struct igb_adapter *adapter = netdev_priv(dev);
>  	bool running = netif_running(dev);
>  	struct bpf_prog *old_prog;
> @@ -3950,8 +3950,7 @@ static int igb_sw_init(struct igb_adapter *adapter)
>  	/* set default work limits */
>  	adapter->tx_work_limit = IGB_DEFAULT_TX_WORK;
>  
> -	adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN +
> -				  VLAN_HLEN;
> +	adapter->max_frame_size = netdev->mtu + IGB_ETH_PKT_HDR_PAD;
>  	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
>  
>  	spin_lock_init(&adapter->nfc_lock);
> @@ -6491,7 +6490,7 @@ static void igb_get_stats64(struct net_device *netdev,
>  static int igb_change_mtu(struct net_device *netdev, int new_mtu)
>  {
>  	struct igb_adapter *adapter = netdev_priv(netdev);
> -	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> +	int max_frame = new_mtu + IGB_ETH_PKT_HDR_PAD;
>  
>  	if (adapter->xdp_prog) {
>  		int i;
> -- 
> 2.20.1
> 

Re: [PATCH 3/7] igb: XDP extack message on error

[Maciej Fijalkowski]

On Wed, Oct 07, 2020 at 05:25:02PM +0200, sven.auhagen@voleatech.de wrote:
> From: Sven Auhagen <sven.auhagen@voleatech.de>
> 
> Add an extack error message when the RX buffer size is too small
> for the frame size.
> 
> Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>

Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Re: [PATCH 4/7] igb: skb add metasize for xdp

[Maciej Fijalkowski]

On Wed, Oct 07, 2020 at 05:25:03PM +0200, sven.auhagen@voleatech.de wrote:
> From: Sven Auhagen <sven.auhagen@voleatech.de>
> 
> add metasize if it is set in xdp
> 
> Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>

Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Re: [PATCH 5/7] igb: use igb_rx_buffer_flip

[Maciej Fijalkowski]

On Wed, Oct 07, 2020 at 05:25:04PM +0200, sven.auhagen@voleatech.de wrote:
> From: Sven Auhagen <sven.auhagen@voleatech.de>
> 
> Also use the new helper function igb_rx_buffer_flip in
> igb_build_skb/igb_add_rx_frag.
> 
> Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 87 +++++++++--------------
>  1 file changed, 35 insertions(+), 52 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 36ff8725fdaf..f34faf24190a 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -8255,6 +8255,34 @@ static bool igb_can_reuse_rx_page(struct igb_rx_buffer *rx_buffer)
>  	return true;
>  }
>  
> +static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
> +					  unsigned int size)
> +{
> +	unsigned int truesize;
> +
> +#if (PAGE_SIZE < 8192)
> +	truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
> +#else
> +	truesize = ring_uses_build_skb(rx_ring) ?
> +		SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
> +		SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
> +		SKB_DATA_ALIGN(size);
> +#endif
> +	return truesize;
> +}
> +
> +static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
> +			       struct igb_rx_buffer *rx_buffer,
> +			       unsigned int size)
> +{
> +	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> +#if (PAGE_SIZE < 8192)
> +	rx_buffer->page_offset ^= truesize;
> +#else
> +	rx_buffer->page_offset += truesize;
> +#endif
> +}
> +
>  /**
>   *  igb_add_rx_frag - Add contents of Rx buffer to sk_buff
>   *  @rx_ring: rx descriptor ring to transact packets on
> @@ -8269,20 +8297,12 @@ static void igb_add_rx_frag(struct igb_ring *rx_ring,
>  			    struct sk_buff *skb,
>  			    unsigned int size)
>  {
> -#if (PAGE_SIZE < 8192)
> -	unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
> -#else
> -	unsigned int truesize = ring_uses_build_skb(rx_ring) ?
> -				SKB_DATA_ALIGN(IGB_SKB_PAD + size) :
> -				SKB_DATA_ALIGN(size);
> -#endif
> +	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);

I don't think we need to account the size of skb_shared_info when adding
another frag as we already have the skb in place with its skb_shared_info.

Also, please make sure that you list all of the changes that patch
contains in the commit message, you simply skipped the fact that you're
making use of igb_rx_frame_truesize on other places.

> +
>  	skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buffer->page,
>  			rx_buffer->page_offset, size, truesize);
> -#if (PAGE_SIZE < 8192)
> -	rx_buffer->page_offset ^= truesize;
> -#else
> -	rx_buffer->page_offset += truesize;
> -#endif
> +
> +	igb_rx_buffer_flip(rx_ring, rx_buffer, size);
>  }
>  
>  static struct sk_buff *igb_construct_skb(struct igb_ring *rx_ring,
> @@ -8345,14 +8365,9 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
>  				     struct xdp_buff *xdp,
>  				     union e1000_adv_rx_desc *rx_desc)
>  {
> +	unsigned int size = xdp->data_end - xdp->data_hard_start;
> +	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);

Here you will be counting the IGB_SKB_PAD twice for pages > 4k.
xdp->data_end - xdp->data_hard_start already includes the IGB_SKB_PAD and
then igb_rx_frame_truesize will add this IGB_SKB_PAD once again to the
size you're providing.

Please drop the additional usage of igb_rx_frame_truesize in this patch.

>  	unsigned int metasize = xdp->data - xdp->data_meta;
> -#if (PAGE_SIZE < 8192)
> -	unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
> -#else
> -	unsigned int truesize = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
> -				SKB_DATA_ALIGN(xdp->data_end -
> -					       xdp->data_hard_start);
> -#endif
>  	struct sk_buff *skb;
>  
>  	/* prefetch first cache line of first page */
> @@ -8377,11 +8392,7 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
>  	}
>  
>  	/* update buffer offset */
> -#if (PAGE_SIZE < 8192)
> -	rx_buffer->page_offset ^= truesize;
> -#else
> -	rx_buffer->page_offset += truesize;
> -#endif
> +	igb_rx_buffer_flip(rx_ring, rx_buffer, size);
>  
>  	return skb;
>  }
> @@ -8431,34 +8442,6 @@ static struct sk_buff *igb_run_xdp(struct igb_adapter *adapter,
>  	return ERR_PTR(-result);
>  }
>  
> -static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
> -					  unsigned int size)
> -{
> -	unsigned int truesize;
> -
> -#if (PAGE_SIZE < 8192)
> -	truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
> -#else
> -	truesize = ring_uses_build_skb(rx_ring) ?
> -		SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
> -		SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
> -		SKB_DATA_ALIGN(size);
> -#endif
> -	return truesize;
> -}
> -
> -static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
> -			       struct igb_rx_buffer *rx_buffer,
> -			       unsigned int size)
> -{
> -	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> -#if (PAGE_SIZE < 8192)
> -	rx_buffer->page_offset ^= truesize;
> -#else
> -	rx_buffer->page_offset += truesize;
> -#endif
> -}
> -
>  static inline void igb_rx_checksum(struct igb_ring *ring,
>  				   union e1000_adv_rx_desc *rx_desc,
>  				   struct sk_buff *skb)
> -- 
> 2.20.1
> 

Re: [PATCH 6/7] igb: use xdp_do_flush

[Maciej Fijalkowski]

On Wed, Oct 07, 2020 at 05:25:05PM +0200, sven.auhagen@voleatech.de wrote:
> From: Sven Auhagen <sven.auhagen@voleatech.de>
> 
> Since it is a new XDP implementation change xdp_do_flush_map
> to xdp_do_flush.
> 
> Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>

Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Re: [PATCH 7/7] igb: avoid transmit queue timeout in xdp path

[Maciej Fijalkowski]

On Wed, Oct 07, 2020 at 05:25:06PM +0200, sven.auhagen@voleatech.de wrote:
> From: Sven Auhagen <sven.auhagen@voleatech.de>
> 
> Since we share the transmit queue with the slow path,
> it is possible that we run into a transmit queue timeout.
> This will reset the queue.
> This happens under high load when the fast path is using the
> transmit queue pretty much exclusively.

Please mention in the commit message *how* you are fixing this issue.

> 
> Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
> ---
>  drivers/net/ethernet/intel/igb/igb_main.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> index 6a2828b96eef..d84a99359e95 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -2916,6 +2916,8 @@ static int igb_xdp_xmit_back(struct igb_adapter *adapter, struct xdp_buff *xdp)
>  
>  	nq = txring_txq(tx_ring);
>  	__netif_tx_lock(nq, cpu);
> +	/* Avoid transmit queue timeout since we share it with the slow path */
> +	nq->trans_start = jiffies;
>  	ret = igb_xmit_xdp_ring(adapter, tx_ring, xdpf);
>  	__netif_tx_unlock(nq);
>  
> @@ -2948,6 +2950,9 @@ static int igb_xdp_xmit(struct net_device *dev, int n,
>  	nq = txring_txq(tx_ring);
>  	__netif_tx_lock(nq, cpu);
>  
> +	/* Avoid transmit queue timeout since we share it with the slow path */
> +	nq->trans_start = jiffies;
> +
>  	for (i = 0; i < n; i++) {
>  		struct xdp_frame *xdpf = frames[i];
>  		int err;
> -- 
> 2.20.1
> 

Re: [PATCH 2/7] igb: take vlan double header into account

[Sven Auhagen]

On Wed, Oct 07, 2020 at 11:06:15PM +0200, Maciej Fijalkowski wrote:
> On Wed, Oct 07, 2020 at 05:25:01PM +0200, sven.auhagen@voleatech.de wrote:
> > From: Sven Auhagen <sven.auhagen@voleatech.de>
> > 
> > Increase the packet header padding to include double VLAN tagging.
> > This patch uses a macro for this.
> > 
> > Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
> > ---
> >  drivers/net/ethernet/intel/igb/igb.h      | 5 +++++
> >  drivers/net/ethernet/intel/igb/igb_main.c | 7 +++----
> >  2 files changed, 8 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/igb/igb.h b/drivers/net/ethernet/intel/igb/igb.h
> > index 0286d2fceee4..7afb67cf9b94 100644
> > --- a/drivers/net/ethernet/intel/igb/igb.h
> > +++ b/drivers/net/ethernet/intel/igb/igb.h
> > @@ -138,6 +138,8 @@ struct vf_mac_filter {
> >  /* this is the size past which hardware will drop packets when setting LPE=0 */
> >  #define MAXIMUM_ETHERNET_VLAN_SIZE 1522
> >  
> > +#define IGB_ETH_PKT_HDR_PAD	(ETH_HLEN + ETH_FCS_LEN + (VLAN_HLEN * 2))
> > +
> >  /* Supported Rx Buffer Sizes */
> >  #define IGB_RXBUFFER_256	256
> >  #define IGB_RXBUFFER_1536	1536
> > @@ -247,6 +249,9 @@ enum igb_tx_flags {
> >  #define IGB_SFF_ADDRESSING_MODE		0x4
> >  #define IGB_SFF_8472_UNSUP		0x00
> >  
> > +/* TX ressources are shared between XDP and netstack
> > + * and we need to tag the buffer type to distinguish them
> > + */
> 
> s/ressources/resources/
> 
> This comment sort of does not belong to this commit but I'm not sure what
> place would be better.

I had the same problem.
I don't think it is enough for an extra patch so I just added it here.
I fix the spelling.

> 
> >  enum igb_tx_buf_type {
> >  	IGB_TYPE_SKB = 0,
> >  	IGB_TYPE_XDP,
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index 08cc6f59aa2e..0a9198037b98 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -2826,7 +2826,7 @@ static int igb_setup_tc(struct net_device *dev, enum tc_setup_type type,
> >  
> >  static int igb_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
> >  {
> > -	int i, frame_size = dev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> > +	int i, frame_size = dev->mtu + IGB_ETH_PKT_HDR_PAD;
> >  	struct igb_adapter *adapter = netdev_priv(dev);
> >  	bool running = netif_running(dev);
> >  	struct bpf_prog *old_prog;
> > @@ -3950,8 +3950,7 @@ static int igb_sw_init(struct igb_adapter *adapter)
> >  	/* set default work limits */
> >  	adapter->tx_work_limit = IGB_DEFAULT_TX_WORK;
> >  
> > -	adapter->max_frame_size = netdev->mtu + ETH_HLEN + ETH_FCS_LEN +
> > -				  VLAN_HLEN;
> > +	adapter->max_frame_size = netdev->mtu + IGB_ETH_PKT_HDR_PAD;
> >  	adapter->min_frame_size = ETH_ZLEN + ETH_FCS_LEN;
> >  
> >  	spin_lock_init(&adapter->nfc_lock);
> > @@ -6491,7 +6490,7 @@ static void igb_get_stats64(struct net_device *netdev,
> >  static int igb_change_mtu(struct net_device *netdev, int new_mtu)
> >  {
> >  	struct igb_adapter *adapter = netdev_priv(netdev);
> > -	int max_frame = new_mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> > +	int max_frame = new_mtu + IGB_ETH_PKT_HDR_PAD;
> >  
> >  	if (adapter->xdp_prog) {
> >  		int i;
> > -- 
> > 2.20.1
> > 

Re: [PATCH 5/7] igb: use igb_rx_buffer_flip

[Sven Auhagen]

On Wed, Oct 07, 2020 at 11:32:57PM +0200, Maciej Fijalkowski wrote:
> On Wed, Oct 07, 2020 at 05:25:04PM +0200, sven.auhagen@voleatech.de wrote:
> > From: Sven Auhagen <sven.auhagen@voleatech.de>
> > 
> > Also use the new helper function igb_rx_buffer_flip in
> > igb_build_skb/igb_add_rx_frag.
> > 
> > Signed-off-by: Sven Auhagen <sven.auhagen@voleatech.de>
> > ---
> >  drivers/net/ethernet/intel/igb/igb_main.c | 87 +++++++++--------------
> >  1 file changed, 35 insertions(+), 52 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index 36ff8725fdaf..f34faf24190a 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -8255,6 +8255,34 @@ static bool igb_can_reuse_rx_page(struct igb_rx_buffer *rx_buffer)
> >  	return true;
> >  }
> >  
> > +static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
> > +					  unsigned int size)
> > +{
> > +	unsigned int truesize;
> > +
> > +#if (PAGE_SIZE < 8192)
> > +	truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
> > +#else
> > +	truesize = ring_uses_build_skb(rx_ring) ?
> > +		SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
> > +		SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
> > +		SKB_DATA_ALIGN(size);
> > +#endif
> > +	return truesize;
> > +}
> > +
> > +static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
> > +			       struct igb_rx_buffer *rx_buffer,
> > +			       unsigned int size)
> > +{
> > +	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> > +#if (PAGE_SIZE < 8192)
> > +	rx_buffer->page_offset ^= truesize;
> > +#else
> > +	rx_buffer->page_offset += truesize;
> > +#endif
> > +}
> > +
> >  /**
> >   *  igb_add_rx_frag - Add contents of Rx buffer to sk_buff
> >   *  @rx_ring: rx descriptor ring to transact packets on
> > @@ -8269,20 +8297,12 @@ static void igb_add_rx_frag(struct igb_ring *rx_ring,
> >  			    struct sk_buff *skb,
> >  			    unsigned int size)
> >  {
> > -#if (PAGE_SIZE < 8192)
> > -	unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
> > -#else
> > -	unsigned int truesize = ring_uses_build_skb(rx_ring) ?
> > -				SKB_DATA_ALIGN(IGB_SKB_PAD + size) :
> > -				SKB_DATA_ALIGN(size);
> > -#endif
> > +	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> 
> I don't think we need to account the size of skb_shared_info when adding
> another frag as we already have the skb in place with its skb_shared_info.
> 
> Also, please make sure that you list all of the changes that patch
> contains in the commit message, you simply skipped the fact that you're
> making use of igb_rx_frame_truesize on other places.

Do you have a suggestion on how to add the truesize and flip function
at this point? The truesize is not suited here then and returns 
the wrong size for frags.

> 
> > +
> >  	skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buffer->page,
> >  			rx_buffer->page_offset, size, truesize);
> > -#if (PAGE_SIZE < 8192)
> > -	rx_buffer->page_offset ^= truesize;
> > -#else
> > -	rx_buffer->page_offset += truesize;
> > -#endif
> > +
> > +	igb_rx_buffer_flip(rx_ring, rx_buffer, size);
> >  }
> >  
> >  static struct sk_buff *igb_construct_skb(struct igb_ring *rx_ring,
> > @@ -8345,14 +8365,9 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
> >  				     struct xdp_buff *xdp,
> >  				     union e1000_adv_rx_desc *rx_desc)
> >  {
> > +	unsigned int size = xdp->data_end - xdp->data_hard_start;
> > +	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> 
> Here you will be counting the IGB_SKB_PAD twice for pages > 4k.
> xdp->data_end - xdp->data_hard_start already includes the IGB_SKB_PAD and
> then igb_rx_frame_truesize will add this IGB_SKB_PAD once again to the
> size you're providing.
> 
> Please drop the additional usage of igb_rx_frame_truesize in this patch.
> 

The igb_rx_buffer_flip will use the wrong truesize though even when I
remove igb_rx_frame_truesize here.
Should I drop the entire patch as the truesize for the flip will be
incorrect in both places?

> >  	unsigned int metasize = xdp->data - xdp->data_meta;
> > -#if (PAGE_SIZE < 8192)
> > -	unsigned int truesize = igb_rx_pg_size(rx_ring) / 2;
> > -#else
> > -	unsigned int truesize = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
> > -				SKB_DATA_ALIGN(xdp->data_end -
> > -					       xdp->data_hard_start);
> > -#endif
> >  	struct sk_buff *skb;
> >  
> >  	/* prefetch first cache line of first page */
> > @@ -8377,11 +8392,7 @@ static struct sk_buff *igb_build_skb(struct igb_ring *rx_ring,
> >  	}
> >  
> >  	/* update buffer offset */
> > -#if (PAGE_SIZE < 8192)
> > -	rx_buffer->page_offset ^= truesize;
> > -#else
> > -	rx_buffer->page_offset += truesize;
> > -#endif
> > +	igb_rx_buffer_flip(rx_ring, rx_buffer, size);
> >  
> >  	return skb;
> >  }
> > @@ -8431,34 +8442,6 @@ static struct sk_buff *igb_run_xdp(struct igb_adapter *adapter,
> >  	return ERR_PTR(-result);
> >  }
> >  
> > -static unsigned int igb_rx_frame_truesize(struct igb_ring *rx_ring,
> > -					  unsigned int size)
> > -{
> > -	unsigned int truesize;
> > -
> > -#if (PAGE_SIZE < 8192)
> > -	truesize = igb_rx_pg_size(rx_ring) / 2; /* Must be power-of-2 */
> > -#else
> > -	truesize = ring_uses_build_skb(rx_ring) ?
> > -		SKB_DATA_ALIGN(IGB_SKB_PAD + size) +
> > -		SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) :
> > -		SKB_DATA_ALIGN(size);
> > -#endif
> > -	return truesize;
> > -}
> > -
> > -static void igb_rx_buffer_flip(struct igb_ring *rx_ring,
> > -			       struct igb_rx_buffer *rx_buffer,
> > -			       unsigned int size)
> > -{
> > -	unsigned int truesize = igb_rx_frame_truesize(rx_ring, size);
> > -#if (PAGE_SIZE < 8192)
> > -	rx_buffer->page_offset ^= truesize;
> > -#else
> > -	rx_buffer->page_offset += truesize;
> > -#endif
> > -}
> > -
> >  static inline void igb_rx_checksum(struct igb_ring *ring,
> >  				   union e1000_adv_rx_desc *rx_desc,
> >  				   struct sk_buff *skb)
> > -- 
> > 2.20.1
> >