ixgbe: delay tail write to every 'n' packets
Subsystems (2)
RFC PATCH 1/1 ixgbe: delay tail write to every 'n' packets
Current XDP implementation hits the tail on every XDP_TX return code. This patch changes driver behavior to only hit the tail after packet processing is complete.
RFC for now as I test this, it looks promising on my dev box but want to do some more tests before official submission.
--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +11 -3
@@ -2282,6 +2282,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,unsigned int mss =0 ;#endif /* IXGBE_FCOE */ u16 cleaned_count =ixgbe_desc_unused (rx_ring); +bool xdp_xmit = false;while (likely (total_rx_packets < budget)) {union ixgbe_adv_rx_desc *rx_desc;
@@ -2321,10 +2322,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, }if (IS_ERR (skb)) { -if (PTR_ERR (skb) == -IXGBE_XDP_TX ) +if (PTR_ERR (skb) == -IXGBE_XDP_TX ) { + xdp_xmit = true;ixgbe_rx_buffer_flip (rx_ring, rx_buffer, size); -else + }else { rx_buffer->pagecnt_bias++; + } total_rx_packets++; total_rx_bytes += size; }else if (skb) {
@@ -2392,6 +2395,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, total_rx_packets++; } +if (xdp_xmit) { +struct ixgbe_ring *ring = adapter->xdp_ring[smp_processor_id ()]; + +writel (ring->next_to_use, ring->tail); + } +u64_stats_update_begin (&rx_ring->syncp); rx_ring->stats.packets += total_rx_packets; rx_ring->stats.bytes += total_rx_bytes;
@@ -8251,7 +8260,6 @@ static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, tx_buffer->next_to_watch = tx_desc; ring->next_to_use = i; -writel (i, ring->tail);return IXGBE_XDP_TX ; }
RFC PATCH 1/1 ixgbe: delay tail write to every 'n' packets
Quoted 40 lines · John Fastabend
Current XDP implementation hits the tail on every
XDP_TXreturn code. This patch changes driver behavior to only hit the tail after packet processing is complete.RFC for now as I test this, it looks promising on my dev box but want to do some more tests before official submission.
Signed-off-by John Fastabend <[email protected]>--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +5
-2@@ -2282,6 +2282,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,unsigned int mss =0 ;#endif /* IXGBE_FCOE */ u16 cleaned_count =ixgbe_desc_unused (rx_ring); +bool xdp_xmit = false;while (likely (total_rx_packets < budget)) {union ixgbe_adv_rx_desc *rx_desc;@@ -2321,10 +2322,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, }if (IS_ERR (skb)) { -if (PTR_ERR (skb) == -IXGBE_XDP_TX ) +if (PTR_ERR (skb) == -IXGBE_XDP_TX ) { + xdp_xmit = true;ixgbe_rx_buffer_flip (rx_ring, rx_buffer, size); -else + }else { rx_buffer->pagecnt_bias++; + } total_rx_packets++; total_rx_bytes += size; }else if (skb) {
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, · line 2403
Quoted 9 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +6
-0@@ -2392,6 +2395,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, total_rx_packets++; } +if (xdp_xmit) { +struct ixgbe_ring *ring = adapter->xdp_ring[smp_processor_id ()]; + +writel (ring->next_to_use, ring->tail); + } +
We will need a wmb here.
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, · line 8254
Quoted 8 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-1u64_stats_update_begin (&rx_ring->syncp); rx_ring->stats.packets += total_rx_packets; rx_ring->stats.bytes += total_rx_bytes;@@ -8251,7 +8260,6 @@ static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, tx_buffer->next_to_watch = tx_desc; ring->next_to_use = i; -writel (i, ring->tail);
So you might want to change the barrier setup for all this to use smp_wmb instead. We need the wmb to be paired with the writel. That should give you a slight performance boost since smp_wmb breaks down to just a barrier on x86 systems.
Quoted 4 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0return IXGBE_XDP_TX ; }
RFC PATCH 1/1 ixgbe: delay tail write to every 'n' packets
Quoted 61 lines · Alexander Duyck
Quoted 40 lines · John Fastabend
Current XDP implementation hits the tail on every
XDP_TXreturn code. This patch changes driver behavior to only hit the tail after packet processing is complete.RFC for now as I test this, it looks promising on my dev box but want to do some more tests before official submission.
Signed-off-by John Fastabend <[email protected]>--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +5
-2@@ -2282,6 +2282,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,unsigned int mss =0 ;#endif /* IXGBE_FCOE */ u16 cleaned_count =ixgbe_desc_unused (rx_ring); +bool xdp_xmit = false;while (likely (total_rx_packets < budget)) {union ixgbe_adv_rx_desc *rx_desc;@@ -2321,10 +2322,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, }if (IS_ERR (skb)) { -if (PTR_ERR (skb) == -IXGBE_XDP_TX ) +if (PTR_ERR (skb) == -IXGBE_XDP_TX ) { + xdp_xmit = true;ixgbe_rx_buffer_flip (rx_ring, rx_buffer, size); -else + }else { rx_buffer->pagecnt_bias++; + } total_rx_packets++; total_rx_bytes += size; }else if (skb) {drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, · line 2403
Quoted 9 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +6
-0@@ -2392,6 +2395,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, total_rx_packets++; } +if (xdp_xmit) { +struct ixgbe_ring *ring = adapter->xdp_ring[smp_processor_id ()]; + +writel (ring->next_to_use, ring->tail); + } +We will need a wmb here.
Quoted 7 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0u64_stats_update_begin (&rx_ring->syncp); rx_ring->stats.packets += total_rx_packets; rx_ring->stats.bytes += total_rx_bytes;@@ -8251,7 +8260,6 @@ static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, tx_buffer->next_to_watch = tx_desc; ring->next_to_use = i;
Quoted 7 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, · line 8254
Quoted 1 line
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-1-writel (i, ring->tail);So you might want to change the barrier setup for all this to use
smp_wmbinstead. We need the wmb to be paired with the writel. That should give you a slight performance boost sincesmp_wmbbreaks down to just a barrier on x86 systems.
Not sure I grok this description entirely, but I think you are just saying replace,
if (xdp_xmit) {
...
writel(...)
}with
if (xdp_xmit) {
...
smp_rmb()
writel()
}Correct? Did you have some other change in mind ... "for all this"?
Thanks,
John
Quoted 4 lines
Quoted 4 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0return IXGBE_XDP_TX ; }
RFC PATCH 1/1 ixgbe: delay tail write to every 'n' packets
Quoted 86 lines · John Fastabend
Quoted 61 lines · Alexander Duyck
Quoted 40 lines · John Fastabend
Current XDP implementation hits the tail on every
XDP_TXreturn code. This patch changes driver behavior to only hit the tail after packet processing is complete.RFC for now as I test this, it looks promising on my dev box but want to do some more tests before official submission.
Signed-off-by John Fastabend <[email protected]>--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +5
-2@@ -2282,6 +2282,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,unsigned int mss =0 ;#endif /* IXGBE_FCOE */ u16 cleaned_count =ixgbe_desc_unused (rx_ring); +bool xdp_xmit = false;while (likely (total_rx_packets < budget)) {union ixgbe_adv_rx_desc *rx_desc;@@ -2321,10 +2322,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, }if (IS_ERR (skb)) { -if (PTR_ERR (skb) == -IXGBE_XDP_TX ) +if (PTR_ERR (skb) == -IXGBE_XDP_TX ) { + xdp_xmit = true;ixgbe_rx_buffer_flip (rx_ring, rx_buffer, size); -else + }else { rx_buffer->pagecnt_bias++; + } total_rx_packets++; total_rx_bytes += size; }else if (skb) {drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, · line 2403
Quoted 9 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +6
-0@@ -2392,6 +2395,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, total_rx_packets++; } +if (xdp_xmit) { +struct ixgbe_ring *ring = adapter->xdp_ring[smp_processor_id ()]; + +writel (ring->next_to_use, ring->tail); + } +We will need a wmb here.
Quoted 7 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0u64_stats_update_begin (&rx_ring->syncp); rx_ring->stats.packets += total_rx_packets; rx_ring->stats.bytes += total_rx_bytes;@@ -8251,7 +8260,6 @@ static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, tx_buffer->next_to_watch = tx_desc; ring->next_to_use = i;Quoted 7 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, · line 8254
Quoted 1 line
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-1-writel (i, ring->tail);So you might want to change the barrier setup for all this to use
smp_wmbinstead. We need the wmb to be paired with the writel. That should give you a slight performance boost sincesmp_wmbbreaks down to just a barrier on x86 systems.Not sure I grok this description entirely, but I think you are just saying replace,
if (xdp_xmit) { ... writel(...) }with
if (xdp_xmit) { ... smp_rmb() writel() }
Quoted 4 lines
Correct? Did you have some other change in mind ... "for all this"?
Thanks,
John
No. Basically what it should be is find and replace wmb with smp_wmb() in ixgbe_xmit_xdp_ring. That way you won't have to worry about a race between Tx and clean-up.
Then the if statement should be:
if (xdp_xmit) {
...
/* wmb required to flush writes to coherent memory before writing
to non-coherent memory*/
wmb();
/* writel is a write to non-coherent memory mapped I/O */
writel();
}Quoted 5 lines
Quoted 4 lines
Quoted 4 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0return IXGBE_XDP_TX ; }
Fastabend
RFC PATCH 1/1 ixgbe: delay tail write to every 'n' packets
FWIW with a revised version of this patch I see 14.6Mpps on RX drop tests and 13.5Mpps on TX test case.
Alex, maybe we can sync up and squeeze the last mpps out of the design.
Thanks,
John
________________________________________
From: Intel-wired-lan [intel-wired-lan-bounces at lists.osuosl.org] on behalf of Alexander Duyck [alexander.duyck at gmail.com]
Sent: Tuesday, March 14, 2017 6:14 PM
To: John Fastabend
Cc: intel-wired-lan; Alexei Starovoitov; Daniel Borkmann; William Tu
Subject: Re: [Intel-wired-lan] [RFC PATCH] ixgbe: delay tail write to every 'n' packets
Quoted 86 lines · John Fastabend
Quoted 61 lines · Alexander Duyck
Quoted 40 lines · John Fastabend
Current XDP implementation hits the tail on every
XDP_TXreturn code. This patch changes driver behavior to only hit the tail after packet processing is complete.RFC for now as I test this, it looks promising on my dev box but want to do some more tests before official submission.
Signed-off-by John Fastabend <[email protected]>--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +5
-2@@ -2282,6 +2282,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,unsigned int mss =0 ;#endif /* IXGBE_FCOE */ u16 cleaned_count =ixgbe_desc_unused (rx_ring); +bool xdp_xmit = false;while (likely (total_rx_packets < budget)) {union ixgbe_adv_rx_desc *rx_desc;@@ -2321,10 +2322,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, }if (IS_ERR (skb)) { -if (PTR_ERR (skb) == -IXGBE_XDP_TX ) +if (PTR_ERR (skb) == -IXGBE_XDP_TX ) { + xdp_xmit = true;ixgbe_rx_buffer_flip (rx_ring, rx_buffer, size); -else + }else { rx_buffer->pagecnt_bias++; + } total_rx_packets++; total_rx_bytes += size; }else if (skb) {drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, · line 2403
Quoted 9 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +6
-0@@ -2392,6 +2395,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, total_rx_packets++; } +if (xdp_xmit) { +struct ixgbe_ring *ring = adapter->xdp_ring[smp_processor_id ()]; + +writel (ring->next_to_use, ring->tail); + } +We will need a wmb here.
Quoted 7 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0u64_stats_update_begin (&rx_ring->syncp); rx_ring->stats.packets += total_rx_packets; rx_ring->stats.bytes += total_rx_bytes;@@ -8251,7 +8260,6 @@ static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, tx_buffer->next_to_watch = tx_desc; ring->next_to_use = i;Quoted 7 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, · line 8254
Quoted 1 line
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-1-writel (i, ring->tail);So you might want to change the barrier setup for all this to use
smp_wmbinstead. We need the wmb to be paired with the writel. That should give you a slight performance boost sincesmp_wmbbreaks down to just a barrier on x86 systems.Not sure I grok this description entirely, but I think you are just saying replace,
if (xdp_xmit) { ... writel(...) }with
if (xdp_xmit) { ... smp_rmb() writel() }
Quoted 4 lines
Correct? Did you have some other change in mind ... "for all this"?
Thanks,
John
No. Basically what it should be is find and replace wmb with smp_wmb() in ixgbe_xmit_xdp_ring. That way you won't have to worry about a race between Tx and clean-up.
Then the if statement should be:
if (xdp_xmit) {
...
/* wmb required to flush writes to coherent memory before writing
to non-coherent memory*/
wmb();
/* writel is a write to non-coherent memory mapped I/O */
writel();
}Quoted 5 lines
Quoted 4 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, · line 8265
Quoted 4 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0return IXGBE_XDP_TX ; }
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Duyck
RFC PATCH 1/1 ixgbe: delay tail write to every 'n' packets
Sounds good. ?We can probably discuss it tomorrow at our 1pm meeting.
Thanks.
- Alex
Quoted 141 lines · John R
FWIW with a revised version of this patch I see 14.6Mpps on RX drop tests and 13.5Mpps on TX test case.
Alex, maybe we can sync up and squeeze the last mpps out of the design.
Thanks,
John
________________________________________
From: Intel-wired-lan [intel-wired-lan-bounces at lists.osuosl.org] on behalf of Alexander Duyck [alexander.duyck at gmail.com]
Sent: Tuesday, March 14, 2017 6:14 PM
To: John Fastabend
Cc: intel-wired-lan; Alexei Starovoitov; Daniel Borkmann; William Tu
Subject: Re: [Intel-wired-lan] [RFC PATCH] ixgbe: delay tail write to every 'n' packetsQuoted 90 lines · John Fastabend
Quoted 64 lines · Alexander Duyck
Quoted 41 lines · John Fastabend
Current XDP implementation hits the tail on every
XDP_TXreturn code. This patch changes driver behavior to only hit the tail after packet processing is complete.RFC for now as I test this, it looks promising on my dev box but want to do some more tests before official submission.
Signed-off-by John Fastabend <[email protected]>--- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +5
-2@@ -2282,6 +2282,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,unsigned int mss =0 ;#endif /* IXGBE_FCOE */ u16 cleaned_count =ixgbe_desc_unused (rx_ring); +bool xdp_xmit = false;while (likely (total_rx_packets < budget)) {union ixgbe_adv_rx_desc *rx_desc;@@ -2321,10 +2322,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, }if (IS_ERR (skb)) { -if (PTR_ERR (skb) == -IXGBE_XDP_TX ) +if (PTR_ERR (skb) == -IXGBE_XDP_TX ) { + xdp_xmit = true;ixgbe_rx_buffer_flip (rx_ring, rx_buffer, size); -else + }else { rx_buffer->pagecnt_bias++; + } total_rx_packets++; total_rx_bytes += size; }else if (skb) {drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, · line 2403
Quoted 9 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +6
-0@@ -2392,6 +2395,12 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector, total_rx_packets++; } +if (xdp_xmit) { +struct ixgbe_ring *ring = adapter->xdp_ring[smp_processor_id ()]; + +writel (ring->next_to_use, ring->tail); + } +We will need a wmb here.
Quoted 8 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0u64_stats_update_begin (&rx_ring->syncp); rx_ring->stats.packets += total_rx_packets; rx_ring->stats.bytes += total_rx_bytes;@@ -8251,7 +8260,6 @@ static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, tx_buffer->next_to_watch = tx_desc; ring->next_to_use = i;Quoted 7 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, · line 8254
Quoted 1 line
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-1-writel (i, ring->tail);So you might want to change the barrier setup for all this to use
smp_wmbinstead. We need the wmb to be paired with the writel. That should give you a slight performance boost sincesmp_wmbbreaks down to just a barrier on x86 systems.Not sure I grok this description entirely, but I think you are just saying replace,
if (xdp_xmit) { ... writel(...) }with
if (xdp_xmit) { ... smp_rmb() writel() }Quoted 4 lines
Correct? Did you have some other change in mind ... "for all this"?
Thanks,
JohnNo. Basically what it should be is find and replace wmb with
smp_wmb()inixgbe_xmit_xdp_ring. That way you won't have to worry about a race between Tx and clean-up.Then the if statement should be:
if (xdp_xmit) { ... /* wmb required to flush writes to coherent memory before writing to non-coherent memory*/ wmb(); /* writel is a write to non-coherent memory mapped I/O */ writel(); }Quoted 8 lines
Quoted 7 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter, · line 8265
Quoted 7 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +0
-0return IXGBE_XDP_TX ; }_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan
_______________________________________________
Intel-wired-lan mailing list
Intel-wired-lan at lists.osuosl.org
http://lists.osuosl.org/mailman/listinfo/intel-wired-lan