[PATCH v2 0/6] igb: xdp patches followup

[PATCH v2 0/6] 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>

Change from v1:
    * Drop patch 5 as the igb_rx_frame_truesize won't match
    * Fix typo in comment
    * Add Suggested-by and Reviewed-by tags
    * Add how to avoid transmit queue timeout in xdp path
      is fixed in the commit message

Sven Auhagen (6):
  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 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 | 32 +++++++++++++++--------
 2 files changed, 26 insertions(+), 11 deletions(-)

-- 
2.20.1

[PATCH v2 1/6] 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 v2 2/6] 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>
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 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..aaa954aae574 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 resources 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 v2 3/6] 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>
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 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 v2 4/6] 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>
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 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 v2 5/6] 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>
Suggested-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Reviewed-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
---
 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 36ff8725fdaf..55e708f75187 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -8776,7 +8776,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 v2 6/6] 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.

By setting the transmit queues trans_start variable to jiffies
in the two xdp xmit functions we avoid the timeout.

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 55e708f75187..4a082c06f48d 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 v2 6/6] igb: avoid transmit queue timeout in xdp path

[Maciej Fijalkowski]

On Sat, Oct 17, 2020 at 09:12:38AM +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.

I'm kinda not leaning towards slow/fast path distinction here, IMHO it
would be better to state that transmit queues are shared between network
stack and XDP, but that's just a rant.

> 
> By setting the transmit queues trans_start variable to jiffies
> in the two xdp xmit functions we avoid the timeout.

Probably a few more words of explanation would help here, specifically I
would say that netdev_start_xmit() sets trans_start to jiffies which is
later utilized by dev_watchdog(), so to avoid timeout, let stack know that
XDP xmit happened by bumping the trans_start within XDP Tx routines.

> 
> 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 55e708f75187..4a082c06f48d 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 v2 0/6] igb: xdp patches followup

[Maciej Fijalkowski]

On Sat, Oct 17, 2020 at 09:12:32AM +0200, sven.auhagen@voleatech.de wrote:
> 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>

Sorry for not getting back at v1 discussion, I took some time off.

For the series:
Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

Couple nits:
- you don't need SOB line within cover letter, I suppose
- next time please specify the tree in the subject that you're targetting
  this set to land; is it net or net-next? net-next is currently closed so
  you probably would have to come back with this once it will be open
  again
- SOB line should be at the end of tags within commit message of patch;
  I'm saying 'should' because I'm not sure if it's hard requirement.

> 
> Change from v1:
>     * Drop patch 5 as the igb_rx_frame_truesize won't match
>     * Fix typo in comment
>     * Add Suggested-by and Reviewed-by tags
>     * Add how to avoid transmit queue timeout in xdp path
>       is fixed in the commit message
> 
> Sven Auhagen (6):
>   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 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 | 32 +++++++++++++++--------
>  2 files changed, 26 insertions(+), 11 deletions(-)
> 
> -- 
> 2.20.1
> 

Re: [PATCH v2 0/6] igb: xdp patches followup

[Jakub Kicinski]

On Sun, 18 Oct 2020 15:39:51 +0200 Maciej Fijalkowski wrote:
> - next time please specify the tree in the subject that you're targetting
>   this set to land; is it net or net-next? net-next is currently closed so
>   you probably would have to come back with this once it will be open
>   again

Most of the patches here look like fixes, so we can take them into net
but please repost them rather soon.

Re: [PATCH v2 0/6] igb: xdp patches followup

[Sven Auhagen]

On Sun, Oct 18, 2020 at 03:39:51PM +0200, Maciej Fijalkowski wrote:
> On Sat, Oct 17, 2020 at 09:12:32AM +0200, sven.auhagen@voleatech.de wrote:
> > 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>
> 
> Sorry for not getting back at v1 discussion, I took some time off.
> 
> For the series:
> Acked-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
> 
> Couple nits:
> - you don't need SOB line within cover letter, I suppose
> - next time please specify the tree in the subject that you're targetting
>   this set to land; is it net or net-next? net-next is currently closed so
>   you probably would have to come back with this once it will be open
>   again
> - SOB line should be at the end of tags within commit message of patch;
>   I'm saying 'should' because I'm not sure if it's hard requirement.

Thank you, I will fix that and send a v3.

Best
Sven

> 
> > 
> > Change from v1:
> >     * Drop patch 5 as the igb_rx_frame_truesize won't match
> >     * Fix typo in comment
> >     * Add Suggested-by and Reviewed-by tags
> >     * Add how to avoid transmit queue timeout in xdp path
> >       is fixed in the commit message
> > 
> > Sven Auhagen (6):
> >   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 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 | 32 +++++++++++++++--------
> >  2 files changed, 26 insertions(+), 11 deletions(-)
> > 
> > -- 
> > 2.20.1
> > 

Re: [PATCH v2 0/6] igb: xdp patches followup

[Sven Auhagen]

On Sun, Oct 18, 2020 at 12:03:36PM -0700, Jakub Kicinski wrote:
> On Sun, 18 Oct 2020 15:39:51 +0200 Maciej Fijalkowski wrote:
> > - next time please specify the tree in the subject that you're targetting
> >   this set to land; is it net or net-next? net-next is currently closed so
> >   you probably would have to come back with this once it will be open
> >   again
> 
> Most of the patches here look like fixes, so we can take them into net
> but please repost them rather soon.

I will repost them today.

Best
Sven