diff mbox

[v2] ath10k: Implement get_expected_throughput callback

Message ID 1521814034-17880-1-git-send-email-akolli@codeaurora.org (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

Anilkumar Kolli March 23, 2018, 2:07 p.m. UTC 
Enable support for 'drv_get_expected_throughput' callback.
Export expected throughput if available to cfg80211/nl80211.

Signed-off-by: Anilkumar Kolli <akolli@codeaurora.org>
Signed-off-by: Tamizh chelvam <tamizhr@codeaurora.org>
---
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(+)

Comments

Sven Eckelmann March 26, 2018, 7:22 a.m. UTC | #1 
On Freitag, 23. März 2018 19:37:14 CEST Anilkumar Kolli wrote:
> +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);
> +}

On Freitag, 23. März 2018 19:11:48 CEST akolli@codeaurora.org wrote:
> > 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).
> >
> 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
Anilkumar Kolli March 28, 2018, 6:11 a.m. UTC | #2 
On 2018-03-26 12:52, Sven Eckelmann wrote:
> On Freitag, 23. März 2018 19:37:14 CEST Anilkumar Kolli wrote:
>> +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);
>> +}
> 
> On Freitag, 23. März 2018 19:11:48 CEST akolli@codeaurora.org wrote:
>> > 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).
>> >
>> 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 March 28, 2018, 6:37 a.m. UTC | #3 
On Mittwoch, 28. März 2018 11:41:51 CEST akolli@codeaurora.org wrote:
[...]
> 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 April 13, 2018, 2:22 a.m. UTC | #4 
Hi Anilkumar,


On 03/27/2018 11:37 PM, Sven Eckelmann wrote:
> On Mittwoch, 28. März 2018 11:41:51 CEST akolli@codeaurora.org wrote:
> [...]
>> 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 April 13, 2018, 1:48 p.m. UTC | #5 
Sven Eckelmann <sven.eckelmann@openmesh.com> writes:

> On Mittwoch, 28. März 2018 11:41:51 CEST akolli@codeaurora.org wrote:
> [...]
>> 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.

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?

> 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.

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?
Peter Oh April 13, 2018, 8:24 p.m. UTC | #6 
On 04/13/2018 06:48 AM, Kalle Valo wrote:
> Sven Eckelmann <sven.eckelmann@openmesh.com> writes:
>
>> 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.
> 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.
Anilkumar Kolli April 16, 2018, 6:30 a.m. UTC | #7 
On 2018-04-14 01:54, Peter Oh wrote:
> On 04/13/2018 06:48 AM, Kalle Valo wrote:
>> Sven Eckelmann <sven.eckelmann@openmesh.com> writes:
>> 
>>> 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.
>> 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.

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.
diff mbox

Patch

diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
index fe6b30356d3b..8e10f7d83a79 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -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 */
diff --git a/drivers/net/wireless/ath/ath10k/htt_rx.c b/drivers/net/wireless/ath/ath10k/htt_rx.c
index 6d96f9560950..9fd8b22552c6 100644
--- a/drivers/net/wireless/ath/ath10k/htt_rx.c
+++ b/drivers/net/wireless/ath/ath10k/htt_rx.c
@@ -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,
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 7e02ca02b28e..33e790cf5c42 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -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)