Per MLO link TX stats

Per MLO link TX stats

[Ben Greear]

Hello,

I've been trying to get better per MLO link stats.  I see that the rx path
stats are there, but in the tx path, I do not see anything other than 'deflink'
getting updated.

And, at least with iwlwifi, I cannot find any way to actually see what link-id
was used to transmit a frame in the tx status callbacks (or anywhere else I looked).

In the case where there is a single active link, then I can hack something together
that should be at least mostly right, but that won't fix any future radio that can
do 2+ active links.

Any suggestions for best path forward on this?

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Per MLO link TX stats

[Johannes Berg]

On Fri, 2024-07-12 at 10:52 -0700, Ben Greear wrote:
> Hello,
> 
> I've been trying to get better per MLO link stats.  I see that the rx path
> stats are there, but in the tx path, I do not see anything other than 'deflink'
> getting updated.

Yeah, nobody really cared too much about statistics yet. I know. Sorry.
Not much time to work on it either now.

> And, at least with iwlwifi, I cannot find any way to actually see what link-id
> was used to transmit a frame in the tx status callbacks (or anywhere else I looked).

You can probably derive that from the band for iwlwifi, since it only
supports one active link on each 2.4 / 5 / 6 GHz if at all. And the band
I guess you'd get from the LMAC ID or something. Should be in there
somewhere... IOW, I think it's possible, but I'm also not entirely sure
how we'd do it.

> In the case where there is a single active link, then I can hack something together
> that should be at least mostly right, but that won't fix any future radio that can
> do 2+ active links.
> 
> Any suggestions for best path forward on this?

I really think we also need to do some work on the API/cfg80211 level,
and have link station statistics in cfg80211 instead of full station,
and then combine them to (older) userspace in cfg80211, i.e. if
userspace doesn't request broken out per-link statistics. There's
probably a bunch of work here, and I only have a vague idea of how it
should be done...

Re: Per MLO link TX stats

[Ben Greear]

On 8/27/24 08:56, Johannes Berg wrote:
> On Fri, 2024-07-12 at 10:52 -0700, Ben Greear wrote:
>> Hello,
>>
>> I've been trying to get better per MLO link stats.  I see that the rx path
>> stats are there, but in the tx path, I do not see anything other than 'deflink'
>> getting updated.
> 
> Yeah, nobody really cared too much about statistics yet. I know. Sorry.
> Not much time to work on it either now.
> 
>> And, at least with iwlwifi, I cannot find any way to actually see what link-id
>> was used to transmit a frame in the tx status callbacks (or anywhere else I looked).
> 
> You can probably derive that from the band for iwlwifi, since it only
> supports one active link on each 2.4 / 5 / 6 GHz if at all. And the band
> I guess you'd get from the LMAC ID or something. Should be in there
> somewhere... IOW, I think it's possible, but I'm also not entirely sure
> how we'd do it.

When be200 goes into eMLSR mode, both 5 and 6Ghz links are shown as active, so at least
you cannot use 'active link' to reliably update stats.

>> In the case where there is a single active link, then I can hack something together
>> that should be at least mostly right, but that won't fix any future radio that can
>> do 2+ active links.
>>
>> Any suggestions for best path forward on this?
> 
> I really think we also need to do some work on the API/cfg80211 level,
> and have link station statistics in cfg80211 instead of full station,
> and then combine them to (older) userspace in cfg80211, i.e. if
> userspace doesn't request broken out per-link statistics. There's
> probably a bunch of work here, and I only have a vague idea of how it
> should be done...

I think first step is to get the driver(s) able to report the link-id in
the tx-status.  After that, mac80211 can gather the stats.  I hacked
tx/rx link stats into mac80211 ethtool (for first 3 links), but it is still not reliable since
mac80211 doesn't know the actual tx link id.

After that, then certainly I'd be happy to have per-link stats available,
and combining them in cfg80211 seems like a fine idea as well.  Some things
that don't combine well (rssi, link rates, etc) would take a bit of kludging
if trying to provide a single 'sta' view of stats.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Per MLO link TX stats

[Johannes Berg]

On Tue, 2024-08-27 at 09:12 -0700, Ben Greear wrote:
> 
> When be200 goes into eMLSR mode, both 5 and 6Ghz links are shown as active, so at least
> you cannot use 'active link' to reliably update stats.

Sure, not active link - but there's an LMAC bit somewhere ... Ah, it's
not documented, it's actually documented *differently*, but it should be
bit 31 in len_n_flags in struct iwl_rx_packet.

Given the LMAC ID on the TX response notification we should know which
LMAC transmitted it, and then I think it's a simple mapping to the
active link. But I haven't actually really tried it.

> > > In the case where there is a single active link, then I can hack something together
> > > that should be at least mostly right, but that won't fix any future radio that can
> > > do 2+ active links.
> > > 
> > > Any suggestions for best path forward on this?
> > 
> > I really think we also need to do some work on the API/cfg80211 level,
> > and have link station statistics in cfg80211 instead of full station,
> > and then combine them to (older) userspace in cfg80211, i.e. if
> > userspace doesn't request broken out per-link statistics. There's
> > probably a bunch of work here, and I only have a vague idea of how it
> > should be done...
> 
> I think first step is to get the driver(s) able to report the link-id in
> the tx-status.  After that, mac80211 can gather the stats.  

Yeah, that makes sense, at least partially that's needed. I suspect that
also we need to extend the API down to the sta_statistics call though to
return per-link statistics, e.g. the TX bitrate would seem should be
reported per link, and that's done through that call now I believe.

> I hacked
> tx/rx link stats into mac80211 ethtool (for first 3 links), but it is still not reliable since
> mac80211 doesn't know the actual tx link id.

Right.

> After that, then certainly I'd be happy to have per-link stats available,
> and combining them in cfg80211 seems like a fine idea as well.  Some things
> that don't combine well (rssi, link rates, etc) would take a bit of kludging
> if trying to provide a single 'sta' view of stats.

True, some can't just be added up and we'd have to find a sane different
"best" view, perhaps for rates it'd be the better of the two or the sum
if only reporting the bitrate, or better of the RSSI, etc. Case by case,
I guess.

johannes

Re: Per MLO link TX stats

[Ben Greear]

On 8/27/24 09:20, Johannes Berg wrote:
> On Tue, 2024-08-27 at 09:12 -0700, Ben Greear wrote:
>>
>> When be200 goes into eMLSR mode, both 5 and 6Ghz links are shown as active, so at least
>> you cannot use 'active link' to reliably update stats.
> 
> Sure, not active link - but there's an LMAC bit somewhere ... Ah, it's
> not documented, it's actually documented *differently*, but it should be
> bit 31 in len_n_flags in struct iwl_rx_packet.
> 
> Given the LMAC ID on the TX response notification we should know which
> LMAC transmitted it, and then I think it's a simple mapping to the
> active link. But I haven't actually really tried it.

If you can share a patch that documents this bit (like 0 means 5Ghz and 1 means 6Ghz??)
then we can try it out.

And maybe your idea for how to report it in tx-status too since that will touch
mac80211?

>>>> In the case where there is a single active link, then I can hack something together
>>>> that should be at least mostly right, but that won't fix any future radio that can
>>>> do 2+ active links.
>>>>
>>>> Any suggestions for best path forward on this?
>>>
>>> I really think we also need to do some work on the API/cfg80211 level,
>>> and have link station statistics in cfg80211 instead of full station,
>>> and then combine them to (older) userspace in cfg80211, i.e. if
>>> userspace doesn't request broken out per-link statistics. There's
>>> probably a bunch of work here, and I only have a vague idea of how it
>>> should be done...
>>
>> I think first step is to get the driver(s) able to report the link-id in
>> the tx-status.  After that, mac80211 can gather the stats.
> 
> Yeah, that makes sense, at least partially that's needed. I suspect that
> also we need to extend the API down to the sta_statistics call though to
> return per-link statistics, e.g. the TX bitrate would seem should be
> reported per link, and that's done through that call now I believe.

Yes.  At least at cfg80211 level, I think we should be able to query all
link stats in a single call into mac80211.  Down in mac80211, then per-link
stats structs appears to be how things are done now and seems like a good
solution.

> 
>> I hacked
>> tx/rx link stats into mac80211 ethtool (for first 3 links), but it is still not reliable since
>> mac80211 doesn't know the actual tx link id.
> 
> Right.
> 
>> After that, then certainly I'd be happy to have per-link stats available,
>> and combining them in cfg80211 seems like a fine idea as well.  Some things
>> that don't combine well (rssi, link rates, etc) would take a bit of kludging
>> if trying to provide a single 'sta' view of stats.
> 
> True, some can't just be added up and we'd have to find a sane different
> "best" view, perhaps for rates it'd be the better of the two or the sum
> if only reporting the bitrate, or better of the RSSI, etc. Case by case,
> I guess.

I'd suggest using highest active band when we have no better option for
summing/combining.

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Per MLO link TX stats

[Johannes Berg]

On Tue, 2024-08-27 at 09:28 -0700, Ben Greear wrote:
> On 8/27/24 09:20, Johannes Berg wrote:
> > On Tue, 2024-08-27 at 09:12 -0700, Ben Greear wrote:
> > > 
> > > When be200 goes into eMLSR mode, both 5 and 6Ghz links are shown as active, so at least
> > > you cannot use 'active link' to reliably update stats.
> > 
> > Sure, not active link - but there's an LMAC bit somewhere ... Ah, it's
> > not documented, it's actually documented *differently*, but it should be
> > bit 31 in len_n_flags in struct iwl_rx_packet.
> > 
> > Given the LMAC ID on the TX response notification we should know which
> > LMAC transmitted it, and then I think it's a simple mapping to the
> > active link. But I haven't actually really tried it.
> 
> If you can share a patch that documents this bit (like 0 means 5Ghz and 1 means 6Ghz??)
> then we can try it out.

I think yes, 5 GHz should be on LMAC 0 and 6 GHz on LMAC 1, and that's
the only case where we can have two active links simultaneously.

There's still a race though, when we change the active links while
transmitting, not sure how to handle that.

Oh wait, it's simpler than that - we have the STA pointer in there
already (see iwl_mvm_rx_tx_cmd_single and iwl_mvm_rx_tx_cmd_agg), but
since we get that from the FW STA ID, we obviously also know the *link*
STA since the FW STA IDs are per link, so we can just go from there to
the link ID directly.

link_sta = rcu_dereference(mvm->fw_id_to_link_sta[notif->sta_id]);

link_sta->link_id

> And maybe your idea for how to report it in tx-status too since that will touch
> mac80211?

I hadn't really thought about that ... I guess we could use the
IEEE80211_TX_CTRL_MLO_LINK space also for status? It's already filled to
the link ID by mac80211 for TX if the frame must go out on a specific
link (or 0xF otherwise which is an invalid link ID anyway.)

> > > > > In the case where there is a single active link, then I can hack something together
> > > > > that should be at least mostly right, but that won't fix any future radio that can
> > > > > do 2+ active links.
> > > > > 
> > > > > Any suggestions for best path forward on this?
> > > > 
> > > > I really think we also need to do some work on the API/cfg80211 level,
> > > > and have link station statistics in cfg80211 instead of full station,
> > > > and then combine them to (older) userspace in cfg80211, i.e. if
> > > > userspace doesn't request broken out per-link statistics. There's
> > > > probably a bunch of work here, and I only have a vague idea of how it
> > > > should be done...
> > > 
> > > I think first step is to get the driver(s) able to report the link-id in
> > > the tx-status.  After that, mac80211 can gather the stats.
> > 
> > Yeah, that makes sense, at least partially that's needed. I suspect that
> > also we need to extend the API down to the sta_statistics call though to
> > return per-link statistics, e.g. the TX bitrate would seem should be
> > reported per link, and that's done through that call now I believe.
> 
> Yes.  At least at cfg80211 level, I think we should be able to query all
> link stats in a single call into mac80211.  Down in mac80211, then per-link
> stats structs appears to be how things are done now and seems like a good
> solution.

Not sure I have a strong opinion on this being a single call or not,
either way seems reasonable really.

> > 
> > > I hacked
> > > tx/rx link stats into mac80211 ethtool (for first 3 links), but it is still not reliable since
> > > mac80211 doesn't know the actual tx link id.
> > 
> > Right.
> > 
> > > After that, then certainly I'd be happy to have per-link stats available,
> > > and combining them in cfg80211 seems like a fine idea as well.  Some things
> > > that don't combine well (rssi, link rates, etc) would take a bit of kludging
> > > if trying to provide a single 'sta' view of stats.
> > 
> > True, some can't just be added up and we'd have to find a sane different
> > "best" view, perhaps for rates it'd be the better of the two or the sum
> > if only reporting the bitrate, or better of the RSSI, etc. Case by case,
> > I guess.
> 
> I'd suggest using highest active band when we have no better option for
> summing/combining.

But is that the most useful thing? What if the RSSI is quite bad there
(but still good) and then NetworkManager shows a really weak signal when
in reality you have a good connection on another link? Anyway, dunno,
we're not near that bridge yet I think ;)

johannes

Re: Per MLO link TX stats

[Ben Greear]

On 8/27/24 09:41, Johannes Berg wrote:
> On Tue, 2024-08-27 at 09:28 -0700, Ben Greear wrote:
>> On 8/27/24 09:20, Johannes Berg wrote:
>>> On Tue, 2024-08-27 at 09:12 -0700, Ben Greear wrote:
>>>>
>>>> When be200 goes into eMLSR mode, both 5 and 6Ghz links are shown as active, so at least
>>>> you cannot use 'active link' to reliably update stats.
>>>
>>> Sure, not active link - but there's an LMAC bit somewhere ... Ah, it's
>>> not documented, it's actually documented *differently*, but it should be
>>> bit 31 in len_n_flags in struct iwl_rx_packet.
>>>
>>> Given the LMAC ID on the TX response notification we should know which
>>> LMAC transmitted it, and then I think it's a simple mapping to the
>>> active link. But I haven't actually really tried it.
>>
>> If you can share a patch that documents this bit (like 0 means 5Ghz and 1 means 6Ghz??)
>> then we can try it out.
> 
> I think yes, 5 GHz should be on LMAC 0 and 6 GHz on LMAC 1, and that's
> the only case where we can have two active links simultaneously.
> 
> There's still a race though, when we change the active links while
> transmitting, not sure how to handle that.
> 
> Oh wait, it's simpler than that - we have the STA pointer in there
> already (see iwl_mvm_rx_tx_cmd_single and iwl_mvm_rx_tx_cmd_agg), but
> since we get that from the FW STA ID, we obviously also know the *link*
> STA since the FW STA IDs are per link, so we can just go from there to
> the link ID directly.
> 
> link_sta = rcu_dereference(mvm->fw_id_to_link_sta[notif->sta_id]);
> 
> link_sta->link_id
> 
>> And maybe your idea for how to report it in tx-status too since that will touch
>> mac80211?
> 
> I hadn't really thought about that ... I guess we could use the
> IEEE80211_TX_CTRL_MLO_LINK space also for status? It's already filled to
> the link ID by mac80211 for TX if the frame must go out on a specific
> link (or 0xF otherwise which is an invalid link ID anyway.)

So, something like this (compile tested only at this point)?

diff --git a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
index e7cb6dcde182..92f7d0d34421 100644
--- a/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
+++ b/drivers/net/wireless/intel/iwlwifi/mvm/tx.c
@@ -1963,6 +1963,9 @@ static void iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm,
         u8 lq_color;
         u16 next_reclaimed, seq_ctl;
         bool is_ndp = false;
+       struct ieee80211_link_sta *link_sta;
+
+       link_sta = rcu_dereference(mvm->fw_id_to_link_sta[sta_id]);

         __skb_queue_head_init(&skbs);

@@ -1989,6 +1992,10 @@ static void iwl_mvm_rx_tx_cmd_single(struct iwl_mvm *mvm,

                 memset(&info->status, 0, sizeof(info->status));
                 info->flags &= ~(IEEE80211_TX_STAT_ACK | IEEE80211_TX_STAT_TX_FILTERED);
+               if (link_sta) {
+                       info->control.flags &= ~(u32_encode_bits(0xF, IEEE80211_TX_CTRL_MLO_LINK));
+                       info->control.flags |= u32_encode_bits(link_sta->link_id, IEEE80211_TX_CTRL_MLO_LINK);
+               }

                 /* inform mac80211 about what happened with the frame */
                 switch (status & TX_STATUS_MSK) {

And something similar for the agg path....

Thanks,
Ben

-- 
Ben Greear <greearb@candelatech.com>
Candela Technologies Inc  http://www.candelatech.com

Re: Per MLO link TX stats

[Johannes Berg]

On Tue, 2024-08-27 at 11:17 -0700, Ben Greear wrote:
> 
> So, something like this (compile tested only at this point)?

Something like, yeah.

But also need to define that mac80211 actually consumes that value on TX
status etc., so this isn't really the first thing to do.

> +               if (link_sta) {
> +                       info->control.flags &= ~(u32_encode_bits(0xF, IEEE80211_TX_CTRL_MLO_LINK));

don't really need the encode-bits here since the mask is already 4 bits
:)

johannes