ath9k: Implement rx copy-break.

From: Ben Greear<[email protected]>

This saves us constantly allocating large, multi-page skbs. It should fix the order-1 allocation errors reported, and in a 60-vif scenario, this significantly decreases CPU utilization, and latency, and increases bandwidth.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 b2497b8... ea2f67c... M drivers/net/wireless/ath/ath9k/recv.c

  drivers/net/wireless/ath/ath9k/recv.c |   92 ++++++++++++++++++++++-----------
  1 files changed, 61 insertions(+), 31 deletions(-)

drivers/net/wireless/ath/ath9k/recv.c +0 -14

index b2497b8..ea2f67c 100644
@@ -1702,42 +1704,70 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
  		    unlikely(tsf_lower - rs.rs_tstamp>  0x10000000))
  			rxs->mactime += 0x100000000ULL;
 
-		/* Ensure we always have an skb to requeue once we are done
-		 * processing the current buffer's skb */
-		requeue_skb = ath_rxbuf_alloc(common, common->rx_bufsize, GFP_ATOMIC);
-
-		/* If there is no memory we ignore the current RX'd frame,
-		 * tell hardware it can give us a new frame using the old
-		 * skb and put it at the tail of the sc->rx.rxbuf list for
-		 * processing. */
-		if (!requeue_skb)
-			goto requeue;
-
-		/* Unmap the frame */
-		dma_unmap_single(sc->dev, bf->bf_buf_addr,
-				 common->rx_bufsize,

Quoted 41 lines · greearb at candelatech.com

From: Ben Greear<[email protected]>

This saves us constantly allocating large, multi-page skbs. It should fix the order-1 allocation errors reported, and in a 60-vif scenario, this significantly decreases CPU utilization, and latency, and increases bandwidth.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 b2497b8... ea2f67c... M drivers/net/wireless/ath/ath9k/recv.c drivers/net/wireless/ath/ath9k/recv.c | 92 ++++++++++++++++++++++-----------

1 files changed, 61 insertions(+), 31 deletions(-)

drivers/net/wireless/ath/ath9k/recv.c +5 -15

index b2497b8..ea2f67c 100644
@@ -1702,42 +1704,70 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp)
 unlikely(tsf_lower - rs.rs_tstamp> 0x10000000))
 rxs->mactime += 0x100000000ULL;
 
- /* Ensure we always have an skb to requeue once we are done
- * processing the current buffer's skb */
- requeue_skb = ath_rxbuf_alloc(common, common->rx_bufsize, GFP_ATOMIC);
-
- /* If there is no memory we ignore the current RX'd frame,
- * tell hardware it can give us a new frame using the old
- * skb and put it at the tail of the sc->rx.rxbuf list for
- * processing. */
- if (!requeue_skb)
- goto requeue;
-
- /* Unmap the frame */
- dma_unmap_single(sc->dev, bf->bf_buf_addr,
- common->rx_bufsize,
- dma_type);
+ len = rs.rs_datalen + ah->caps.rx_status_len;
+ if (use_copybreak) {
+ skb = netdev_alloc_skb(NULL, len);
+ if (!skb) {
+ skb = bf->bf_mpdu;

drivers/net/wireless/ath/ath9k/recv.c int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp) · line 1715

Quoted 4 lines

drivers/net/wireless/ath/ath9k/recv.c +4 -0

+ use_copybreak = false;
+ goto non_copybreak;
+ }
+ } else {

I think this should be dependent on packet size, maybe even based on the architecture. Especially on embedded hardware, copying large frames is probably quite a bit more expensive than allocating large buffers. Cache sizes are small, memory access takes several cycles, especially during concurrent DMA. Once I'm back home, I could try a few packet size threshold to find a sweet spot for the typical MIPS hardware that I'm playing with. I expect a visible performance regression from this patch when applied as-is.

- Felix

Quoted 4 lines · Felix Fietkau

I think this should be dependent on packet size, maybe even based on the architecture. Especially on embedded hardware, copying large frames is probably quite a bit more expensive than allocating large buffers. Cache sizes are small, memory access takes several cycles, especially during concurrent DMA. Once I'm back home, I could try a few packet size threshold to find a sweet spot for the typical MIPS hardware that I'm playing with. I expect a visible performance regression from this patch when applied as-is.

I see a serious performance improvement with this patch. My current test is sending 1024 byte UDP payloads to/from each of 60 stations at 128kbps. Please do try it out on your system and see how it performs there. I'm guessing that any time you have more than 1 VIF this will be a good improvement since mac80211 does skb_copy (and you would typically be copying a much smaller packet with this patch).

If we do see performance differences on different platforms, this could perhaps be something we could tune at run-time.

Quoted 12 lines · Ben Greear

Quoted 4 lines · Felix Fietkau

I think this should be dependent on packet size, maybe even based on the architecture. Especially on embedded hardware, copying large frames is probably quite a bit more expensive than allocating large buffers. Cache sizes are small, memory access takes several cycles, especially during concurrent DMA. Once I'm back home, I could try a few packet size threshold to find a sweet spot for the typical MIPS hardware that I'm playing with. I expect a visible performance regression from this patch when applied as-is.

I see a serious performance improvement with this patch. My current test is sending 1024 byte UDP payloads to/from each of 60 stations at 128kbps. Please do try it out on your system and see how it performs there. I'm guessing that any time you have more than 1 VIF this will be a good improvement since mac80211 does skb_copy (and you would typically be copying a much smaller packet with this patch).

Quoted 2 lines

If we do see performance differences on different platforms, this could perhaps be something we could tune at run-time.

What kind of system are you testing on? If it's a PC, then the performance characteristics will be completely different compared to embedded hardware. I've had to remove a few copybreak-like implementations from various ethernet drivers on similar hardware, because even taking the hit of unaligned load/store exceptions (which are already *very* expensive on MIPS) was less than copying the full packet data, even with packet sizes less than what you're using.

I don't have suitable test hardware with me right now, but I'll do some tests in a week or so.

- Felix

Quoted 38 lines · Felix Fietkau

Quoted 23 lines · Ben Greear

Quoted 10 lines · Felix Fietkau

I think this should be dependent on packet size, maybe even based on the architecture. Especially on embedded hardware, copying large frames is probably quite a
bit more expensive than allocating large buffers. Cache sizes are small, memory access takes several cycles, especially during concurrent DMA. Once I'm back home, I could try a few packet size threshold to find a sweet spot for the typical MIPS hardware that I'm playing with. I expect a visible
performance regression from this patch when applied as-is.

I see a serious performance improvement with this patch. My current test is sending 1024 byte UDP
payloads to/from each of 60 stations at 128kbps. Please do try it out on your system and see how
it performs there. I'm guessing that any time you have more than 1 VIF this will be a good
improvement since mac80211 does skb_copy (and you would typically be copying a much smaller
packet with this patch).

Quoted 3 lines

If we do see performance differences on different platforms, this could perhaps be
something we could tune at run-time.

What kind of system are you testing on? If it's a PC, then the performance characteristics will be completely different compared to embedded hardware. I've had
to remove a few copybreak-like implementations from various ethernet drivers on similar hardware, because even taking the hit of unaligned load/store exceptions
(which are already *very* expensive on MIPS) was less than copying the full packet data, even with packet sizes less than what you're using.

Quoted 2 lines

I don't have suitable test hardware with me right now, but I'll do some tests in a week or so.

I'm on a dual-core Atom processor. ?I'm interested in your MIPs results when you get them...

I think we should also consider the added stability/saneness with this patch. I for one would be willing to live with some extra cpu load if that means the unstoppable rx dma problem can be contained (all the time).

Quoted 5 lines

I think we should also consider the added stability/saneness with this patch. I for one would be willing to live with some extra cpu load if that means the unstoppable rx dma problem can be contained (all the time).

I don't think this patch has anything to do with the rx dma stop issue.

Quoted 6 lines · Felix Fietkau

Quoted 5 lines · greearb at candelatech.com

From: Ben Greear<[email protected]>
This saves us constantly allocating large, multi-page skbs. It should fix the order-1 allocation errors reported, and in a 60-vif scenario, this significantly decreases CPU utilization, and latency, and increases bandwidth.

I see a serious performance improvement with this patch. My current test is sending 1024 byte UDP payloads to/from each of 60 stations at 128kbps. Please do try it out on your system and see how it performs there. I'm guessing that any time you have more than 1 VIF this will be a good improvement since mac80211 does skb_copy (and you would typically be copying a much smaller packet with this patch).

If we do see performance differences on different platforms, this could perhaps be something we could tune at run-time.

Quoted 7 lines · Ben Greear

Quoted 6 lines · Felix Fietkau

Quoted 5 lines · greearb at candelatech.com

From: Ben Greear<[email protected]>
This saves us constantly allocating large, multi-page skbs. It should fix the order-1 allocation errors reported, and in a 60-vif scenario, this significantly decreases CPU utilization, and latency, and increases bandwidth.

As far as CPU use is concerned, 60 VIF scenario should not be the one to use for checking what is most efficient.. This really needs to be tested on something that uses a single VIF on an embedded (low-power CPU)..

For the order-1 allocation issues, it would be interesting to see if someone could take a look at using paged skbs or multiple RX descriptors with shorter skbs (and copying only for the case where a long frame is received so that only the A-MSDU RX case would suffer from extra copying).

Quoted 13 lines · On Sunday 09 January 2011 19:13:04 Jouni Malinen wrote:

Quoted 7 lines · Ben Greear

Quoted 6 lines · Felix Fietkau

Quoted 5 lines · greearb at candelatech.com

From: Ben Greear<[email protected]>
This saves us constantly allocating large, multi-page skbs. It should fix the order-1 allocation errors reported, and in a 60-vif scenario, this significantly decreases CPU utilization, and latency, and increases bandwidth.

As far as CPU use is concerned, 60 VIF scenario should not be the one to use for checking what is most efficient.. This really needs to be tested on something that uses a single VIF on an embedded (low-power CPU)..

Quoted 5 lines

For the order-1 allocation issues, it would be interesting to see if someone could take a look at using paged skbs or multiple RX descriptors with shorter skbs (and copying only for the case where a long frame is received so that only the A-MSDU RX case would suffer from extra copying).

Well, here's a shot at paged rx. It'll only work when PAGE_SIZE> buf_size (which will be true for most system, I guess)

No idea how to handle EDMA... In fact I don't have any ath9k* solution at the moment to test ;), so you better backup your data!

Quoted 5 lines · On Sunday 09 January 2011 19:13:04 Jouni Malinen wrote:

For the order-1 allocation issues, it would be interesting to see if someone could take a look at using paged skbs or multiple RX descriptors with shorter skbs (and copying only for the case where a long frame is received so that only the A-MSDU RX case would suffer from extra copying).

Well, here's a shot at paged rx. It'll only work when PAGE_SIZE > buf_size (which will be true for most system, I guess)

Quoted 7 lines · Ben Greear

Quoted 6 lines · Felix Fietkau

Quoted 5 lines · greearb at candelatech.com

From: Ben Greear<[email protected]>
This saves us constantly allocating large, multi-page skbs. It should fix the order-1 allocation errors reported, and in a 60-vif scenario, this significantly decreases CPU utilization, and latency, and increases bandwidth.

As far as CPU use is concerned, 60 VIF scenario should not be the one to use for checking what is most efficient.. This really needs to be tested on something that uses a single VIF on an embedded (low-power CPU)..

For the order-1 allocation issues, it would be interesting to see if someone could take a look at using paged skbs or multiple RX descriptors with shorter skbs (and copying only for the case where a long frame is received so that only the A-MSDU RX case would suffer from extra copying).

Quoted 5 lines

I see a serious performance improvement with this patch. My current test is sending 1024 byte UDP payloads to/from each of 60 stations at 128kbps. Please do try it out on your system and see how it performs there. I'm guessing that any time you have more than 1 VIF this will be a good improvement since mac80211 does skb_copy (and you would typically be copying a much smaller packet with this patch).

How would this patch change the number of bytes copied by skb_copy?

Quoted 3 lines

If we do see performance differences on different platforms, this could perhaps be something we could tune at run-time.

I guess that could be looked at, but as long as that is not the case, the test setup you used is not exactly the most common case for ath9k in the upstream kernel and should not be used to figure out default behavior.

From: Ben Greear <[email protected]>

This saves us constantly allocating large, multi-page skbs. It should fix the order-1 allocation errors reported, and in a 60-vif scenario, this significantly decreases CPU utilization, and latency, and increases bandwidth.

Signed-off-by Ben Greear <[email protected]>

---
:100644 100644 b2497b8... ea2f67c... M drivers/net/wireless/ath/ath9k/recv.c

 drivers/net/wireless/ath/ath9k/recv.c |   92 ++++++++++++++++++++++-----------
 1 files changed, 61 insertions(+), 31 deletions(-)

Hi Ben,

Quoted 12 lines

From: Ben Greear<[email protected]>

This saves us constantly allocating large, multi-page skbs. It should fix the order-1 allocation errors reported, and in a 60-vif scenario, this significantly decreases CPU utilization, and latency, and increases bandwidth.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 b2497b8... ea2f67c... M drivers/net/wireless/ath/ath9k/recv.c

  drivers/net/wireless/ath/ath9k/recv.c |   92 ++++++++++++++++++++++-----------
  1 files changed, 61 insertions(+), 31 deletions(-)

<...>

Review

Quoted 2 lines

(quoted diff) +2 -0

+		if (use_copybreak) {
+			struct pci_dev *pdev = to_pci_dev(sc->dev);

This would cause undefined behaviour with ath9k devices sitting on an AHB bus.

drivers/net/wireless/ath/ath9k/recv.c int ath_rx_tasklet(struct ath_softc *sc, int flush, bool hp) · line 1746

Quoted 5 lines

drivers/net/wireless/ath/ath9k/recv.c +5 -0

+			pci_dma_sync_single_for_cpu(pdev, bf->bf_buf_addr,
+						    len, PCI_DMA_FROMDEVICE);
+			skb_copy_from_linear_data(bf->bf_mpdu, skb->data, len);
+			pci_dma_sync_single_for_device(pdev, bf->bf_buf_addr,
+						       len, PCI_DMA_FROMDEVICE);

Please use the bus agnostic equivalents of these DMA functions.

Quoted 28 lines

Hi Ben,

Quoted 12 lines

From: Ben Greear<[email protected]>

This saves us constantly allocating large, multi-page skbs. It should fix the order-1 allocation errors reported, and in a 60-vif scenario, this significantly decreases CPU utilization, and latency, and increases bandwidth.

Signed-off-by Ben Greear<[email protected]>

---
:100644 100644 b2497b8... ea2f67c... M drivers/net/wireless/ath/ath9k/recv.c

  drivers/net/wireless/ath/ath9k/recv.c |   92 ++++++++++++++++++++++-----------
  1 files changed, 61 insertions(+), 31 deletions(-)

<...>

Review

Quoted 2 lines

(quoted diff) +2 -0

+        if (use_copybreak) {
+            struct pci_dev *pdev = to_pci_dev(sc->dev);

This would cause undefined behaviour with ath9k devices sitting on an AHB bus.

Quoted 5 lines

drivers/net/wireless/ath/ath9k/recv.c +5 -0

+            pci_dma_sync_single_for_cpu(pdev, bf->bf_buf_addr,
+                            len, PCI_DMA_FROMDEVICE);
+            skb_copy_from_linear_data(bf->bf_mpdu, skb->data, len);
+            pci_dma_sync_single_for_device(pdev, bf->bf_buf_addr,
+                               len, PCI_DMA_FROMDEVICE);

Quoted 1 line

Please use the bus agnostic equivalents of these DMA functions.

Any idea what that might be?

Should we just disable copybreak for things on AHB bus?