* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
@ 2016-12-19 16:49 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 52+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-12-19 16:49 UTC (permalink / raw)
To: Christian Lamparter; +Cc: Kalle Valo, linux-wireless, ath10k
Hi Christian,
On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> The 10.4 firmware adds extended peer information to the
> firmware's statistics payload. This additional info is
> stored as a separate data field. During review of
> "ath10k: add accounting for the extended peer statistics" [0]
>
> Mohammed Shafi Shajakhan commented that the extended peer statistics
> lists are of little use:"... there is not much use in appending
> the extended peer stats (which gets periodically updated) to the
> linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> rid of the below (and the required cleanup as well)
>
> list_splice_tail_init(&stats.peers_extd,
> &ar->debug.fw_stats.peers_extd);
>
> since rx_duration is getting updated periodically to the per sta
> information."
[shafi] thanks !
>
> This patch replaces the extended peers list with a lookup and
> puts the retrieved data (rx_duration) into the existing
> ath10k_fw_stats_peer entry that was created earlier.
[shafi] Its good to maintain the extended stats variable
and retain the two different functions to update rx_duration.
a) extended peer stats supported - mainly for 10.4
b) extender peer stats not supported - for 10.2
>
> [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> ---
> drivers/net/wireless/ath/ath10k/core.h | 2 --
> drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> 4 files changed, 28 insertions(+), 57 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> index 09ff8b8a6441..3fffbbb18c25 100644
> --- a/drivers/net/wireless/ath/ath10k/core.h
> +++ b/drivers/net/wireless/ath/ath10k/core.h
> @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> };
>
> struct ath10k_fw_stats {
> - bool extended;
> struct list_head pdevs;
> struct list_head vdevs;
> struct list_head peers;
> - struct list_head peers_extd;
> };
>
> #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> index 82a4c67f3672..89f7fde77cdf 100644
> --- a/drivers/net/wireless/ath/ath10k/debug.c
> +++ b/drivers/net/wireless/ath/ath10k/debug.c
> @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> }
> }
>
> -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> -{
> - struct ath10k_fw_extd_stats_peer *i, *tmp;
> -
> - list_for_each_entry_safe(i, tmp, head, list) {
> - list_del(&i->list);
> - kfree(i);
> - }
> -}
> -
> static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> {
> spin_lock_bh(&ar->data_lock);
> ar->debug.fw_stats_done = false;
> - ar->debug.fw_stats.extended = false;
[shafi] this looks fine, but not removing the 'extended' variable
from ath10k_fw_stats structure, I see the design for 'rx_duration'
arguably some what convoluted !
*We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
*Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
{certainly 'stats' object is for this particular update only, and freed
up later)
*Update immediately using 'ath10k_sta_update_rx_duration'
'ath10k_wmi_pull_fw_stats' has a slightly different implementation
for 10.2 and 10.4 (the later supporting extended peer stats)
> ath10k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs);
> ath10k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs);
> ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers);
> - ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd);
> spin_unlock_bh(&ar->data_lock);
> }
>
> @@ -348,7 +336,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> INIT_LIST_HEAD(&stats.pdevs);
> INIT_LIST_HEAD(&stats.vdevs);
> INIT_LIST_HEAD(&stats.peers);
> - INIT_LIST_HEAD(&stats.peers_extd);
>
> spin_lock_bh(&ar->data_lock);
> ret = ath10k_wmi_pull_fw_stats(ar, skb, &stats);
> @@ -411,8 +398,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
>
> list_splice_tail_init(&stats.peers, &ar->debug.fw_stats.peers);
> list_splice_tail_init(&stats.vdevs, &ar->debug.fw_stats.vdevs);
> - list_splice_tail_init(&stats.peers_extd,
> - &ar->debug.fw_stats.peers_extd);
> }
>
> complete(&ar->debug.fw_stats_complete);
> @@ -424,7 +409,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> ath10k_fw_stats_pdevs_free(&stats.pdevs);
> ath10k_fw_stats_vdevs_free(&stats.vdevs);
> ath10k_fw_stats_peers_free(&stats.peers);
> - ath10k_fw_extd_stats_peers_free(&stats.peers_extd);
>
> spin_unlock_bh(&ar->data_lock);
> }
> @@ -2347,7 +2331,6 @@ int ath10k_debug_create(struct ath10k *ar)
> INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs);
> INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs);
> INIT_LIST_HEAD(&ar->debug.fw_stats.peers);
> - INIT_LIST_HEAD(&ar->debug.fw_stats.peers_extd);
>
> return 0;
> }
> diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
> index fce6f8137d33..bf2d49cbb3bb 100644
> --- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c
> +++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
> @@ -18,27 +18,8 @@
> #include "wmi-ops.h"
> #include "debug.h"
>
> -static void ath10k_sta_update_extd_stats_rx_duration(struct ath10k *ar,
> - struct ath10k_fw_stats *stats)
> -{
> - struct ath10k_fw_extd_stats_peer *peer;
> - struct ieee80211_sta *sta;
> - struct ath10k_sta *arsta;
> -
> - rcu_read_lock();
> - list_for_each_entry(peer, &stats->peers_extd, list) {
> - sta = ieee80211_find_sta_by_ifaddr(ar->hw, peer->peer_macaddr,
> - NULL);
> - if (!sta)
> - continue;
> - arsta = (struct ath10k_sta *)sta->drv_priv;
> - arsta->rx_duration += (u64)peer->rx_duration;
> - }
> - rcu_read_unlock();
> -}
> -
> -static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar,
> - struct ath10k_fw_stats *stats)
> +void ath10k_sta_update_rx_duration(struct ath10k *ar,
> + struct ath10k_fw_stats *stats)
> {
> struct ath10k_fw_stats_peer *peer;
> struct ieee80211_sta *sta;
> @@ -56,15 +37,6 @@ static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar,
> rcu_read_unlock();
> }
>
> -void ath10k_sta_update_rx_duration(struct ath10k *ar,
> - struct ath10k_fw_stats *stats)
> -{
> - if (stats->extended)
> - ath10k_sta_update_extd_stats_rx_duration(ar, stats);
> - else
> - ath10k_sta_update_stats_rx_duration(ar, stats);
> -}
> -
> void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> struct ieee80211_sta *sta,
> struct station_info *sinfo)
> diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> index c893314a191f..c7ec7b9e9b55 100644
> --- a/drivers/net/wireless/ath/ath10k/wmi.c
> +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> return 0;
>
> - stats->extended = true;
> -
> for (i = 0; i < num_peer_stats; i++) {
> const struct wmi_10_4_peer_extd_stats *src;
> - struct ath10k_fw_extd_stats_peer *dst;
> + struct ath10k_fw_stats_peer *dst;
>
> src = (void *)skb->data;
> if (!skb_pull(skb, sizeof(*src)))
> return -EPROTO;
>
> - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> - if (!dst)
> - continue;
> + /* Because the stat data may exceed htc-wmi buffer
> + * limit the firmware might split the stats data
> + * and delivers it in multiple update events.
> + * if we can't find the entry in the current event
> + * payload, we have to look in main list as well.
> + */
> + list_for_each_entry(dst, &stats->peers, list) {
> + if (ether_addr_equal(dst->peer_macaddr,
> + src->peer_macaddr.addr))
> + goto found;
> + }
> +
> +#ifdef CONFIG_ATH10K_DEBUGFS
> + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> + if (ether_addr_equal(dst->peer_macaddr,
> + src->peer_macaddr.addr))
> + goto found;
> + }
> +#endif
> +
> + ath10k_dbg(ar, ATH10K_DBG_WMI,
> + "Orphaned extended stats entry for station %pM.\n",
> + src->peer_macaddr.addr);
> + continue;
>
> - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> +found:
> dst->rx_duration = __le32_to_cpu(src->rx_duration);
> - list_add_tail(&dst->list, &stats->peers_extd);
> }
>
> return 0;
[shafi] Yes i am bit concerned about this change making 10.4 to update
over the existing peer_stats structure, the idea is to maintain uniformity
between the structures shared between ath10k and its corresponding to avoid
confusion/ bugs in the future. Kindly let me know your thoughts, feel free
to correct me if any of my analysis is incorrect. thank you !
regards,
shafi
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
2016-12-19 16:49 ` Mohammed Shafi Shajakhan
@ 2016-12-19 16:58 ` Mohammed Shafi Shajakhan
-1 siblings, 0 replies; 52+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-12-19 16:58 UTC (permalink / raw)
To: Christian Lamparter; +Cc: Kalle Valo, linux-wireless, ath10k
On Mon, Dec 19, 2016 at 10:19:57PM +0530, Mohammed Shafi Shajakhan wrote:
> Hi Christian,
>
> On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > The 10.4 firmware adds extended peer information to the
> > firmware's statistics payload. This additional info is
> > stored as a separate data field. During review of
> > "ath10k: add accounting for the extended peer statistics" [0]
> >
> > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > lists are of little use:"... there is not much use in appending
> > the extended peer stats (which gets periodically updated) to the
> > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > rid of the below (and the required cleanup as well)
> >
> > list_splice_tail_init(&stats.peers_extd,
> > &ar->debug.fw_stats.peers_extd);
> >
> > since rx_duration is getting updated periodically to the per sta
> > information."
>
> [shafi] thanks !
>
> >
> > This patch replaces the extended peers list with a lookup and
> > puts the retrieved data (rx_duration) into the existing
> > ath10k_fw_stats_peer entry that was created earlier.
>
> [shafi] Its good to maintain the extended stats variable
> and retain the two different functions to update rx_duration.
>
> a) extended peer stats supported - mainly for 10.4
> b) extender peer stats not supported - for 10.2
>
>
> >
> > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > 4 files changed, 28 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > index 09ff8b8a6441..3fffbbb18c25 100644
> > --- a/drivers/net/wireless/ath/ath10k/core.h
> > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > };
> >
> > struct ath10k_fw_stats {
> > - bool extended;
> > struct list_head pdevs;
> > struct list_head vdevs;
> > struct list_head peers;
> > - struct list_head peers_extd;
> > };
> >
> > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > index 82a4c67f3672..89f7fde77cdf 100644
> > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > }
> > }
> >
> > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > -{
> > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > -
> > - list_for_each_entry_safe(i, tmp, head, list) {
> > - list_del(&i->list);
> > - kfree(i);
> > - }
> > -}
> > -
> > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > {
> > spin_lock_bh(&ar->data_lock);
> > ar->debug.fw_stats_done = false;
> > - ar->debug.fw_stats.extended = false;
>
> [shafi] this looks fine, but not removing the 'extended' variable
> from ath10k_fw_stats structure, I see the design for 'rx_duration'
> arguably some what convoluted !
>
> *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> {certainly 'stats' object is for this particular update only, and freed
> up later)
> *Update immediately using 'ath10k_sta_update_rx_duration'
>
> 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> for 10.2 and 10.4 (the later supporting extended peer stats)
>
>
>
> > ath10k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs);
> > ath10k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs);
> > ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers);
> > - ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd);
> > spin_unlock_bh(&ar->data_lock);
> > }
> >
> > @@ -348,7 +336,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > INIT_LIST_HEAD(&stats.pdevs);
> > INIT_LIST_HEAD(&stats.vdevs);
> > INIT_LIST_HEAD(&stats.peers);
> > - INIT_LIST_HEAD(&stats.peers_extd);
> >
> > spin_lock_bh(&ar->data_lock);
> > ret = ath10k_wmi_pull_fw_stats(ar, skb, &stats);
> > @@ -411,8 +398,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> >
> > list_splice_tail_init(&stats.peers, &ar->debug.fw_stats.peers);
> > list_splice_tail_init(&stats.vdevs, &ar->debug.fw_stats.vdevs);
> > - list_splice_tail_init(&stats.peers_extd,
> > - &ar->debug.fw_stats.peers_extd);
> > }
> >
> > complete(&ar->debug.fw_stats_complete);
> > @@ -424,7 +409,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > ath10k_fw_stats_pdevs_free(&stats.pdevs);
> > ath10k_fw_stats_vdevs_free(&stats.vdevs);
> > ath10k_fw_stats_peers_free(&stats.peers);
> > - ath10k_fw_extd_stats_peers_free(&stats.peers_extd);
> >
> > spin_unlock_bh(&ar->data_lock);
> > }
> > @@ -2347,7 +2331,6 @@ int ath10k_debug_create(struct ath10k *ar)
> > INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs);
> > INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs);
> > INIT_LIST_HEAD(&ar->debug.fw_stats.peers);
> > - INIT_LIST_HEAD(&ar->debug.fw_stats.peers_extd);
> >
> > return 0;
> > }
> > diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
> > index fce6f8137d33..bf2d49cbb3bb 100644
> > --- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c
> > +++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
> > @@ -18,27 +18,8 @@
> > #include "wmi-ops.h"
> > #include "debug.h"
> >
> > -static void ath10k_sta_update_extd_stats_rx_duration(struct ath10k *ar,
> > - struct ath10k_fw_stats *stats)
> > -{
> > - struct ath10k_fw_extd_stats_peer *peer;
> > - struct ieee80211_sta *sta;
> > - struct ath10k_sta *arsta;
> > -
> > - rcu_read_lock();
> > - list_for_each_entry(peer, &stats->peers_extd, list) {
> > - sta = ieee80211_find_sta_by_ifaddr(ar->hw, peer->peer_macaddr,
> > - NULL);
> > - if (!sta)
> > - continue;
> > - arsta = (struct ath10k_sta *)sta->drv_priv;
> > - arsta->rx_duration += (u64)peer->rx_duration;
> > - }
> > - rcu_read_unlock();
> > -}
> > -
> > -static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar,
> > - struct ath10k_fw_stats *stats)
> > +void ath10k_sta_update_rx_duration(struct ath10k *ar,
> > + struct ath10k_fw_stats *stats)
> > {
> > struct ath10k_fw_stats_peer *peer;
> > struct ieee80211_sta *sta;
> > @@ -56,15 +37,6 @@ static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar,
> > rcu_read_unlock();
> > }
> >
> > -void ath10k_sta_update_rx_duration(struct ath10k *ar,
> > - struct ath10k_fw_stats *stats)
> > -{
> > - if (stats->extended)
> > - ath10k_sta_update_extd_stats_rx_duration(ar, stats);
> > - else
> > - ath10k_sta_update_stats_rx_duration(ar, stats);
> > -}
> > -
> > void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > struct ieee80211_sta *sta,
> > struct station_info *sinfo)
> > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > index c893314a191f..c7ec7b9e9b55 100644
> > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > return 0;
> >
> > - stats->extended = true;
> > -
> > for (i = 0; i < num_peer_stats; i++) {
> > const struct wmi_10_4_peer_extd_stats *src;
> > - struct ath10k_fw_extd_stats_peer *dst;
> > + struct ath10k_fw_stats_peer *dst;
> >
> > src = (void *)skb->data;
> > if (!skb_pull(skb, sizeof(*src)))
> > return -EPROTO;
> >
> > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > - if (!dst)
> > - continue;
> > + /* Because the stat data may exceed htc-wmi buffer
> > + * limit the firmware might split the stats data
> > + * and delivers it in multiple update events.
> > + * if we can't find the entry in the current event
> > + * payload, we have to look in main list as well.
> > + */
> > + list_for_each_entry(dst, &stats->peers, list) {
> > + if (ether_addr_equal(dst->peer_macaddr,
> > + src->peer_macaddr.addr))
> > + goto found;
> > + }
> > +
> > +#ifdef CONFIG_ATH10K_DEBUGFS
> > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > + if (ether_addr_equal(dst->peer_macaddr,
> > + src->peer_macaddr.addr))
> > + goto found;
> > + }
> > +#endif
> > +
> > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > + "Orphaned extended stats entry for station %pM.\n",
> > + src->peer_macaddr.addr);
> > + continue;
> >
> > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > +found:
> > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > - list_add_tail(&dst->list, &stats->peers_extd);
> > }
> >
> > return 0;
>
> [shafi] Yes i am bit concerned about this change making 10.4 to update
> over the existing peer_stats structure, the idea is to maintain uniformity
> between the structures shared between ath10k and its corresponding *firmware* to avoid
> bugs in the future. Kindly let me know your thoughts, feel free
> to correct me if any of my analysis is incorrect. thank you !
>
> regards,
> shafi
>
>
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
@ 2016-12-19 16:58 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 52+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-12-19 16:58 UTC (permalink / raw)
To: Christian Lamparter; +Cc: Kalle Valo, linux-wireless, ath10k
On Mon, Dec 19, 2016 at 10:19:57PM +0530, Mohammed Shafi Shajakhan wrote:
> Hi Christian,
>
> On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > The 10.4 firmware adds extended peer information to the
> > firmware's statistics payload. This additional info is
> > stored as a separate data field. During review of
> > "ath10k: add accounting for the extended peer statistics" [0]
> >
> > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > lists are of little use:"... there is not much use in appending
> > the extended peer stats (which gets periodically updated) to the
> > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > rid of the below (and the required cleanup as well)
> >
> > list_splice_tail_init(&stats.peers_extd,
> > &ar->debug.fw_stats.peers_extd);
> >
> > since rx_duration is getting updated periodically to the per sta
> > information."
>
> [shafi] thanks !
>
> >
> > This patch replaces the extended peers list with a lookup and
> > puts the retrieved data (rx_duration) into the existing
> > ath10k_fw_stats_peer entry that was created earlier.
>
> [shafi] Its good to maintain the extended stats variable
> and retain the two different functions to update rx_duration.
>
> a) extended peer stats supported - mainly for 10.4
> b) extender peer stats not supported - for 10.2
>
>
> >
> > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > 4 files changed, 28 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > index 09ff8b8a6441..3fffbbb18c25 100644
> > --- a/drivers/net/wireless/ath/ath10k/core.h
> > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > };
> >
> > struct ath10k_fw_stats {
> > - bool extended;
> > struct list_head pdevs;
> > struct list_head vdevs;
> > struct list_head peers;
> > - struct list_head peers_extd;
> > };
> >
> > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > index 82a4c67f3672..89f7fde77cdf 100644
> > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > }
> > }
> >
> > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > -{
> > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > -
> > - list_for_each_entry_safe(i, tmp, head, list) {
> > - list_del(&i->list);
> > - kfree(i);
> > - }
> > -}
> > -
> > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > {
> > spin_lock_bh(&ar->data_lock);
> > ar->debug.fw_stats_done = false;
> > - ar->debug.fw_stats.extended = false;
>
> [shafi] this looks fine, but not removing the 'extended' variable
> from ath10k_fw_stats structure, I see the design for 'rx_duration'
> arguably some what convoluted !
>
> *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> {certainly 'stats' object is for this particular update only, and freed
> up later)
> *Update immediately using 'ath10k_sta_update_rx_duration'
>
> 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> for 10.2 and 10.4 (the later supporting extended peer stats)
>
>
>
> > ath10k_fw_stats_pdevs_free(&ar->debug.fw_stats.pdevs);
> > ath10k_fw_stats_vdevs_free(&ar->debug.fw_stats.vdevs);
> > ath10k_fw_stats_peers_free(&ar->debug.fw_stats.peers);
> > - ath10k_fw_extd_stats_peers_free(&ar->debug.fw_stats.peers_extd);
> > spin_unlock_bh(&ar->data_lock);
> > }
> >
> > @@ -348,7 +336,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > INIT_LIST_HEAD(&stats.pdevs);
> > INIT_LIST_HEAD(&stats.vdevs);
> > INIT_LIST_HEAD(&stats.peers);
> > - INIT_LIST_HEAD(&stats.peers_extd);
> >
> > spin_lock_bh(&ar->data_lock);
> > ret = ath10k_wmi_pull_fw_stats(ar, skb, &stats);
> > @@ -411,8 +398,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> >
> > list_splice_tail_init(&stats.peers, &ar->debug.fw_stats.peers);
> > list_splice_tail_init(&stats.vdevs, &ar->debug.fw_stats.vdevs);
> > - list_splice_tail_init(&stats.peers_extd,
> > - &ar->debug.fw_stats.peers_extd);
> > }
> >
> > complete(&ar->debug.fw_stats_complete);
> > @@ -424,7 +409,6 @@ void ath10k_debug_fw_stats_process(struct ath10k *ar, struct sk_buff *skb)
> > ath10k_fw_stats_pdevs_free(&stats.pdevs);
> > ath10k_fw_stats_vdevs_free(&stats.vdevs);
> > ath10k_fw_stats_peers_free(&stats.peers);
> > - ath10k_fw_extd_stats_peers_free(&stats.peers_extd);
> >
> > spin_unlock_bh(&ar->data_lock);
> > }
> > @@ -2347,7 +2331,6 @@ int ath10k_debug_create(struct ath10k *ar)
> > INIT_LIST_HEAD(&ar->debug.fw_stats.pdevs);
> > INIT_LIST_HEAD(&ar->debug.fw_stats.vdevs);
> > INIT_LIST_HEAD(&ar->debug.fw_stats.peers);
> > - INIT_LIST_HEAD(&ar->debug.fw_stats.peers_extd);
> >
> > return 0;
> > }
> > diff --git a/drivers/net/wireless/ath/ath10k/debugfs_sta.c b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
> > index fce6f8137d33..bf2d49cbb3bb 100644
> > --- a/drivers/net/wireless/ath/ath10k/debugfs_sta.c
> > +++ b/drivers/net/wireless/ath/ath10k/debugfs_sta.c
> > @@ -18,27 +18,8 @@
> > #include "wmi-ops.h"
> > #include "debug.h"
> >
> > -static void ath10k_sta_update_extd_stats_rx_duration(struct ath10k *ar,
> > - struct ath10k_fw_stats *stats)
> > -{
> > - struct ath10k_fw_extd_stats_peer *peer;
> > - struct ieee80211_sta *sta;
> > - struct ath10k_sta *arsta;
> > -
> > - rcu_read_lock();
> > - list_for_each_entry(peer, &stats->peers_extd, list) {
> > - sta = ieee80211_find_sta_by_ifaddr(ar->hw, peer->peer_macaddr,
> > - NULL);
> > - if (!sta)
> > - continue;
> > - arsta = (struct ath10k_sta *)sta->drv_priv;
> > - arsta->rx_duration += (u64)peer->rx_duration;
> > - }
> > - rcu_read_unlock();
> > -}
> > -
> > -static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar,
> > - struct ath10k_fw_stats *stats)
> > +void ath10k_sta_update_rx_duration(struct ath10k *ar,
> > + struct ath10k_fw_stats *stats)
> > {
> > struct ath10k_fw_stats_peer *peer;
> > struct ieee80211_sta *sta;
> > @@ -56,15 +37,6 @@ static void ath10k_sta_update_stats_rx_duration(struct ath10k *ar,
> > rcu_read_unlock();
> > }
> >
> > -void ath10k_sta_update_rx_duration(struct ath10k *ar,
> > - struct ath10k_fw_stats *stats)
> > -{
> > - if (stats->extended)
> > - ath10k_sta_update_extd_stats_rx_duration(ar, stats);
> > - else
> > - ath10k_sta_update_stats_rx_duration(ar, stats);
> > -}
> > -
> > void ath10k_sta_statistics(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
> > struct ieee80211_sta *sta,
> > struct station_info *sinfo)
> > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > index c893314a191f..c7ec7b9e9b55 100644
> > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > return 0;
> >
> > - stats->extended = true;
> > -
> > for (i = 0; i < num_peer_stats; i++) {
> > const struct wmi_10_4_peer_extd_stats *src;
> > - struct ath10k_fw_extd_stats_peer *dst;
> > + struct ath10k_fw_stats_peer *dst;
> >
> > src = (void *)skb->data;
> > if (!skb_pull(skb, sizeof(*src)))
> > return -EPROTO;
> >
> > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > - if (!dst)
> > - continue;
> > + /* Because the stat data may exceed htc-wmi buffer
> > + * limit the firmware might split the stats data
> > + * and delivers it in multiple update events.
> > + * if we can't find the entry in the current event
> > + * payload, we have to look in main list as well.
> > + */
> > + list_for_each_entry(dst, &stats->peers, list) {
> > + if (ether_addr_equal(dst->peer_macaddr,
> > + src->peer_macaddr.addr))
> > + goto found;
> > + }
> > +
> > +#ifdef CONFIG_ATH10K_DEBUGFS
> > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > + if (ether_addr_equal(dst->peer_macaddr,
> > + src->peer_macaddr.addr))
> > + goto found;
> > + }
> > +#endif
> > +
> > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > + "Orphaned extended stats entry for station %pM.\n",
> > + src->peer_macaddr.addr);
> > + continue;
> >
> > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > +found:
> > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > - list_add_tail(&dst->list, &stats->peers_extd);
> > }
> >
> > return 0;
>
> [shafi] Yes i am bit concerned about this change making 10.4 to update
> over the existing peer_stats structure, the idea is to maintain uniformity
> between the structures shared between ath10k and its corresponding *firmware* to avoid
> bugs in the future. Kindly let me know your thoughts, feel free
> to correct me if any of my analysis is incorrect. thank you !
>
> regards,
> shafi
>
>
> _______________________________________________
> ath10k mailing list
> ath10k@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/ath10k
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 52+ messages in thread
* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
2016-12-19 16:49 ` Mohammed Shafi Shajakhan
@ 2016-12-19 18:37 ` Christian Lamparter
-1 siblings, 0 replies; 52+ messages in thread
From: Christian Lamparter @ 2016-12-19 18:37 UTC (permalink / raw)
To: Mohammed Shafi Shajakhan; +Cc: linux-wireless, ath10k, Kalle Valo
Hello Shafi,
On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote:
> On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > The 10.4 firmware adds extended peer information to the
> > firmware's statistics payload. This additional info is
> > stored as a separate data field. During review of
> > "ath10k: add accounting for the extended peer statistics" [0]
> >
> > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > lists are of little use:"... there is not much use in appending
> > the extended peer stats (which gets periodically updated) to the
> > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > rid of the below (and the required cleanup as well)
> >
> > list_splice_tail_init(&stats.peers_extd,
> > &ar->debug.fw_stats.peers_extd);
> >
> > since rx_duration is getting updated periodically to the per sta
> > information."
> >
> > This patch replaces the extended peers list with a lookup and
> > puts the retrieved data (rx_duration) into the existing
> > ath10k_fw_stats_peer entry that was created earlier.
>
> [shafi] Its good to maintain the extended stats variable
> and retain the two different functions to update rx_duration.
>
> a) extended peer stats supported - mainly for 10.4
> b) extender peer stats not supported - for 10.2
Well, I have to ask why you want to retain the two different
functions to update the same arsta->rx_duration? I think a
little bit of code that helps to explain what's on your mind
(or how you would do it) would help immensely in this case.
Since I have the feeling that this is the problem here.
So please explain it in C(lang).
> > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > 4 files changed, 28 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > index 09ff8b8a6441..3fffbbb18c25 100644
> > --- a/drivers/net/wireless/ath/ath10k/core.h
> > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > };
> >
> > struct ath10k_fw_stats {
> > - bool extended;
> > struct list_head pdevs;
> > struct list_head vdevs;
> > struct list_head peers;
> > - struct list_head peers_extd;
> > };
> >
> > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > index 82a4c67f3672..89f7fde77cdf 100644
> > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > }
> > }
> >
> > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > -{
> > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > -
> > - list_for_each_entry_safe(i, tmp, head, list) {
> > - list_del(&i->list);
> > - kfree(i);
> > - }
> > -}
> > -
> > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > {
> > spin_lock_bh(&ar->data_lock);
> > ar->debug.fw_stats_done = false;
> > - ar->debug.fw_stats.extended = false;
>
> [shafi] this looks fine, but not removing the 'extended' variable
> from ath10k_fw_stats structure, I see the design for 'rx_duration'
> arguably some what convoluted !
I removed extended because it is now a write-only variable.
So I figured, there's no point in keeping it around? I don't have
access to the firmware interface documentation, so I don't know
if there's a reason why it would be good to have it later.
So please tell me, what information do we gain from it?
> *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> {certainly 'stats' object is for this particular update only, and freed
> up later)
> *Update immediately using 'ath10k_sta_update_rx_duration'
>
> 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> for 10.2 and 10.4 (the later supporting extended peer stats)
I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats()
passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats
element which is basically a carbon-copy of wmi_10_2_4_peer_stats
(but with one extra __le32 for the rx_duration at the end.)
This rx_duration is then used to set the rx_duration field in the
generated ath10k_fw_stats_peer struct.
10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats
element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for
the communicating the rx_duration to the driver.
Thing is, both function have the same signature. They produce the same
struct ath10k_fw_stats_peer for the given data in the sk_buff input. So
why does 10.4 need to have it's peer_extd infrastructure, when it can use
the existing rx_duration field in the *universal* ath10k_fw_stats_peer?
What's with the extra layers / HAL here? Because it looks like it's merged
back together in the same manner into the same arsta->rx_duration?
[ath10k_sta_update_stats_rx_duration() vs.
ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too]
> > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > index c893314a191f..c7ec7b9e9b55 100644
> > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > return 0;
> >
> > - stats->extended = true;
> > -
> > for (i = 0; i < num_peer_stats; i++) {
> > const struct wmi_10_4_peer_extd_stats *src;
> > - struct ath10k_fw_extd_stats_peer *dst;
> > + struct ath10k_fw_stats_peer *dst;
> >
> > src = (void *)skb->data;
> > if (!skb_pull(skb, sizeof(*src)))
> > return -EPROTO;
> >
> > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > - if (!dst)
> > - continue;
> > + /* Because the stat data may exceed htc-wmi buffer
> > + * limit the firmware might split the stats data
> > + * and delivers it in multiple update events.
> > + * if we can't find the entry in the current event
> > + * payload, we have to look in main list as well.
> > + */
> > + list_for_each_entry(dst, &stats->peers, list) {
> > + if (ether_addr_equal(dst->peer_macaddr,
> > + src->peer_macaddr.addr))
> > + goto found;
> > + }
> > +
> > +#ifdef CONFIG_ATH10K_DEBUGFS
> > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > + if (ether_addr_equal(dst->peer_macaddr,
> > + src->peer_macaddr.addr))
> > + goto found;
> > + }
> > +#endif
> > +
> > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > + "Orphaned extended stats entry for station %pM.\n",
> > + src->peer_macaddr.addr);
> > + continue;
> >
> > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > +found:
> > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > - list_add_tail(&dst->list, &stats->peers_extd);
> > }
> >
> > return 0;
>
> [shafi] Yes i am bit concerned about this change making 10.4 to update
> over the existing peer_stats structure, the idea is to maintain uniformity
> between the structures shared between ath10k and its corresponding to avoid
> confusion/ bugs in the future. Kindly let me know your thoughts, feel free
> to correct me if any of my analysis is incorrect. thank you !
Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make
a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure)
that other functions can use, without caring about if the FW was 10.4
or 10.2.4?
There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats
conveys any different information than the rx_duration in
wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make
wmi_10_4_peer_extd_stats's rx_duration use the existing field in
ath10k_fw_stats_peer? And if not: why exactly?
Regards,
Christian
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
@ 2016-12-19 18:37 ` Christian Lamparter
0 siblings, 0 replies; 52+ messages in thread
From: Christian Lamparter @ 2016-12-19 18:37 UTC (permalink / raw)
To: Mohammed Shafi Shajakhan; +Cc: Kalle Valo, linux-wireless, ath10k
Hello Shafi,
On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote:
> On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > The 10.4 firmware adds extended peer information to the
> > firmware's statistics payload. This additional info is
> > stored as a separate data field. During review of
> > "ath10k: add accounting for the extended peer statistics" [0]
> >
> > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > lists are of little use:"... there is not much use in appending
> > the extended peer stats (which gets periodically updated) to the
> > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > rid of the below (and the required cleanup as well)
> >
> > list_splice_tail_init(&stats.peers_extd,
> > &ar->debug.fw_stats.peers_extd);
> >
> > since rx_duration is getting updated periodically to the per sta
> > information."
> >
> > This patch replaces the extended peers list with a lookup and
> > puts the retrieved data (rx_duration) into the existing
> > ath10k_fw_stats_peer entry that was created earlier.
>
> [shafi] Its good to maintain the extended stats variable
> and retain the two different functions to update rx_duration.
>
> a) extended peer stats supported - mainly for 10.4
> b) extender peer stats not supported - for 10.2
Well, I have to ask why you want to retain the two different
functions to update the same arsta->rx_duration? I think a
little bit of code that helps to explain what's on your mind
(or how you would do it) would help immensely in this case.
Since I have the feeling that this is the problem here.
So please explain it in C(lang).
> > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > ---
> > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > 4 files changed, 28 insertions(+), 57 deletions(-)
> >
> > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > index 09ff8b8a6441..3fffbbb18c25 100644
> > --- a/drivers/net/wireless/ath/ath10k/core.h
> > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > };
> >
> > struct ath10k_fw_stats {
> > - bool extended;
> > struct list_head pdevs;
> > struct list_head vdevs;
> > struct list_head peers;
> > - struct list_head peers_extd;
> > };
> >
> > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > index 82a4c67f3672..89f7fde77cdf 100644
> > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > }
> > }
> >
> > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > -{
> > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > -
> > - list_for_each_entry_safe(i, tmp, head, list) {
> > - list_del(&i->list);
> > - kfree(i);
> > - }
> > -}
> > -
> > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > {
> > spin_lock_bh(&ar->data_lock);
> > ar->debug.fw_stats_done = false;
> > - ar->debug.fw_stats.extended = false;
>
> [shafi] this looks fine, but not removing the 'extended' variable
> from ath10k_fw_stats structure, I see the design for 'rx_duration'
> arguably some what convoluted !
I removed extended because it is now a write-only variable.
So I figured, there's no point in keeping it around? I don't have
access to the firmware interface documentation, so I don't know
if there's a reason why it would be good to have it later.
So please tell me, what information do we gain from it?
> *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> {certainly 'stats' object is for this particular update only, and freed
> up later)
> *Update immediately using 'ath10k_sta_update_rx_duration'
>
> 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> for 10.2 and 10.4 (the later supporting extended peer stats)
I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats()
passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats
element which is basically a carbon-copy of wmi_10_2_4_peer_stats
(but with one extra __le32 for the rx_duration at the end.)
This rx_duration is then used to set the rx_duration field in the
generated ath10k_fw_stats_peer struct.
10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats
element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for
the communicating the rx_duration to the driver.
Thing is, both function have the same signature. They produce the same
struct ath10k_fw_stats_peer for the given data in the sk_buff input. So
why does 10.4 need to have it's peer_extd infrastructure, when it can use
the existing rx_duration field in the *universal* ath10k_fw_stats_peer?
What's with the extra layers / HAL here? Because it looks like it's merged
back together in the same manner into the same arsta->rx_duration?
[ath10k_sta_update_stats_rx_duration() vs.
ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too]
> > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > index c893314a191f..c7ec7b9e9b55 100644
> > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > return 0;
> >
> > - stats->extended = true;
> > -
> > for (i = 0; i < num_peer_stats; i++) {
> > const struct wmi_10_4_peer_extd_stats *src;
> > - struct ath10k_fw_extd_stats_peer *dst;
> > + struct ath10k_fw_stats_peer *dst;
> >
> > src = (void *)skb->data;
> > if (!skb_pull(skb, sizeof(*src)))
> > return -EPROTO;
> >
> > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > - if (!dst)
> > - continue;
> > + /* Because the stat data may exceed htc-wmi buffer
> > + * limit the firmware might split the stats data
> > + * and delivers it in multiple update events.
> > + * if we can't find the entry in the current event
> > + * payload, we have to look in main list as well.
> > + */
> > + list_for_each_entry(dst, &stats->peers, list) {
> > + if (ether_addr_equal(dst->peer_macaddr,
> > + src->peer_macaddr.addr))
> > + goto found;
> > + }
> > +
> > +#ifdef CONFIG_ATH10K_DEBUGFS
> > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > + if (ether_addr_equal(dst->peer_macaddr,
> > + src->peer_macaddr.addr))
> > + goto found;
> > + }
> > +#endif
> > +
> > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > + "Orphaned extended stats entry for station %pM.\n",
> > + src->peer_macaddr.addr);
> > + continue;
> >
> > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > +found:
> > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > - list_add_tail(&dst->list, &stats->peers_extd);
> > }
> >
> > return 0;
>
> [shafi] Yes i am bit concerned about this change making 10.4 to update
> over the existing peer_stats structure, the idea is to maintain uniformity
> between the structures shared between ath10k and its corresponding to avoid
> confusion/ bugs in the future. Kindly let me know your thoughts, feel free
> to correct me if any of my analysis is incorrect. thank you !
Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make
a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure)
that other functions can use, without caring about if the FW was 10.4
or 10.2.4?
There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats
conveys any different information than the rx_duration in
wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make
wmi_10_4_peer_extd_stats's rx_duration use the existing field in
ath10k_fw_stats_peer? And if not: why exactly?
Regards,
Christian
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
2016-12-19 18:37 ` Christian Lamparter
@ 2016-12-22 15:48 ` Mohammed Shafi Shajakhan
-1 siblings, 0 replies; 52+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-12-22 15:48 UTC (permalink / raw)
To: Christian Lamparter; +Cc: linux-wireless, ath10k, Kalle Valo
Hi Christian,
once again sorry for the delay
>
> On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote:
> > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > > The 10.4 firmware adds extended peer information to the
> > > firmware's statistics payload. This additional info is
> > > stored as a separate data field. During review of
> > > "ath10k: add accounting for the extended peer statistics" [0]
> > >
> > > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > > lists are of little use:"... there is not much use in appending
> > > the extended peer stats (which gets periodically updated) to the
> > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > > rid of the below (and the required cleanup as well)
> > >
> > > list_splice_tail_init(&stats.peers_extd,
> > > &ar->debug.fw_stats.peers_extd);
> > >
> > > since rx_duration is getting updated periodically to the per sta
> > > information."
> > >
> > > This patch replaces the extended peers list with a lookup and
> > > puts the retrieved data (rx_duration) into the existing
> > > ath10k_fw_stats_peer entry that was created earlier.
> >
> > [shafi] Its good to maintain the extended stats variable
> > and retain the two different functions to update rx_duration.
> >
> > a) extended peer stats supported - mainly for 10.4
> > b) extender peer stats not supported - for 10.2
> Well, I have to ask why you want to retain the two different
> functions to update the same arsta->rx_duration? I think a
> little bit of code that helps to explain what's on your mind
> (or how you would do it) would help immensely in this case.
> Since I have the feeling that this is the problem here.
> So please explain it in C(lang).
[shafi] I see you prefer to stuff the 'rx_duration' from
the extended stats to the existing global 'ath10k_peer_stats'
The idea of extended stats is, ideally its not going to stop
for 'rx_duration' . Extended stats is maintained as a seperate
list / entry for addition of new fields as well
>
> > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > > ---
> > > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > > 4 files changed, 28 insertions(+), 57 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > > index 09ff8b8a6441..3fffbbb18c25 100644
> > > --- a/drivers/net/wireless/ath/ath10k/core.h
> > > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > > };
> > >
> > > struct ath10k_fw_stats {
> > > - bool extended;
> > > struct list_head pdevs;
> > > struct list_head vdevs;
> > > struct list_head peers;
> > > - struct list_head peers_extd;
> > > };
> > >
> > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > > index 82a4c67f3672..89f7fde77cdf 100644
> > > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > > }
> > > }
> > >
> > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > > -{
> > > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > > -
> > > - list_for_each_entry_safe(i, tmp, head, list) {
> > > - list_del(&i->list);
> > > - kfree(i);
> > > - }
> > > -}
> > > -
> > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > > {
> > > spin_lock_bh(&ar->data_lock);
> > > ar->debug.fw_stats_done = false;
> > > - ar->debug.fw_stats.extended = false;
> >
> > [shafi] this looks fine, but not removing the 'extended' variable
> > from ath10k_fw_stats structure, I see the design for 'rx_duration'
> > arguably some what convoluted !
> I removed extended because it is now a write-only variable.
> So I figured, there's no point in keeping it around? I don't have
> access to the firmware interface documentation, so I don't know
> if there's a reason why it would be good to have it later.
> So please tell me, what information do we gain from it?
[shafi] while parsing the stats from 'wmi' we clearly mark there whether
the extended stats is available (or) not and if its there parse it and
deal with it seperately
>
> > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> > {certainly 'stats' object is for this particular update only, and freed
> > up later)
> > *Update immediately using 'ath10k_sta_update_rx_duration'
> >
> > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> > for 10.2 and 10.4 (the later supporting extended peer stats)
>
> I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats()
> passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats
> element which is basically a carbon-copy of wmi_10_2_4_peer_stats
> (but with one extra __le32 for the rx_duration at the end.)
> This rx_duration is then used to set the rx_duration field in the
> generated ath10k_fw_stats_peer struct.
>
> 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats
> element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for
> the communicating the rx_duration to the driver.
>
> Thing is, both function have the same signature. They produce the same
> struct ath10k_fw_stats_peer for the given data in the sk_buff input. So
> why does 10.4 need to have it's peer_extd infrastructure, when it can use
> the existing rx_duration field in the *universal* ath10k_fw_stats_peer?
[shafi] agreed we need to fix that, it would have been easier if 10.2.4
and 10.4 had the same definitions.
>
> What's with the extra layers / HAL here? Because it looks like it's merged
> back together in the same manner into the same arsta->rx_duration?
> [ath10k_sta_update_stats_rx_duration() vs.
> ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too]
>
> > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > > index c893314a191f..c7ec7b9e9b55 100644
> > > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > > return 0;
> > >
> > > - stats->extended = true;
> > > -
> > > for (i = 0; i < num_peer_stats; i++) {
> > > const struct wmi_10_4_peer_extd_stats *src;
> > > - struct ath10k_fw_extd_stats_peer *dst;
> > > + struct ath10k_fw_stats_peer *dst;
> > >
> > > src = (void *)skb->data;
> > > if (!skb_pull(skb, sizeof(*src)))
> > > return -EPROTO;
> > >
> > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > > - if (!dst)
> > > - continue;
> > > + /* Because the stat data may exceed htc-wmi buffer
> > > + * limit the firmware might split the stats data
> > > + * and delivers it in multiple update events.
> > > + * if we can't find the entry in the current event
> > > + * payload, we have to look in main list as well.
> > > + */
[shafi] thanks ! sorry i might have missed this bug, did you happen
to see this condition being hit ?
> > > + list_for_each_entry(dst, &stats->peers, list) {
> > > + if (ether_addr_equal(dst->peer_macaddr,
> > > + src->peer_macaddr.addr))
> > > + goto found;
> > > + }
> > > +
[shafi] Again, we can simply cache the macaddress and rx_duration
and deal with it later, rather than doing the whole lookup here ?
Will it be an overhead for large number of clients ?
> > > +#ifdef CONFIG_ATH10K_DEBUGFS
> > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > > + if (ether_addr_equal(dst->peer_macaddr,
> > > + src->peer_macaddr.addr))
> > > + goto found;
> > > + }
> > > +#endif
[shafi] again, this could be handled seperately.
> > > +
> > > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > > + "Orphaned extended stats entry for station %pM.\n",
> > > + src->peer_macaddr.addr);
> > > + continue;
> > >
> > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > > +found:
> > > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > > - list_add_tail(&dst->list, &stats->peers_extd);
> > > }
> > >
> > > return 0;
> >
> > [shafi] Yes i am bit concerned about this change making 10.4 to update
> > over the existing peer_stats structure, the idea is to maintain uniformity
> > between the structures shared between ath10k and its corresponding to avoid
> > confusion/ bugs in the future. Kindly let me know your thoughts, feel free
> > to correct me if any of my analysis is incorrect. thank you !
> Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make
> a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure)
> that other functions can use, without caring about if the FW was 10.4
> or 10.2.4?
>
> There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats
> conveys any different information than the rx_duration in
> wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make
> wmi_10_4_peer_extd_stats's rx_duration use the existing field in
> ath10k_fw_stats_peer? And if not: why exactly?
>
[shafi] I will soon test your change and keep you posted. We will also
get Kalle's take/view on this.
regards,
shafi
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
@ 2016-12-22 15:48 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 52+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-12-22 15:48 UTC (permalink / raw)
To: Christian Lamparter; +Cc: Kalle Valo, linux-wireless, ath10k
Hi Christian,
once again sorry for the delay
>
> On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote:
> > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > > The 10.4 firmware adds extended peer information to the
> > > firmware's statistics payload. This additional info is
> > > stored as a separate data field. During review of
> > > "ath10k: add accounting for the extended peer statistics" [0]
> > >
> > > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > > lists are of little use:"... there is not much use in appending
> > > the extended peer stats (which gets periodically updated) to the
> > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > > rid of the below (and the required cleanup as well)
> > >
> > > list_splice_tail_init(&stats.peers_extd,
> > > &ar->debug.fw_stats.peers_extd);
> > >
> > > since rx_duration is getting updated periodically to the per sta
> > > information."
> > >
> > > This patch replaces the extended peers list with a lookup and
> > > puts the retrieved data (rx_duration) into the existing
> > > ath10k_fw_stats_peer entry that was created earlier.
> >
> > [shafi] Its good to maintain the extended stats variable
> > and retain the two different functions to update rx_duration.
> >
> > a) extended peer stats supported - mainly for 10.4
> > b) extender peer stats not supported - for 10.2
> Well, I have to ask why you want to retain the two different
> functions to update the same arsta->rx_duration? I think a
> little bit of code that helps to explain what's on your mind
> (or how you would do it) would help immensely in this case.
> Since I have the feeling that this is the problem here.
> So please explain it in C(lang).
[shafi] I see you prefer to stuff the 'rx_duration' from
the extended stats to the existing global 'ath10k_peer_stats'
The idea of extended stats is, ideally its not going to stop
for 'rx_duration' . Extended stats is maintained as a seperate
list / entry for addition of new fields as well
>
> > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > > ---
> > > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > > 4 files changed, 28 insertions(+), 57 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > > index 09ff8b8a6441..3fffbbb18c25 100644
> > > --- a/drivers/net/wireless/ath/ath10k/core.h
> > > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > > };
> > >
> > > struct ath10k_fw_stats {
> > > - bool extended;
> > > struct list_head pdevs;
> > > struct list_head vdevs;
> > > struct list_head peers;
> > > - struct list_head peers_extd;
> > > };
> > >
> > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > > index 82a4c67f3672..89f7fde77cdf 100644
> > > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > > }
> > > }
> > >
> > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > > -{
> > > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > > -
> > > - list_for_each_entry_safe(i, tmp, head, list) {
> > > - list_del(&i->list);
> > > - kfree(i);
> > > - }
> > > -}
> > > -
> > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > > {
> > > spin_lock_bh(&ar->data_lock);
> > > ar->debug.fw_stats_done = false;
> > > - ar->debug.fw_stats.extended = false;
> >
> > [shafi] this looks fine, but not removing the 'extended' variable
> > from ath10k_fw_stats structure, I see the design for 'rx_duration'
> > arguably some what convoluted !
> I removed extended because it is now a write-only variable.
> So I figured, there's no point in keeping it around? I don't have
> access to the firmware interface documentation, so I don't know
> if there's a reason why it would be good to have it later.
> So please tell me, what information do we gain from it?
[shafi] while parsing the stats from 'wmi' we clearly mark there whether
the extended stats is available (or) not and if its there parse it and
deal with it seperately
>
> > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> > {certainly 'stats' object is for this particular update only, and freed
> > up later)
> > *Update immediately using 'ath10k_sta_update_rx_duration'
> >
> > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> > for 10.2 and 10.4 (the later supporting extended peer stats)
>
> I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats()
> passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats
> element which is basically a carbon-copy of wmi_10_2_4_peer_stats
> (but with one extra __le32 for the rx_duration at the end.)
> This rx_duration is then used to set the rx_duration field in the
> generated ath10k_fw_stats_peer struct.
>
> 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats
> element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for
> the communicating the rx_duration to the driver.
>
> Thing is, both function have the same signature. They produce the same
> struct ath10k_fw_stats_peer for the given data in the sk_buff input. So
> why does 10.4 need to have it's peer_extd infrastructure, when it can use
> the existing rx_duration field in the *universal* ath10k_fw_stats_peer?
[shafi] agreed we need to fix that, it would have been easier if 10.2.4
and 10.4 had the same definitions.
>
> What's with the extra layers / HAL here? Because it looks like it's merged
> back together in the same manner into the same arsta->rx_duration?
> [ath10k_sta_update_stats_rx_duration() vs.
> ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too]
>
> > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > > index c893314a191f..c7ec7b9e9b55 100644
> > > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > > return 0;
> > >
> > > - stats->extended = true;
> > > -
> > > for (i = 0; i < num_peer_stats; i++) {
> > > const struct wmi_10_4_peer_extd_stats *src;
> > > - struct ath10k_fw_extd_stats_peer *dst;
> > > + struct ath10k_fw_stats_peer *dst;
> > >
> > > src = (void *)skb->data;
> > > if (!skb_pull(skb, sizeof(*src)))
> > > return -EPROTO;
> > >
> > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > > - if (!dst)
> > > - continue;
> > > + /* Because the stat data may exceed htc-wmi buffer
> > > + * limit the firmware might split the stats data
> > > + * and delivers it in multiple update events.
> > > + * if we can't find the entry in the current event
> > > + * payload, we have to look in main list as well.
> > > + */
[shafi] thanks ! sorry i might have missed this bug, did you happen
to see this condition being hit ?
> > > + list_for_each_entry(dst, &stats->peers, list) {
> > > + if (ether_addr_equal(dst->peer_macaddr,
> > > + src->peer_macaddr.addr))
> > > + goto found;
> > > + }
> > > +
[shafi] Again, we can simply cache the macaddress and rx_duration
and deal with it later, rather than doing the whole lookup here ?
Will it be an overhead for large number of clients ?
> > > +#ifdef CONFIG_ATH10K_DEBUGFS
> > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > > + if (ether_addr_equal(dst->peer_macaddr,
> > > + src->peer_macaddr.addr))
> > > + goto found;
> > > + }
> > > +#endif
[shafi] again, this could be handled seperately.
> > > +
> > > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > > + "Orphaned extended stats entry for station %pM.\n",
> > > + src->peer_macaddr.addr);
> > > + continue;
> > >
> > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > > +found:
> > > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > > - list_add_tail(&dst->list, &stats->peers_extd);
> > > }
> > >
> > > return 0;
> >
> > [shafi] Yes i am bit concerned about this change making 10.4 to update
> > over the existing peer_stats structure, the idea is to maintain uniformity
> > between the structures shared between ath10k and its corresponding to avoid
> > confusion/ bugs in the future. Kindly let me know your thoughts, feel free
> > to correct me if any of my analysis is incorrect. thank you !
> Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make
> a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure)
> that other functions can use, without caring about if the FW was 10.4
> or 10.2.4?
>
> There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats
> conveys any different information than the rx_duration in
> wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make
> wmi_10_4_peer_extd_stats's rx_duration use the existing field in
> ath10k_fw_stats_peer? And if not: why exactly?
>
[shafi] I will soon test your change and keep you posted. We will also
get Kalle's take/view on this.
regards,
shafi
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
2016-12-22 15:48 ` Mohammed Shafi Shajakhan
@ 2016-12-22 17:58 ` Christian Lamparter
-1 siblings, 0 replies; 52+ messages in thread
From: Christian Lamparter @ 2016-12-22 17:58 UTC (permalink / raw)
To: Mohammed Shafi Shajakhan
Cc: linux-wireless, ath10k, Kalle Valo, michal.kazior
Hello Shafi,
On Thursday, December 22, 2016 9:18:01 PM CET Mohammed Shafi Shajakhan wrote:
> > On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote:
> > > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > > > The 10.4 firmware adds extended peer information to the
> > > > firmware's statistics payload. This additional info is
> > > > stored as a separate data field. During review of
> > > > "ath10k: add accounting for the extended peer statistics" [0]
> > > >
> > > > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > > > lists are of little use:"... there is not much use in appending
> > > > the extended peer stats (which gets periodically updated) to the
> > > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > > > rid of the below (and the required cleanup as well)
> > > >
> > > > list_splice_tail_init(&stats.peers_extd,
> > > > &ar->debug.fw_stats.peers_extd);
> > > >
> > > > since rx_duration is getting updated periodically to the per sta
> > > > information."
> > > >
> > > > This patch replaces the extended peers list with a lookup and
> > > > puts the retrieved data (rx_duration) into the existing
> > > > ath10k_fw_stats_peer entry that was created earlier.
> > >
> > > [shafi] Its good to maintain the extended stats variable
> > > and retain the two different functions to update rx_duration.
> > >
> > > a) extended peer stats supported - mainly for 10.4
> > > b) extender peer stats not supported - for 10.2
> > Well, I have to ask why you want to retain the two different
> > functions to update the same arsta->rx_duration? I think a
> > little bit of code that helps to explain what's on your mind
> > (or how you would do it) would help immensely in this case.
> > Since I have the feeling that this is the problem here.
> > So please explain it in C(lang).
>
> [shafi] I see you prefer to stuff the 'rx_duration' from
> the extended stats to the existing global 'ath10k_peer_stats'
> The idea of extended stats is, ideally its not going to stop
> for 'rx_duration' . Extended stats is maintained as a seperate
> list / entry for addition of new fields as well
I'm guessing you are trying to say here:
replace:
dst->rx_duration = __le32_to_cpu(src->rx_duration);
with
dst->rx_duration += __le32_to_cpu(src->rx_duration);
in ath10k_wmi_10_4_op_pull_fw_stats()?
Is this correct? If so then can you tell me why ath10k_wmi_10_4_op_pull_fw_stats()
is using for (i = 0; i < num_peer_stats; i++) to iterate over the extended peer
stats instead of looking up the number of extended peer items. Because this does
imply that there are the "same" (and every peer stat has a matching extended
peer stat)... (This will be important for the comment below - so ***).
Of course, if this isn't true then this is a bug and should be fixed because
otherwise the ath10k_wmi_10_4_op_pull_fw_stats functions might return -EPROTO
at some point which causes a "failed to pull fw stats: -71" and unprocessed/lost
stats.
> >
> > > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > > > ---
> > > > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > > > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > > > 4 files changed, 28 insertions(+), 57 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > > > index 09ff8b8a6441..3fffbbb18c25 100644
> > > > --- a/drivers/net/wireless/ath/ath10k/core.h
> > > > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > > > };
> > > >
> > > > struct ath10k_fw_stats {
> > > > - bool extended;
> > > > struct list_head pdevs;
> > > > struct list_head vdevs;
> > > > struct list_head peers;
> > > > - struct list_head peers_extd;
> > > > };
> > > >
> > > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > > > index 82a4c67f3672..89f7fde77cdf 100644
> > > > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > > > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > > > }
> > > > }
> > > >
> > > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > > > -{
> > > > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > > > -
> > > > - list_for_each_entry_safe(i, tmp, head, list) {
> > > > - list_del(&i->list);
> > > > - kfree(i);
> > > > - }
> > > > -}
> > > > -
> > > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > > > {
> > > > spin_lock_bh(&ar->data_lock);
> > > > ar->debug.fw_stats_done = false;
> > > > - ar->debug.fw_stats.extended = false;
> > >
> > > [shafi] this looks fine, but not removing the 'extended' variable
> > > from ath10k_fw_stats structure, I see the design for 'rx_duration'
> > > arguably some what convoluted !
> > I removed extended because it is now a write-only variable.
> > So I figured, there's no point in keeping it around? I don't have
> > access to the firmware interface documentation, so I don't know
> > if there's a reason why it would be good to have it later.
> > So please tell me, what information do we gain from it?
>
> [shafi] while parsing the stats from 'wmi' we clearly mark there whether
> the extended stats is available (or) not and if its there parse it and
> deal with it seperately
No, there's no difference between stats or extended stats (10.2.4 vs 10.4):
both ath10k_sta_update_extd_stats_rx_duration() [0]
and ath10k_sta_update_stats_rx_duration() [1] are adding the
ath10k_fw_stats_peer(_extd)->rx_duration to ath10k_sta->rx_duration.
With the merge of the peer stats and extended peer stats, this also
removed the only usage of stats->extended. And hence it becomes a
write-only variable. So there's no point in keeping it around ATM (as
all other extended peer stats entries aren't used).
> > > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> > > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> > > {certainly 'stats' object is for this particular update only, and freed
> > > up later)
> > > *Update immediately using 'ath10k_sta_update_rx_duration'
> > >
> > > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> > > for 10.2 and 10.4 (the later supporting extended peer stats)
> >
> > I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats()
> > passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats
> > element which is basically a carbon-copy of wmi_10_2_4_peer_stats
> > (but with one extra __le32 for the rx_duration at the end.)
> > This rx_duration is then used to set the rx_duration field in the
> > generated ath10k_fw_stats_peer struct.
> >
> > 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats
> > element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for
> > the communicating the rx_duration to the driver.
> >
> > Thing is, both function have the same signature. They produce the same
> > struct ath10k_fw_stats_peer for the given data in the sk_buff input. So
> > why does 10.4 need to have it's peer_extd infrastructure, when it can use
> > the existing rx_duration field in the *universal* ath10k_fw_stats_peer?
>
> [shafi] agreed we need to fix that, it would have been easier if 10.2.4
> and 10.4 had the same definitions.
Ok, I don't know the internals of the firmware to know what's the difference
between 10.2.4 and 10.4's rx_duration (how both firmwares define them
differently in this context) ? From what I can tell, it's just that
the entry has moved from the peer stats to extended peer stats.
Of course, this is based on the logic that both 10.2.4 and 10.4 rx_durations
end up being added to arsta->rx_duration in the same way. There's no scaling
or a comment that would indicate a difference.
> >
> > What's with the extra layers / HAL here? Because it looks like it's merged
> > back together in the same manner into the same arsta->rx_duration?
> > [ath10k_sta_update_stats_rx_duration() vs.
> > ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too]
> >
> > > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > > > index c893314a191f..c7ec7b9e9b55 100644
> > > > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > > > return 0;
> > > >
> > > > - stats->extended = true;
> > > > -
> > > > for (i = 0; i < num_peer_stats; i++) {
> > > > const struct wmi_10_4_peer_extd_stats *src;
> > > > - struct ath10k_fw_extd_stats_peer *dst;
> > > > + struct ath10k_fw_stats_peer *dst;
> > > >
> > > > src = (void *)skb->data;
> > > > if (!skb_pull(skb, sizeof(*src)))
> > > > return -EPROTO;
> > > >
> > > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > > > - if (!dst)
> > > > - continue;
> > > > + /* Because the stat data may exceed htc-wmi buffer
> > > > + * limit the firmware might split the stats data
> > > > + * and delivers it in multiple update events.
> > > > + * if we can't find the entry in the current event
> > > > + * payload, we have to look in main list as well.
> > > > + */
>
> [shafi] thanks ! sorry i might have missed this bug, did you happen
> to see this condition being hit ?
This was copied from ath10k_debug_fw_stats_process() [2]. I've added Michal
Kazior to the discussion since his patch "ath10k: fix fw stats processing"
added this in debug.c. I think he knows the firmware internals well enough
to tell if this is a problem or not. As it stands now, it is and will be
in the future.
> > > > + list_for_each_entry(dst, &stats->peers, list) {
> > > > + if (ether_addr_equal(dst->peer_macaddr,
> > > > + src->peer_macaddr.addr))
> > > > + goto found;
> > > > + }
> > > > +
>
> [shafi] Again, we can simply cache the macaddress and rx_duration
> and deal with it later, rather than doing the whole lookup here ?
> Will it be an overhead for large number of clients ?
Well, show me how you would do it more efficiently otherwise? I don't
see how you can cache the macaddress and rx_duration since you are
basically forced to process all the peer stats first and later all
the extended peer stats. They don't mix.
As for how expensive it is: From what I can tell, the 10.4 firmware
sends the stat events every few seconds. So they are not part of any
rx or tx hot-paths. The list_for_each within the for
(i = 0; i < num_peers;i++) is actually in the O(1) class as far
as both loops go. This might sound funny at first, but the number of
extended peer list is limited by TARGET_10_4_MAX_PEER_EXT_STATS to 16.
And thanks to (***) the limit of num_peers is also 16. Furthermore
we can add a if (ath10k_peer_stats_enabled(ar)) guard to skip it
entirely if the stats are disabled.
> > > > +#ifdef CONFIG_ATH10K_DEBUGFS
> > > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > > > + if (ether_addr_equal(dst->peer_macaddr,
> > > > + src->peer_macaddr.addr))
> > > > + goto found;
> > > > + }
> > > > +#endif
>
> [shafi] again, this could be handled seperately.
This is more expensive. As fw_stats.peers can contain more entries than 16.
However, this might be unnecessary if both peers and extended peers stats
entries in the wmi event are always for the same stations.
Note: There's an alternative way too. Instead of writing rx_duration into
ath10k_fw_stats, we could skip the middle man write it directly into
arsta->rx_duration. This would also mean that we can get rid of
ath10k_sta_update_extd_stats_rx_duration(), ath10k_sta_update_stats_rx_duration()
and ath10k_sta_update_rx_duration(). This has the benifit that we can
remove even more.
> > > > +
> > > > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > > > + "Orphaned extended stats entry for station %pM.\n",
> > > > + src->peer_macaddr.addr);
> > > > + continue;
> > > >
> > > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > > > +found:
> > > > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > > > - list_add_tail(&dst->list, &stats->peers_extd);
> > > > }
> > > >
> > > > return 0;
> > >
> > > [shafi] Yes i am bit concerned about this change making 10.4 to update
> > > over the existing peer_stats structure, the idea is to maintain uniformity
> > > between the structures shared between ath10k and its corresponding to avoid
> > > confusion/ bugs in the future. Kindly let me know your thoughts, feel free
> > > to correct me if any of my analysis is incorrect. thank you !
> > Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make
> > a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure)
> > that other functions can use, without caring about if the FW was 10.4
> > or 10.2.4?
> >
> > There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats
> > conveys any different information than the rx_duration in
> > wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make
> > wmi_10_4_peer_extd_stats's rx_duration use the existing field in
> > ath10k_fw_stats_peer? And if not: why exactly?
> >
> [shafi] I will soon test your change and keep you posted. We will also
> get Kalle's take/view on this.
Ok. That said, I added him to the CC from the beginning and so far
he silently agreed.
Regards,
Christian
[0] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L21>
[1] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L40>
[2] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debug.c#L360>
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
@ 2016-12-22 17:58 ` Christian Lamparter
0 siblings, 0 replies; 52+ messages in thread
From: Christian Lamparter @ 2016-12-22 17:58 UTC (permalink / raw)
To: Mohammed Shafi Shajakhan
Cc: Kalle Valo, linux-wireless, michal.kazior, ath10k
Hello Shafi,
On Thursday, December 22, 2016 9:18:01 PM CET Mohammed Shafi Shajakhan wrote:
> > On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote:
> > > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > > > The 10.4 firmware adds extended peer information to the
> > > > firmware's statistics payload. This additional info is
> > > > stored as a separate data field. During review of
> > > > "ath10k: add accounting for the extended peer statistics" [0]
> > > >
> > > > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > > > lists are of little use:"... there is not much use in appending
> > > > the extended peer stats (which gets periodically updated) to the
> > > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > > > rid of the below (and the required cleanup as well)
> > > >
> > > > list_splice_tail_init(&stats.peers_extd,
> > > > &ar->debug.fw_stats.peers_extd);
> > > >
> > > > since rx_duration is getting updated periodically to the per sta
> > > > information."
> > > >
> > > > This patch replaces the extended peers list with a lookup and
> > > > puts the retrieved data (rx_duration) into the existing
> > > > ath10k_fw_stats_peer entry that was created earlier.
> > >
> > > [shafi] Its good to maintain the extended stats variable
> > > and retain the two different functions to update rx_duration.
> > >
> > > a) extended peer stats supported - mainly for 10.4
> > > b) extender peer stats not supported - for 10.2
> > Well, I have to ask why you want to retain the two different
> > functions to update the same arsta->rx_duration? I think a
> > little bit of code that helps to explain what's on your mind
> > (or how you would do it) would help immensely in this case.
> > Since I have the feeling that this is the problem here.
> > So please explain it in C(lang).
>
> [shafi] I see you prefer to stuff the 'rx_duration' from
> the extended stats to the existing global 'ath10k_peer_stats'
> The idea of extended stats is, ideally its not going to stop
> for 'rx_duration' . Extended stats is maintained as a seperate
> list / entry for addition of new fields as well
I'm guessing you are trying to say here:
replace:
dst->rx_duration = __le32_to_cpu(src->rx_duration);
with
dst->rx_duration += __le32_to_cpu(src->rx_duration);
in ath10k_wmi_10_4_op_pull_fw_stats()?
Is this correct? If so then can you tell me why ath10k_wmi_10_4_op_pull_fw_stats()
is using for (i = 0; i < num_peer_stats; i++) to iterate over the extended peer
stats instead of looking up the number of extended peer items. Because this does
imply that there are the "same" (and every peer stat has a matching extended
peer stat)... (This will be important for the comment below - so ***).
Of course, if this isn't true then this is a bug and should be fixed because
otherwise the ath10k_wmi_10_4_op_pull_fw_stats functions might return -EPROTO
at some point which causes a "failed to pull fw stats: -71" and unprocessed/lost
stats.
> >
> > > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > > > ---
> > > > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > > > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > > > 4 files changed, 28 insertions(+), 57 deletions(-)
> > > >
> > > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > > > index 09ff8b8a6441..3fffbbb18c25 100644
> > > > --- a/drivers/net/wireless/ath/ath10k/core.h
> > > > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > > > };
> > > >
> > > > struct ath10k_fw_stats {
> > > > - bool extended;
> > > > struct list_head pdevs;
> > > > struct list_head vdevs;
> > > > struct list_head peers;
> > > > - struct list_head peers_extd;
> > > > };
> > > >
> > > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > > > index 82a4c67f3672..89f7fde77cdf 100644
> > > > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > > > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > > > }
> > > > }
> > > >
> > > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > > > -{
> > > > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > > > -
> > > > - list_for_each_entry_safe(i, tmp, head, list) {
> > > > - list_del(&i->list);
> > > > - kfree(i);
> > > > - }
> > > > -}
> > > > -
> > > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > > > {
> > > > spin_lock_bh(&ar->data_lock);
> > > > ar->debug.fw_stats_done = false;
> > > > - ar->debug.fw_stats.extended = false;
> > >
> > > [shafi] this looks fine, but not removing the 'extended' variable
> > > from ath10k_fw_stats structure, I see the design for 'rx_duration'
> > > arguably some what convoluted !
> > I removed extended because it is now a write-only variable.
> > So I figured, there's no point in keeping it around? I don't have
> > access to the firmware interface documentation, so I don't know
> > if there's a reason why it would be good to have it later.
> > So please tell me, what information do we gain from it?
>
> [shafi] while parsing the stats from 'wmi' we clearly mark there whether
> the extended stats is available (or) not and if its there parse it and
> deal with it seperately
No, there's no difference between stats or extended stats (10.2.4 vs 10.4):
both ath10k_sta_update_extd_stats_rx_duration() [0]
and ath10k_sta_update_stats_rx_duration() [1] are adding the
ath10k_fw_stats_peer(_extd)->rx_duration to ath10k_sta->rx_duration.
With the merge of the peer stats and extended peer stats, this also
removed the only usage of stats->extended. And hence it becomes a
write-only variable. So there's no point in keeping it around ATM (as
all other extended peer stats entries aren't used).
> > > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> > > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> > > {certainly 'stats' object is for this particular update only, and freed
> > > up later)
> > > *Update immediately using 'ath10k_sta_update_rx_duration'
> > >
> > > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> > > for 10.2 and 10.4 (the later supporting extended peer stats)
> >
> > I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats()
> > passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats
> > element which is basically a carbon-copy of wmi_10_2_4_peer_stats
> > (but with one extra __le32 for the rx_duration at the end.)
> > This rx_duration is then used to set the rx_duration field in the
> > generated ath10k_fw_stats_peer struct.
> >
> > 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats
> > element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for
> > the communicating the rx_duration to the driver.
> >
> > Thing is, both function have the same signature. They produce the same
> > struct ath10k_fw_stats_peer for the given data in the sk_buff input. So
> > why does 10.4 need to have it's peer_extd infrastructure, when it can use
> > the existing rx_duration field in the *universal* ath10k_fw_stats_peer?
>
> [shafi] agreed we need to fix that, it would have been easier if 10.2.4
> and 10.4 had the same definitions.
Ok, I don't know the internals of the firmware to know what's the difference
between 10.2.4 and 10.4's rx_duration (how both firmwares define them
differently in this context) ? From what I can tell, it's just that
the entry has moved from the peer stats to extended peer stats.
Of course, this is based on the logic that both 10.2.4 and 10.4 rx_durations
end up being added to arsta->rx_duration in the same way. There's no scaling
or a comment that would indicate a difference.
> >
> > What's with the extra layers / HAL here? Because it looks like it's merged
> > back together in the same manner into the same arsta->rx_duration?
> > [ath10k_sta_update_stats_rx_duration() vs.
> > ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too]
> >
> > > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > > > index c893314a191f..c7ec7b9e9b55 100644
> > > > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > > > return 0;
> > > >
> > > > - stats->extended = true;
> > > > -
> > > > for (i = 0; i < num_peer_stats; i++) {
> > > > const struct wmi_10_4_peer_extd_stats *src;
> > > > - struct ath10k_fw_extd_stats_peer *dst;
> > > > + struct ath10k_fw_stats_peer *dst;
> > > >
> > > > src = (void *)skb->data;
> > > > if (!skb_pull(skb, sizeof(*src)))
> > > > return -EPROTO;
> > > >
> > > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > > > - if (!dst)
> > > > - continue;
> > > > + /* Because the stat data may exceed htc-wmi buffer
> > > > + * limit the firmware might split the stats data
> > > > + * and delivers it in multiple update events.
> > > > + * if we can't find the entry in the current event
> > > > + * payload, we have to look in main list as well.
> > > > + */
>
> [shafi] thanks ! sorry i might have missed this bug, did you happen
> to see this condition being hit ?
This was copied from ath10k_debug_fw_stats_process() [2]. I've added Michal
Kazior to the discussion since his patch "ath10k: fix fw stats processing"
added this in debug.c. I think he knows the firmware internals well enough
to tell if this is a problem or not. As it stands now, it is and will be
in the future.
> > > > + list_for_each_entry(dst, &stats->peers, list) {
> > > > + if (ether_addr_equal(dst->peer_macaddr,
> > > > + src->peer_macaddr.addr))
> > > > + goto found;
> > > > + }
> > > > +
>
> [shafi] Again, we can simply cache the macaddress and rx_duration
> and deal with it later, rather than doing the whole lookup here ?
> Will it be an overhead for large number of clients ?
Well, show me how you would do it more efficiently otherwise? I don't
see how you can cache the macaddress and rx_duration since you are
basically forced to process all the peer stats first and later all
the extended peer stats. They don't mix.
As for how expensive it is: From what I can tell, the 10.4 firmware
sends the stat events every few seconds. So they are not part of any
rx or tx hot-paths. The list_for_each within the for
(i = 0; i < num_peers;i++) is actually in the O(1) class as far
as both loops go. This might sound funny at first, but the number of
extended peer list is limited by TARGET_10_4_MAX_PEER_EXT_STATS to 16.
And thanks to (***) the limit of num_peers is also 16. Furthermore
we can add a if (ath10k_peer_stats_enabled(ar)) guard to skip it
entirely if the stats are disabled.
> > > > +#ifdef CONFIG_ATH10K_DEBUGFS
> > > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > > > + if (ether_addr_equal(dst->peer_macaddr,
> > > > + src->peer_macaddr.addr))
> > > > + goto found;
> > > > + }
> > > > +#endif
>
> [shafi] again, this could be handled seperately.
This is more expensive. As fw_stats.peers can contain more entries than 16.
However, this might be unnecessary if both peers and extended peers stats
entries in the wmi event are always for the same stations.
Note: There's an alternative way too. Instead of writing rx_duration into
ath10k_fw_stats, we could skip the middle man write it directly into
arsta->rx_duration. This would also mean that we can get rid of
ath10k_sta_update_extd_stats_rx_duration(), ath10k_sta_update_stats_rx_duration()
and ath10k_sta_update_rx_duration(). This has the benifit that we can
remove even more.
> > > > +
> > > > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > > > + "Orphaned extended stats entry for station %pM.\n",
> > > > + src->peer_macaddr.addr);
> > > > + continue;
> > > >
> > > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > > > +found:
> > > > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > > > - list_add_tail(&dst->list, &stats->peers_extd);
> > > > }
> > > >
> > > > return 0;
> > >
> > > [shafi] Yes i am bit concerned about this change making 10.4 to update
> > > over the existing peer_stats structure, the idea is to maintain uniformity
> > > between the structures shared between ath10k and its corresponding to avoid
> > > confusion/ bugs in the future. Kindly let me know your thoughts, feel free
> > > to correct me if any of my analysis is incorrect. thank you !
> > Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make
> > a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure)
> > that other functions can use, without caring about if the FW was 10.4
> > or 10.2.4?
> >
> > There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats
> > conveys any different information than the rx_duration in
> > wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make
> > wmi_10_4_peer_extd_stats's rx_duration use the existing field in
> > ath10k_fw_stats_peer? And if not: why exactly?
> >
> [shafi] I will soon test your change and keep you posted. We will also
> get Kalle's take/view on this.
Ok. That said, I added him to the CC from the beginning and so far
he silently agreed.
Regards,
Christian
[0] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L21>
[1] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L40>
[2] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debug.c#L360>
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
2016-12-22 17:58 ` Christian Lamparter
@ 2016-12-29 14:35 ` Mohammed Shafi Shajakhan
-1 siblings, 0 replies; 52+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-12-29 14:35 UTC (permalink / raw)
To: Christian Lamparter; +Cc: linux-wireless, ath10k, Kalle Valo, michal.kazior
Hi Christian,
On Thu, Dec 22, 2016 at 06:58:41PM +0100, Christian Lamparter wrote:
> Hello Shafi,
>
> On Thursday, December 22, 2016 9:18:01 PM CET Mohammed Shafi Shajakhan wrote:
> > > On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote:
> > > > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > > > > The 10.4 firmware adds extended peer information to the
> > > > > firmware's statistics payload. This additional info is
> > > > > stored as a separate data field. During review of
> > > > > "ath10k: add accounting for the extended peer statistics" [0]
> > > > >
> > > > > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > > > > lists are of little use:"... there is not much use in appending
> > > > > the extended peer stats (which gets periodically updated) to the
> > > > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > > > > rid of the below (and the required cleanup as well)
> > > > >
> > > > > list_splice_tail_init(&stats.peers_extd,
> > > > > &ar->debug.fw_stats.peers_extd);
> > > > >
> > > > > since rx_duration is getting updated periodically to the per sta
> > > > > information."
> > > > >
> > > > > This patch replaces the extended peers list with a lookup and
> > > > > puts the retrieved data (rx_duration) into the existing
> > > > > ath10k_fw_stats_peer entry that was created earlier.
> > > >
> > > > [shafi] Its good to maintain the extended stats variable
> > > > and retain the two different functions to update rx_duration.
> > > >
> > > > a) extended peer stats supported - mainly for 10.4
> > > > b) extender peer stats not supported - for 10.2
> > > Well, I have to ask why you want to retain the two different
> > > functions to update the same arsta->rx_duration? I think a
> > > little bit of code that helps to explain what's on your mind
> > > (or how you would do it) would help immensely in this case.
> > > Since I have the feeling that this is the problem here.
> > > So please explain it in C(lang).
> >
> > [shafi] I see you prefer to stuff the 'rx_duration' from
> > the extended stats to the existing global 'ath10k_peer_stats'
> > The idea of extended stats is, ideally its not going to stop
> > for 'rx_duration' . Extended stats is maintained as a seperate
> > list / entry for addition of new fields as well
> I'm guessing you are trying to say here:
>
> replace:
>
> dst->rx_duration = __le32_to_cpu(src->rx_duration);
>
> with
>
> dst->rx_duration += __le32_to_cpu(src->rx_duration);
>
> in ath10k_wmi_10_4_op_pull_fw_stats()?
[shafi] oh no sorry, I am trying to say more members related
to stats shall be added in extended stats structure . The extended
stats structure is specifically introduced for adding more stats related
variables.
>
> Is this correct? If so then can you tell me why ath10k_wmi_10_4_op_pull_fw_stats()
> is using for (i = 0; i < num_peer_stats; i++) to iterate over the extended peer
> stats instead of looking up the number of extended peer items. Because this does
> imply that there are the "same" (and every peer stat has a matching extended
> peer stat)... (This will be important for the comment below - so ***).
> Of course, if this isn't true then this is a bug and should be fixed because
> otherwise the ath10k_wmi_10_4_op_pull_fw_stats functions might return -EPROTO
> at some point which causes a "failed to pull fw stats: -71" and unprocessed/lost
> stats.
[shafi] sorry i am not sure I got you, have you come across this error, please
let me know ?
> > >
> > > > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > > > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > > > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > > > > ---
> > > > > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > > > > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > > > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > > > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > > > > 4 files changed, 28 insertions(+), 57 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > > > > index 09ff8b8a6441..3fffbbb18c25 100644
> > > > > --- a/drivers/net/wireless/ath/ath10k/core.h
> > > > > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > > > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > > > > };
> > > > >
> > > > > struct ath10k_fw_stats {
> > > > > - bool extended;
> > > > > struct list_head pdevs;
> > > > > struct list_head vdevs;
> > > > > struct list_head peers;
> > > > > - struct list_head peers_extd;
> > > > > };
> > > > >
> > > > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > > > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > > > > index 82a4c67f3672..89f7fde77cdf 100644
> > > > > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > > > > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > > > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > > > > }
> > > > > }
> > > > >
> > > > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > > > > -{
> > > > > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > > > > -
> > > > > - list_for_each_entry_safe(i, tmp, head, list) {
> > > > > - list_del(&i->list);
> > > > > - kfree(i);
> > > > > - }
> > > > > -}
> > > > > -
> > > > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > > > > {
> > > > > spin_lock_bh(&ar->data_lock);
> > > > > ar->debug.fw_stats_done = false;
> > > > > - ar->debug.fw_stats.extended = false;
> > > >
> > > > [shafi] this looks fine, but not removing the 'extended' variable
> > > > from ath10k_fw_stats structure, I see the design for 'rx_duration'
> > > > arguably some what convoluted !
> > > I removed extended because it is now a write-only variable.
> > > So I figured, there's no point in keeping it around? I don't have
> > > access to the firmware interface documentation, so I don't know
> > > if there's a reason why it would be good to have it later.
> > > So please tell me, what information do we gain from it?
[shafi] sorry this is purely ath10k specific nothing to map with the firmware
> >
> > [shafi] while parsing the stats from 'wmi' we clearly mark there whether
> > the extended stats is available (or) not and if its there parse it and
> > deal with it seperately
> No, there's no difference between stats or extended stats (10.2.4 vs 10.4):
> both ath10k_sta_update_extd_stats_rx_duration() [0]
> and ath10k_sta_update_stats_rx_duration() [1] are adding the
> ath10k_fw_stats_peer(_extd)->rx_duration to ath10k_sta->rx_duration.
>
> With the merge of the peer stats and extended peer stats, this also
> removed the only usage of stats->extended. And hence it becomes a
> write-only variable. So there's no point in keeping it around ATM (as
> all other extended peer stats entries aren't used).
[shafi] ok, I see the extended stats structure is introduced for 10.4 and i was
thinking its good to have two seperate API's for updating the rx_duration ..
>
> > > > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> > > > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> > > > {certainly 'stats' object is for this particular update only, and freed
> > > > up later)
> > > > *Update immediately using 'ath10k_sta_update_rx_duration'
> > > >
> > > > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> > > > for 10.2 and 10.4 (the later supporting extended peer stats)
> > >
> > > I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats()
> > > passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats
> > > element which is basically a carbon-copy of wmi_10_2_4_peer_stats
> > > (but with one extra __le32 for the rx_duration at the end.)
> > > This rx_duration is then used to set the rx_duration field in the
> > > generated ath10k_fw_stats_peer struct.
[shafi] ok
> > >
> > > 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats
> > > element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for
> > > the communicating the rx_duration to the driver.
[shafi] ok
> > >
> > > Thing is, both function have the same signature. They produce the same
> > > struct ath10k_fw_stats_peer for the given data in the sk_buff input. So
> > > why does 10.4 need to have it's peer_extd infrastructure, when it can use
> > > the existing rx_duration field in the *universal* ath10k_fw_stats_peer?
> >
> > [shafi] agreed we need to fix that, it would have been easier if 10.2.4
> > and 10.4 had the same definitions.
> Ok, I don't know the internals of the firmware to know what's the difference
> between 10.2.4 and 10.4's rx_duration (how both firmwares define them
> differently in this context) ? From what I can tell, it's just that
> the entry has moved from the peer stats to extended peer stats.
> Of course, this is based on the logic that both 10.2.4 and 10.4 rx_durations
> end up being added to arsta->rx_duration in the same way. There's no scaling
> or a comment that would indicate a difference.
[shafi] ok
>
> > >
> > > What's with the extra layers / HAL here? Because it looks like it's merged
> > > back together in the same manner into the same arsta->rx_duration?
> > > [ath10k_sta_update_stats_rx_duration() vs.
> > > ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too]
[shafi] my concern was for updating 'rx_duration' we just iterate over the list
and update the same.
> > >
> > > > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > > > > index c893314a191f..c7ec7b9e9b55 100644
> > > > > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > > > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > > > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > > > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > > > > return 0;
> > > > >
> > > > > - stats->extended = true;
> > > > > -
> > > > > for (i = 0; i < num_peer_stats; i++) {
> > > > > const struct wmi_10_4_peer_extd_stats *src;
> > > > > - struct ath10k_fw_extd_stats_peer *dst;
> > > > > + struct ath10k_fw_stats_peer *dst;
> > > > >
> > > > > src = (void *)skb->data;
> > > > > if (!skb_pull(skb, sizeof(*src)))
> > > > > return -EPROTO;
> > > > >
> > > > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > > > > - if (!dst)
> > > > > - continue;
> > > > > + /* Because the stat data may exceed htc-wmi buffer
> > > > > + * limit the firmware might split the stats data
> > > > > + * and delivers it in multiple update events.
> > > > > + * if we can't find the entry in the current event
> > > > > + * payload, we have to look in main list as well.
> > > > > + */
> >
> > [shafi] thanks ! sorry i might have missed this bug, did you happen
> > to see this condition being hit ?
> This was copied from ath10k_debug_fw_stats_process() [2]. I've added Michal
> Kazior to the discussion since his patch "ath10k: fix fw stats processing"
> added this in debug.c. I think he knows the firmware internals well enough
> to tell if this is a problem or not. As it stands now, it is and will be
> in the future.
[shafi] sure.
>
> > > > > + list_for_each_entry(dst, &stats->peers, list) {
> > > > > + if (ether_addr_equal(dst->peer_macaddr,
> > > > > + src->peer_macaddr.addr))
> > > > > + goto found;
> > > > > + }
> > > > > +
> >
> > [shafi] Again, we can simply cache the macaddress and rx_duration
> > and deal with it later, rather than doing the whole lookup here ?
> > Will it be an overhead for large number of clients ?
> Well, show me how you would do it more efficiently otherwise? I don't
> see how you can cache the macaddress and rx_duration since you are
> basically forced to process all the peer stats first and later all
> the extended peer stats. They don't mix.
[shafi] hmmm, ok.
>
> As for how expensive it is: From what I can tell, the 10.4 firmware
> sends the stat events every few seconds. So they are not part of any
> rx or tx hot-paths. The list_for_each within the for
> (i = 0; i < num_peers;i++) is actually in the O(1) class as far
> as both loops go. This might sound funny at first, but the number of
> extended peer list is limited by TARGET_10_4_MAX_PEER_EXT_STATS to 16.
> And thanks to (***) the limit of num_peers is also 16. Furthermore
> we can add a if (ath10k_peer_stats_enabled(ar)) guard to skip it
> entirely if the stats are disabled.
[shafi] ok.
>
>
> > > > > +#ifdef CONFIG_ATH10K_DEBUGFS
> > > > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > > > > + if (ether_addr_equal(dst->peer_macaddr,
> > > > > + src->peer_macaddr.addr))
> > > > > + goto found;
> > > > > + }
> > > > > +#endif
> >
> > [shafi] again, this could be handled seperately.
> This is more expensive. As fw_stats.peers can contain more entries than 16.
> However, this might be unnecessary if both peers and extended peers stats
> entries in the wmi event are always for the same stations.
[shafi] ok
>
> Note: There's an alternative way too. Instead of writing rx_duration into
> ath10k_fw_stats, we could skip the middle man write it directly into
> arsta->rx_duration. This would also mean that we can get rid of
> ath10k_sta_update_extd_stats_rx_duration(), ath10k_sta_update_stats_rx_duration()
> and ath10k_sta_update_rx_duration(). This has the benifit that we can
> remove even more.
>
> > > > > +
> > > > > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > > > > + "Orphaned extended stats entry for station %pM.\n",
> > > > > + src->peer_macaddr.addr);
> > > > > + continue;
> > > > >
> > > > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > > > > +found:
> > > > > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > > > > - list_add_tail(&dst->list, &stats->peers_extd);
> > > > > }
> > > > >
> > > > > return 0;
> > > >
> > > > [shafi] Yes i am bit concerned about this change making 10.4 to update
> > > > over the existing peer_stats structure, the idea is to maintain uniformity
> > > > between the structures shared between ath10k and its corresponding to avoid
> > > > confusion/ bugs in the future. Kindly let me know your thoughts, feel free
> > > > to correct me if any of my analysis is incorrect. thank you !
> > > Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make
> > > a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure)
> > > that other functions can use, without caring about if the FW was 10.4
> > > or 10.2.4?
> > >
> > > There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats
> > > conveys any different information than the rx_duration in
> > > wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make
> > > wmi_10_4_peer_extd_stats's rx_duration use the existing field in
> > > ath10k_fw_stats_peer? And if not: why exactly?
> > >
> > [shafi] I will soon test your change and keep you posted. We will also
> > get Kalle's take/view on this.
> Ok. That said, I added him to the CC from the beginning and so far
> he silently agreed.
[shafi] sure, I guess i quickly tested this change and I found things
are fine, it would be good if you can share your test results as well please
Yes we can discuss with Kalle and Michal ! If you guys think this is a more
optimized version of updating the peer stats, I am fine with that as well.
There is no doubt you guys know about open source better than me :-)
>
> Regards,
> Christian
>
> [0] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L21>
> [1] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L40>
> [2] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debug.c#L360>
>
>
^ permalink raw reply [flat|nested] 52+ messages in thread* Re: [PATCH] ath10k: merge extended peer info data with existing peers info
@ 2016-12-29 14:35 ` Mohammed Shafi Shajakhan
0 siblings, 0 replies; 52+ messages in thread
From: Mohammed Shafi Shajakhan @ 2016-12-29 14:35 UTC (permalink / raw)
To: Christian Lamparter; +Cc: Kalle Valo, linux-wireless, michal.kazior, ath10k
Hi Christian,
On Thu, Dec 22, 2016 at 06:58:41PM +0100, Christian Lamparter wrote:
> Hello Shafi,
>
> On Thursday, December 22, 2016 9:18:01 PM CET Mohammed Shafi Shajakhan wrote:
> > > On Monday, December 19, 2016 10:19:57 PM CET Mohammed Shafi Shajakhan wrote:
> > > > On Sat, Dec 17, 2016 at 06:46:34PM +0100, Christian Lamparter wrote:
> > > > > The 10.4 firmware adds extended peer information to the
> > > > > firmware's statistics payload. This additional info is
> > > > > stored as a separate data field. During review of
> > > > > "ath10k: add accounting for the extended peer statistics" [0]
> > > > >
> > > > > Mohammed Shafi Shajakhan commented that the extended peer statistics
> > > > > lists are of little use:"... there is not much use in appending
> > > > > the extended peer stats (which gets periodically updated) to the
> > > > > linked list '&ar->debug.fw_stats.peers_extd)' and should we get
> > > > > rid of the below (and the required cleanup as well)
> > > > >
> > > > > list_splice_tail_init(&stats.peers_extd,
> > > > > &ar->debug.fw_stats.peers_extd);
> > > > >
> > > > > since rx_duration is getting updated periodically to the per sta
> > > > > information."
> > > > >
> > > > > This patch replaces the extended peers list with a lookup and
> > > > > puts the retrieved data (rx_duration) into the existing
> > > > > ath10k_fw_stats_peer entry that was created earlier.
> > > >
> > > > [shafi] Its good to maintain the extended stats variable
> > > > and retain the two different functions to update rx_duration.
> > > >
> > > > a) extended peer stats supported - mainly for 10.4
> > > > b) extender peer stats not supported - for 10.2
> > > Well, I have to ask why you want to retain the two different
> > > functions to update the same arsta->rx_duration? I think a
> > > little bit of code that helps to explain what's on your mind
> > > (or how you would do it) would help immensely in this case.
> > > Since I have the feeling that this is the problem here.
> > > So please explain it in C(lang).
> >
> > [shafi] I see you prefer to stuff the 'rx_duration' from
> > the extended stats to the existing global 'ath10k_peer_stats'
> > The idea of extended stats is, ideally its not going to stop
> > for 'rx_duration' . Extended stats is maintained as a seperate
> > list / entry for addition of new fields as well
> I'm guessing you are trying to say here:
>
> replace:
>
> dst->rx_duration = __le32_to_cpu(src->rx_duration);
>
> with
>
> dst->rx_duration += __le32_to_cpu(src->rx_duration);
>
> in ath10k_wmi_10_4_op_pull_fw_stats()?
[shafi] oh no sorry, I am trying to say more members related
to stats shall be added in extended stats structure . The extended
stats structure is specifically introduced for adding more stats related
variables.
>
> Is this correct? If so then can you tell me why ath10k_wmi_10_4_op_pull_fw_stats()
> is using for (i = 0; i < num_peer_stats; i++) to iterate over the extended peer
> stats instead of looking up the number of extended peer items. Because this does
> imply that there are the "same" (and every peer stat has a matching extended
> peer stat)... (This will be important for the comment below - so ***).
> Of course, if this isn't true then this is a bug and should be fixed because
> otherwise the ath10k_wmi_10_4_op_pull_fw_stats functions might return -EPROTO
> at some point which causes a "failed to pull fw stats: -71" and unprocessed/lost
> stats.
[shafi] sorry i am not sure I got you, have you come across this error, please
let me know ?
> > >
> > > > > [0] <https://lkml.kernel.org/r/992a4e2676037a06f482cdbe2d3d39e287530be5.1480974623.git.chunkeey@googlemail.com>
> > > > > Cc: Mohammed Shafi Shajakhan <mohammed@codeaurora.org>
> > > > > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> > > > > ---
> > > > > drivers/net/wireless/ath/ath10k/core.h | 2 --
> > > > > drivers/net/wireless/ath/ath10k/debug.c | 17 --------------
> > > > > drivers/net/wireless/ath/ath10k/debugfs_sta.c | 32 ++-----------------------
> > > > > drivers/net/wireless/ath/ath10k/wmi.c | 34 ++++++++++++++++++++-------
> > > > > 4 files changed, 28 insertions(+), 57 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/wireless/ath/ath10k/core.h b/drivers/net/wireless/ath/ath10k/core.h
> > > > > index 09ff8b8a6441..3fffbbb18c25 100644
> > > > > --- a/drivers/net/wireless/ath/ath10k/core.h
> > > > > +++ b/drivers/net/wireless/ath/ath10k/core.h
> > > > > @@ -268,11 +268,9 @@ struct ath10k_fw_stats_pdev {
> > > > > };
> > > > >
> > > > > struct ath10k_fw_stats {
> > > > > - bool extended;
> > > > > struct list_head pdevs;
> > > > > struct list_head vdevs;
> > > > > struct list_head peers;
> > > > > - struct list_head peers_extd;
> > > > > };
> > > > >
> > > > > #define ATH10K_TPC_TABLE_TYPE_FLAG 1
> > > > > diff --git a/drivers/net/wireless/ath/ath10k/debug.c b/drivers/net/wireless/ath/ath10k/debug.c
> > > > > index 82a4c67f3672..89f7fde77cdf 100644
> > > > > --- a/drivers/net/wireless/ath/ath10k/debug.c
> > > > > +++ b/drivers/net/wireless/ath/ath10k/debug.c
> > > > > @@ -315,25 +315,13 @@ static void ath10k_fw_stats_peers_free(struct list_head *head)
> > > > > }
> > > > > }
> > > > >
> > > > > -static void ath10k_fw_extd_stats_peers_free(struct list_head *head)
> > > > > -{
> > > > > - struct ath10k_fw_extd_stats_peer *i, *tmp;
> > > > > -
> > > > > - list_for_each_entry_safe(i, tmp, head, list) {
> > > > > - list_del(&i->list);
> > > > > - kfree(i);
> > > > > - }
> > > > > -}
> > > > > -
> > > > > static void ath10k_debug_fw_stats_reset(struct ath10k *ar)
> > > > > {
> > > > > spin_lock_bh(&ar->data_lock);
> > > > > ar->debug.fw_stats_done = false;
> > > > > - ar->debug.fw_stats.extended = false;
> > > >
> > > > [shafi] this looks fine, but not removing the 'extended' variable
> > > > from ath10k_fw_stats structure, I see the design for 'rx_duration'
> > > > arguably some what convoluted !
> > > I removed extended because it is now a write-only variable.
> > > So I figured, there's no point in keeping it around? I don't have
> > > access to the firmware interface documentation, so I don't know
> > > if there's a reason why it would be good to have it later.
> > > So please tell me, what information do we gain from it?
[shafi] sorry this is purely ath10k specific nothing to map with the firmware
> >
> > [shafi] while parsing the stats from 'wmi' we clearly mark there whether
> > the extended stats is available (or) not and if its there parse it and
> > deal with it seperately
> No, there's no difference between stats or extended stats (10.2.4 vs 10.4):
> both ath10k_sta_update_extd_stats_rx_duration() [0]
> and ath10k_sta_update_stats_rx_duration() [1] are adding the
> ath10k_fw_stats_peer(_extd)->rx_duration to ath10k_sta->rx_duration.
>
> With the merge of the peer stats and extended peer stats, this also
> removed the only usage of stats->extended. And hence it becomes a
> write-only variable. So there's no point in keeping it around ATM (as
> all other extended peer stats entries aren't used).
[shafi] ok, I see the extended stats structure is introduced for 10.4 and i was
thinking its good to have two seperate API's for updating the rx_duration ..
>
> > > > *We get periodic events from firmware updating 'ath10k_debug_fw_stats_process'
> > > > *Fetch rx_duration from 'ath10k_wmi_pull_fw_stats(ar, skb, &stats)'
> > > > {certainly 'stats' object is for this particular update only, and freed
> > > > up later)
> > > > *Update immediately using 'ath10k_sta_update_rx_duration'
> > > >
> > > > 'ath10k_wmi_pull_fw_stats' has a slightly different implementation
> > > > for 10.2 and 10.4 (the later supporting extended peer stats)
> > >
> > > I see that 10.2.4's ath10k_wmi_10_2_4_op_pull_fw_stats()
> > > passes the rx_duration as part of the wmi_10_2_4_ext_peer_stats
> > > element which is basically a carbon-copy of wmi_10_2_4_peer_stats
> > > (but with one extra __le32 for the rx_duration at the end.)
> > > This rx_duration is then used to set the rx_duration field in the
> > > generated ath10k_fw_stats_peer struct.
[shafi] ok
> > >
> > > 10.4's ath10k_wmi_10_4_op_pull_fw_stats() has a "fixed" wmi_10_4_peer_stats
> > > element and uses an separate "fixed" wmi_10_4_peer_extd_stats element for
> > > the communicating the rx_duration to the driver.
[shafi] ok
> > >
> > > Thing is, both function have the same signature. They produce the same
> > > struct ath10k_fw_stats_peer for the given data in the sk_buff input. So
> > > why does 10.4 need to have it's peer_extd infrastructure, when it can use
> > > the existing rx_duration field in the *universal* ath10k_fw_stats_peer?
> >
> > [shafi] agreed we need to fix that, it would have been easier if 10.2.4
> > and 10.4 had the same definitions.
> Ok, I don't know the internals of the firmware to know what's the difference
> between 10.2.4 and 10.4's rx_duration (how both firmwares define them
> differently in this context) ? From what I can tell, it's just that
> the entry has moved from the peer stats to extended peer stats.
> Of course, this is based on the logic that both 10.2.4 and 10.4 rx_durations
> end up being added to arsta->rx_duration in the same way. There's no scaling
> or a comment that would indicate a difference.
[shafi] ok
>
> > >
> > > What's with the extra layers / HAL here? Because it looks like it's merged
> > > back together in the same manner into the same arsta->rx_duration?
> > > [ath10k_sta_update_stats_rx_duration() vs.
> > > ath10k_sta_update_extd_stats_rx_duration() - they are almost carbon copies too]
[shafi] my concern was for updating 'rx_duration' we just iterate over the list
and update the same.
> > >
> > > > > diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
> > > > > index c893314a191f..c7ec7b9e9b55 100644
> > > > > --- a/drivers/net/wireless/ath/ath10k/wmi.c
> > > > > +++ b/drivers/net/wireless/ath/ath10k/wmi.c
> > > > > @@ -3044,23 +3044,41 @@ static int ath10k_wmi_10_4_op_pull_fw_stats(struct ath10k *ar,
> > > > > if ((stats_id & WMI_10_4_STAT_PEER_EXTD) == 0)
> > > > > return 0;
> > > > >
> > > > > - stats->extended = true;
> > > > > -
> > > > > for (i = 0; i < num_peer_stats; i++) {
> > > > > const struct wmi_10_4_peer_extd_stats *src;
> > > > > - struct ath10k_fw_extd_stats_peer *dst;
> > > > > + struct ath10k_fw_stats_peer *dst;
> > > > >
> > > > > src = (void *)skb->data;
> > > > > if (!skb_pull(skb, sizeof(*src)))
> > > > > return -EPROTO;
> > > > >
> > > > > - dst = kzalloc(sizeof(*dst), GFP_ATOMIC);
> > > > > - if (!dst)
> > > > > - continue;
> > > > > + /* Because the stat data may exceed htc-wmi buffer
> > > > > + * limit the firmware might split the stats data
> > > > > + * and delivers it in multiple update events.
> > > > > + * if we can't find the entry in the current event
> > > > > + * payload, we have to look in main list as well.
> > > > > + */
> >
> > [shafi] thanks ! sorry i might have missed this bug, did you happen
> > to see this condition being hit ?
> This was copied from ath10k_debug_fw_stats_process() [2]. I've added Michal
> Kazior to the discussion since his patch "ath10k: fix fw stats processing"
> added this in debug.c. I think he knows the firmware internals well enough
> to tell if this is a problem or not. As it stands now, it is and will be
> in the future.
[shafi] sure.
>
> > > > > + list_for_each_entry(dst, &stats->peers, list) {
> > > > > + if (ether_addr_equal(dst->peer_macaddr,
> > > > > + src->peer_macaddr.addr))
> > > > > + goto found;
> > > > > + }
> > > > > +
> >
> > [shafi] Again, we can simply cache the macaddress and rx_duration
> > and deal with it later, rather than doing the whole lookup here ?
> > Will it be an overhead for large number of clients ?
> Well, show me how you would do it more efficiently otherwise? I don't
> see how you can cache the macaddress and rx_duration since you are
> basically forced to process all the peer stats first and later all
> the extended peer stats. They don't mix.
[shafi] hmmm, ok.
>
> As for how expensive it is: From what I can tell, the 10.4 firmware
> sends the stat events every few seconds. So they are not part of any
> rx or tx hot-paths. The list_for_each within the for
> (i = 0; i < num_peers;i++) is actually in the O(1) class as far
> as both loops go. This might sound funny at first, but the number of
> extended peer list is limited by TARGET_10_4_MAX_PEER_EXT_STATS to 16.
> And thanks to (***) the limit of num_peers is also 16. Furthermore
> we can add a if (ath10k_peer_stats_enabled(ar)) guard to skip it
> entirely if the stats are disabled.
[shafi] ok.
>
>
> > > > > +#ifdef CONFIG_ATH10K_DEBUGFS
> > > > > + list_for_each_entry(dst, &ar->debug.fw_stats.peers, list) {
> > > > > + if (ether_addr_equal(dst->peer_macaddr,
> > > > > + src->peer_macaddr.addr))
> > > > > + goto found;
> > > > > + }
> > > > > +#endif
> >
> > [shafi] again, this could be handled seperately.
> This is more expensive. As fw_stats.peers can contain more entries than 16.
> However, this might be unnecessary if both peers and extended peers stats
> entries in the wmi event are always for the same stations.
[shafi] ok
>
> Note: There's an alternative way too. Instead of writing rx_duration into
> ath10k_fw_stats, we could skip the middle man write it directly into
> arsta->rx_duration. This would also mean that we can get rid of
> ath10k_sta_update_extd_stats_rx_duration(), ath10k_sta_update_stats_rx_duration()
> and ath10k_sta_update_rx_duration(). This has the benifit that we can
> remove even more.
>
> > > > > +
> > > > > + ath10k_dbg(ar, ATH10K_DBG_WMI,
> > > > > + "Orphaned extended stats entry for station %pM.\n",
> > > > > + src->peer_macaddr.addr);
> > > > > + continue;
> > > > >
> > > > > - ether_addr_copy(dst->peer_macaddr, src->peer_macaddr.addr);
> > > > > +found:
> > > > > dst->rx_duration = __le32_to_cpu(src->rx_duration);
> > > > > - list_add_tail(&dst->list, &stats->peers_extd);
> > > > > }
> > > > >
> > > > > return 0;
> > > >
> > > > [shafi] Yes i am bit concerned about this change making 10.4 to update
> > > > over the existing peer_stats structure, the idea is to maintain uniformity
> > > > between the structures shared between ath10k and its corresponding to avoid
> > > > confusion/ bugs in the future. Kindly let me know your thoughts, feel free
> > > > to correct me if any of my analysis is incorrect. thank you !
> > > Isn't the point of the ath10k_wmi_10_*_op_pull_fw_stats() functions to make
> > > a "universal" statistic (in your case a unified ath10k_fw_stats_peer structure)
> > > that other functions can use, without caring about if the FW was 10.4
> > > or 10.2.4?
> > >
> > > There's no indication that the rx_duration field in wmi_10_2_4_ext_peer_stats
> > > conveys any different information than the rx_duration in
> > > wmi_10_4_peer_extd_stats. So, what's going on here? Can't you just make
> > > wmi_10_4_peer_extd_stats's rx_duration use the existing field in
> > > ath10k_fw_stats_peer? And if not: why exactly?
> > >
> > [shafi] I will soon test your change and keep you posted. We will also
> > get Kalle's take/view on this.
> Ok. That said, I added him to the CC from the beginning and so far
> he silently agreed.
[shafi] sure, I guess i quickly tested this change and I found things
are fine, it would be good if you can share your test results as well please
Yes we can discuss with Kalle and Michal ! If you guys think this is a more
optimized version of updating the peer stats, I am fine with that as well.
There is no doubt you guys know about open source better than me :-)
>
> Regards,
> Christian
>
> [0] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L21>
> [1] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debugfs_sta.c#L40>
> [2] <http://lxr.free-electrons.com/source/drivers/net/wireless/ath/ath10k/debug.c#L360>
>
>
_______________________________________________
ath10k mailing list
ath10k@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/ath10k
^ permalink raw reply [flat|nested] 52+ messages in thread