| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CA+BoTQmRBog+oSMCKUUzTV1Jm_Mip0ZA9mnpkZ14hDUq32Dszg@mail.gmail.com>
Date: Thu, 24 Mar 2016 08:49:12 +0100
From: Michal Kazior <michal.kazior@...to.com>
To: Mohammed Shafi Shajakhan <mohammed@...eaurora.org>
Cc: linux-wireless <linux-wireless@...r.kernel.org>,
Felix Fietkau <nbd@...nwrt.org>,
Emmanuel Grumbach <emmanuel.grumbach@...el.com>,
Network Development <netdev@...r.kernel.org>,
Dave Taht <dave.taht@...il.com>,
"ath10k@...ts.infradead.org" <ath10k@...ts.infradead.org>,
codel@...ts.bufferbloat.net, make-wifi-fast@...ts.bufferbloat.net,
Johannes Berg <johannes@...solutions.net>,
Tim Shepard <shep@...m.mit.edu>
Subject: Re: [RFCv2 2/3] ath10k: report per-station tx/rate rates to mac80211
On 24 March 2016 at 08:19, Mohammed Shafi Shajakhan
<mohammed@...eaurora.org> wrote:
> Hi Michal,
>
> On Wed, Mar 16, 2016 at 11:17:57AM +0100, Michal Kazior wrote:
>> The rate control is offloaded by firmware so it's
>> challanging to provide expected throughput value
>> for given station.
>>
>> This approach is naive as it reports last tx rate
>> used for given station as provided by firmware
>> stat event.
>>
>> This should be sufficient for airtime estimation
>> used for fq-codel-in-mac80211 tx scheduling
>> purposes now.
>>
>> This patch uses a very hacky way to get the stats.
>> This is sufficient for proof-of-concept but must
>> be cleaned up properly eventually.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@...to.com>
>> ---
>> drivers/net/wireless/ath/ath10k/core.h | 5 +++
>> drivers/net/wireless/ath/ath10k/debug.c | 61 +++++++++++++++++++++++++++++----
>> drivers/net/wireless/ath/ath10k/mac.c | 26 ++++++++------
>> drivers/net/wireless/ath/ath10k/wmi.h | 2 +-
>> 4 files changed, 76 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
>> index 23ba03fb7a5f..3f76669d44cf 100644
>> --- a/drivers/net/wireless/ath/ath10k/core.h
>> +++ b/drivers/net/wireless/ath/ath10k/core.h
>> @@ -331,6 +331,9 @@ struct ath10k_sta {
>> /* protected by conf_mutex */
>> bool aggr_mode;
>> u64 rx_duration;
>> +
>> + u32 tx_rate_kbps;
>> + u32 rx_rate_kbps;
>> #endif
>> };
>>
>> @@ -372,6 +375,8 @@ struct ath10k_vif {
>> s8 def_wep_key_idx;
>>
>> u16 tx_seq_no;
>> + u32 tx_rate_kbps;
>> + u32 rx_rate_kbps;
>>
>> union {
>> struct {
>> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
>> index 076d29b53ddf..cc7ebf04ae00 100644
>> --- a/drivers/net/wireless/ath/ath10k/debug.c
>> +++ b/drivers/net/wireless/ath/ath10k/debug.c
>> @@ -316,6 +316,58 @@ static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
>> spin_unlock_bh(&ar->data_lock);
>> }
>>
>> +static void ath10k_mac_update_txrx_rate_iter(void *data,
>> + u8 *mac,
>> + struct ieee80211_vif *vif)
>> +{
>> + struct ath10k_fw_stats_peer *peer = data;
>> + struct ath10k_vif *arvif;
>> +
>> + if (memcmp(vif->addr, peer->peer_macaddr, ETH_ALEN))
>> + return;
>> +
>> + arvif = (void *)vif->drv_priv;
>> + arvif->tx_rate_kbps = peer->peer_tx_rate;
>> + arvif->rx_rate_kbps = peer->peer_rx_rate;
>> +}
>> +
>> +static void ath10k_mac_update_txrx_rate(struct ath10k *ar,
>> + struct ath10k_fw_stats *stats)
>> +{
>> + struct ieee80211_hw *hw = ar->hw;
>> + struct ath10k_fw_stats_peer *peer;
>> + struct ath10k_sta *arsta;
>> + struct ieee80211_sta *sta;
>> + const u8 *localaddr = NULL;
>> +
>> + rcu_read_lock();
>> +
>> + list_for_each_entry(peer, &stats->peers, list) {
>> + /* This doesn't account for multiple STA connected on different
>> + * vifs. Unfortunately there's no way to derive that from the available
>> + * information.
>> + */
>> + sta = ieee80211_find_sta_by_ifaddr(hw,
>> + peer->peer_macaddr,
>> + localaddr);
>> + if (!sta) {
>> + /* This tries to update multicast rates */
>> + ieee80211_iterate_active_interfaces_atomic(
>> + hw,
>> + IEEE80211_IFACE_ITER_NORMAL,
>> + ath10k_mac_update_txrx_rate_iter,
>> + peer);
>> + continue;
>> + }
>> +
>> + arsta = (void *)sta->drv_priv;
>> + arsta->tx_rate_kbps = peer->peer_tx_rate;
>> + arsta->rx_rate_kbps = peer->peer_rx_rate;
>> + }
>> +
>> + rcu_read_unlock();
>> +}
>> +
>> void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
>> {
>> struct ath10k_fw_stats stats = {};
>> @@ -335,6 +387,8 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
>> goto free;
>> }
>>
>> + ath10k_mac_update_txrx_rate(ar, &stats);
>> +
>> /* Stat data may exceed htc-wmi buffer limit. In such case firmware
>> * splits the stats data and delivers it in a ping-pong fashion of
>> * request cmd-update event.
>> @@ -351,13 +405,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
>> if (peer_stats_svc)
>> ath10k_sta_update_rx_duration(ar, &stats.peers);
>>
>> - if (ar->debug.fw_stats_done) {
>> - if (!peer_stats_svc)
>> - ath10k_warn(ar, "received unsolicited stats update event\n");
>> -
>> - goto free;
>> - }
>> -
>
> [shafi] As you had suggested previously, should we completely clean up this ping
> - pong response approach for f/w stats, (or) this should be retained to support
> backward compatibility and also for supporting ping - pong response when user
> cats for fw-stats (via debugfs) (i did see in the commit message this needs to
> be cleaned up)
I think it makes sense to remove the ping-pong logic and rely on
periodic updates alone, including fw_stats and ethstats handling.
>> - if (test_bit(WMI_SERVICE_PEER_STATS, ar->wmi.svc_map)) {
>> - param = ar->wmi.pdev_param->peer_stats_update_period;
>> - ret = ath10k_wmi_pdev_set_param(ar, param,
>> - PEER_DEFAULT_STATS_UPDATE_PERIOD);
>> - if (ret) {
>> - ath10k_warn(ar,
>> - "failed to set peer stats period : %d\n",
>> - ret);
>> - goto err_core_stop;
>> - }
>> + param = ar->wmi.pdev_param->peer_stats_update_period;
>> + ret = ath10k_wmi_pdev_set_param(ar, param,
>> + PEER_DEFAULT_STATS_UPDATE_PERIOD);
>> + if (ret) {
>> + ath10k_warn(ar,
>> + "failed to set peer stats period : %d\n",
>> + ret);
>> + goto err_core_stop;
>> }
>
> [shafi] If i am correct this change requires 'PEER_STATS' to be enabled by
> default.
No, it does not. Periodic stats have been available since forever.
>> diff --git a/drivers/net/wireless/ath/ath10k/wmi.h b/drivers/net/wireless/ath/ath10k/wmi.h
>> index 4d3cbc44fcd2..2877a3a27b95 100644
>> --- a/drivers/net/wireless/ath/ath10k/wmi.h
>> +++ b/drivers/net/wireless/ath/ath10k/wmi.h
>> @@ -3296,7 +3296,7 @@ struct wmi_csa_event {
>> /* the definition of different PDEV parameters */
>> #define PDEV_DEFAULT_STATS_UPDATE_PERIOD 500
>> #define VDEV_DEFAULT_STATS_UPDATE_PERIOD 500
>> -#define PEER_DEFAULT_STATS_UPDATE_PERIOD 500
>> +#define PEER_DEFAULT_STATS_UPDATE_PERIOD 100
>
> [shafi] Is this for more granularity since 500ms is not sufficient ?, I understand
> the firmware has default stats_update_period as 500ms and i hope it supports
> 100ms as well, Also if we are going to support period stats update we may need to
> accumulate the information in driver (like this change and rx_duration
The patch is used for rough rate estimation which is used to keep Tx
queues filled only with 1-2 txops worth of data. Signal conditions can
change vastly so I figured I need the peer stat update events come
more often. I didn't really verify if they come every 100ms. The patch
already served it's job as a proof-of-concept for smarter tx queuing.
> I will try to take this change, rebase it to TOT and see how it goes.
There's really no benefit it taking this patch as a basis for periodic
stat handling. Majority of this patch is just handling peer_tx_rate
for rate estimation purposes. Feel free to knock yourself out with
ripping out the ping-pong stat stuff though :)
Michał
Powered by blists - more mailing lists