On Tue, 2025-04-15 at 09:50 +0530, Sarika Sharma wrote:
> Sarika Sharma (11):
> wifi: mac80211: add support towards MLO handling of station statistics
> wifi: mac80211: refactoring sta_set_sinfo() to add support towards MLO statistics
> wifi: cfg80211: refactoring nl80211_set_station() for link attributes towards MLO
These three seem fine, modulo the nits. But of course that's just
initial refactoring anyway.
> wifi: cfg80211: reorg sinfo structure elements for MLO
Here I'm really not very happy with the massive changes in all drivers
that now have to use link[0]. Especially since you later add back the
station_info statistics again for accumulated statistics anyway ...
But to be honest I completely forgot all the history here. Looks like
your initial patches couldn't even build, so I can't compare well with
what it would've looked like before.
I think it might make sense to treat "accumulated" and "non-MLO"
statistics the same.
In the accumulated statistics you add
+ u32 rx_packets;
+ u32 tx_packets;
+ u64 rx_bytes;
+ u64 tx_bytes;
+ u32 tx_retries;
+ u32 tx_failed;
+ u32 rx_dropped_misc;
+ u32 beacon_loss_count;
+ u32 expected_throughput;
and realistically we could also have _most_ of the others (not all
really make sense though, admittedly), I think it'd be easier to just
*add* per-link statistics instead of refactoring things completely in
all the drivers, at least for an initial version. We can see more
cleanups later.
We can also prohibit filling entries that make no sense for multi-link
statistics when provided on a global level, though honestly I feel like
for most of them we should actually provide some value, even if tx/rx
rate is just the best rate of all the links, or something like that?
However especially initially as the code isn't complete, we can also say
that things like pertid stats aren't allowed on the STA level, only on
the per-link level, if per-link statistics are provided at all; that
would save some code in the accumulation in cfg80211.
So I think overall it'd be nicer to
1) Add a separate struct link_station_info to what we have now
(instead of refactoring everything from station_info into that
new struct).
Maybe also need to see if everything really makes sense - you showed
the connected time as global in the commit message, but have it per
link in the structure?
This would also not yet require any drivers or mac80211 to change at
all, since it's just adding a new capability.
Oh, in terms of allocation - the struct isn't huge now, do we even
care about simply adding 15 links? It still stays well below a
page as far as I can tell ... keep it simpler for now?
That also saves the back and forth you have with patch 08 where you
basically just add stuff back that was there before. But now you've
already converted all the drivers, needlessly in a way.
2) Add the needed nl80211 to show the per-link values, and that then
immediately requires the accumulation logic. For values that we
don't have accumulation logic for (yet) we can suppress the ->filled
value, possibly with a warning/message?
3) I don't think we need this wiphy flag really, as long as we have all
the links allocated we can see if anything was filled. If nothing
was filled at all we can know the link wasn't there?
But also that's to say that it doesn't really _matter_ if a driver
has multi-link or not, even if it has multi-link and can only
provide accumulated statistics then - apart from it having to figure
out how to provide the rates if it wants to - that should be fine?
Up to this point the this outline should give us maybe a slightly less
efficient internal representation, but an internal representation that
doesn't require changing hundreds of lines of code across every driver.
In fact, it should require _zero_ changes in any driver up to this point
because it's perfectly valid for an MLO connection to provide *only*
accumulated statistics (of course not what we want, eventually.)
Next would be actually making use of this, of course, which is more
work:
4) If we now have station_info with links[] in it, statically
allocated, mac80211 can be refactored to fill that, basically what
you have in patch 9 except without the allocations.
5) Patches 10 and 11 basically stay as is.
I don't really see _much_ disadvantage with this. Yes, we allocate more
memory, but these are ephemeral allocations, we don't even hang on to
them very long. An array that is literally of structs rather than
pointers to them is easier to deal with. Having pointers would also
work, or having fewer entries and not indexing by link ID, or having the
per-link as a variable array at the end and then it's still an array
just allocating how much it needs ... But I'm not sure that's all even
worth it for an allocation that'd now be ~2.7k and very short-lived? And
allocating more smaller structs probably isn't really all that much
better than one bigger one either?
Anyway if we find an issue with this we can still rework the internals
later, but I'd rather not have so many driver changes unless we know we
need them.
johannes
