[PATCH] ath9k: switch to rate table based lookup

[PATCH] ath9k: switch to rate table based lookup

[Jonas Jelonek]

This patch changes mac80211 rate control for the ath9k driver.
The rate lookup per packet is changed from legacy usage of
ieee80211_get_tx_rates() to the new rate table based lookup
in struct ieee80211_sta.rates.

Co-developed-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
Co-developed-by: Thomas Huehn <thomas.huehn@hs-nordhausen.de>
Signed-off-by: Thomas Huehn <thomas.huehn@hs-nordhausen.de>
---
 drivers/net/wireless/ath/ath9k/xmit.c | 45 +++++++++++++++++++++++++--
 1 file changed, 43 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath9k/xmit.c b/drivers/net/wireless/ath/ath9k/xmit.c
index 5691bd6eb82c..d0caf1de2bde 100644
--- a/drivers/net/wireless/ath/ath9k/xmit.c
+++ b/drivers/net/wireless/ath/ath9k/xmit.c
@@ -154,11 +154,52 @@ static void ath_send_bar(struct ath_atx_tid *tid, u16 seqno)
 			   seqno << IEEE80211_SEQ_SEQ_SHIFT);
 }
 
+static bool ath_merge_ratetbl(struct ieee80211_sta *sta, struct ath_buf *bf,
+			      struct ieee80211_tx_info *tx_info)
+{
+	struct ieee80211_sta_rates *ratetbl;
+	u8 i;
+
+	if (!sta)
+		return false;
+
+	ratetbl = rcu_dereference(sta->rates);
+	if (!ratetbl)
+		return false;
+
+	if (tx_info->control.rates[0].idx < 0 ||
+	    tx_info->control.rates[0].count == 0)
+	{
+		i = 0;
+	} else {
+		bf->rates[0] = tx_info->control.rates[0];
+		i = 1;
+	}
+
+	for ( ; i < IEEE80211_TX_MAX_RATES; i++) {
+		bf->rates[i].idx = ratetbl->rate[i].idx;
+		bf->rates[i].flags = ratetbl->rate[i].flags;
+		if (tx_info->control.use_rts)
+			bf->rates[i].count = ratetbl->rate[i].count_rts;
+		else if (tx_info->control.use_cts_prot)
+			bf->rates[i].count = ratetbl->rate[i].count_cts;
+		else
+			bf->rates[i].count = ratetbl->rate[i].count;
+	}
+
+	return true;
+}
+
 static void ath_set_rates(struct ieee80211_vif *vif, struct ieee80211_sta *sta,
 			  struct ath_buf *bf)
 {
-	ieee80211_get_tx_rates(vif, sta, bf->bf_mpdu, bf->rates,
-			       ARRAY_SIZE(bf->rates));
+	struct ieee80211_tx_info *tx_info;
+
+	tx_info = IEEE80211_SKB_CB(bf->bf_mpdu);
+
+	if (!ath_merge_ratetbl(sta, bf, tx_info))
+		ieee80211_get_tx_rates(vif, sta, bf->bf_mpdu, bf->rates,
+				       ARRAY_SIZE(bf->rates));
 }
 
 static void ath_txq_skb_done(struct ath_softc *sc, struct ath_txq *txq,
-- 
2.30.2

Re: [PATCH] ath9k: switch to rate table based lookup

[Toke Høiland-Jørgensen]

Jonas Jelonek <jelonek.jonas@gmail.com> writes:

> This patch changes mac80211 rate control for the ath9k driver.
> The rate lookup per packet is changed from legacy usage of
> ieee80211_get_tx_rates() to the new rate table based lookup
> in struct ieee80211_sta.rates.

What's the practical implication of this? Performance benefits, better
rates selected, or what? Got any benchmark numbers?

> Co-developed-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> Signed-off-by: Jonas Jelonek <jelonek.jonas@gmail.com>
> Co-developed-by: Thomas Huehn <thomas.huehn@hs-nordhausen.de>
> Signed-off-by: Thomas Huehn <thomas.huehn@hs-nordhausen.de>

You don't generally need a co-developed-by for yourself, and your S-o-b
should go at the end when you're the submitter...

-Toke

Re: [PATCH] ath9k: switch to rate table based lookup

[Jonas Jelonek]

> What's the practical implication of this? Performance benefits, better
> rates selected, or what? Got any benchmark numbers?

We're planning to annotate and implement 'transmit power control' per
packet / per MRR
and to improve the rate control API in mac80211 to support newer hardware.
SKB->CB is limited in space, tx power annotation also does not fit in
there. Future
perspective is that rate control won't use SKB->CB anymore, neither
for normal rate
setting nor for probing.

The new rate control API (introduced with commit
0d528d85c519b755b6f4e1bafa3a39984370e1c1) allows drivers to directly
get rates from sta->rates. This is not used by every driver yet,
ieee80211_get_tx_rates performs the
translation/merge for the drivers.
The call to ieee80211_get_tx_rates and subsequent calls in ath9k can
be avoided by directly fetching rates from sta->rates. This may also
improve performance.
ath9k does not expect rates in SKB->CB, therefore table merge does not
need to consider this
(except for first rate in SKB->CB for probing).

> You don't generally need a co-developed-by for yourself, and your S-o-b
> should go at the end when you're the submitter...

will fix this in V2.

Re: [PATCH] ath9k: switch to rate table based lookup

[Kalle Valo]

Toke Høiland-Jørgensen <toke@redhat.com> writes:

> Jonas Jelonek <jelonek.jonas@gmail.com> writes:
>
>> This patch changes mac80211 rate control for the ath9k driver.
>> The rate lookup per packet is changed from legacy usage of
>> ieee80211_get_tx_rates() to the new rate table based lookup
>> in struct ieee80211_sta.rates.
>
> What's the practical implication of this? Performance benefits, better
> rates selected, or what? Got any benchmark numbers?

And how did you test this? Are both PCI and USB devices affected?

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH] ath9k: switch to rate table based lookup

[Toke Høiland-Jørgensen]

Jonas Jelonek <jelonek.jonas@gmail.com> writes:

>> What's the practical implication of this? Performance benefits, better
>> rates selected, or what? Got any benchmark numbers?
>
> We're planning to annotate and implement 'transmit power control' per
> packet / per MRR
> and to improve the rate control API in mac80211 to support newer
> hardware.

Ah, great!

> SKB->CB is limited in space, tx power annotation also does not fit in
> there. Future
> perspective is that rate control won't use SKB->CB anymore, neither
> for normal rate
> setting nor for probing.
>
> The new rate control API (introduced with commit
> 0d528d85c519b755b6f4e1bafa3a39984370e1c1) allows drivers to directly
> get rates from sta->rates. This is not used by every driver yet,
> ieee80211_get_tx_rates performs the
> translation/merge for the drivers.
> The call to ieee80211_get_tx_rates and subsequent calls in ath9k can
> be avoided by directly fetching rates from sta->rates. This may also
> improve performance.
> ath9k does not expect rates in SKB->CB, therefore table merge does not
> need to consider this
> (except for first rate in SKB->CB for probing).

Please put something like the above explanation in the commit message so
people can understand the context. And as Kalle pointed out, some
information on how you tested this is also needed, and can go into the
commit message as well :)

-Toke

Re: [PATCH] ath9k: switch to rate table based lookup

[Kalle Valo]

Jonas Jelonek <jelonek.jonas@gmail.com> writes:

>> And how did you test this? Are both PCI and USB devices affected?
>
> I tested this on a 8devices Rambutan with QCA9558 SoC, but didn‘t
> explicitly test this with a USB device. I am not sure whether the
> ath9k_htc is affected. First I tested this without the patch to get a
> reference for comparison. I connected three devices via WiFi 2.4GHz
> and 5GHz, generated traffic multiple times with iperf3 and captured
> the rc_stats for each station. Then I applied the patch and did the
> same again. The throughput was overall the same like without the
> patch, compared to the first run of each station. Rate selection
> worked fine, the stats were nearly identical, same rates selected in
> both runs.

Thanks. Can someone review this from ath9k_htc point of view?

Also please don't use HTML in your emails, our lists drop all HTML
mails.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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

Re: [PATCH] ath9k: switch to rate table based lookup

[Toke Høiland-Jørgensen]

Kalle Valo <kvalo@codeaurora.org> writes:

> Jonas Jelonek <jelonek.jonas@gmail.com> writes:
>
>>> And how did you test this? Are both PCI and USB devices affected?
>>
>> I tested this on a 8devices Rambutan with QCA9558 SoC, but didn‘t
>> explicitly test this with a USB device. I am not sure whether the
>> ath9k_htc is affected. First I tested this without the patch to get a
>> reference for comparison. I connected three devices via WiFi 2.4GHz
>> and 5GHz, generated traffic multiple times with iperf3 and captured
>> the rc_stats for each station. Then I applied the patch and did the
>> same again. The throughput was overall the same like without the
>> patch, compared to the first run of each station. Rate selection
>> worked fine, the stats were nearly identical, same rates selected in
>> both runs.
>
> Thanks. Can someone review this from ath9k_htc point of view?

Pretty sure ath9k_htc devices do rate control in the firmware. Certainly
ath9k_htc sets the HAS_RATE_CONTROL flag in mac80211, and the only calls
to ath_set_rates are from within xmit.c, which is not used by ath9k_htc.
So I think we're fine as far as that is concerned...

-Toke

Re: [PATCH] ath9k: switch to rate table based lookup

[Kalle Valo]

Toke Høiland-Jørgensen <toke@redhat.com> writes:

> Kalle Valo <kvalo@codeaurora.org> writes:
>
>> Jonas Jelonek <jelonek.jonas@gmail.com> writes:
>>
>>>> And how did you test this? Are both PCI and USB devices affected?
>>>
>>> I tested this on a 8devices Rambutan with QCA9558 SoC, but didn‘t
>>> explicitly test this with a USB device. I am not sure whether the
>>> ath9k_htc is affected. First I tested this without the patch to get a
>>> reference for comparison. I connected three devices via WiFi 2.4GHz
>>> and 5GHz, generated traffic multiple times with iperf3 and captured
>>> the rc_stats for each station. Then I applied the patch and did the
>>> same again. The throughput was overall the same like without the
>>> patch, compared to the first run of each station. Rate selection
>>> worked fine, the stats were nearly identical, same rates selected in
>>> both runs.
>>
>> Thanks. Can someone review this from ath9k_htc point of view?
>
> Pretty sure ath9k_htc devices do rate control in the firmware. Certainly
> ath9k_htc sets the HAS_RATE_CONTROL flag in mac80211, and the only calls
> to ath_set_rates are from within xmit.c, which is not used by ath9k_htc.
> So I think we're fine as far as that is concerned...

Very good, thanks for checking.

-- 
https://patchwork.kernel.org/project/linux-wireless/list/

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