2021-12-15 21:52:14

by Jonas Jelonek

[permalink] [raw]
Subject: [PATCH] ath5k: switch to rate table based lookup

Switching from legacy usage of ieee80211_get_tx_rates() lookup to direct
rate table lookup in struct ieee80211_sta->rates.

The current rate control API allows drivers to directly get rates from
ieee80211_sta->rates. ath5k is currently one of the legacy drivers that
perform translation/merge with the internal rate table via
ieee80211_get_tx_rates provided by rate control API.
For our upcoming changes to rate control API and the implementation of
transmit power control, this patch changes the behaviour. The call to
ieee80211_get_tx_rates and subsequent calls are also avoided. ath5k now
directly reads rates from sta->rates into its internal rate table. Cause
ath5k does not rely on the rate array in SKB->CB, this is not considered
anymore except for the first entry (used for probing).

Tested this on a PCEngines ALIX with CMP9-GP miniPCI wifi card (Atheros
AR5213A). Generated traffic between AP and multiple STAs before and
after applying the patch and simultaneously measured throughput and
captured rc_stats. Comparison resulted in same rate selection and no
performance loss between both runs.

Co-developed-by: Thomas Huehn <[email protected]>
Signed-off-by: Thomas Huehn <[email protected]>
Signed-off-by: Jonas Jelonek <[email protected]>
---
drivers/net/wireless/ath/ath5k/base.c | 44 +++++++++++++++++++++++++--
1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index cef17f33c69e..647b3b76495c 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -727,6 +727,43 @@ ath5k_get_rate_hw_value(const struct ieee80211_hw *hw,
return hw_rate;
}

+static bool ath5k_merge_ratetbl(struct ieee80211_sta *sta,
+ struct ath5k_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 int
ath5k_txbuf_setup(struct ath5k_hw *ah, struct ath5k_buf *bf,
struct ath5k_txq *txq, int padsize,
@@ -753,8 +790,11 @@ ath5k_txbuf_setup(struct ath5k_hw *ah, struct ath5k_buf *bf,
if (dma_mapping_error(ah->dev, bf->skbaddr))
return -ENOSPC;

- ieee80211_get_tx_rates(info->control.vif, (control) ? control->sta : NULL, skb, bf->rates,
- ARRAY_SIZE(bf->rates));
+ if (!ath5k_merge_ratetbl((control) ? control->sta : NULL, bf, info)) {
+ ieee80211_get_tx_rates(info->control.vif,
+ (control) ? control->sta : NULL, skb, bf->rates,
+ ARRAY_SIZE(bf->rates));
+ }

rate = ath5k_get_rate(ah->hw, info, bf, 0);

--
2.30.2



2021-12-16 16:14:41

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] ath5k: switch to rate table based lookup

Jonas Jelonek <[email protected]> writes:

> Switching from legacy usage of ieee80211_get_tx_rates() lookup to direct
> rate table lookup in struct ieee80211_sta->rates.
>
> The current rate control API allows drivers to directly get rates from
> ieee80211_sta->rates. ath5k is currently one of the legacy drivers that
> perform translation/merge with the internal rate table via
> ieee80211_get_tx_rates provided by rate control API.
> For our upcoming changes to rate control API and the implementation of
> transmit power control, this patch changes the behaviour. The call to
> ieee80211_get_tx_rates and subsequent calls are also avoided. ath5k now
> directly reads rates from sta->rates into its internal rate table. Cause
> ath5k does not rely on the rate array in SKB->CB, this is not considered
> anymore except for the first entry (used for probing).
>
> Tested this on a PCEngines ALIX with CMP9-GP miniPCI wifi card (Atheros
> AR5213A). Generated traffic between AP and multiple STAs before and
> after applying the patch and simultaneously measured throughput and
> captured rc_stats. Comparison resulted in same rate selection and no
> performance loss between both runs.

Good that you explained how you tested this.

> @@ -753,8 +790,11 @@ ath5k_txbuf_setup(struct ath5k_hw *ah, struct ath5k_buf *bf,
> if (dma_mapping_error(ah->dev, bf->skbaddr))
> return -ENOSPC;
>
> - ieee80211_get_tx_rates(info->control.vif, (control) ? control->sta : NULL, skb, bf->rates,
> - ARRAY_SIZE(bf->rates));
> + if (!ath5k_merge_ratetbl((control) ? control->sta : NULL, bf, info)) {
> + ieee80211_get_tx_rates(info->control.vif,
> + (control) ? control->sta : NULL, skb, bf->rates,
> + ARRAY_SIZE(bf->rates));
> + }

I'm not really a fan of ? operator, so in the pending branch I changed
this to use a normal if statement. Please double check that I didn't
break anything.

https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=9e7c92df342f884434ccc8635d2606c1d1b11ef9

diff --git a/drivers/net/wireless/ath/ath5k/base.c b/drivers/net/wireless/ath/ath5k/base.c
index 647b3b76495c..66d123f48085 100644
--- a/drivers/net/wireless/ath/ath5k/base.c
+++ b/drivers/net/wireless/ath/ath5k/base.c
@@ -774,6 +774,7 @@ ath5k_txbuf_setup(struct ath5k_hw *ah, struct ath5k_buf *bf,
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
unsigned int pktlen, flags, keyidx = AR5K_TXKEYIX_INVALID;
struct ieee80211_rate *rate;
+ struct ieee80211_sta *sta;
unsigned int mrr_rate[3], mrr_tries[3];
int i, ret;
u16 hw_rate;
@@ -790,10 +791,15 @@ ath5k_txbuf_setup(struct ath5k_hw *ah, struct ath5k_buf *bf,
if (dma_mapping_error(ah->dev, bf->skbaddr))
return -ENOSPC;

- if (!ath5k_merge_ratetbl((control) ? control->sta : NULL, bf, info)) {
+ if (control)
+ sta = control->sta;
+ else
+ sta = NULL;
+
+ if (!ath5k_merge_ratetbl(sta, bf, info)) {
ieee80211_get_tx_rates(info->control.vif,
- (control) ? control->sta : NULL, skb, bf->rates,
- ARRAY_SIZE(bf->rates));
+ sta, skb, bf->rates,
+ ARRAY_SIZE(bf->rates));
}

rate = ath5k_get_rate(ah->hw, info, bf, 0);

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

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

2021-12-16 17:01:31

by Jonas Jelonek

[permalink] [raw]
Subject: Re: [PATCH] ath5k: switch to rate table based lookup

Thanks for your review!

Kalle Valo <[email protected]> writes:

> I'm not really a fan of ? operator, so in the pending branch I changed
> this to use a normal if statement. Please double check that I didn't
> break anything.
>
> https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/ath.git/commit/?h=pending&id=9e7c92df342f884434ccc8635d2606c1d1b11ef9

Checked this just a few moments ago, still works fine.

2021-12-20 16:02:35

by Kalle Valo

[permalink] [raw]
Subject: Re: [PATCH] ath5k: switch to rate table based lookup

Jonas Jelonek <[email protected]> wrote:

> Switching from legacy usage of ieee80211_get_tx_rates() lookup to direct
> rate table lookup in struct ieee80211_sta->rates.
>
> The current rate control API allows drivers to directly get rates from
> ieee80211_sta->rates. ath5k is currently one of the legacy drivers that
> perform translation/merge with the internal rate table via
> ieee80211_get_tx_rates provided by rate control API.
> For our upcoming changes to rate control API and the implementation of
> transmit power control, this patch changes the behaviour. The call to
> ieee80211_get_tx_rates and subsequent calls are also avoided. ath5k now
> directly reads rates from sta->rates into its internal rate table. Cause
> ath5k does not rely on the rate array in SKB->CB, this is not considered
> anymore except for the first entry (used for probing).
>
> Tested this on a PCEngines ALIX with CMP9-GP miniPCI wifi card (Atheros
> AR5213A). Generated traffic between AP and multiple STAs before and
> after applying the patch and simultaneously measured throughput and
> captured rc_stats. Comparison resulted in same rate selection and no
> performance loss between both runs.
>
> Co-developed-by: Thomas Huehn <[email protected]>
> Signed-off-by: Thomas Huehn <[email protected]>
> Signed-off-by: Jonas Jelonek <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>

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

a5d862da9105 ath5k: switch to rate table based lookup

--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/

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