ath10k: Implement get_expected_throughput callback

PATCH In review Anilkumar Kolli 4 participants 8 messages Mar 23, 2018 – Apr 16, 2018 Qualcomm Atheros ATH10K wireless driver

lore

Share & tools

lore

Anilkumar Kolli

author

2018-03-23 14:06 UTC #0 lore raw

PATCH v2 1/1 ath10k: Implement get_expected_throughput callback

Enable support for 'drv_get_expected_throughput' callback. Export expected throughput if available to cfg80211/nl80211.

Signed-off-by Anilkumar Kolli <[email protected]>

Signed-off-by Tamizh chelvam <[email protected]>

---
v2:
- update the avg for all the transmitted frames(Sven)
- remove the unnecessary NL80211_STA_INFO_EXPECTED_THROUGHPUT update(Sven)

 drivers/net/wireless/ath/ath10k/core.h   |    5 +++++
 drivers/net/wireless/ath/ath10k/htt_rx.c |    4 ++++
 drivers/net/wireless/ath/ath10k/mac.c    |    9 +++++++++
 3 files changed, 18 insertions(+)

drivers/net/wireless/ath/ath10k/core.h +5 -0

index fe6b30356d3b..8e10f7d83a79 100644
@@ -1,6 +1,7 @@
 /*
  * Copyright (c) 2005-2011 Atheros Communications Inc.
  * Copyright (c) 2011-2017 Qualcomm Atheros, Inc.
+ * Copyright (c) 2018, The Linux Foundation. All rights reserved.
  *
  * Permission to use, copy, modify, and/or distribute this software for any
  * purpose with or without fee is hereby granted, provided that the above

@@ -37,6 +38,7 @@
 #include "thermal.h"
 #include "wow.h"
 #include "swap.h"
+#include <linux/average.h>
 
 #define MS(_v, _f) (((_v) & _f##_MASK) >> _f##_LSB)
 #define SM(_v, _f) (((_v) << _f##_LSB) & _f##_MASK)

@@ -354,6 +356,8 @@ struct ath10k_txq {
 	unsigned long num_push_allowed;
 };
 
+DECLARE_EWMA(sta_txrate, 4, 16)
+
 struct ath10k_sta {
 	struct ath10k_vif *arvif;

@@ -367,6 +371,7 @@ struct ath10k_sta {
 
 	struct work_struct update_wk;
 	u64 rx_duration;
+	struct ewma_sta_txrate ave_sta_txrate;
 
 #ifdef CONFIG_MAC80211_DEBUGFS
 	/* protected by conf_mutex */

drivers/net/wireless/ath/ath10k/htt_rx.c +4 -0

index 6d96f9560950..9fd8b22552c6 100644
@@ -2449,6 +2449,7 @@ static inline bool is_valid_legacy_rate(u8 rate)
 	struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
 	u8 rate = 0, sgi;
 	struct rate_info txrate;
+	u32 tx_bitrate;
 
 	lockdep_assert_held(&ar->data_lock);

@@ -2500,6 +2501,9 @@ static inline bool is_valid_legacy_rate(u8 rate)
 
 	arsta->txrate.nss = txrate.nss;
 	arsta->txrate.bw = txrate.bw + RATE_INFO_BW_20;
+
+	tx_bitrate = cfg80211_calculate_bitrate(&arsta->txrate);
+	ewma_sta_txrate_add(&arsta->ave_sta_txrate, tx_bitrate);
 }
 
 static void ath10k_htt_fetch_peer_stats(struct ath10k *ar,

drivers/net/wireless/ath/ath10k/mac.c +9 -0

index 7e02ca02b28e..33e790cf5c42 100644
@@ -7688,6 +7688,14 @@ static void ath10k_sta_statistics(struct ieee80211_hw *hw,
 	sinfo->filled |= 1ULL << NL80211_STA_INFO_TX_BITRATE;
 }
 
+static u32 ath10k_get_expected_throughput(struct ieee80211_hw *hw,
+					  struct ieee80211_sta *sta)
+{
+	struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+
+	return ewma_sta_txrate_read(&arsta->ave_sta_txrate);
+}
+
 static const struct ieee80211_ops ath10k_ops = {
 	.tx				= ath10k_mac_op_tx,
 	.wake_tx_queue			= ath10k_mac_op_wake_tx_queue,

@@ -7730,6 +7738,7 @@ static void ath10k_sta_statistics(struct ieee80211_hw *hw,
 	.switch_vif_chanctx		= ath10k_mac_op_switch_vif_chanctx,
 	.sta_pre_rcu_remove		= ath10k_mac_op_sta_pre_rcu_remove,
 	.sta_statistics			= ath10k_sta_statistics,
+	.get_expected_throughput	= ath10k_get_expected_throughput,
 
 	CFG80211_TESTMODE_CMD(ath10k_tm_cmd)

Signature · 1 line

1.7.9.5

Sven Eckelmann

2018-03-26 07:23 UTC #1 lore raw

Quoted 7 lines · On Freitag, 23. März 2018 19:37:14 CEST Anilkumar Kolli wrote:

drivers/net/wireless/ath/ath10k/mac.c +7 -0

+static u32 ath10k_get_expected_throughput(struct ieee80211_hw *hw,
+                                         struct ieee80211_sta *sta)
+{
+       struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+
+       return ewma_sta_txrate_read(&arsta->ave_sta_txrate);
+}

Quoted 30 lines · On Freitag, 23. März 2018 19:11:48 CEST [email protected] wrote:

Quoted 22 lines
Antonio and Felix, please correct me when this statement is incorrect.
The expected_throughput as initially implemented for minstrel(_ht) is not
about the raw physical bitrate but about the throughput which is expected for
things running on top of the wifi link. See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cca674d47e59665630f3005291b61bb883015fc5
for more details
when I interpret your change correctly then your it doesn't get the information about packet loss or aggregation and doesn't do anything convert
from raw physical rate to something the user could get see. It will just
overestimate the throughput for ath10k links and thus give wrong information
to routing algorithms. This could for example cause them to prefer links over
ath10k based hw when mt76 would actually provide a significant better throughput.
Quoted 8 lines
Beside that - why is the ave_sta_txrate only filled when with new information
when someone requests the current expected_throughput via get_expected_throughput. I would have expected that it is filled everytime you
get new information about the current rate from the firmware (ath10k_sta_statistics).

Quoted 1 line

Yes. ideally it should be doing the rate avg. of all the sent packets.

No, not the PHY rate average - but the "throughput avg". And the "ideally" here sounds a little bit like in "Our medical doctor would ideally not decapitate each patient but we have at least an MD".

Kind regards,
Sven

[email protected]

author

2018-03-28 06:11 UTC #2 lore raw

Quoted 40 lines · Sven Eckelmann

Quoted 7 lines · On Freitag, 23. März 2018 19:37:14 CEST Anilkumar Kolli wrote:
drivers/net/wireless/ath/ath10k/mac.c +7 -0
+static u32 ath10k_get_expected_throughput(struct ieee80211_hw *hw,
+                                         struct ieee80211_sta *sta)
+{
+       struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+
+       return ewma_sta_txrate_read(&arsta->ave_sta_txrate);
+}
Quoted 30 lines · On Freitag, 23. März 2018 19:11:48 CEST [email protected] wrote:
Quoted 30 lines
Antonio and Felix, please correct me when this statement is incorrect.
The expected_throughput as initially implemented for minstrel(_ht) is not
about the raw physical bitrate but about the throughput which is expected for
things running on top of the wifi link. See
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=cca674d47e59665630f3005291b61bb883015fc5
for more details
when I interpret your change correctly then your it doesn't get the information about packet loss or aggregation and doesn't do anything convert
from raw physical rate to something the user could get see. It will just
overestimate the throughput for ath10k links and thus give wrong information
to routing algorithms. This could for example cause them to prefer links over
ath10k based hw when mt76 would actually provide a significant better throughput.
Beside that - why is the ave_sta_txrate only filled when with new information
when someone requests the current expected_throughput via get_expected_throughput. I would have expected that it is filled everytime you
get new information about the current rate from the firmware (ath10k_sta_statistics).

Quoted 6 lines

Quoted 1 line
Yes. ideally it should be doing the rate avg. of all the sent packets.
No, not the PHY rate average - but the "throughput avg". And the "ideally" here sounds a little bit like in "Our medical doctor would ideally not decapitate each patient but we have at least an MD".

The rate average and throughput are relative. no?

- Anil.

Sven Eckelmann

2018-03-28 06:38 UTC #3 lore raw

On Mittwoch, 28. März 2018 11:41:51 CEST [email protected] wrote:
[...]

Quoted 1 line

The rate average and throughput are relative. no?

In the sense that rate has a tendency to affect the expected throughput - yes. But it is not like the expected throughput perfectly correlates with the rate and you only have to multiply with a factor X (which seems to be missing in your code) to get from rate to expected throughput. The average packet loss for this selected rate, interframe spacing and the aggregation are important factors here too.

But of course, I cannot say much about how the rate control from QCA works and in which form these information are already available.

If you want to know the average PHY rate then wouldn't it be better to report the rates to one of the upper layers and let them to the averaging? Similar to what there already is for NL80211_STA_INFO_CHAIN_SIGNAL (NL80211_STA_INFO_CHAIN_SIGNAL_AVG) just for NL80211_STA_INFO_TX_BITRATE/ NL80211_STA_INFO_RX_BITRATE. Not sure whether this makes sense or whether someone has an use-case for it.

Kind regards,
Sven

Peter Oh

2018-04-13 02:22 UTC #4 lore raw

Hi Anilkumar,

Quoted 3 lines · Sven Eckelmann

On Mittwoch, 28. März 2018 11:41:51 CEST [email protected] wrote:
[...]
Quoted 1 line
The rate average and throughput are relative. no?

Can you share the output number from your new function?
It may help us to understand a little bit more how well the new function works.

Thanks,
Peter

Kalle Valo

2018-04-13 13:48 UTC #5 lore raw

In reply to Sven Eckelmann

Quoted 5 lines · Sven Eckelmann

On Mittwoch, 28. M=C3=A4rz 2018 11:41:51 CEST [email protected] wrote:
[...]
Quoted 1 line
The rate average and throughput are relative. no?
In the sense that rate has a tendency to affect the expected throughput -=

yes.=20

Quoted 1 line

But it is not like the expected throughput perfectly correlates with the =

rate=20

Quoted 1 line

and you only have to multiply with a factor X (which seems to be missing =

in=20

Quoted 1 line

your code) to get from rate to expected throughput. The average packet lo=

ss=20

Quoted 1 line

for this selected rate, interframe spacing and the aggregation are import=

ant=20

Quoted 1 line

factors here too.

I doubt we can get that information from the firmware so should we just go with the "multiple with X" method? What would be good X?

Quoted 1 line

But of course, I cannot say much about how the rate control from QCA work=

s and=20

Quoted 3 lines

in which form these information are already available.
If you want to know the average PHY rate then wouldn't it be better to re=

port=20

Quoted 1 line

the rates to one of the upper layers and let them to the averaging? Simil=

ar to=20

Quoted 3 lines

what there already is for NL80211_STA_INFO_CHAIN_SIGNAL=20 (NL80211_STA_INFO_CHAIN_SIGNAL_AVG) just for NL80211_STA_INFO_TX_BITRATE/ NL80211_STA_INFO_RX_BITRATE. Not sure whether this makes sense or whether=

=20

Quoted 1 line

someone has an use-case for it.

Sounds like a good idea, but I don't see it preventing to apply this patch. We can always change the implementation later as this is just communication between ath10k and mac80211, right?

--=20
Kalle Valo

Peter Oh

2018-04-13 20:24 UTC #6 lore raw

Quoted 11 lines · Kalle Valo

Quoted 9 lines · Sven Eckelmann
But of course, I cannot say much about how the rate control from QCA works and in which form these information are already available.
If you want to know the average PHY rate then wouldn't it be better to report the rates to one of the upper layers and let them to the averaging? Similar to what there already is for NL80211_STA_INFO_CHAIN_SIGNAL (NL80211_STA_INFO_CHAIN_SIGNAL_AVG) just for NL80211_STA_INFO_TX_BITRATE/ NL80211_STA_INFO_RX_BITRATE. Not sure whether this makes sense or whether someone has an use-case for it.

Quoted 4 lines

Sounds like a good idea, but I don't see it preventing to apply this patch. We can always change the implementation later as this is just communication between ath10k and mac80211, right?

I agree with Sven on the usage or expectation of get_expected_throughput cabllback.
It's not really ab expected throughput implementation.
However I'm with Kalle about approving this patch as Sven also mentioned "here sounds a little bit like in "Our medical doctor would ideally not decapitate each patient but we have at least an MD"". I could improve it once merged since there are more members in ath10k_per_peer_tx_stats useful such as succ_bytes, failed_bytes, duration, and etc.

[email protected]

author

2018-04-16 06:30 UTC #7 lore raw

Quoted 16 lines · Peter Oh

Quoted 11 lines · Kalle Valo
Quoted 9 lines · Sven Eckelmann
But of course, I cannot say much about how the rate control from QCA works and in which form these information are already available.
If you want to know the average PHY rate then wouldn't it be better to report the rates to one of the upper layers and let them to the averaging? Similar to what there already is for NL80211_STA_INFO_CHAIN_SIGNAL (NL80211_STA_INFO_CHAIN_SIGNAL_AVG) just for NL80211_STA_INFO_TX_BITRATE/ NL80211_STA_INFO_RX_BITRATE. Not sure whether this makes sense or whether someone has an use-case for it.
Quoted 4 lines
Sounds like a good idea, but I don't see it preventing to apply this patch. We can always change the implementation later as this is just communication between ath10k and mac80211, right?

Quoted 9 lines

I agree with Sven on the usage or expectation of get_expected_throughput cabllback.
It's not really ab expected throughput implementation. However I'm with Kalle about approving this patch as Sven also mentioned "here sounds a little bit like in "Our medical doctor would ideally not decapitate each patient but we have at least an MD"". I could improve it once merged since there are more members in ath10k_per_peer_tx_stats useful such as succ_bytes, failed_bytes, duration, and etc.

On each packet sent successfully, driver has the success_bytes details. Throughput calculation can be done using these bytes and tx rate (tx_rate * bytes), send this average value to mac80211. is this you are thinking of?

Thanks,
Anil.