Re: [PATCH] ath10k: Implement get_expected_throughput callback

[akolli@codeaurora.org]

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.