[PATCH] mac80211: Return avg sig, rx, tx values in ethtool stats.

[PATCH] mac80211: Return avg sig, rx, tx values in ethtool stats.

[greearb]

From: Ben Greear <greearb@candelatech.com>

For non-station devices.  This gives at least some useful
summary in some cases (especially when we know AP has only one
station attached, for instance).

Signed-off-by: Ben Greear <greearb@candelatech.com>
---

This is against 4.7.10+, applied to 4.4 and 4.9 for me as well.

 net/mac80211/ethtool.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/net/mac80211/ethtool.c b/net/mac80211/ethtool.c
index 4e937c1..dc6f76f 100644
--- a/net/mac80211/ethtool.c
+++ b/net/mac80211/ethtool.c
@@ -12,6 +12,25 @@
 #include "ieee80211_i.h"
 #include "sta_info.h"
 #include "driver-ops.h"
+#include <asm/div64.h>
+
+static inline __s64 mac_div(__s64 n, __u32 base)
+{
+	if (n < 0) {
+		__u64 tmp = -n;
+		do_div(tmp, base);
+		/* printk("pktgen: pg_div, n: %llu  base: %d  rv: %llu\n",
+		   n, base, tmp); */
+		return -tmp;
+	}
+	else {
+		__u64 tmp = n;
+		do_div(tmp, base);
+		/* printk("pktgen: pg_div, n: %llu  base: %d  rv: %llu\n",
+		   n, base, tmp); */
+		return tmp;
+	}
+}
 
 static int ieee80211_set_ringparam(struct net_device *dev,
 				   struct ethtool_ringparam *rp)
@@ -128,6 +147,12 @@ static void ieee80211_get_stats(struct net_device *dev,
 			data[i] = (u8)sinfo.signal_avg;
 		i++;
 	} else {
+		int amt_tx = 0;
+		int amt_rx = 0;
+		int amt_sig = 0;
+		s64 tx_accum = 0;
+		s64 rx_accum = 0;
+		s64 sig_accum = 0;
 		list_for_each_entry(sta, &local->sta_list, list) {
 			/* Make sure this station belongs to the proper dev */
 			if (sta->sdata->dev != dev)
@@ -137,6 +162,37 @@ static void ieee80211_get_stats(struct net_device *dev,
 			sta_set_sinfo(sta, &sinfo);
 			i = 0;
 			ADD_STA_STATS(sta);
+
+			i++; /* skip sta state */
+
+			if (sinfo.filled & BIT(NL80211_STA_INFO_TX_BITRATE)) {
+				tx_accum += 100000 *
+					cfg80211_calculate_bitrate(&sinfo.txrate);
+				amt_tx++;
+				data[i] = mac_div(tx_accum, amt_tx);
+			}
+			i++;
+
+			if (sinfo.filled & BIT(NL80211_STA_INFO_RX_BITRATE)) {
+				rx_accum += 100000 *
+					cfg80211_calculate_bitrate(&sinfo.rxrate);
+				amt_rx++;
+				data[i] = mac_div(rx_accum, amt_rx);
+			}
+			i++;
+
+			if (sinfo.filled & BIT(NL80211_STA_INFO_SIGNAL_AVG)) {
+				sig_accum += sinfo.signal_avg;
+				amt_sig++;
+				data[i] = (mac_div(sig_accum, amt_sig) & 0xFF);
+			}
+			i++;
+
+			/*pr_err("sta: %p (%s) sig_accum: %ld  amt-sig: %d filled: 0x%x:%08x rpt-sig-avg: %d  i: %d  div: %d sinfo.signal_avg: %d\n",
+			       sta, sta->sdata->name, (long)(sig_accum), amt_sig, (u32)(sinfo.filled >> 32),
+			       (u32)(sinfo.filled), (u32)(data[i-1]), i-1, (u32)(mac_div(sig_accum, amt_sig)),
+			       sinfo.signal_avg);*/
+
 		}
 	}
 
-- 
2.4.11

Re: [PATCH] mac80211: Return avg sig, rx, tx values in ethtool stats.

[Johannes Berg]

On Tue, 2016-11-29 at 10:07 -0800, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> For non-station devices.  This gives at least some useful
> summary in some cases (especially when we know AP has only one
> station attached, for instance).

I never saw the point in having ethtool statistics for something you
can get elsewhere, but I'm certainly not making it return a random
station's data disregarding everything else. At least the existing ones
are sums so that all stations are taken into account...

You really should just get per-station stuff through nl80211.

johannes

Re: [PATCH] mac80211: Return avg sig, rx, tx values in ethtool stats.

[Ben Greear]

On 12/05/2016 06:08 AM, Johannes Berg wrote:
> On Tue, 2016-11-29 at 10:07 -0800, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> For non-station devices.  This gives at least some useful
>> summary in some cases (especially when we know AP has only one
>> station attached, for instance).
>
> I never saw the point in having ethtool statistics for something you
> can get elsewhere, but I'm certainly not making it return a random
> station's data disregarding everything else. At least the existing ones
> are sums so that all stations are taken into account...

Unless I screwed up, this patch also returns an average.

But, easy enough to carry out of tree, and my use case for this
is pretty specific.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] mac80211: Return avg sig, rx, tx values in ethtool stats.

[Johannes Berg]

> Unless I screwed up, this patch also returns an average.

Oops, sorry. I missed the whole mac_div() indirection thing.

I'm not super convinced anyway though - all of this data already is
available in a much more reliable fashion, even trackable when stations
are removed (all data gets sent in the DEL_STATION notification), so
adding a crippled way to get the same data seems a bit strange?

In any case I'd want you to resend with the /* pr_..*/ stuff removed.

johannes

Re: [PATCH] mac80211: Return avg sig, rx, tx values in ethtool stats.

[Ben Greear]

On 12/05/2016 06:53 AM, Johannes Berg wrote:
>
>> Unless I screwed up, this patch also returns an average.
>
> Oops, sorry. I missed the whole mac_div() indirection thing.
>
> I'm not super convinced anyway though - all of this data already is
> available in a much more reliable fashion, even trackable when stations
> are removed (all data gets sent in the DEL_STATION notification), so
> adding a crippled way to get the same data seems a bit strange?

Ethtool stats are easy to program against, and I am already making the
get-stats call to get other things, so it was a quick tweak to return
these additional values.  I agree the stats are of somewhat limited worth,
but the cost to get them is also pretty small code wise :)

> In any case I'd want you to resend with the /* pr_..*/ stuff removed.

I can respin with the comment removed.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: [PATCH] mac80211: Return avg sig, rx, tx values in ethtool stats.

[Felix Fietkau]

On 2016-12-05 16:01, Ben Greear wrote:
> 
> 
> On 12/05/2016 06:53 AM, Johannes Berg wrote:
>>
>>> Unless I screwed up, this patch also returns an average.
>>
>> Oops, sorry. I missed the whole mac_div() indirection thing.
>>
>> I'm not super convinced anyway though - all of this data already is
>> available in a much more reliable fashion, even trackable when stations
>> are removed (all data gets sent in the DEL_STATION notification), so
>> adding a crippled way to get the same data seems a bit strange?
> 
> Ethtool stats are easy to program against, and I am already making the
> get-stats call to get other things, so it was a quick tweak to return
> these additional values.  I agree the stats are of somewhat limited worth,
> but the cost to get them is also pretty small code wise :)
You're still adding bloat for everybody for the sake of a little
convenience on your side. I think that's a bad trade-off.
nl80211 is not *that* hard to program against...

I don't really see much point in duplicating an arbitrary subset of
nl80211 information in ethtool.

- Felix

Re: [PATCH] mac80211: Return avg sig, rx, tx values in ethtool stats.

[Ben Greear]

On 12/05/2016 09:28 AM, Felix Fietkau wrote:
> On 2016-12-05 16:01, Ben Greear wrote:
>>
>>
>> On 12/05/2016 06:53 AM, Johannes Berg wrote:
>>>
>>>> Unless I screwed up, this patch also returns an average.
>>>
>>> Oops, sorry. I missed the whole mac_div() indirection thing.
>>>
>>> I'm not super convinced anyway though - all of this data already is
>>> available in a much more reliable fashion, even trackable when stations
>>> are removed (all data gets sent in the DEL_STATION notification), so
>>> adding a crippled way to get the same data seems a bit strange?
>>
>> Ethtool stats are easy to program against, and I am already making the
>> get-stats call to get other things, so it was a quick tweak to return
>> these additional values.  I agree the stats are of somewhat limited worth,
>> but the cost to get them is also pretty small code wise :)
> You're still adding bloat for everybody for the sake of a little
> convenience on your side. I think that's a bad trade-off.
> nl80211 is not *that* hard to program against...
>
> I don't really see much point in duplicating an arbitrary subset of
> nl80211 information in ethtool.

I don't care that much either way.  I already have local features
that cannot go upstream (per radio station hashes, ability to
set an advertised rateset, VHT on 2.4Ghz, 4.9Ghz support,
forked ath10k driver, etc), so I need to run my own kernel regardless.

I'll repost with the comment removed since I said I would.  Johannes
can apply it if he wishes.

I'm more interested in getting things like the 'iterate' logic fixed
and the ath10k crash bugs resolved.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com