[PATCH] ath10k: Implement get_expected_throughput callback

[PATCH] ath10k: Implement get_expected_throughput callback

[Anilkumar Kolli]

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>
---
 drivers/net/wireless/ath/ath10k/core.h |    5 +++++
 drivers/net/wireless/ath/ath10k/mac.c  |   17 +++++++++++++++++
 2 files changed, 22 insertions(+)

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/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index 7e02ca02b28e..e04fbe9ccfee 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -7686,6 +7686,22 @@ static void ath10k_sta_statistics(struct ieee80211_hw *hw,
 	}
 	sinfo->txrate.flags = arsta->txrate.flags;
 	sinfo->filled |= 1ULL << NL80211_STA_INFO_TX_BITRATE;
+
+	sinfo->expected_throughput =
+				ewma_sta_txrate_read(&arsta->ave_sta_txrate);
+	sinfo->filled |= BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT);
+}
+
+static u32 ath10k_get_expected_throughput(struct ieee80211_hw *hw,
+					  struct ieee80211_sta *sta)
+{
+	struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
+	u32 tx_bitrate;
+
+	tx_bitrate = cfg80211_calculate_bitrate(&arsta->txrate);
+	ewma_sta_txrate_add(&arsta->ave_sta_txrate, tx_bitrate);
+
+	return ewma_sta_txrate_read(&arsta->ave_sta_txrate);
 }
 
 static const struct ieee80211_ops ath10k_ops = {
@@ -7730,6 +7746,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)
 
-- 
1.7.9.5

Re: [PATCH] ath10k: Implement get_expected_throughput callback

[Sven Eckelmann]

[-- Attachment #1: Type: text/plain, Size: 2407 bytes --]

On Freitag, 23. März 2018 13:07:00 CET 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;
> +       u32 tx_bitrate;
> +
> +       tx_bitrate = cfg80211_calculate_bitrate(&arsta->txrate);
> +       ewma_sta_txrate_add(&arsta->ave_sta_txrate, tx_bitrate);
> +
> +       return ewma_sta_txrate_read(&arsta->ave_sta_txrate);
>  }

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

> @@ -7686,6 +7686,22 @@ static void ath10k_sta_statistics(struct ieee80211_hw *hw,
>         }
>         sinfo->txrate.flags = arsta->txrate.flags;
>         sinfo->filled |= 1ULL << NL80211_STA_INFO_TX_BITRATE;
> +
> +       sinfo->expected_throughput =
> +                               ewma_sta_txrate_read(&arsta->ave_sta_txrate);
> +       sinfo->filled |= BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT);
> +}

This brings me directly to the next point. Why are you changing these values 
here? Isn't sta_set_sinfo is expected to do that? This is at least what we
expect how it works when we call cfg80211_get_station() in batman-adv.

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH] ath10k: Implement get_expected_throughput callback

[akolli]

Hi Sven,

Thanks for the review.

On 2018-03-23 13:39, Sven Eckelmann wrote:
> On Freitag, 23. März 2018 13:07:00 CET 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;
>> +       u32 tx_bitrate;
>> +
>> +       tx_bitrate = cfg80211_calculate_bitrate(&arsta->txrate);
>> +       ewma_sta_txrate_add(&arsta->ave_sta_txrate, tx_bitrate);
>> +
>> +       return ewma_sta_txrate_read(&arsta->ave_sta_txrate);
>>  }
> 
> 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.

>> @@ -7686,6 +7686,22 @@ static void ath10k_sta_statistics(struct 
>> ieee80211_hw *hw,
>>         }
>>         sinfo->txrate.flags = arsta->txrate.flags;
>>         sinfo->filled |= 1ULL << NL80211_STA_INFO_TX_BITRATE;
>> +
>> +       sinfo->expected_throughput =
>> +                               
>> ewma_sta_txrate_read(&arsta->ave_sta_txrate);
>> +       sinfo->filled |= BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT);
>> +}
> 
> This brings me directly to the next point. Why are you changing these 
> values
> here? Isn't sta_set_sinfo is expected to do that? This is at least what 
> we
> expect how it works when we call cfg80211_get_station() in batman-adv.
> 

Yes. This looks redundant, I will remove this line,
  sinfo->filled |= BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT);

I will make these changes and send v2.

Thanks,
Anil.

Re: [PATCH] ath10k: Implement get_expected_throughput callback

[Johannes Berg]

On Fri, 2018-03-23 at 19:11 +0530, akolli@codeaurora.org wrote:
> 
> > > +
> > > +       sinfo->expected_throughput =
> > > +                               
> > > ewma_sta_txrate_read(&arsta->ave_sta_txrate);
> > > +       sinfo->filled |=
> > > BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT);
> > > +}

> Yes. This looks redundant, I will remove this line,
>   sinfo->filled |= BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT);

You don't need any of the changes here in ath10k_sta_statistics().

johannes

Re: [PATCH] ath10k: Implement get_expected_throughput callback

[akolli]

On 2018-03-23 19:14, Johannes Berg wrote:
> On Fri, 2018-03-23 at 19:11 +0530, akolli@codeaurora.org wrote:
>> 
>> > > +
>> > > +       sinfo->expected_throughput =
>> > > +
>> > > ewma_sta_txrate_read(&arsta->ave_sta_txrate);
>> > > +       sinfo->filled |=
>> > > BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT);
>> > > +}
> 
>> Yes. This looks redundant, I will remove this line,
>>   sinfo->filled |= BIT(NL80211_STA_INFO_EXPECTED_THROUGHPUT);
> 
> You don't need any of the changes here in ath10k_sta_statistics().

Hi Jaohannes,

Yes. I removed this in ath10k driver and sent a v2 patch.

Thanks,
Anil.