diff mbox series

aquantia: Remove the build_skb path

Message ID CY4PR1001MB2311E1B5D8E2700C92E7BE2DE8E00@CY4PR1001MB2311.namprd10.prod.outlook.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series aquantia: Remove the build_skb path | expand

Checks

Context Check Description
netdev/tree_selection success Guessing tree name failed - patch did not apply

Commit Message

Ramsay, Lincoln Nov. 19, 2020, 10:01 p.m. UTC 
The build_skb path fails to allow for an SKB header, but the hardware
buffer it is built around won't allow for this anyway.

Just always use the slower codepath that copies memory into an
allocated SKB.

Signed-off-by: Lincoln Ramsay <lincoln.ramsay@opengear.com>
---
 .../net/ethernet/aquantia/atlantic/aq_ring.c  | 118 ++++++++----------
 1 file changed, 50 insertions(+), 68 deletions(-)

Comments

Igor Russkikh Nov. 20, 2020, 7:52 a.m. UTC | #1 
On 20/11/2020 1:01 am, Ramsay, Lincoln wrote:
> External Email
> 
> ----------------------------------------------------------------------
> The build_skb path fails to allow for an SKB header, but the hardware
> buffer it is built around won't allow for this anyway.
> 
> Just always use the slower codepath that copies memory into an
> allocated SKB.
> 
> Signed-off-by: Lincoln Ramsay <lincoln.ramsay@opengear.com>

Acked-by: Igor Russkikh <irusskikh@marvell.com>

Yep, that could be the only way to fix this for now.

Have you tried to estimate any performance drops from this?

The most harm may be here on smaller packets, for stuff like UDP.

Regards,
  Igor
Ramsay, Lincoln Nov. 23, 2020, 4:20 a.m. UTC | #2 
> Yep, that could be the only way to fix this for now.
> Have you tried to estimate any performance drops from this?

Unfortunately, I am not in a very good position to do this. The 10G interfaces on our device don't actually have enough raw PCI bandwidth available to hit 10G transfer rates.

I did use iperf3 and saw bursts over 2Gbit/sec (with average closer to 1.3Gbit/sec on a good run). There was no significant difference between running with and without the patch. I am told that this is about as good as can be expected.

Make of that what you will :)

Lincoln
Igor Russkikh Nov. 24, 2020, 2:29 p.m. UTC | #3 
On 23/11/2020 7:20 am, Ramsay, Lincoln wrote:
>> Yep, that could be the only way to fix this for now.
>> Have you tried to estimate any performance drops from this?
> 
> Unfortunately, I am not in a very good position to do this. The 10G
> interfaces on our device don't actually have enough raw PCI bandwidth
> available to hit 10G transfer rates.
> 
> I did use iperf3 and saw bursts over 2Gbit/sec (with average closer to
> 1.3Gbit/sec on a good run). There was no significant difference between
> running with and without the patch. I am told that this is about as good
> as can be expected.
> 
> Make of that what you will :)

Thats not very useful, but since we anyway have to fix that - lets do it.

I'll try to estimate potential perf drop on my setup when possible.

Thanks,
  Igor
diff mbox series

Patch

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
index 68fdb3994088..74f6f41b57e9 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_ring.c
@@ -385,79 +385,61 @@  int aq_ring_rx_clean(struct aq_ring_s *self,
 					      buff->rxdata.pg_off,
 					      buff->len, DMA_FROM_DEVICE);
 
-		/* for single fragment packets use build_skb() */
-		if (buff->is_eop &&
-		    buff->len <= AQ_CFG_RX_FRAME_MAX - AQ_SKB_ALIGN) {
-			skb = build_skb(aq_buf_vaddr(&buff->rxdata),
+		skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
+		if (unlikely(!skb)) {
+			err = -ENOMEM;
+			goto err_exit;
+		}
+		if (is_ptp_ring)
+			buff->len -=
+				aq_ptp_extract_ts(self->aq_nic, skb,
+					aq_buf_vaddr(&buff->rxdata),
+					buff->len);
+
+		hdr_len = buff->len;
+		if (hdr_len > AQ_CFG_RX_HDR_SIZE)
+			hdr_len = eth_get_headlen(skb->dev,
+							aq_buf_vaddr(&buff->rxdata),
+							AQ_CFG_RX_HDR_SIZE);
+
+		memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
+			ALIGN(hdr_len, sizeof(long)));
+
+		if (buff->len - hdr_len > 0) {
+			skb_add_rx_frag(skb, 0, buff->rxdata.page,
+					buff->rxdata.pg_off + hdr_len,
+					buff->len - hdr_len,
 					AQ_CFG_RX_FRAME_MAX);
-			if (unlikely(!skb)) {
-				err = -ENOMEM;
-				goto err_exit;
-			}
-			if (is_ptp_ring)
-				buff->len -=
-					aq_ptp_extract_ts(self->aq_nic, skb,
-						aq_buf_vaddr(&buff->rxdata),
-						buff->len);
-			skb_put(skb, buff->len);
 			page_ref_inc(buff->rxdata.page);
-		} else {
-			skb = napi_alloc_skb(napi, AQ_CFG_RX_HDR_SIZE);
-			if (unlikely(!skb)) {
-				err = -ENOMEM;
-				goto err_exit;
-			}
-			if (is_ptp_ring)
-				buff->len -=
-					aq_ptp_extract_ts(self->aq_nic, skb,
-						aq_buf_vaddr(&buff->rxdata),
-						buff->len);
-
-			hdr_len = buff->len;
-			if (hdr_len > AQ_CFG_RX_HDR_SIZE)
-				hdr_len = eth_get_headlen(skb->dev,
-							  aq_buf_vaddr(&buff->rxdata),
-							  AQ_CFG_RX_HDR_SIZE);
-
-			memcpy(__skb_put(skb, hdr_len), aq_buf_vaddr(&buff->rxdata),
-			       ALIGN(hdr_len, sizeof(long)));
-
-			if (buff->len - hdr_len > 0) {
-				skb_add_rx_frag(skb, 0, buff->rxdata.page,
-						buff->rxdata.pg_off + hdr_len,
-						buff->len - hdr_len,
-						AQ_CFG_RX_FRAME_MAX);
-				page_ref_inc(buff->rxdata.page);
-			}
+		}
 
-			if (!buff->is_eop) {
-				buff_ = buff;
-				i = 1U;
-				do {
-					next_ = buff_->next,
-					buff_ = &self->buff_ring[next_];
+		if (!buff->is_eop) {
+			buff_ = buff;
+			i = 1U;
+			do {
+				next_ = buff_->next,
+				buff_ = &self->buff_ring[next_];
 
-					dma_sync_single_range_for_cpu(
-							aq_nic_get_dev(self->aq_nic),
-							buff_->rxdata.daddr,
-							buff_->rxdata.pg_off,
-							buff_->len,
-							DMA_FROM_DEVICE);
-					skb_add_rx_frag(skb, i++,
-							buff_->rxdata.page,
-							buff_->rxdata.pg_off,
-							buff_->len,
-							AQ_CFG_RX_FRAME_MAX);
-					page_ref_inc(buff_->rxdata.page);
-					buff_->is_cleaned = 1;
-
-					buff->is_ip_cso &= buff_->is_ip_cso;
-					buff->is_udp_cso &= buff_->is_udp_cso;
-					buff->is_tcp_cso &= buff_->is_tcp_cso;
-					buff->is_cso_err |= buff_->is_cso_err;
+				dma_sync_single_range_for_cpu(
+						aq_nic_get_dev(self->aq_nic),
+						buff_->rxdata.daddr,
+						buff_->rxdata.pg_off,
+						buff_->len,
+						DMA_FROM_DEVICE);
+				skb_add_rx_frag(skb, i++,
+						buff_->rxdata.page,
+						buff_->rxdata.pg_off,
+						buff_->len,
+						AQ_CFG_RX_FRAME_MAX);
+				page_ref_inc(buff_->rxdata.page);
+				buff_->is_cleaned = 1;
 
-				} while (!buff_->is_eop);
-			}
+				buff->is_ip_cso &= buff_->is_ip_cso;
+				buff->is_udp_cso &= buff_->is_udp_cso;
+				buff->is_tcp_cso &= buff_->is_tcp_cso;
+				buff->is_cso_err |= buff_->is_cso_err;
+
+			} while (!buff_->is_eop);
 		}
 
 		if (buff->is_vlan)