[PATCH] ath9k: clean up and fix ath_tx_count_airtime

[PATCH] ath9k: clean up and fix ath_tx_count_airtime

[Felix Fietkau]

ath_tx_count_airtime is doing a lot of unnecessary work:

- Redundant station lookup
- Redundant rcu_read_lock/unlock
- Useless memcpy of bf->rates
- Useless NULL check of bf->bf_mpdu
- Redundant lookup of the skb tid

Additionally, it tries to look up the mac80211 queue index from the txq,
which fails if the frame was delivered via the power save queue.

This patch fixes all of these issues by passing down the right set of
pointers instead of doing extra work

Cc: stable@vger.kernel.org
Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling between stations")
Signed-off-by: Felix Fietkau <nbd@nbd.name>
---
 drivers/net/wireless/ath/ath9k/xmit.c | 52 +++++++++++------------------------
 1 file changed, 16 insertions(+), 36 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 8f9a87915ba9..30efe79e9d89 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -723,51 +723,31 @@ static bool bf_is_ampdu_not_probing(struct ath_buf *bf)
     return bf_isampdu(bf) && !(info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE);
 }
 
-static void ath_tx_count_airtime(struct ath_softc *sc, struct ath_txq *txq,
-				 struct ath_buf *bf, struct ath_tx_status *ts)
+static void ath_tx_count_airtime(struct ath_softc *sc, struct ath_node *an,
+				 struct ath_atx_tid *tid, struct ath_buf *bf,
+				 struct ath_tx_status *ts)
 {
-	struct ath_node *an;
-	struct ath_acq *acq = &sc->cur_chan->acq[txq->mac80211_qnum];
-	struct sk_buff *skb;
-	struct ieee80211_hdr *hdr;
-	struct ieee80211_hw *hw = sc->hw;
-	struct ieee80211_tx_rate rates[4];
-	struct ieee80211_sta *sta;
-	int i;
+	struct ath_txq *txq = tid->txq;
 	u32 airtime = 0;
-
-	skb = bf->bf_mpdu;
-	if(!skb)
-		return;
-
-	hdr = (struct ieee80211_hdr *)skb->data;
-	memcpy(rates, bf->rates, sizeof(rates));
-
-	rcu_read_lock();
-
-	sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr1, hdr->addr2);
-	if(!sta)
-		goto exit;
-
-
-	an = (struct ath_node *) sta->drv_priv;
+	int i;
 
 	airtime += ts->duration * (ts->ts_longretry + 1);
+	for(i = 0; i < ts->ts_rateindex; i++) {
+		int rate_dur = ath9k_hw_get_duration(sc->sc_ah, bf->bf_desc, i);
+		airtime += rate_dur * bf->rates[i].count;
+	}
 
-	for(i=0; i < ts->ts_rateindex; i++)
-		airtime += ath9k_hw_get_duration(sc->sc_ah, bf->bf_desc, i) * rates[i].count;
+	if (sc->airtime_flags & AIRTIME_USE_TX) {
+		int q = txq->mac80211_qnum;
+		struct ath_acq *acq = &sc->cur_chan->acq[q];
 
-	if (!!(sc->airtime_flags & AIRTIME_USE_TX)) {
 		spin_lock_bh(&acq->lock);
-		an->airtime_deficit[txq->mac80211_qnum] -= airtime;
-		if (an->airtime_deficit[txq->mac80211_qnum] <= 0)
-			__ath_tx_queue_tid(sc, ath_get_skb_tid(sc, an, skb));
+		an->airtime_deficit[q] -= airtime;
+		if (an->airtime_deficit[q] <= 0)
+			__ath_tx_queue_tid(sc, tid);
 		spin_unlock_bh(&acq->lock);
 	}
 	ath_debug_airtime(sc, an, 0, airtime);
-
-exit:
-	rcu_read_unlock();
 }
 
 static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
@@ -791,13 +771,13 @@ static void ath_tx_process_buffer(struct ath_softc *sc, struct ath_txq *txq,
 
 	ts->duration = ath9k_hw_get_duration(sc->sc_ah, bf->bf_desc,
 					     ts->ts_rateindex);
-	ath_tx_count_airtime(sc, txq, bf, ts);
 
 	hdr = (struct ieee80211_hdr *) bf->bf_mpdu->data;
 	sta = ieee80211_find_sta_by_ifaddr(hw, hdr->addr1, hdr->addr2);
 	if (sta) {
 		struct ath_node *an = (struct ath_node *)sta->drv_priv;
 		tid = ath_get_skb_tid(sc, an, bf->bf_mpdu);
+		ath_tx_count_airtime(sc, an, tid, bf, ts);
 		if (ts->ts_status & (ATH9K_TXERR_FILT | ATH9K_TXERR_XRETRY))
 			tid->clear_ps_filter = true;
 	}
-- 
2.11.0

Re: [PATCH] ath9k: clean up and fix ath_tx_count_airtime

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> ath_tx_count_airtime is doing a lot of unnecessary work:
>
> - Redundant station lookup
> - Redundant rcu_read_lock/unlock
> - Useless memcpy of bf->rates
> - Useless NULL check of bf->bf_mpdu
> - Redundant lookup of the skb tid
>
> Additionally, it tries to look up the mac80211 queue index from the txq,
> which fails if the frame was delivered via the power save queue.
>
> This patch fixes all of these issues by passing down the right set of
> pointers instead of doing extra work
>
> Cc: stable@vger.kernel.org
> Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling betwee=
n stations")
> Signed-off-by: Felix Fietkau <nbd@nbd.name>

Not sure if there's anything for stable to do with this; don't think the
airtime fairness code has gone into a release yet? Otherwise:

Acked-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk>

-Toke

Re: [PATCH] ath9k: clean up and fix ath_tx_count_airtime

[Felix Fietkau]

On 2017-02-12 16:22, Toke Høiland-Jørgensen wrote:
> Felix Fietkau <nbd@nbd.name> writes:
> 
>> ath_tx_count_airtime is doing a lot of unnecessary work:
>>
>> - Redundant station lookup
>> - Redundant rcu_read_lock/unlock
>> - Useless memcpy of bf->rates
>> - Useless NULL check of bf->bf_mpdu
>> - Redundant lookup of the skb tid
>>
>> Additionally, it tries to look up the mac80211 queue index from the txq,
>> which fails if the frame was delivered via the power save queue.
>>
>> This patch fixes all of these issues by passing down the right set of
>> pointers instead of doing extra work
>>
>> Cc: stable@vger.kernel.org
>> Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling between stations")
>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> 
> Not sure if there's anything for stable to do with this; don't think the
> airtime fairness code has gone into a release yet? Otherwise:
> 
> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
I added this, because I'm not sure this patch will make it to 4.10 in
time, since we're really close to a release. I assume this patch will
probably go into 4.11.

- Felix

Re: [PATCH] ath9k: clean up and fix ath_tx_count_airtime

[Kalle Valo]

Felix Fietkau <nbd@nbd.name> writes:

> On 2017-02-12 16:22, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>>=20
>>> ath_tx_count_airtime is doing a lot of unnecessary work:
>>>
>>> - Redundant station lookup
>>> - Redundant rcu_read_lock/unlock
>>> - Useless memcpy of bf->rates
>>> - Useless NULL check of bf->bf_mpdu
>>> - Redundant lookup of the skb tid
>>>
>>> Additionally, it tries to look up the mac80211 queue index from the txq,
>>> which fails if the frame was delivered via the power save queue.

What does this mean in practise, what's the user level impact?

>>> This patch fixes all of these issues by passing down the right set of
>>> pointers instead of doing extra work
>>>
>>> Cc: stable@vger.kernel.org
>>> Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling
>>> between stations")
>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>=20
>> Not sure if there's anything for stable to do with this; don't think the
>> airtime fairness code has gone into a release yet? Otherwise:
>>=20
>> Acked-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk>
>
> I added this, because I'm not sure this patch will make it to 4.10 in
> time, since we're really close to a release. I assume this patch will
> probably go into 4.11.

Yeah, to try to get a patch to 4.10 at this point needs to be a really
high profile regression. That is if Linus doesn't release 4.10 today, of
course.

--=20
Kalle Valo

Re: [PATCH] ath9k: clean up and fix ath_tx_count_airtime

[Felix Fietkau]

On 2017-02-12 17:28, Kalle Valo wrote:
> Felix Fietkau <nbd@nbd.name> writes:
> 
>> On 2017-02-12 16:22, Toke Høiland-Jørgensen wrote:
>>> Felix Fietkau <nbd@nbd.name> writes:
>>> 
>>>> ath_tx_count_airtime is doing a lot of unnecessary work:
>>>>
>>>> - Redundant station lookup
>>>> - Redundant rcu_read_lock/unlock
>>>> - Useless memcpy of bf->rates
>>>> - Useless NULL check of bf->bf_mpdu
>>>> - Redundant lookup of the skb tid
>>>>
>>>> Additionally, it tries to look up the mac80211 queue index from the txq,
>>>> which fails if the frame was delivered via the power save queue.
> 
> What does this mean in practise, what's the user level impact?
> 
>>>> This patch fixes all of these issues by passing down the right set of
>>>> pointers instead of doing extra work
>>>>
>>>> Cc: stable@vger.kernel.org
>>>> Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling
>>>> between stations")
>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>> 
>>> Not sure if there's anything for stable to do with this; don't think the
>>> airtime fairness code has gone into a release yet? Otherwise:
>>> 
>>> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>
>> I added this, because I'm not sure this patch will make it to 4.10 in
>> time, since we're really close to a release. I assume this patch will
>> probably go into 4.11.
> 
> Yeah, to try to get a patch to 4.10 at this point needs to be a really
> high profile regression. That is if Linus doesn't release 4.10 today, of
> course.
The symptoms are kernel crashes at least when operating in AP mode.
It's pretty severe, so getting it into 4.10 would be preferable.

- Felix

Re: [PATCH] ath9k: clean up and fix ath_tx_count_airtime

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> On 2017-02-12 17:28, Kalle Valo wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>>=20
>>> On 2017-02-12 16:22, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>=20
>>>>> ath_tx_count_airtime is doing a lot of unnecessary work:
>>>>>
>>>>> - Redundant station lookup
>>>>> - Redundant rcu_read_lock/unlock
>>>>> - Useless memcpy of bf->rates
>>>>> - Useless NULL check of bf->bf_mpdu
>>>>> - Redundant lookup of the skb tid
>>>>>
>>>>> Additionally, it tries to look up the mac80211 queue index from the t=
xq,
>>>>> which fails if the frame was delivered via the power save queue.
>>=20
>> What does this mean in practise, what's the user level impact?
>>=20
>>>>> This patch fixes all of these issues by passing down the right set of
>>>>> pointers instead of doing extra work
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling
>>>>> between stations")
>>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>>=20
>>>> Not sure if there's anything for stable to do with this; don't think t=
he
>>>> airtime fairness code has gone into a release yet? Otherwise:
>>>>=20
>>>> Acked-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk>
>>>
>>> I added this, because I'm not sure this patch will make it to 4.10 in
>>> time, since we're really close to a release. I assume this patch will
>>> probably go into 4.11.
>>=20
>> Yeah, to try to get a patch to 4.10 at this point needs to be a really
>> high profile regression. That is if Linus doesn't release 4.10 today, of
>> course.
> The symptoms are kernel crashes at least when operating in AP mode.
> It's pretty severe, so getting it into 4.10 would be preferable.

I'm confused now. Wasn't the airtime fairness patch queued for 4.11?

-Toke

Re: [PATCH] ath9k: clean up and fix ath_tx_count_airtime

[Felix Fietkau]

On 2017-02-12 17:36, Toke Høiland-Jørgensen wrote:
> Felix Fietkau <nbd@nbd.name> writes:
> 
>> On 2017-02-12 17:28, Kalle Valo wrote:
>>> Felix Fietkau <nbd@nbd.name> writes:
>>> 
>>>> On 2017-02-12 16:22, Toke Høiland-Jørgensen wrote:
>>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>> 
>>>>>> ath_tx_count_airtime is doing a lot of unnecessary work:
>>>>>>
>>>>>> - Redundant station lookup
>>>>>> - Redundant rcu_read_lock/unlock
>>>>>> - Useless memcpy of bf->rates
>>>>>> - Useless NULL check of bf->bf_mpdu
>>>>>> - Redundant lookup of the skb tid
>>>>>>
>>>>>> Additionally, it tries to look up the mac80211 queue index from the txq,
>>>>>> which fails if the frame was delivered via the power save queue.
>>> 
>>> What does this mean in practise, what's the user level impact?
>>> 
>>>>>> This patch fixes all of these issues by passing down the right set of
>>>>>> pointers instead of doing extra work
>>>>>>
>>>>>> Cc: stable@vger.kernel.org
>>>>>> Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling
>>>>>> between stations")
>>>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>>> 
>>>>> Not sure if there's anything for stable to do with this; don't think the
>>>>> airtime fairness code has gone into a release yet? Otherwise:
>>>>> 
>>>>> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>
>>>>
>>>> I added this, because I'm not sure this patch will make it to 4.10 in
>>>> time, since we're really close to a release. I assume this patch will
>>>> probably go into 4.11.
>>> 
>>> Yeah, to try to get a patch to 4.10 at this point needs to be a really
>>> high profile regression. That is if Linus doesn't release 4.10 today, of
>>> course.
>> The symptoms are kernel crashes at least when operating in AP mode.
>> It's pretty severe, so getting it into 4.10 would be preferable.
> 
> I'm confused now. Wasn't the airtime fairness patch queued for 4.11?
I'll check again, maybe I got the git-describe output wrong.

- Felix

Re: [PATCH] ath9k: clean up and fix ath_tx_count_airtime

[Toke Høiland-Jørgensen]

Felix Fietkau <nbd@nbd.name> writes:

> On 2017-02-12 17:36, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>>=20
>>> On 2017-02-12 17:28, Kalle Valo wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>=20
>>>>> On 2017-02-12 16:22, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>>>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>>>=20
>>>>>>> ath_tx_count_airtime is doing a lot of unnecessary work:
>>>>>>>
>>>>>>> - Redundant station lookup
>>>>>>> - Redundant rcu_read_lock/unlock
>>>>>>> - Useless memcpy of bf->rates
>>>>>>> - Useless NULL check of bf->bf_mpdu
>>>>>>> - Redundant lookup of the skb tid
>>>>>>>
>>>>>>> Additionally, it tries to look up the mac80211 queue index from the=
 txq,
>>>>>>> which fails if the frame was delivered via the power save queue.
>>>>=20
>>>> What does this mean in practise, what's the user level impact?
>>>>=20
>>>>>>> This patch fixes all of these issues by passing down the right set =
of
>>>>>>> pointers instead of doing extra work
>>>>>>>
>>>>>>> Cc: stable@vger.kernel.org
>>>>>>> Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling
>>>>>>> between stations")
>>>>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>>>>=20
>>>>>> Not sure if there's anything for stable to do with this; don't think=
 the
>>>>>> airtime fairness code has gone into a release yet? Otherwise:
>>>>>>=20
>>>>>> Acked-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk>
>>>>>
>>>>> I added this, because I'm not sure this patch will make it to 4.10 in
>>>>> time, since we're really close to a release. I assume this patch will
>>>>> probably go into 4.11.
>>>>=20
>>>> Yeah, to try to get a patch to 4.10 at this point needs to be a really
>>>> high profile regression. That is if Linus doesn't release 4.10 today, =
of
>>>> course.
>>> The symptoms are kernel crashes at least when operating in AP mode.
>>> It's pretty severe, so getting it into 4.10 would be preferable.
>>=20
>> I'm confused now. Wasn't the airtime fairness patch queued for 4.11?
> I'll check again, maybe I got the git-describe output wrong.

$ git describe --contains 63fefa050477
wireless-drivers-next-for-davem-2017-01-02~2^2~14

So I think we're good as long as this gets into the 4.11 cycle :)

-Toke

Re: [PATCH] ath9k: clean up and fix ath_tx_count_airtime

[Kalle Valo]

Felix Fietkau <nbd@nbd.name> writes:

> On 2017-02-12 17:28, Kalle Valo wrote:
>> Felix Fietkau <nbd@nbd.name> writes:
>>=20
>>> On 2017-02-12 16:22, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>>>> Felix Fietkau <nbd@nbd.name> writes:
>>>>=20
>>>>> ath_tx_count_airtime is doing a lot of unnecessary work:
>>>>>
>>>>> - Redundant station lookup
>>>>> - Redundant rcu_read_lock/unlock
>>>>> - Useless memcpy of bf->rates
>>>>> - Useless NULL check of bf->bf_mpdu
>>>>> - Redundant lookup of the skb tid
>>>>>
>>>>> Additionally, it tries to look up the mac80211 queue index from the t=
xq,
>>>>> which fails if the frame was delivered via the power save queue.
>>=20
>> What does this mean in practise, what's the user level impact?
>>=20
>>>>> This patch fixes all of these issues by passing down the right set of
>>>>> pointers instead of doing extra work
>>>>>
>>>>> Cc: stable@vger.kernel.org
>>>>> Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling
>>>>> between stations")
>>>>> Signed-off-by: Felix Fietkau <nbd@nbd.name>
>>>>=20
>>>> Not sure if there's anything for stable to do with this; don't think t=
he
>>>> airtime fairness code has gone into a release yet? Otherwise:
>>>>=20
>>>> Acked-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk>
>>>
>>> I added this, because I'm not sure this patch will make it to 4.10 in
>>> time, since we're really close to a release. I assume this patch will
>>> probably go into 4.11.
>>=20
>> Yeah, to try to get a patch to 4.10 at this point needs to be a really
>> high profile regression. That is if Linus doesn't release 4.10 today, of
>> course.
>
> The symptoms are kernel crashes at least when operating in AP mode.
> It's pretty severe, so getting it into 4.10 would be preferable.

Indeed, that's pretty severe :) But better to mention that in the commit
log, I'll add that during commit.

Like Toke said, luckily 63fefa050477 is not in 4.10 so I'll push this to
4.11.

--=20
Kalle Valo

Re: ath9k: clean up and fix ath_tx_count_airtime

[Kalle Valo]

Felix Fietkau <nbd@nbd.name> wrote:
> ath_tx_count_airtime is doing a lot of unnecessary work:
> 
> - Redundant station lookup
> - Redundant rcu_read_lock/unlock
> - Useless memcpy of bf->rates
> - Useless NULL check of bf->bf_mpdu
> - Redundant lookup of the skb tid
> 
> Additionally, it tries to look up the mac80211 queue index from the txq,
> which fails if the frame was delivered via the power save queue.
> 
> This patch fixes all of these issues by passing down the right set of
> pointers instead of doing extra work
> 
> Cc: stable@vger.kernel.org
> Fixes: 63fefa050477 ("ath9k: Introduce airtime fairness scheduling between stations")
> Signed-off-by: Felix Fietkau <nbd@nbd.name>
> Acked-by: Toke Høiland-Jørgensen <toke@toke.dk>

Patch applied to ath-next branch of ath.git, thanks.

a6e56d749f1b ath9k: clean up and fix ath_tx_count_airtime

-- 
https://patchwork.kernel.org/patch/9568369/

Documentation about submitting wireless patches and checking status
from patchwork:

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches