On 4/23/2025 10:22 PM, Johannes Berg wrote:
On Tue, 2025-04-15 at 09:50 +0530, Sarika Sharma wrote:
Current implementation of NL80211_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.
Therefore to support for MLO, start reorganizing sinfo structure
related data elements and add link_sinfo structure for link-level
statistics and keep station related data at sinfo structure.
Currently, changes are done at the deflink(or one of the links) level.
Actual link-level changes will be added in subsequent changes.
Also currently, mac80211 ops .sta_statistics() is mapped to fill sinfo
structure. But to add support for station statistics at link level,
change the ops to .link_sta_statistics() to fill link_sinfo structure.
Additionally, move connected_time before assoc_at in station_info
structure to get minimal holes.
pahole summary before this change:
- size: 232, cachelines: 4, members: 23
- sum members: 223, holes: 3, sum holes: 9
- forced alignments: 1
- last cacheline: 40 bytes
pahole summary after this change:
- size: 224, cachelines: 4, members: 23
- sum members: 223, holes: 1, sum holes: 1
- forced alignments: 1
- last cacheline: 32 bytes
Signed-off-by: Sarika Sharma <quic_sarishar@xxxxxxxxxxx>
---
NOTE:
- Included driver changes for fixing compilation issue.
Does this really need to do all the changes in mac80211 and the drivers?
OTOH maybe if not then it would cause much more back and forth?
Yes, true this patch includes only the minimum necessary changes to
resolve the compilation issues in mac80211 and the drivers.
Without these changes, compilation issues will persist.
+++ b/drivers/net/wireless/ath/ath6kl/cfg80211.c
@@ -1810,47 +1810,51 @@ static int ath6kl_get_station(struct wiphy *wiphy, struct net_device *dev,
else if (left < 0)
return left;
+ sinfo->links[0] = kzalloc(sizeof(*sinfo->links[0]), GFP_KERNEL);
+ if (!sinfo->links[0])
+ return -ENOMEM;
This seems rather error-prone to me.
We already have sinfo->pertid today, allocated and freed by cfg80211,
and here you've added something that's allocated by the driver and freed
by mac80211. That seems odd in comparison?
I'm not sure what the choices are, but I can't say I like this one ;-)
Maybe it's still the least bad option.
Options what I can think of here, other then above approach, may be can
allocate memory during get_station() call only, in cfg80211(but this may
not be memory efficient as have to allocate for all possible links).
or, may be can introduce an API in cfg80211 to allocate the memory for
sinfo->links, and call the API from drivers/mac80211 while filling the
sinfo->links[] data.
if (vif->target_stats.rx_byte) {
- sinfo->rx_bytes = vif->target_stats.rx_byte;
- sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_BYTES64);
- sinfo->rx_packets = vif->target_stats.rx_pkt;
- sinfo->filled |= BIT_ULL(NL80211_STA_INFO_RX_PACKETS);
+ sinfo->links[0]->rx_bytes = vif->target_stats.rx_byte;
+ sinfo->links[0]->filled |= BIT_ULL(NL80211_STA_INFO_RX_BYTES64);
+ sinfo->links[0]->rx_packets = vif->target_stats.rx_pkt;
+ sinfo->links[0]->filled |= BIT_ULL(NL80211_STA_INFO_RX_PACKETS);
}
You later (patch 7) add support for accumulated statistics. These are in
the struct station_info.
Yes, possibly here it is true, all fields are present.
But there might be some drivers which have fields that is not in sinfo
structure.
So, the structure filling and meaning for structure fields should be
consistent across the drivers, therefore I guess sinfo->links[0] is
better and efficient way to be followed for non-ML drivers.
(Pseudo code for suggested approach mentioned below)
Why do we need to make these changes to non-MLO drivers at all? It
stands to reason that for non-MLO drivers we could mostly use the
accumulated statistics? That'd at least be 9 of the values changed here,
they just come back as is in patch 7.
I don't think it really matters for non-MLO whether or not link[0] is
used or the overall STA/accumulated fields?
True, it doesn't matters for non-ML station, values could be taken from
sinfo or sinfo->links[0].
But I believe taking sinfo->links[0], will be better way as code will be
even, while filling the sinfo and link_sinfo structure for ML/non-ML
drivers.
(Pseudo code for suggested approach mentioned below)
A similar argument can be made for what's later called 'mld_addr' and
some other fields, I'd say?
Perhaps it'd be better to structure this patchset the other way around:
start with the existing non-MLO and move out things that are clearly not
applicable at all to the MLD level (such as RSSI, rates, etc.). It seems
plausible that'd really be less than you have now, since the
accumulation (patch 7) adds back a bunch and should possibly add back
more than it does (e.g. tx/rx duration, MPDU count, etc.)
We can do with above way- however may be few points why I started with
this approach not the other one:
Pseudo code for above suggested approach:
- Remove fields from station_info structure, not applicable for MLD
level and add sinfo_links and add links applicable fields.
- cfg80211/mac80211:
structure sinfo {
filled
packets
bytes...
// Fields applicable at MLD level
// Remove fields not applicable at MLD level (RSSI, etc.)
link_station_info *links[IEEE80211_MLD_MAX_NUM_LINKS]
}
structure link_station_info {
filled
packets
bytes...
// All link-level applicable fields (RSSI, etc.)
}
- Call get_station from cfg80211, which will call sta_set_sinfo in mac80211.
- sta_set_sinfo() -> refactor code and split for non-ML stats and MLO
stats in different ways:
non-ML -> fill all station_info data and other link-level data in
sinfo->links[0] (RSSI, etc. // fields not applicable at MLD level)
MLO -> fill station-specific data only (avoid filling accumulated
stats( data will be accumulated in cfg80211) and refactor code
to fill all link-specific data applicable (rx_byte, etc.)
- Call driver to fill sinfo data + sinfo->links[0] data (for some
fields) for non-ML and fill link_sinfo data for MLO.
Reasons for not considering this approach:
- Some fields in the sinfo structure have different meanings for MLO and
non-ML stations. For example:
signal - for non-ML, it is the station signal, but for MLO, it is
the best signal of all links.
etc.
So, the structure parameters' meanings will be inconsistent.
- Code flow will be uneven and clumsy (in mac80211 - split the code to
fill station info fields for non-ML and a few fields to fill for
sinfo->links[0], for MLO, fill some fields only at station_info and most
fields at the link level).
But if this approach looks more reliable, could send a new version with
these changes?
I probably need to spend more time with this ...
johannes
