Search Linux Wireless

Re: [PATCH] ath10k: Implement get_expected_throughput callback

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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.



[Index of Archives]     [Linux Host AP]     [ATH6KL]     [Linux Wireless Personal Area Network]     [Linux Bluetooth]     [Wireless Regulations]     [Linux Netdev]     [Kernel Newbies]     [Linux Kernel]     [IDE]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Hiking]     [MIPS Linux]     [ARM Linux]     [Linux RAID]

  Powered by Linux