| Message ID | 20250515054904.1214096-7-quic_sarishar@quicinc.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | wifi: cfg80211/mac80211: add support to handle per link statistics of multi-link station | expand |
Also, On Thu, 2025-05-15 at 11:19 +0530, Sarika Sharma wrote: > > +static void sta_accumulate_removed_link_stats(struct sta_info *sta, int link_id) > +{ > + struct link_sta_info *link_sta = wiphy_dereference(sta->local->hw.wiphy, > + sta->link[link_id]); > + int ac; > + > + for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) { > + sta->rem_link_stats.tx_packets += > + link_sta->tx_stats.packets[ac]; > + sta->rem_link_stats.tx_bytes += link_sta->tx_stats.bytes[ac]; It seems odd to take per-AC values and flatten them in this case? How do they even get reported, as overall values only? Then you get the same inconsistency again on per-AC values since those are (or at least could/should be) summed up over all links, but aren't kept post removal? johannes
On 5/15/2025 5:00 PM, Johannes Berg wrote: > On Thu, 2025-05-15 at 11:19 +0530, Sarika Sharma wrote: >> >> +void sta_set_accumulated_removed_links_sinfo(struct sta_info *sta, >> + struct station_info *sinfo) >> +{ >> + /* Resetting the MLO statistics for accumulated fields, to >> + * avoid duplication. >> + */ >> + sinfo->tx_packets = 0; >> + sinfo->rx_packets = 0; >> + sinfo->tx_bytes = 0; >> + sinfo->rx_bytes = 0; >> + sinfo->tx_retries = 0; >> + sinfo->tx_failed = 0; >> + sinfo->rx_dropped_misc = 0; >> + sinfo->beacon_loss_count = 0; >> + sinfo->expected_throughput = 0; >> + sinfo->rx_mpdu_count = 0; >> + sinfo->fcs_err_count = 0; >> + sinfo->rx_beacon = 0; >> + sinfo->rx_duration = 0; >> + sinfo->tx_duration = 0; >> + >> + /* Accumulating the removed link statistics. */ >> + sinfo->tx_packets += sta->rem_link_stats.tx_packets; >> + sinfo->rx_packets += sta->rem_link_stats.rx_packets; >> + sinfo->tx_bytes += sta->rem_link_stats.tx_bytes; >> + sinfo->rx_bytes += sta->rem_link_stats.rx_bytes; >> + sinfo->tx_retries += sta->rem_link_stats.tx_retries; >> + sinfo->tx_failed += sta->rem_link_stats.tx_failed; >> + sinfo->rx_dropped_misc += sta->rem_link_stats.rx_dropped_misc; > > Setting something to 0 just to += it seems silly? > > However I think it also needs a bit more explanation - it's sinfo, so > it's zeroed at allocation, where would non-zero numbers come from? Currently, the station information for MLO is populated with some values from sta->deflink, as the sta_set_sinfo() call is common for both non-MLO and MLO. When updating the station_info structure in cfg80211_sta_set_mld_sinfo(), the accumulated fields (such as packets, bytes, etc.) will already contain values set by mac80211 (from the deflink fields). Therefore, directly adding to these fields would be incorrect, so they should be reset to zero. May be this, resetting can be done directly in cfg80211 during cfg80211_sta_set_mld_sinfo(), will correct this. > > johannes >
On 5/15/2025 11:08 PM, Ben Greear wrote: > On 5/15/25 10:35, Sarika Sharma wrote: >> On 5/15/2025 5:00 PM, Johannes Berg wrote: >>> On Thu, 2025-05-15 at 11:19 +0530, Sarika Sharma wrote: >>>> >>>> +void sta_set_accumulated_removed_links_sinfo(struct sta_info *sta, >>>> + struct station_info *sinfo) >>>> +{ >>>> + /* Resetting the MLO statistics for accumulated fields, to >>>> + * avoid duplication. >>>> + */ >>>> + sinfo->tx_packets = 0; >>>> + sinfo->rx_packets = 0; >>>> + sinfo->tx_bytes = 0; >>>> + sinfo->rx_bytes = 0; >>>> + sinfo->tx_retries = 0; >>>> + sinfo->tx_failed = 0; >>>> + sinfo->rx_dropped_misc = 0; >>>> + sinfo->beacon_loss_count = 0; >>>> + sinfo->expected_throughput = 0; >>>> + sinfo->rx_mpdu_count = 0; >>>> + sinfo->fcs_err_count = 0; >>>> + sinfo->rx_beacon = 0; >>>> + sinfo->rx_duration = 0; >>>> + sinfo->tx_duration = 0; >>>> + >>>> + /* Accumulating the removed link statistics. */ >>>> + sinfo->tx_packets += sta->rem_link_stats.tx_packets; >>>> + sinfo->rx_packets += sta->rem_link_stats.rx_packets; >>>> + sinfo->tx_bytes += sta->rem_link_stats.tx_bytes; >>>> + sinfo->rx_bytes += sta->rem_link_stats.rx_bytes; >>>> + sinfo->tx_retries += sta->rem_link_stats.tx_retries; >>>> + sinfo->tx_failed += sta->rem_link_stats.tx_failed; >>>> + sinfo->rx_dropped_misc += sta->rem_link_stats.rx_dropped_misc; >>> >>> Setting something to 0 just to += it seems silly? >>> >>> However I think it also needs a bit more explanation - it's sinfo, so >>> it's zeroed at allocation, where would non-zero numbers come from? >> >> Currently, the station information for MLO is populated with some >> values from sta->deflink, as the sta_set_sinfo() call is common for >> both non-MLO and MLO. >> >> When updating the station_info structure in >> cfg80211_sta_set_mld_sinfo(), the accumulated fields (such as packets, >> bytes, etc.) will already contain values set by mac80211 (from the >> deflink fields). >> >> Therefore, directly adding to these fields would be incorrect, so they >> should be reset to zero. >> >> May be this, resetting can be done directly in cfg80211 during >> cfg80211_sta_set_mld_sinfo(), will correct this. > > If nothing else, you could just do assignment instead of setting to zero > and then > incrementing? > > I did not actually review the over-all logic, so perhaps there are bigger > issues I'm not aware of. Yes, here I can directly use assignment. But some of fields I need to reset to 0 in cfg80211. Will check and update this in next version. > > Thanks, > Ben > >
On Thu, 2025-05-15 at 23:05 +0530, Sarika Sharma wrote: > On 5/15/2025 5:00 PM, Johannes Berg wrote: > > On Thu, 2025-05-15 at 11:19 +0530, Sarika Sharma wrote: > > > > > > +void sta_set_accumulated_removed_links_sinfo(struct sta_info *sta, > > > + struct station_info *sinfo) > > > +{ > > > + /* Resetting the MLO statistics for accumulated fields, to > > > + * avoid duplication. > > > + */ > > > + sinfo->tx_packets = 0; > > > + sinfo->rx_packets = 0; > > > + sinfo->tx_bytes = 0; > > > + sinfo->rx_bytes = 0; > > > + sinfo->tx_retries = 0; > > > + sinfo->tx_failed = 0; > > > + sinfo->rx_dropped_misc = 0; > > > + sinfo->beacon_loss_count = 0; > > > + sinfo->expected_throughput = 0; > > > + sinfo->rx_mpdu_count = 0; > > > + sinfo->fcs_err_count = 0; > > > + sinfo->rx_beacon = 0; > > > + sinfo->rx_duration = 0; > > > + sinfo->tx_duration = 0; > > > + > > > + /* Accumulating the removed link statistics. */ > > > + sinfo->tx_packets += sta->rem_link_stats.tx_packets; > > > + sinfo->rx_packets += sta->rem_link_stats.rx_packets; > > > + sinfo->tx_bytes += sta->rem_link_stats.tx_bytes; > > > + sinfo->rx_bytes += sta->rem_link_stats.rx_bytes; > > > + sinfo->tx_retries += sta->rem_link_stats.tx_retries; > > > + sinfo->tx_failed += sta->rem_link_stats.tx_failed; > > > + sinfo->rx_dropped_misc += sta->rem_link_stats.rx_dropped_misc; > > > > Setting something to 0 just to += it seems silly? > > > > However I think it also needs a bit more explanation - it's sinfo, so > > it's zeroed at allocation, where would non-zero numbers come from? > > Currently, the station information for MLO is populated with some values > from sta->deflink, as the sta_set_sinfo() call is common for both > non-MLO and MLO. OK but deflink will not actually _have_ any values. And again, mac80211 shouldn't be doing work the results of which are then only discarded... > When updating the station_info structure in > cfg80211_sta_set_mld_sinfo(), the accumulated fields (such as packets, > bytes, etc.) will already contain values set by mac80211 (from the > deflink fields). Which arguably is the problem? But also not because those are zero anyway? I still don't think this makes any sense. Either it's not filled and remains zero, or it should be filled only with some sensible values like the accumulated stats from removed links. johannes
On Thu, 2025-05-15 at 23:17 +0530, Sarika Sharma wrote: > > Looks like we can flatten them. In existing code as well we have > flattened and use, during sta_set_sinfo() > > "if (!(sinfo->filled & BIT_ULL(NL80211_STA_INFO_TX_PACKETS))) { > sinfo->tx_packets = 0; > for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) > sinfo->tx_packets += > sta->deflink.tx_stats.packets[ac]; > sinfo->filled |= BIT_ULL(NL80211_STA_INFO_TX_PACKETS); > } " > Indeed, weird, why do we even bother counting them per AC? johannes
On 5/20/2025 2:08 PM, Johannes Berg wrote: > On Thu, 2025-05-15 at 23:05 +0530, Sarika Sharma wrote: >> On 5/15/2025 5:00 PM, Johannes Berg wrote: >>> On Thu, 2025-05-15 at 11:19 +0530, Sarika Sharma wrote: >>>> >>>> +void sta_set_accumulated_removed_links_sinfo(struct sta_info *sta, >>>> + struct station_info *sinfo) >>>> +{ >>>> + /* Resetting the MLO statistics for accumulated fields, to >>>> + * avoid duplication. >>>> + */ >>>> + sinfo->tx_packets = 0; >>>> + sinfo->rx_packets = 0; >>>> + sinfo->tx_bytes = 0; >>>> + sinfo->rx_bytes = 0; >>>> + sinfo->tx_retries = 0; >>>> + sinfo->tx_failed = 0; >>>> + sinfo->rx_dropped_misc = 0; >>>> + sinfo->beacon_loss_count = 0; >>>> + sinfo->expected_throughput = 0; >>>> + sinfo->rx_mpdu_count = 0; >>>> + sinfo->fcs_err_count = 0; >>>> + sinfo->rx_beacon = 0; >>>> + sinfo->rx_duration = 0; >>>> + sinfo->tx_duration = 0; >>>> + >>>> + /* Accumulating the removed link statistics. */ >>>> + sinfo->tx_packets += sta->rem_link_stats.tx_packets; >>>> + sinfo->rx_packets += sta->rem_link_stats.rx_packets; >>>> + sinfo->tx_bytes += sta->rem_link_stats.tx_bytes; >>>> + sinfo->rx_bytes += sta->rem_link_stats.rx_bytes; >>>> + sinfo->tx_retries += sta->rem_link_stats.tx_retries; >>>> + sinfo->tx_failed += sta->rem_link_stats.tx_failed; >>>> + sinfo->rx_dropped_misc += sta->rem_link_stats.rx_dropped_misc; >>> >>> Setting something to 0 just to += it seems silly? >>> >>> However I think it also needs a bit more explanation - it's sinfo, so >>> it's zeroed at allocation, where would non-zero numbers come from? >> >> Currently, the station information for MLO is populated with some values >> from sta->deflink, as the sta_set_sinfo() call is common for both >> non-MLO and MLO. > > OK but deflink will not actually _have_ any values. And again, mac80211 > shouldn't be doing work the results of which are then only discarded... > >> When updating the station_info structure in >> cfg80211_sta_set_mld_sinfo(), the accumulated fields (such as packets, >> bytes, etc.) will already contain values set by mac80211 (from the >> deflink fields). > > Which arguably is the problem? But also not because those are zero > anyway? I still don't think this makes any sense. Either it's not filled > and remains zero, or it should be filled only with some sensible values > like the accumulated stats from removed links. okay, then either changes needed to be done so, some fields way of filling data is different for non-ML and MLO station or need to maintain all accumulated removed links data for addable fields. Let me check and add this. > > johannes
diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c index 2cd8731d8275..a523a6a5db32 100644 --- a/net/mac80211/cfg.c +++ b/net/mac80211/cfg.c @@ -885,6 +885,13 @@ static int ieee80211_dump_station(struct wiphy *wiphy, struct net_device *dev, ret = 0; memcpy(mac, sta->sta.addr, ETH_ALEN); sta_set_sinfo(sta, sinfo, true); + + /* Add accumulated removed link data to sinfo data for + * consistency for MLO + */ + if (sinfo->valid_links) + sta_set_accumulated_removed_links_sinfo(sta, sinfo); + } return ret; @@ -912,6 +919,12 @@ static int ieee80211_get_station(struct wiphy *wiphy, struct net_device *dev, if (sta) { ret = 0; sta_set_sinfo(sta, sinfo, true); + + /* Add accumulated removed link data to sinfo data for + * consistency for MLO + */ + if (sinfo->valid_links) + sta_set_accumulated_removed_links_sinfo(sta, sinfo); } return ret; diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c index 5cc28eb1005b..709250ba37c9 100644 --- a/net/mac80211/sta_info.c +++ b/net/mac80211/sta_info.c @@ -355,6 +355,25 @@ static void sta_info_free_link(struct link_sta_info *link_sta) free_percpu(link_sta->pcpu_rx_stats); } +static void sta_accumulate_removed_link_stats(struct sta_info *sta, int link_id) +{ + struct link_sta_info *link_sta = wiphy_dereference(sta->local->hw.wiphy, + sta->link[link_id]); + int ac; + + for (ac = 0; ac < IEEE80211_NUM_ACS; ac++) { + sta->rem_link_stats.tx_packets += + link_sta->tx_stats.packets[ac]; + sta->rem_link_stats.tx_bytes += link_sta->tx_stats.bytes[ac]; + } + + sta->rem_link_stats.rx_packets += link_sta->rx_stats.packets; + sta->rem_link_stats.rx_bytes += link_sta->rx_stats.bytes; + sta->rem_link_stats.tx_retries += link_sta->status_stats.retry_count; + sta->rem_link_stats.tx_failed += link_sta->status_stats.retry_failed; + sta->rem_link_stats.rx_dropped_misc += link_sta->rx_stats.dropped; +} + static void sta_remove_link(struct sta_info *sta, unsigned int link_id, bool unhash) { @@ -377,6 +396,10 @@ static void sta_remove_link(struct sta_info *sta, unsigned int link_id, alloc = container_of(link_sta, typeof(*alloc), info); sta->sta.valid_links &= ~BIT(link_id); + + /* store removed link info for accumulated stats consistency */ + sta_accumulate_removed_link_stats(sta, link_id); + RCU_INIT_POINTER(sta->link[link_id], NULL); RCU_INIT_POINTER(sta->sta.link[link_id], NULL); if (alloc) { @@ -2644,6 +2667,37 @@ static void sta_set_mesh_sinfo(struct sta_info *sta, } #endif +void sta_set_accumulated_removed_links_sinfo(struct sta_info *sta, + struct station_info *sinfo) +{ + /* Resetting the MLO statistics for accumulated fields, to + * avoid duplication. + */ + sinfo->tx_packets = 0; + sinfo->rx_packets = 0; + sinfo->tx_bytes = 0; + sinfo->rx_bytes = 0; + sinfo->tx_retries = 0; + sinfo->tx_failed = 0; + sinfo->rx_dropped_misc = 0; + sinfo->beacon_loss_count = 0; + sinfo->expected_throughput = 0; + sinfo->rx_mpdu_count = 0; + sinfo->fcs_err_count = 0; + sinfo->rx_beacon = 0; + sinfo->rx_duration = 0; + sinfo->tx_duration = 0; + + /* Accumulating the removed link statistics. */ + sinfo->tx_packets += sta->rem_link_stats.tx_packets; + sinfo->rx_packets += sta->rem_link_stats.rx_packets; + sinfo->tx_bytes += sta->rem_link_stats.tx_bytes; + sinfo->rx_bytes += sta->rem_link_stats.rx_bytes; + sinfo->tx_retries += sta->rem_link_stats.tx_retries; + sinfo->tx_failed += sta->rem_link_stats.tx_failed; + sinfo->rx_dropped_misc += sta->rem_link_stats.rx_dropped_misc; +} + void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, bool tidstats) { diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h index e5b91e60405b..6851cf10a1da 100644 --- a/net/mac80211/sta_info.h +++ b/net/mac80211/sta_info.h @@ -568,6 +568,35 @@ struct link_sta_info { struct ieee80211_link_sta *pub; }; +/** + * struct ieee80211_sta_removed_link_stats - Removed link sta data + * + * keep required accumulated removed link data for stats + * + * @rx_packets: accumulated packets (MSDUs & MMPDUs) received from + * this station for removed links + * @tx_packets: accumulated packets (MSDUs & MMPDUs) transmitted to + * this station for removed links + * @rx_bytes: accumulated bytes (size of MPDUs) received from this + * station for removed links + * @tx_bytes: accumulated bytes (size of MPDUs) transmitted to this + * station for removed links + * @tx_retries: cumulative retry counts (MPDUs) for removed links + * @tx_failed: accumulated number of failed transmissions (MPDUs) + * (retries exceeded, no ACK) for removed links + * @rx_dropped_misc: accumulated dropped packets for un-specified reason + * from this station for removed links + */ +struct ieee80211_sta_removed_link_stats { + u32 rx_packets; + u32 tx_packets; + u64 rx_bytes; + u64 tx_bytes; + u32 tx_retries; + u32 tx_failed; + u32 rx_dropped_misc; +}; + /** * struct sta_info - STA information * @@ -644,6 +673,7 @@ struct link_sta_info { * @deflink address and remaining would be allocated and the address * would be assigned to link[link_id] where link_id is the id assigned * by the AP. + * @rem_link_stats: accumulated removed link stats */ struct sta_info { /* General information, mostly static */ @@ -718,6 +748,7 @@ struct sta_info { struct ieee80211_sta_aggregates cur; struct link_sta_info deflink; struct link_sta_info __rcu *link[IEEE80211_MLD_MAX_NUM_LINKS]; + struct ieee80211_sta_removed_link_stats rem_link_stats; /* keep last! */ struct ieee80211_sta sta; @@ -922,6 +953,9 @@ void sta_set_rate_info_tx(struct sta_info *sta, void sta_set_sinfo(struct sta_info *sta, struct station_info *sinfo, bool tidstats); +void sta_set_accumulated_removed_links_sinfo(struct sta_info *sta, + struct station_info *sinfo); + u32 sta_get_expected_throughput(struct sta_info *sta); void ieee80211_sta_expire(struct ieee80211_sub_if_data *sdata,
Currently, if a link gets removed in between for a station then directly accumulated data will fall down to sum of other active links. This will bring inconsistency in station dump statistics. For instance, let's take Tx packets - at t=0-> link-0:2 link-1:3 Tx packets => accumulated = 5 - at t=1-> link-0:4 link-1:6 Tx packets => accumulated = 10 let say at t=2, link-0 went down => link-0:0 link-1:7 => accumulated = 7 Here, suddenly accumulated Tx packets will come down to 7 from 10. This is showing inconsistency. Therefore, store link-0 data when it went down and add to accumulated Tx packet = 11. Hence, store the removed link statistics data in sta structure and add it in accumulated statistics for consistency. Also, initialize accumulatable fields in sinfo structure to 0, to avoid any additional packets account for MLO station. Signed-off-by: Sarika Sharma <quic_sarishar@quicinc.com> --- net/mac80211/cfg.c | 13 ++++++++++ net/mac80211/sta_info.c | 54 +++++++++++++++++++++++++++++++++++++++++ net/mac80211/sta_info.h | 34 ++++++++++++++++++++++++++ 3 files changed, 101 insertions(+)