| Message ID | 20250110042449.1158789-1-quic_sarishar@quicinc.com |
|---|---|
| Headers | show |
| Series | wifi: cfg80211/mac80211: add support to handle per link statistics of multi-link station | expand |
On 1/10/2025 3:15 PM, Johannes Berg wrote: > On Fri, 2025-01-10 at 09:54 +0530, Sarika Sharma wrote: >> >> Current flow: > > FWIW, I really would have preferred to see this discussion separately in > terms of cfg80211 and mac80211. Yes, the design phase obviously requires > both to be addressed, but once you have that I tend to think it's easier > to reason about them individually. Sure, let me check what I can do here. > >> Proposed flow: (Changes in last block) > > Which kind of implies that cfg80211 didn't really change in terms of the > high-level overview, but I'm not sure that's really true? > From cfg80211 still sta_set_sinfo() is called, sinfo structure is getting changed. Sure, will add about the structure change in proposed design. >> +----------------------------------------------------------+ >> | sta_set_sinfo() | >> | 1. fill sinfo structure- info related to station | >> | 2. if MLO | >> | a. call sta_set_link_sinfo() for each valid link | >> | i. Call mac80211 ops- .link_sta_statistics() | >> | to fill link_sinfo structure | >> | ii. fill remaining link_sinfo structure | >> | b. call sta_set_mld_info()- to fill accumulated | >> | stats at MLO level | >> | 3. if non-ML | >> | a. call sta_set_link_sinfo() for deflink | >> | i. Call mac80211 ops - .link_sta_statistics() | >> | to fill deflink link_sinfo structure | >> | ii. fill remaining link_sinfo structure | > > And that's simply too much detail. The ASCII art is also a distraction > rather than an aid if you ask me ;-) Sure, will move to points about the design proposed. >> >> Alternate approach: >> - Keep sinfo structure as it is and use this for non-ML or >> accumulated statistics for ML station. >> - Add link sinfo for links with only certain link specific statistics. >> - Keep mac_op_sta_statistics at MLD level and let driver fill the >> MLO and link level data, if driver not filling let mac80211 fill >> the data. >> - Corresponding changes done to embed statistics into the NL message >> based on the sinfo/link_sinfo. > > And this kind of data-structure based discussion is actually completely > missing for the proposed solution? Sure, let me add about the structural change as well in design flow. > > What about drivers that might offload link decisions and not really tell > you per-link statistics? I mean, I don't even know if such a thing > exists, but with a data structure like that you could still have it? That is taken care, if driver is not filling the link info, mac80211 will check if it filled, if not it will be filled by mac80211. > > Should mac80211 even accumulate? Why not cfg80211 accumulate over the > links? And if mac80211 keeps the removed links per link then nothing > else is even needed? Or it could pre-fill the MLD level info? Sure, we can do the accumulation in cfg80211 instead of mac80211. > > johannes
On 1/10/2025 10:23 PM, Ben Greear wrote: > On 1/9/25 20:24, Sarika Sharma wrote: >> Current implementation of NL80211_CMD_GET_STATION does not work >> for multi-link operation(MLO) since in case of MLO only deflink >> (or one of the links) is considered and not all links. >> >> Hence, add the link_sinfo structure to provide infrastructure >> for link-level station statistics for multi-link operation(MLO). >> >> Additionally, accumulated stats for MLO are included in a concise >> manner to provide a comprehensive overview of the ML stations. >> >> NOTE: >> 1. Current code changes are done to get an early feedback on design. >> 2. Once RFC patches are approved will add the required driver >> changes. >> 3. Ath12k changes are included in this series for reference to other >> driver changes. > >> Alternate approach: >> - Keep sinfo structure as it is and use this for non-ML or >> accumulated statistics for ML station. >> - Add link sinfo for links with only certain link specific >> statistics. >> - Keep mac_op_sta_statistics at MLD level and let driver fill the >> MLO and link level data, if driver not filling let mac80211 fill >> the data. >> - Corresponding changes done to embed statistics into the NL message >> based on the sinfo/link_sinfo. > > My suggestions for general approach to this: > > 1) current sinfo stats should report totals for all links, > but it should not sum them up at query time because links come > and go. So driver/firmware/mac80211 or whatever is keeping count > of the stats needs to count the total tx/rx/ packets/bytes/whatever > as it happens. Alternate approach is not considered here, because some fields at station level could not be much meaningful like rx_beacon, pertid ,etc. I believe the total count is better to do from links data in cfg80211/mac80211, as if link's data is provided it is easy to aggregate them, during query time. Also removed link_data is maintained in implemented approach, so that could be helpful if any link go down. > > 2) Per-link stats can be over duration of the link object. > > 3) For sinfo logic that currently reports tx/rx mcs rate and such that > cannot > be summed, use 'best' link's for those values. Effectively, this > probably means highest > frequency is 'best' link. Every fields have it's own way of best, so based on field need to check what is best for this field value? but some parameters not have much meaning at MLO level like rx_beacon, pertid, etc. So keeping best of it, will be useful here? or you mean need to check for best frequency link and take this link data as MLO data? if yes? will it be useful here? > > 4) Add per-link stats data that looks very much like 'sinfo' data struct > and user-space > that can support that could then get detailed per-link stats at same > time it is getting > per netdev 'sinfo' stats. Again, some fields that are in sinfo structure could not be very useful at link level like generation, sta_info, etc. So keeping same structure for station level and link level is not very useful, instead better to split the structure, fields for station level and some fields at link level. > > 5) Assuming there will never be more than 3 links for any radios > supported by Linux in the near future, > embed the 3 link's 'sinfo-like' stats data structure in the per-netdev > sinfo stats struct > so that we can get all 3 link's data at same time that we are getting > the other stats. > That should save calls into the driver and ensure that per-links stats > can be gathered > at exactly the same time. Yes, I agree currently it's 3 only, but everywhere we are using max-15, so explicitly keeping it 3 here isn't questionable? In future also it will not be useful. > > Thanks, > Ben > Currently, addressing the comments on current implemented approach.
On 1/14/25 21:11, Sarika Sharma wrote: > On 1/10/2025 10:23 PM, Ben Greear wrote: >> On 1/9/25 20:24, Sarika Sharma wrote: >>> Current implementation of NL80211_CMD_GET_STATION does not work >>> for multi-link operation(MLO) since in case of MLO only deflink >>> (or one of the links) is considered and not all links. >>> >>> Hence, add the link_sinfo structure to provide infrastructure >>> for link-level station statistics for multi-link operation(MLO). >>> >>> Additionally, accumulated stats for MLO are included in a concise >>> manner to provide a comprehensive overview of the ML stations. >>> >>> NOTE: >>> 1. Current code changes are done to get an early feedback on design. >>> 2. Once RFC patches are approved will add the required driver changes. >>> 3. Ath12k changes are included in this series for reference to other >>> driver changes. >> >>> Alternate approach: >>> - Keep sinfo structure as it is and use this for non-ML or >>> accumulated statistics for ML station. >>> - Add link sinfo for links with only certain link specific statistics. >>> - Keep mac_op_sta_statistics at MLD level and let driver fill the >>> MLO and link level data, if driver not filling let mac80211 fill >>> the data. >>> - Corresponding changes done to embed statistics into the NL message >>> based on the sinfo/link_sinfo. >> >> My suggestions for general approach to this: >> >> 1) current sinfo stats should report totals for all links, >> but it should not sum them up at query time because links come >> and go. So driver/firmware/mac80211 or whatever is keeping count >> of the stats needs to count the total tx/rx/ packets/bytes/whatever >> as it happens. > > Alternate approach is not considered here, because some fields at station level could not be much meaningful like rx_beacon, pertid ,etc. > I believe the total count is better to do from links data in cfg80211/mac80211, as if link's data is provided it is easy to aggregate them, during query time. > > Also removed link_data is maintained in implemented approach, so that could be helpful if any link go down. > >> >> 2) Per-link stats can be over duration of the link object. >> >> 3) For sinfo logic that currently reports tx/rx mcs rate and such that cannot >> be summed, use 'best' link's for those values. Effectively, this probably means highest >> frequency is 'best' link. > > Every fields have it's own way of best, so based on field need to check what is best for this field value? but some parameters not have much meaning at MLO > level like rx_beacon, pertid, etc. So keeping best of it, will be useful here? > > or you mean need to check for best frequency link and take this link data as MLO data? if yes? will it be useful here? My goal is to allow legacy user-space to report something useful for MLO netdevs, and in my opinion, reporting tx/rx mcs rates in this case should use the highest frequency link that is active. >> 4) Add per-link stats data that looks very much like 'sinfo' data struct and user-space >> that can support that could then get detailed per-link stats at same time it is getting >> per netdev 'sinfo' stats. > > Again, some fields that are in sinfo structure could not be very useful at link level like generation, sta_info, etc. So keeping same structure for station > level and link level is not very useful, instead better to split the structure, fields for station level and some fields at link level. > >> >> 5) Assuming there will never be more than 3 links for any radios supported by Linux in the near future, >> embed the 3 link's 'sinfo-like' stats data structure in the per-netdev sinfo stats struct >> so that we can get all 3 link's data at same time that we are getting the other stats. >> That should save calls into the driver and ensure that per-links stats can be gathered >> at exactly the same time. > > Yes, I agree currently it's 3 only, but everywhere we are using max-15, so explicitly keeping it 3 here isn't questionable? In future also it will not be useful. Changing a 3 to a 4 is easy future change, and should not affect user-space API one way or another. If we can simplify code and stats memory allocation by using a smaller link count for stats gathering, I think we should do that. As soon as something supporting more than 3 links exists, we can increase it. But, it is not a big deal to me, so if you like the other approach, I'll not argue further. Thanks, Ben > >> >> Thanks, >> Ben >> > > Currently, addressing the comments on current implemented approach. > >
Current implementation of NL80211_CMD_GET_STATION does not work for multi-link operation(MLO) since in case of MLO only deflink (or one of the links) is considered and not all links. Hence, add the link_sinfo structure to provide infrastructure for link-level station statistics for multi-link operation(MLO). Additionally, accumulated stats for MLO are included in a concise manner to provide a comprehensive overview of the ML stations. NOTE: 1. Current code changes are done to get an early feedback on design. 2. Once RFC patches are approved will add the required driver changes. 3. Ath12k changes are included in this series for reference to other driver changes. Current flow: +-------------------------+ | From userspace - | | NL80211_CMD_GET_STATION | | | +-------------------------+ | | +---------------------------------------+ | nl80211_get_station() | | | | 1. Locally define sinfo | | 2. call cfg80211 ops - .get_station() | +---------------------------------------+ | | +-------------------------------------------+ | sta_set_sinfo() | | 1. call mac80211 ops - .sta_statistics() | | to fill sinfo structure | | 2. fill remaining sinfo structure, if | | not filled from driver | +-------------------------------------------+ Proposed flow: (Changes in last block) +-------------------------+ | From userspace - | | NL80211_CMD_GET_STATION | | | +-------------------------+ | | +---------------------------------------+ | nl80211_get_station() | | | | 1. Locally define sinfo | | 2. call cfg80211 ops - .get_station() | +---------------------------------------+ | | +----------------------------------------------------------+ | sta_set_sinfo() | | 1. fill sinfo structure- info related to station | | 2. if MLO | | a. call sta_set_link_sinfo() for each valid link | | i. Call mac80211 ops- .link_sta_statistics() | | to fill link_sinfo structure | | ii. fill remaining link_sinfo structure | | b. call sta_set_mld_info()- to fill accumulated | | stats at MLO level | | 3. if non-ML | | a. call sta_set_link_sinfo() for deflink | | i. Call mac80211 ops - .link_sta_statistics() | | to fill deflink link_sinfo structure | | ii. fill remaining link_sinfo structure | +----------------------------------------------------------+ Alternate approach: - Keep sinfo structure as it is and use this for non-ML or accumulated statistics for ML station. - Add link sinfo for links with only certain link specific statistics. - Keep mac_op_sta_statistics at MLD level and let driver fill the MLO and link level data, if driver not filling let mac80211 fill the data. - Corresponding changes done to embed statistics into the NL message based on the sinfo/link_sinfo. pseudo code for alternate approach: - cfg80211/mac80211: structure sinfo { filled packets bytes etc... //retain the structure as it was there before add structure link_sinfo{ filled packets bytes etc ... //all link level applicable fields } } - Call drv_sta_statistics at MLO level or deflink if non-ML and let driver fill MLO statistics and link level statistics. - Check if sinfo->filled is set if not fill the corresponding data in sinfo and link sinfo for MLO or sinfo for non-ML. Why not considered this approach for implementation: - The required changes here to other drivers are minimal. However, since sinfo is retained, every field needs to be filled at the MLO level. MLO statistics for tx_duration, pertid, etc., may not be very useful. - If these elements are not filled for MLO, they will remain unused in sinfo. Sarika Sharma (7): wifi: cfg80211: reorg sinfo structure elements for mesh wifi: cfg80211: reorg sinfo structure elements for MLO wifi: cfg80211: extend statistics for link level in sinfo wifi: cfg80211: add accumulated statistics for MLO links wifi: mac80211: add support to accumulate removed link statistics wifi: cfg80211: add additional MLO statistics wifi: ath12k: correctly fetch arsta for MLO drivers/net/wireless/ath/ath12k/mac.c | 51 +-- include/net/cfg80211.h | 204 ++++++---- include/net/mac80211.h | 21 +- net/mac80211/driver-ops.h | 14 +- net/mac80211/ethtool.c | 30 +- net/mac80211/ibss.c | 4 +- net/mac80211/sta_info.c | 537 ++++++++++++++++++-------- net/mac80211/sta_info.h | 30 +- net/mac80211/trace.h | 2 +- net/mac80211/util.c | 18 +- net/wireless/nl80211.c | 353 +++++++++++------ net/wireless/trace.h | 33 +- net/wireless/util.c | 10 +- net/wireless/wext-compat.c | 22 +- 14 files changed, 889 insertions(+), 440 deletions(-) base-commit: 7bf1659bad4e9413cdba132ef9cbd0caa9cabcc4