| Message ID | 1555489045-18070-1-git-send-email-leiwa@codeaurora.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Kalle Valo |
| Headers | show |
| Series | [v2] ath10k: fix different tx duration output | expand |
Lei Wang <leiwa@codeaurora.org> writes: > TX duration output of tx_stats in debugfs and station dump had big > difference because they got tx duration value from different statistic > data. We should use the same statistic data. So are you sure you picked the most accurate one of the two? :) -Toke
On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote: > Lei Wang <leiwa@codeaurora.org> writes: > >> TX duration output of tx_stats in debugfs and station dump had big >> difference because they got tx duration value from different statistic >> data. We should use the same statistic data. > > So are you sure you picked the most accurate one of the two? :) > > -Toke Hi Toke, Yes. Now for ath10k, there are two ways to get tx duration output. One is got from tx_stats in debugfs reported by firmware. It is a total value including all the frames which created by host and firmware sent to the peer. And the second is calculated from ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here the tx duration just includes the data frames sent from host to the peer. So the first value is preferable for station dump. Thanks. Lei
leiwa@codeaurora.org writes: > On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote: >> Lei Wang <leiwa@codeaurora.org> writes: >> >>> TX duration output of tx_stats in debugfs and station dump had big >>> difference because they got tx duration value from different statistic >>> data. We should use the same statistic data. >> >> So are you sure you picked the most accurate one of the two? :) >> >> -Toke > > Hi Toke, > > Yes. > Now for ath10k, there are two ways to get tx duration output. > One is got from tx_stats in debugfs reported by firmware. It is a total > value including all the frames which created by host and firmware sent > to the peer. > And the second is calculated from > ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here > the tx duration just includes the data frames sent from host to the > peer. So the difference is that the former includes control frames as well? Is that the only difference? And what exactly is a "big difference" (from the commit message)? > So the first value is preferable for station dump. Hmm, I'm not sure if I agree with this. I specifically added the tx_duration to the station dump to be able to get the values used by the airtime scheduler. This breaks with this patch. -Toke
On 2019-04-18 16:07, Toke Høiland-Jørgensen wrote: > leiwa@codeaurora.org writes: > >> On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote: >>> Lei Wang <leiwa@codeaurora.org> writes: >>> >>>> TX duration output of tx_stats in debugfs and station dump had big >>>> difference because they got tx duration value from different >>>> statistic >>>> data. We should use the same statistic data. >>> >>> So are you sure you picked the most accurate one of the two? :) >>> >>> -Toke >> >> Hi Toke, >> >> Yes. >> Now for ath10k, there are two ways to get tx duration output. >> One is got from tx_stats in debugfs reported by firmware. It is a >> total >> value including all the frames which created by host and firmware sent >> to the peer. >> And the second is calculated from >> ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here >> the tx duration just includes the data frames sent from host to the >> peer. > > So the difference is that the former includes control frames as well? > Is > that the only difference? And what exactly is a "big difference" (from > the commit message)? > Yes,it adds the duration time of receiving ACK frames. From my test,TX from AP to station with iperf UDP test in 10s,tx_stats->tx_duration:5496623us, and another value is 3934327us. >> So the first value is preferable for station dump. > > Hmm, I'm not sure if I agree with this. I specifically added the > tx_duration to the station dump to be able to get the values used by > the > airtime scheduler. This breaks with this patch. > > -Toke From our internal discussing, we will revert this change. Thanks. Lei
leiwa@codeaurora.org writes: > On 2019-04-18 16:07, Toke Høiland-Jørgensen wrote: >> leiwa@codeaurora.org writes: >> >>> On 2019-04-17 17:26, Toke Høiland-Jørgensen wrote: >>>> Lei Wang <leiwa@codeaurora.org> writes: >>>> >>>>> TX duration output of tx_stats in debugfs and station dump had big >>>>> difference because they got tx duration value from different >>>>> statistic >>>>> data. We should use the same statistic data. >>>> >>>> So are you sure you picked the most accurate one of the two? :) >>>> >>>> -Toke >>> >>> Hi Toke, >>> >>> Yes. >>> Now for ath10k, there are two ways to get tx duration output. >>> One is got from tx_stats in debugfs reported by firmware. It is a >>> total >>> value including all the frames which created by host and firmware sent >>> to the peer. >>> And the second is calculated from >>> ath10k_htt_rx_tx_compl_ind()-->ieee80211_sta_register_airtime(), here >>> the tx duration just includes the data frames sent from host to the >>> peer. >> >> So the difference is that the former includes control frames as well? >> Is >> that the only difference? And what exactly is a "big difference" (from >> the commit message)? >> > Yes,it adds the duration time of receiving ACK frames. > From my test,TX from AP to station with iperf UDP test in > 10s,tx_stats->tx_duration:5496623us, > and another value is 3934327us. Hmm, that's quite a big difference. Is this really only ACKs, or is it also a question of whether retries are accounted? If so, it may actually be that what we should do is change which value is passed to ieee80211_sta_register_airtime()? >>> So the first value is preferable for station dump. >> >> Hmm, I'm not sure if I agree with this. I specifically added the >> tx_duration to the station dump to be able to get the values used by >> the >> airtime scheduler. This breaks with this patch. >> >> -Toke > From our internal discussing, we will revert this change. Cool, but see above :) -Toke
Lei Wang <leiwa@codeaurora.org> wrote: > TX duration output of tx_stats in debugfs and station dump had big > difference because they got tx duration value from different statistic > data. We should use the same statistic data. > > Tested: QCA988X with firmware ver 10.2.4-1.0-00043 > > Signed-off-by: Lei Wang <leiwa@codeaurora.org> > Signed-off-by: Kalle Valo <kvalo@codeaurora.org> Dropping per discussion, please resend once everything is clarified. Patch set to Changes Requested.
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index b73c23d..5414169 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -7977,6 +7977,11 @@ static void ath10k_sta_statistics(struct ieee80211_hw *hw, sinfo->rx_duration = arsta->rx_duration; sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_DURATION); + if (arsta->tx_stats && ath10k_debug_is_extd_tx_stats_enabled(ar)) { + sinfo->tx_duration = arsta->tx_stats->tx_duration; + sinfo->filled |= 1ULL << NL80211_STA_INFO_TX_DURATION; + } + if (!arsta->txrate.legacy && !arsta->txrate.nss) return;
TX duration output of tx_stats in debugfs and station dump had big difference because they got tx duration value from different statistic data. We should use the same statistic data. Tested: QCA988X with firmware ver 10.2.4-1.0-00043 Signed-off-by: Lei Wang <leiwa@codeaurora.org> --- drivers/net/wireless/ath/ath10k/mac.c | 5 +++++ 1 file changed, 5 insertions(+)