| Message ID | 20220329210228.8137-1-greearb@candelatech.com (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | mac80211: report per-chain signal values through ethtool. | expand |
On Tue, 2022-03-29 at 14:02 -0700, greearb@candelatech.com wrote: > From: Ben Greear <greearb@candelatech.com> > > Combine them into a u64, each byte is one chain. This only works up to 4 chains, but the specs at least support 8. I don't think we have any drivers for that, but ... And it's also rather ugly, IMHO. We're reporting these through nl80211 anyway though, no? Why should we prefer ethtool, which fundamentally limits to a single value for the AP rather than giving the full per-station view. > Re-work the way that APs averaged stats to be more > efficient. Isn't that completely unrelated? johannes
On 7/1/22 2:55 AM, Johannes Berg wrote: > On Tue, 2022-03-29 at 14:02 -0700, greearb@candelatech.com wrote: >> From: Ben Greear <greearb@candelatech.com> >> >> Combine them into a u64, each byte is one chain. > > This only works up to 4 chains, but the specs at least support 8. I > don't think we have any drivers for that, but ... u64 gives 8 bytes, so the ethtool part can support 8 chains. The mac80211 part only supports up to 4 chains currently though. > > And it's also rather ugly, IMHO. > > We're reporting these through nl80211 anyway though, no? Why should we > prefer ethtool, which fundamentally limits to a single value for the AP > rather than giving the full per-station view. I already gather ethtool stats for STA vdevs, so adding another stat is basically free as far as performance is concerned. That is important to me. I do not currently program much against netlink API (just scrape existing tools output). I understand if you don't want it upstream though. > >> Re-work the way that APs averaged stats to be more >> efficient. > > Isn't that completely unrelated? At least somewhat unrelated. Thanks, Ben > > johannes >
On Fri, 2022-07-01 at 06:05 -0700, Ben Greear wrote: > On 7/1/22 2:55 AM, Johannes Berg wrote: > > On Tue, 2022-03-29 at 14:02 -0700, greearb@candelatech.com wrote: > > > From: Ben Greear <greearb@candelatech.com> > > > > > > Combine them into a u64, each byte is one chain. > > > > This only works up to 4 chains, but the specs at least support 8. I > > don't think we have any drivers for that, but ... > > u64 gives 8 bytes, so the ethtool part can support 8 chains. > The mac80211 part only supports up to 4 chains currently though. Oops, right, sorry. Still, I'm not sure I like munging it all up into one value - the value itself then doesn't mean anything, and the normal ethtool APIs userspace tools would be pretty much useless for it? > > We're reporting these through nl80211 anyway though, no? Why should we > > prefer ethtool, which fundamentally limits to a single value for the AP > > rather than giving the full per-station view. > > I already gather ethtool stats for STA vdevs, so adding another stat is > basically free as far as performance is concerned. That is important > to me. I do not currently program much against netlink API (just scrape > existing tools output). Well I guess then I think you probably should program against the netlink API :) > > > Re-work the way that APs averaged stats to be more > > > efficient. > > > > Isn't that completely unrelated? > > At least somewhat unrelated. > Fair enough. Maybe send that separately? I guess that's something I'd understand a bit more and improving the existing code is an easier sell than adding a whole new thing there :) johannes
On 7/1/22 6:08 AM, Johannes Berg wrote: > On Fri, 2022-07-01 at 06:05 -0700, Ben Greear wrote: >> On 7/1/22 2:55 AM, Johannes Berg wrote: >>> On Tue, 2022-03-29 at 14:02 -0700, greearb@candelatech.com wrote: >>>> From: Ben Greear <greearb@candelatech.com> >>>> >>>> Combine them into a u64, each byte is one chain. >>> >>> This only works up to 4 chains, but the specs at least support 8. I >>> don't think we have any drivers for that, but ... >> >> u64 gives 8 bytes, so the ethtool part can support 8 chains. >> The mac80211 part only supports up to 4 chains currently though. > > Oops, right, sorry. > > Still, I'm not sure I like munging it all up into one value - the value > itself then doesn't mean anything, and the normal ethtool APIs userspace > tools would be pretty much useless for it? A human looking at the value would be confused, but a program easily deals with it. In a lot of ways, ethtool is way more straight forward to program against than netlink, and it works well with non-wifi drivers, so one chunk of stats-gathering code for all network devices. > >>> We're reporting these through nl80211 anyway though, no? Why should we >>> prefer ethtool, which fundamentally limits to a single value for the AP >>> rather than giving the full per-station view. >> >> I already gather ethtool stats for STA vdevs, so adding another stat is >> basically free as far as performance is concerned. That is important >> to me. I do not currently program much against netlink API (just scrape >> existing tools output). > > Well I guess then I think you probably should program against the > netlink API :) I've managed to mostly dodge it for 20 years....there is hope to make it another 20! > >>>> Re-work the way that APs averaged stats to be more >>>> efficient. >>> >>> Isn't that completely unrelated? >> >> At least somewhat unrelated. >> > > Fair enough. Maybe send that separately? I guess that's something I'd > understand a bit more and improving the existing code is an easier sell > than adding a whole new thing there :) Ok, I'll add that to my list and will plan to do it next time I rebase on a newer upstream kernel. Thanks, Ben > > johannes >
diff --git a/net/mac80211/ethtool.c b/net/mac80211/ethtool.c index 028ffe1a4d2d..10a9a30bcbf3 100644 --- a/net/mac80211/ethtool.c +++ b/net/mac80211/ethtool.c @@ -64,6 +64,7 @@ static const char ieee80211_gstrings_sta_stats[][ETH_GSTRING_LEN] = { "tx_packets", "tx_bytes", "tx_filtered", "tx_retry_failed", "tx_retries", "sta_state", "txrate", "rxrate", "signal", "signal_beacon", + "signal_chains", "signal_chains_avg", "channel", "noise", "ch_time", "ch_time_busy", "ch_time_ext_busy", "ch_time_rx", "ch_time_tx" }; @@ -96,6 +97,7 @@ static void ieee80211_get_stats2(struct net_device *dev, struct station_info sinfo; struct survey_info survey; int i, q; + int z; #define STA_STATS_SURVEY_LEN 7 memset(data, 0, sizeof(u64) * STA_STATS_LEN); @@ -154,14 +156,49 @@ static void ieee80211_get_stats2(struct net_device *dev, if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_SIGNAL_AVG)) data[i] = (u8)sinfo.rx_beacon_signal_avg; i++; + + if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL)) { + int mn = min_t(int, sizeof(u64), ARRAY_SIZE(sinfo.chain_signal)); + u64 accum = (u8)sinfo.chain_signal[0]; + + mn = min_t(int, mn, sinfo.chains); + for (z = 1; z < mn; z++) { + u64 csz = sinfo.chain_signal[z] & 0xFF; + u64 cs = csz << (8 * z); + + accum |= cs; + } + data[i] = accum; + } + i++; + + if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL_AVG)) { + int mn = min_t(int, sizeof(u64), ARRAY_SIZE(sinfo.chain_signal_avg)); + u64 accum = (u8)sinfo.chain_signal_avg[0]; + + for (z = 1; z < mn; z++) { + u64 csz = sinfo.chain_signal_avg[z] & 0xFF; + u64 cs = csz << (8 * z); + + accum |= cs; + } + data[i] = accum; + } + i++; } else { int amt_tx = 0; int amt_rx = 0; int amt_sig = 0; + s16 amt_accum_chain[8] = {0}; + s16 amt_accum_chain_avg[8] = {0}; s64 tx_accum = 0; s64 rx_accum = 0; s64 sig_accum = 0; s64 sig_accum_beacon = 0; + s64 sig_accum_chain[8] = {0}; + s64 sig_accum_chain_avg[8] = {0}; + int start_accum_idx = 0; + list_for_each_entry(sta, &local->sta_list, list) { /* Make sure this station belongs to the proper dev */ if (sta->sdata->dev != dev) @@ -173,35 +210,48 @@ static void ieee80211_get_stats2(struct net_device *dev, ADD_STA_STATS(sta); i++; /* skip sta state */ + start_accum_idx = i; if (sinfo.filled & BIT(NL80211_STA_INFO_TX_BITRATE)) { tx_accum += 100000ULL * cfg80211_calculate_bitrate(&sinfo.txrate); amt_tx++; } - if (amt_tx) - data[i] = mac_div(tx_accum, amt_tx); - i++; if (sinfo.filled & BIT(NL80211_STA_INFO_RX_BITRATE)) { rx_accum += 100000ULL * cfg80211_calculate_bitrate(&sinfo.rxrate); amt_rx++; } - if (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; sig_accum_beacon += sinfo.rx_beacon_signal_avg; amt_sig++; } - if (amt_sig) { - data[i] = (mac_div(sig_accum, amt_sig) & 0xFF); - data[i+1] = (mac_div(sig_accum_beacon, amt_sig) & 0xFF); + + if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL)) { + int mn = min_t(int, sizeof(u64), ARRAY_SIZE(sinfo.chain_signal)); + + mn = min_t(int, mn, sinfo.chains); + for (z = 0; z < mn; z++) { + sig_accum_chain[z] += sinfo.chain_signal[z]; + amt_accum_chain[z]++; + } } - i += 2; + i++; + + if (sinfo.filled & BIT_ULL(NL80211_STA_INFO_CHAIN_SIGNAL_AVG)) { + int mn; + + mn = min_t(int, sizeof(u64), ARRAY_SIZE(sinfo.chain_signal_avg)); + mn = min_t(int, mn, sinfo.chains); + for (z = 0; z < mn; z++) { + sig_accum_chain_avg[z] += sinfo.chain_signal_avg[z]; + amt_accum_chain_avg[z]++; + } + } + 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), @@ -209,6 +259,39 @@ static void ieee80211_get_stats2(struct net_device *dev, sinfo.signal_avg);*/ } + + /* Do averaging */ + i = start_accum_idx; + + if (amt_tx) + data[i] = mac_div(tx_accum, amt_tx); + i++; + + if (amt_rx) + data[i] = mac_div(rx_accum, amt_rx); + i++; + + if (amt_sig) { + data[i] = (mac_div(sig_accum, amt_sig) & 0xFF); + data[i + 1] = (mac_div(sig_accum_beacon, amt_sig) & 0xFF); + } + i += 2; + + for (z = 0; z < sizeof(u64); z++) { + if (amt_accum_chain[z]) { + u64 val = mac_div(sig_accum_chain[z], amt_accum_chain[z]); + + val |= 0xFF; + data[i] |= (val << (z * 8)); + } + if (amt_accum_chain_avg[z]) { + u64 val = mac_div(sig_accum_chain_avg[z], amt_accum_chain_avg[z]); + + val |= 0xFF; + data[i + 1] |= (val << (z * 8)); + } + } + i += 2; } do_survey: