[3/6] ath10k: sdio: read RX packets in bundles

Commit Message

Erik Stromdahl April 9, 2019, 7:08 p.m. UTC

From: Alagu Sankar <[e-mail]>

The existing implementation of initiating multiple sdio transfers for
receive bundling is slowing down the receive speed.

Instead of having one sdio transfer for each packet in the bundle, we
read all packets in one sdio transfer.

This results in significant performance improvement on some targets.

Co-developed-by: Erik Stromdahl <[e-mail]>
Signed-off-by: Alagu Sankar <[e-mail]>
Signed-off-by: Erik Stromdahl <[e-mail]>
---
 drivers/net/wireless/ath/ath10k/sdio.c | 66 +++++++++++++++++++++-----
 drivers/net/wireless/ath/ath10k/sdio.h |  2 +
 2 files changed, 55 insertions(+), 13 deletions(-)

Comments

Kalle Valo April 12, 2019, 1:08 p.m. UTC | #1

Erik Stromdahl <[e-mail]> writes:

> From: Alagu Sankar <[e-mail]>
>
> The existing implementation of initiating multiple sdio transfers for
> receive bundling is slowing down the receive speed.
>
> Instead of having one sdio transfer for each packet in the bundle, we
> read all packets in one sdio transfer.
>
> This results in significant performance improvement on some targets.

Do you have any numbers? Before and after would be nice to know.

> +static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
>  {
>  	struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> +	struct ath10k_sdio_rx_data *pkt = &ar_sdio->rx_pkts[0];
>  	struct sk_buff *skb = pkt->skb;
>  	int ret;
>  
> -	ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
> -				 skb->data, pkt->alloc_len);
> -	pkt->status = ret;
> -	if (!ret)
> +	ret = ath10k_sdio_read(ar, ar_sdio->mbox_info.htc_addr,
> +			       skb->data, pkt->alloc_len);
> +	if (ret) {
> +		ar_sdio->n_rx_pkts = 0;
> +		ath10k_sdio_mbox_free_rx_pkt(pkt);
> +	} else {
> +		pkt->status = ret;
>  		skb_put(skb, pkt->act_len);
> +	}

With this you can avoid the else branch:

if (ret) {
	ar_sdio->n_rx_pkts = 0;
	ath10k_sdio_mbox_free_rx_pkt(pkt);
        return ret;
}

> -static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
> +static int ath10k_sdio_mbox_rx_fetch_bundle(struct ath10k *ar)
>  {
>  	struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
> +	struct ath10k_sdio_rx_data *pkt;
>  	int ret, i;
> +	u32 pkt_offset = 0, pkt_bundle_len = 0;
> +
> +	for (i = 0; i < ar_sdio->n_rx_pkts; i++)
> +		pkt_bundle_len += ar_sdio->rx_pkts[i].alloc_len;
> +
> +	if (pkt_bundle_len > ATH10K_SDIO_READ_BUF_SIZE) {
> +		ret = -ENOMEM;
> +		ath10k_err(ar, "bundle size (%d) exceeding limit %d\n",
> +			   pkt_bundle_len, ATH10K_SDIO_READ_BUF_SIZE);

As this is a recoverable case please use ath10k_warn(). And would
-ENOSPC be more descriptive error?

> +		goto err;
> +	}
> +
> +	ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
> +				 ar_sdio->sdio_read_buf, pkt_bundle_len);
> +	if (ret)
> +		goto err;
>  
>  	for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
> -		ret = ath10k_sdio_mbox_rx_packet(ar,
> -						 &ar_sdio->rx_pkts[i]);
> -		if (ret)
> -			goto err;
> +		struct sk_buff *skb = ar_sdio->rx_pkts[i].skb;
> +
> +		pkt = &ar_sdio->rx_pkts[i];
> +		memcpy(skb->data, ar_sdio->sdio_read_buf + pkt_offset,
> +		       pkt->alloc_len);
> +		pkt->status = 0;
> +		skb_put(skb, pkt->act_len);

Shouldn't you call first skb_put() and then memcpy()?

Patch

diff --git a/drivers/net/wireless/ath/ath10k/sdio.c b/drivers/net/wireless/ath/ath10k/sdio.c
index fae56c67766f..295e1e7ec3b0 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.c
+++ b/drivers/net/wireless/ath/ath10k/sdio.c
@@ -24,6 +24,8 @@ 
 #include "trace.h"
 #include "sdio.h"
 
+#define ATH10K_SDIO_READ_BUF_SIZE	(32 * 1024)
+
 /* inlined helper functions */
 
 static inline int ath10k_sdio_calc_txrx_padded_len(struct ath10k_sdio *ar_sdio,
@@ -618,41 +620,68 @@  static int ath10k_sdio_mbox_rx_alloc(struct ath10k *ar,
 	return ret;
 }
 
-static int ath10k_sdio_mbox_rx_packet(struct ath10k *ar,
-				      struct ath10k_sdio_rx_data *pkt)
+static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
 {
 	struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
+	struct ath10k_sdio_rx_data *pkt = &ar_sdio->rx_pkts[0];
 	struct sk_buff *skb = pkt->skb;
 	int ret;
 
-	ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
-				 skb->data, pkt->alloc_len);
-	pkt->status = ret;
-	if (!ret)
+	ret = ath10k_sdio_read(ar, ar_sdio->mbox_info.htc_addr,
+			       skb->data, pkt->alloc_len);
+	if (ret) {
+		ar_sdio->n_rx_pkts = 0;
+		ath10k_sdio_mbox_free_rx_pkt(pkt);
+	} else {
+		pkt->status = ret;
 		skb_put(skb, pkt->act_len);
+	}
 
 	return ret;
 }
 
-static int ath10k_sdio_mbox_rx_fetch(struct ath10k *ar)
+static int ath10k_sdio_mbox_rx_fetch_bundle(struct ath10k *ar)
 {
 	struct ath10k_sdio *ar_sdio = ath10k_sdio_priv(ar);
+	struct ath10k_sdio_rx_data *pkt;
 	int ret, i;
+	u32 pkt_offset = 0, pkt_bundle_len = 0;
+
+	for (i = 0; i < ar_sdio->n_rx_pkts; i++)
+		pkt_bundle_len += ar_sdio->rx_pkts[i].alloc_len;
+
+	if (pkt_bundle_len > ATH10K_SDIO_READ_BUF_SIZE) {
+		ret = -ENOMEM;
+		ath10k_err(ar, "bundle size (%d) exceeding limit %d\n",
+			   pkt_bundle_len, ATH10K_SDIO_READ_BUF_SIZE);
+		goto err;
+	}
+
+	ret = ath10k_sdio_readsb(ar, ar_sdio->mbox_info.htc_addr,
+				 ar_sdio->sdio_read_buf, pkt_bundle_len);
+	if (ret)
+		goto err;
 
 	for (i = 0; i < ar_sdio->n_rx_pkts; i++) {
-		ret = ath10k_sdio_mbox_rx_packet(ar,
-						 &ar_sdio->rx_pkts[i]);
-		if (ret)
-			goto err;
+		struct sk_buff *skb = ar_sdio->rx_pkts[i].skb;
+
+		pkt = &ar_sdio->rx_pkts[i];
+		memcpy(skb->data, ar_sdio->sdio_read_buf + pkt_offset,
+		       pkt->alloc_len);
+		pkt->status = 0;
+		skb_put(skb, pkt->act_len);
+		pkt_offset += pkt->alloc_len;
 	}
 
 	return 0;
 
 err:
 	/* Free all packets that was not successfully fetched. */
-	for (; i < ar_sdio->n_rx_pkts; i++)
+	for (i = 0; i < ar_sdio->n_rx_pkts; i++)
 		ath10k_sdio_mbox_free_rx_pkt(&ar_sdio->rx_pkts[i]);
 
+	ar_sdio->n_rx_pkts = 0;
+
 	return ret;
 }
 
@@ -695,7 +724,10 @@  static int ath10k_sdio_mbox_rxmsg_pending_handler(struct ath10k *ar,
 			 */
 			*done = false;
 
-		ret = ath10k_sdio_mbox_rx_fetch(ar);
+		if (ar_sdio->n_rx_pkts > 1)
+			ret = ath10k_sdio_mbox_rx_fetch_bundle(ar);
+		else
+			ret = ath10k_sdio_mbox_rx_fetch(ar);
 
 		/* Process fetched packets. This will potentially update
 		 * n_lookaheads depending on if the packets contain lookahead
@@ -2001,6 +2033,14 @@  static int ath10k_sdio_probe(struct sdio_func *func,
 		goto err_core_destroy;
 	}
 
+	ar_sdio->sdio_read_buf = devm_kzalloc(ar->dev,
+					      ATH10K_SDIO_READ_BUF_SIZE,
+					      GFP_KERNEL);
+	if (!ar_sdio->sdio_read_buf) {
+		ret = -ENOMEM;
+		goto err_core_destroy;
+	}
+
 	ar_sdio->func = func;
 	sdio_set_drvdata(func, ar_sdio);
 
diff --git a/drivers/net/wireless/ath/ath10k/sdio.h b/drivers/net/wireless/ath/ath10k/sdio.h
index b8c7ac0330bd..07e2cc6a3bd8 100644
--- a/drivers/net/wireless/ath/ath10k/sdio.h
+++ b/drivers/net/wireless/ath/ath10k/sdio.h
@@ -196,6 +196,8 @@  struct ath10k_sdio {
 	struct ath10k *ar;
 	struct ath10k_sdio_irq_data irq_data;
 
+	u8 *sdio_read_buf;
+
 	/* temporary buffer for BMI requests */
 	u8 *bmi_buf;