Re: [Codel] [RFCv2 2/3] ath10k: report per-station tx/rate rates to mac80211

[Mohammed Shafi Shajakhan <mohammed@codeaurora.org>]

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


>  	num_peers = ath10k_wmi_fw_stats_num_peers(&ar->debug.fw_stats.peers);
>  	num_vdevs = ath10k_wmi_fw_stats_num_vdevs(&ar->debug.fw_stats.vdevs);
>  	is_start = (list_empty(&ar->debug.fw_stats.pdevs) &&
> diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
> index ebff9c0a0784..addef9179dbe 100644
> --- a/drivers/net/wireless/ath/ath10k/mac.c
> +++ b/drivers/net/wireless/ath/ath10k/mac.c
> @@ -4427,16 +4427,14 @@ static int ath10k_start(struct ieee80211_hw *hw)
>  
>  	ar->ani_enabled = true;
>  
> -	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.

>  
>  	ar->num_started_vdevs = 0;
> @@ -7215,6 +7213,13 @@ ath10k_mac_op_switch_vif_chanctx(struct ieee80211_hw *hw,
>  	return 0;
>  }
>  
> +static u32
> +ath10k_mac_op_get_expected_throughput(struct ieee80211_sta *sta)
> +{
> +	struct ath10k_sta *arsta = (struct ath10k_sta *)sta->drv_priv;
> +	return arsta->tx_rate_kbps;
> +}
> +
>  static const struct ieee80211_ops ath10k_ops = {
>  	.tx				= ath10k_mac_op_tx,
>  	.wake_tx_queue			= ath10k_mac_op_wake_tx_queue,
> @@ -7254,6 +7259,7 @@ static const struct ieee80211_ops ath10k_ops = {
>  	.assign_vif_chanctx		= ath10k_mac_op_assign_vif_chanctx,
>  	.unassign_vif_chanctx		= ath10k_mac_op_unassign_vif_chanctx,
>  	.switch_vif_chanctx		= ath10k_mac_op_switch_vif_chanctx,
> +	.get_expected_throughput	= ath10k_mac_op_get_expected_throughput,
>  
>  	CFG80211_TESTMODE_CMD(ath10k_tm_cmd)
>  
> 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

I will try to take this change, rebase it to TOT and see how it goes.

thanks,
shafi