| Message ID | 1480442872-7358-1-git-send-email-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
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
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
> 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
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
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
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
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);*/ + } }