* [PATCH 0/7] igb: xdp patches followup
@ 2020-10-07 15:24 sven.auhagen
2020-10-07 15:25 ` [PATCH 1/7] igb: XDP xmit back fix error code sven.auhagen
` (6 more replies)
0 siblings, 7 replies; 16+ messages in thread
From: sven.auhagen @ 2020-10-07 15:24 UTC (permalink / raw)
To: anthony.l.nguyen, maciej.fijalkowski
Cc: davem, netdev, nhorman, sassmann, sandeep.penigalapati, brouer
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/7] igb: XDP xmit back fix error code
2020-10-07 15:24 [PATCH 0/7] igb: xdp patches followup sven.auhagen
@ 2020-10-07 15:25 ` sven.auhagen
2020-10-07 15:25 ` [PATCH 2/7] igb: take vlan double header into account sven.auhagen
` (5 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: sven.auhagen @ 2020-10-07 15:25 UTC (permalink / raw)
To: anthony.l.nguyen, maciej.fijalkowski
Cc: davem, netdev, nhorman, sassmann, sandeep.penigalapati, brouer
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/7] igb: take vlan double header into account
2020-10-07 15:24 [PATCH 0/7] igb: xdp patches followup sven.auhagen
2020-10-07 15:25 ` [PATCH 1/7] igb: XDP xmit back fix error code sven.auhagen
@ 2020-10-07 15:25 ` sven.auhagen
2020-10-07 21:06 ` Maciej Fijalkowski
2020-10-07 15:25 ` [PATCH 3/7] igb: XDP extack message on error sven.auhagen
` (4 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: sven.auhagen @ 2020-10-07 15:25 UTC (permalink / raw)
To: anthony.l.nguyen, maciej.fijalkowski
Cc: davem, netdev, nhorman, sassmann, sandeep.penigalapati, brouer
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 3/7] igb: XDP extack message on error
2020-10-07 15:24 [PATCH 0/7] igb: xdp patches followup sven.auhagen
2020-10-07 15:25 ` [PATCH 1/7] igb: XDP xmit back fix error code sven.auhagen
2020-10-07 15:25 ` [PATCH 2/7] igb: take vlan double header into account sven.auhagen
@ 2020-10-07 15:25 ` sven.auhagen
2020-10-07 21:08 ` Maciej Fijalkowski
2020-10-07 15:25 ` [PATCH 4/7] igb: skb add metasize for xdp sven.auhagen
` (3 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: sven.auhagen @ 2020-10-07 15:25 UTC (permalink / raw)
To: anthony.l.nguyen, maciej.fijalkowski
Cc: davem, netdev, nhorman, sassmann, sandeep.penigalapati, brouer
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 4/7] igb: skb add metasize for xdp
2020-10-07 15:24 [PATCH 0/7] igb: xdp patches followup sven.auhagen
` (2 preceding siblings ...)
2020-10-07 15:25 ` [PATCH 3/7] igb: XDP extack message on error sven.auhagen
@ 2020-10-07 15:25 ` sven.auhagen
2020-10-07 21:10 ` Maciej Fijalkowski
2020-10-07 15:25 ` [PATCH 5/7] igb: use igb_rx_buffer_flip sven.auhagen
` (2 subsequent siblings)
6 siblings, 1 reply; 16+ messages in thread
From: sven.auhagen @ 2020-10-07 15:25 UTC (permalink / raw)
To: anthony.l.nguyen, maciej.fijalkowski
Cc: davem, netdev, nhorman, sassmann, sandeep.penigalapati, brouer
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 5/7] igb: use igb_rx_buffer_flip
2020-10-07 15:24 [PATCH 0/7] igb: xdp patches followup sven.auhagen
` (3 preceding siblings ...)
2020-10-07 15:25 ` [PATCH 4/7] igb: skb add metasize for xdp sven.auhagen
@ 2020-10-07 15:25 ` sven.auhagen
2020-10-07 21:32 ` Maciej Fijalkowski
2020-10-07 15:25 ` [PATCH 6/7] igb: use xdp_do_flush sven.auhagen
2020-10-07 15:25 ` [PATCH 7/7] igb: avoid transmit queue timeout in xdp path sven.auhagen
6 siblings, 1 reply; 16+ messages in thread
From: sven.auhagen @ 2020-10-07 15:25 UTC (permalink / raw)
To: anthony.l.nguyen, maciej.fijalkowski
Cc: davem, netdev, nhorman, sassmann, sandeep.penigalapati, brouer
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 6/7] igb: use xdp_do_flush
2020-10-07 15:24 [PATCH 0/7] igb: xdp patches followup sven.auhagen
` (4 preceding siblings ...)
2020-10-07 15:25 ` [PATCH 5/7] igb: use igb_rx_buffer_flip sven.auhagen
@ 2020-10-07 15:25 ` sven.auhagen
2020-10-07 21:34 ` Maciej Fijalkowski
2020-10-07 15:25 ` [PATCH 7/7] igb: avoid transmit queue timeout in xdp path sven.auhagen
6 siblings, 1 reply; 16+ messages in thread
From: sven.auhagen @ 2020-10-07 15:25 UTC (permalink / raw)
To: anthony.l.nguyen, maciej.fijalkowski
Cc: davem, netdev, nhorman, sassmann, sandeep.penigalapati, brouer
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 7/7] igb: avoid transmit queue timeout in xdp path
2020-10-07 15:24 [PATCH 0/7] igb: xdp patches followup sven.auhagen
` (5 preceding siblings ...)
2020-10-07 15:25 ` [PATCH 6/7] igb: use xdp_do_flush sven.auhagen
@ 2020-10-07 15:25 ` sven.auhagen
2020-10-07 21:35 ` Maciej Fijalkowski
6 siblings, 1 reply; 16+ messages in thread
From: sven.auhagen @ 2020-10-07 15:25 UTC (permalink / raw)
To: anthony.l.nguyen, maciej.fijalkowski
Cc: davem, netdev, nhorman, sassmann, sandeep.penigalapati, brouer
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/7] igb: take vlan double header into account
2020-10-07 15:25 ` [PATCH 2/7] igb: take vlan double header into account sven.auhagen
@ 2020-10-07 21:06 ` Maciej Fijalkowski
2020-10-08 5:25 ` Sven Auhagen
0 siblings, 1 reply; 16+ messages in thread
From: Maciej Fijalkowski @ 2020-10-07 21:06 UTC (permalink / raw)
To: sven.auhagen
Cc: anthony.l.nguyen, davem, netdev, nhorman, sassmann,
sandeep.penigalapati, brouer
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 3/7] igb: XDP extack message on error
2020-10-07 15:25 ` [PATCH 3/7] igb: XDP extack message on error sven.auhagen
@ 2020-10-07 21:08 ` Maciej Fijalkowski
0 siblings, 0 replies; 16+ messages in thread
From: Maciej Fijalkowski @ 2020-10-07 21:08 UTC (permalink / raw)
To: sven.auhagen
Cc: anthony.l.nguyen, davem, netdev, nhorman, sassmann,
sandeep.penigalapati, brouer
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 4/7] igb: skb add metasize for xdp
2020-10-07 15:25 ` [PATCH 4/7] igb: skb add metasize for xdp sven.auhagen
@ 2020-10-07 21:10 ` Maciej Fijalkowski
0 siblings, 0 replies; 16+ messages in thread
From: Maciej Fijalkowski @ 2020-10-07 21:10 UTC (permalink / raw)
To: sven.auhagen
Cc: anthony.l.nguyen, davem, netdev, nhorman, sassmann,
sandeep.penigalapati, brouer
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/7] igb: use igb_rx_buffer_flip
2020-10-07 15:25 ` [PATCH 5/7] igb: use igb_rx_buffer_flip sven.auhagen
@ 2020-10-07 21:32 ` Maciej Fijalkowski
2020-10-08 5:29 ` Sven Auhagen
0 siblings, 1 reply; 16+ messages in thread
From: Maciej Fijalkowski @ 2020-10-07 21:32 UTC (permalink / raw)
To: sven.auhagen
Cc: anthony.l.nguyen, davem, netdev, nhorman, sassmann,
sandeep.penigalapati, brouer
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 6/7] igb: use xdp_do_flush
2020-10-07 15:25 ` [PATCH 6/7] igb: use xdp_do_flush sven.auhagen
@ 2020-10-07 21:34 ` Maciej Fijalkowski
0 siblings, 0 replies; 16+ messages in thread
From: Maciej Fijalkowski @ 2020-10-07 21:34 UTC (permalink / raw)
To: sven.auhagen
Cc: anthony.l.nguyen, davem, netdev, nhorman, sassmann,
sandeep.penigalapati, brouer
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>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 7/7] igb: avoid transmit queue timeout in xdp path
2020-10-07 15:25 ` [PATCH 7/7] igb: avoid transmit queue timeout in xdp path sven.auhagen
@ 2020-10-07 21:35 ` Maciej Fijalkowski
0 siblings, 0 replies; 16+ messages in thread
From: Maciej Fijalkowski @ 2020-10-07 21:35 UTC (permalink / raw)
To: sven.auhagen
Cc: anthony.l.nguyen, davem, netdev, nhorman, sassmann,
sandeep.penigalapati, brouer
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
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/7] igb: take vlan double header into account
2020-10-07 21:06 ` Maciej Fijalkowski
@ 2020-10-08 5:25 ` Sven Auhagen
0 siblings, 0 replies; 16+ messages in thread
From: Sven Auhagen @ 2020-10-08 5:25 UTC (permalink / raw)
To: Maciej Fijalkowski
Cc: anthony.l.nguyen, davem, netdev, nhorman, sassmann,
sandeep.penigalapati, brouer
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
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 5/7] igb: use igb_rx_buffer_flip
2020-10-07 21:32 ` Maciej Fijalkowski
@ 2020-10-08 5:29 ` Sven Auhagen
0 siblings, 0 replies; 16+ messages in thread
From: Sven Auhagen @ 2020-10-08 5:29 UTC (permalink / raw)
To: Maciej Fijalkowski
Cc: anthony.l.nguyen, davem, netdev, nhorman, sassmann,
sandeep.penigalapati, brouer
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
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-10-08 5:29 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-10-07 15:24 [PATCH 0/7] igb: xdp patches followup sven.auhagen
2020-10-07 15:25 ` [PATCH 1/7] igb: XDP xmit back fix error code sven.auhagen
2020-10-07 15:25 ` [PATCH 2/7] igb: take vlan double header into account sven.auhagen
2020-10-07 21:06 ` Maciej Fijalkowski
2020-10-08 5:25 ` Sven Auhagen
2020-10-07 15:25 ` [PATCH 3/7] igb: XDP extack message on error sven.auhagen
2020-10-07 21:08 ` Maciej Fijalkowski
2020-10-07 15:25 ` [PATCH 4/7] igb: skb add metasize for xdp sven.auhagen
2020-10-07 21:10 ` Maciej Fijalkowski
2020-10-07 15:25 ` [PATCH 5/7] igb: use igb_rx_buffer_flip sven.auhagen
2020-10-07 21:32 ` Maciej Fijalkowski
2020-10-08 5:29 ` Sven Auhagen
2020-10-07 15:25 ` [PATCH 6/7] igb: use xdp_do_flush sven.auhagen
2020-10-07 21:34 ` Maciej Fijalkowski
2020-10-07 15:25 ` [PATCH 7/7] igb: avoid transmit queue timeout in xdp path sven.auhagen
2020-10-07 21:35 ` Maciej Fijalkowski
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.