[v2] ath10k: transmit queued frames after waking queues

The following problem was observed when running iperf:

[  3]  0.0- 1.0 sec  2.00 MBytes  16.8 Mbits/sec
[  3]  1.0- 2.0 sec  3.12 MBytes  26.2 Mbits/sec
[  3]  2.0- 3.0 sec  3.25 MBytes  27.3 Mbits/sec
[  3]  3.0- 4.0 sec   655 KBytes  5.36 Mbits/sec
[  3]  4.0- 5.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  5.0- 6.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  6.0- 7.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  7.0- 8.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  8.0- 9.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  9.0-10.0 sec  0.00 Bytes  0.00 bits/sec
[  3]  0.0-10.3 sec  9.01 MBytes  7.32 Mbits/sec

There are frames in the ieee80211_txq and there are frames that have
been removed from from this queue, but haven't yet been sent on the wire
(num_pending_tx).

When num_pending_tx reaches max_num_pending_tx, we will stop the queues
by calling ieee80211_stop_queues().

As frames that have previously been sent for transmission
(num_pending_tx) are completed, we will decrease num_pending_tx and wake
the queues by calling ieee80211_wake_queue(). ieee80211_wake_queue()
does not call wake_tx_queue, so we might still have frames in the
queue at this point.

While the queues were stopped, the socket buffer might have filled up,
and in order for user space to write more, we need to free the frames
in the queue, since they are accounted to the socket. In order to free
them, we first need to transmit them.

In order to avoid trying to flush the queue every time we free a frame,
only do this when there are 3 or less frames pending, and while we
actually have frames in the queue. This logic was copied from
mt76_txq_schedule (mt76), one of few other drivers that are actually
using wake_tx_queue.

Suggested-by: Toke Høiland-Jørgensen <toke@toke.dk>
Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>

---
Changes since V1: use READ_ONCE() to disallow the compiler
optimizing things in undesirable ways.

 drivers/net/wireless/ath/ath10k/txrx.c | 3 +++
 1 file changed, 3 insertions(+)

-- 
2.17.0

On Mon, May 21, 2018 at 04:11:38PM -0700, Rajkumar Manoharan wrote:
> On 2018-05-21 13:43, Niklas Cassel wrote:

> > The following problem was observed when running iperf:

> [...]

> > 

> > In order to avoid trying to flush the queue every time we free a frame,

> > only do this when there are 3 or less frames pending, and while we

> > actually have frames in the queue. This logic was copied from

> > mt76_txq_schedule (mt76), one of few other drivers that are actually

> > using wake_tx_queue.

> > 

> > Suggested-by: Toke Høiland-Jørgensen <toke@toke.dk>

> > Signed-off-by: Niklas Cassel <niklas.cassel@linaro.org>

> > ---

> > Changes since V1: use READ_ONCE() to disallow the compiler

> > optimizing things in undesirable ways.

> > 

> >  drivers/net/wireless/ath/ath10k/txrx.c | 3 +++

> >  1 file changed, 3 insertions(+)

> > 

> > diff --git a/drivers/net/wireless/ath/ath10k/txrx.c

> > b/drivers/net/wireless/ath/ath10k/txrx.c

> > index cda164f6e9f6..264cf0bd5c00 100644

> > --- a/drivers/net/wireless/ath/ath10k/txrx.c

> > +++ b/drivers/net/wireless/ath/ath10k/txrx.c

> > @@ -95,6 +95,9 @@ int ath10k_txrx_tx_unref(struct ath10k_htt *htt,

> >  		wake_up(&htt->empty_tx_wq);

> >  	spin_unlock_bh(&htt->tx_lock);

> > 

> > +	if (READ_ONCE(htt->num_pending_tx) <= 3 && !list_empty(&ar->txqs))

> > +		ath10k_mac_tx_push_pending(ar);

> > +

> Niklas,


Hello Rajkumar

> 

> Sorry for the late response. ath10k_mac_tx_push_pending is already called

> at the end of NAPI handler. Isn't that enough to process pending frames?


This is true for e.g. ATH10K_BUS_PCI and ATH10K_BUS_SNOC,
but not for e.g. ATH10K_BUS_SDIO and ATH10K_BUS_USB.

While there is some SDIO code merged in Kalle's tree already,
this problem was found when merging
https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/?h=ath10k-pending-sdio-usb
with Kalle's ath-next branch.

> 

> Earlier we observed performance issues in calling push_pending from each

> tx completion. IMHO this change may introduce the same problem again.


I prefer functional TX over performance issues,
but I agree that it is unfortunate that SDIO doesn't use
ath10k_htt_txrx_compl_task().
Erik, is there a reason for this?

Perhaps it would be possible to call ath10k_mac_tx_push_pending()
from the equivalent to ath10k_htt_txrx_compl_task(),
but from SDIO's point of view.

Another solution might be to change so that we only call
ath10k_mac_tx_push_pending() from ath10k_txrx_tx_unref()
if (htt->num_pending_tx == 0). That should decrease the number
of calls to ath10k_mac_tx_push_pending(), while still avoiding
a "TX deadlock" scenario for SDIO.


Regards,
Niklas

On 05/22/2018 11:15 PM, Niklas Cassel wrote:

<snip>
>>

>> Earlier we observed performance issues in calling push_pending from each

>> tx completion. IMHO this change may introduce the same problem again.

> 

> I prefer functional TX over performance issues,

> but I agree that it is unfortunate that SDIO doesn't use

> ath10k_htt_txrx_compl_task().

> Erik, is there a reason for this?

The reason is that the SDIO code has been derived mainly from qcacld and ath6kl
and they don't implement napi.

ath10k_htt_txrx_compl_task is currently only called from the napi poll function,
and the sdio bus driver doesn't have such a function.

> 

> Perhaps it would be possible to call ath10k_mac_tx_push_pending()

> from the equivalent to ath10k_htt_txrx_compl_task(),

> but from SDIO's point of view.

An equivalent for SDIO would most likely be *ath10k_htt_htc_t2h_msg_handler*
or any of the other functions called from this function.

*ath10k_txrx_tx_unref* is actually called from *ath10k_htt_htc_t2h_msg_handler*,
so that function could be viewed as an equivalent.

If the call should be added in the bus driver (sdio.c) it should most likely be
placed in *ath10k_sdio_mbox_rx_process_packets*

		if (!pkt->trailer_only) {
			ep->ep_ops.ep_rx_complete(ar_sdio->ar, pkt->skb);
			ath10k_mac_tx_push_pending(ar_sdio->ar);
		} else {
			kfree_skb(pkt->skb)
		}

The above call would of course result in lot's of calls to *ath10k_mac_tx_push_pending*
Adding a htt_num_pending check here wouldn't look nice.

The HL RX path differs from the LL path in that the t2h_msg_handler returns
false indicating that it has consumed the skb.

This is because it is the HL RX indication handler that delivers the skb's
to mac80211.

Another solution could be to add an *else-statement* as a part of the *if (release)*
in *ath10k_htt_htc_t2h_msg_handler*, where *ath10k_mac_tx_push_pending* could be called.

Something like this perhaps:

	/* Free the indication buffer */
	if (release)
		dev_kfree_skb_any(skb);
	else if (!ar->htt.num_pending_tx)
		ath10k_mac_tx_push_pending(ar);

I think I prefer your original patch though.
> 

> Another solution might be to change so that we only call

> ath10k_mac_tx_push_pending() from ath10k_txrx_tx_unref()

> if (htt->num_pending_tx == 0). That should decrease the number

> of calls to ath10k_mac_tx_push_pending(), while still avoiding

> a "TX deadlock" scenario for SDIO.

Just out of curiosity, where did the limit of 3 come from?
If it works with a limit of 0, I think it should be used instead.

Another intersting thing that I stumbled upon when looking into the
code (while writing this email) is the *wake_up(&htt->empty_tx_wq);*

For some reason I have considered it not to be applicable for HL devices.

The queue is waited for in the flush op (*ath10k_flush*).
I am unsure what it is used for, but I don't think it affects the TX
deadlock scenario.

--
Erik