| Message ID | 69c63a19-5419-4bbe-858f-6ca100345a28@orange.com (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFC] mac80211: clip ADDBA instead of bailing out | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Guessing tree name failed - patch did not apply |
Hi Alexandre, On 3/19/25 3:58 AM, Alexandre Ferrieux wrote: > When a Linux Wifi{4,5} device talks to a Wifi6 AP, if the AP proposes a Block > Acknowledgement aggregation size (ADDBA) exceeding its expectations, the code in > mac80211 just bails out, rejecting the aggregation. This yields a big > performance penalty on the ack path, which is observable in comparison with > other OSes (Windows and MacOS) which "play smarter" and accept the proposal with > a "clipped" size. Out of curiosity do you have any performance numbers for this, like Linux vs Windows vs MacOS? We ran into a significant performance hit after I added multicast RX support on ath10k (after ~30 clients were on the same channel). After looking into the pcaps we saw many ADDBA failures and ultimately had to disable multicast RX. I want to give this patch a try either way, but I was curious if you had any data on performance improvements. > > A typical scenario would be: > > AP -> Device : ADDBA_request(size=256) > > Current Linux reaction: > > Device -> AP : ADDBA_reply(failure) > > Other OSes reaction: > > Device -> AP : ADDBA_reply(size=64) > > Note that the IEEE802.11 standard allows for both reactions, but it sounds > really suboptimal to be bailing out instead of clipping. The patch below does > the latter. > > Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> > --- > > diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c > index f3fbe5a4395e..264dad847842 100644 > --- a/net/mac80211/agg-rx.c > +++ b/net/mac80211/agg-rx.c > @@ -317,18 +317,20 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta, > max_buf_size = IEEE80211_MAX_AMPDU_BUF_HT; > > /* sanity check for incoming parameters: > - * check if configuration can support the BA policy > - * and if buffer size does not exceeds max value */ > + * check if configuration can support the BA policy */ > /* XXX: check own ht delayed BA capability?? */ > if (((ba_policy != 1) && > - (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA))) || > - (buf_size > max_buf_size)) { > - status = WLAN_STATUS_INVALID_QOS_PARAM; > + (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA)))) { > + status = WLAN_STATUS_INVALID_QOS_PARAM; > ht_dbg_ratelimited(sta->sdata, > "AddBA Req with bad params from %pM on tid > %u. policy %d, buffer size %d\n", > sta->sta.addr, tid, ba_policy, buf_size); > goto end; > } > + if (buf_size > max_buf_size) { > + buf_size = max_buf_size ; // Clip instead of bailing out > + } > + > /* determine default buffer size */ > if (buf_size == 0) > buf_size = max_buf_size; > >
Hi James, There is roughly a 8x slowdown :} I got these numbers from the colleagues who detected the issue - physical available bandwidth 1.733 Gbps (as per iwconfig) - ADDBA offer size=256 - effective bandwidth observed 1.2Gbps with accept-and-clip-ADDBA (size=64) - vs. 150 Mbps with reject-ADDBA Note, as a Wifi rookie it is not immediately obvious to me how the semantics of ack aggregation would interfere with broadcast actions, as ADDBA are supposedly unicast. But you're the expert :) On 19/03/2025 14:21, James Prestwood wrote: > -------------------------------------------------------------------------------------------------------------- > CAUTION : This email originated outside the company. Do not click on any links or open attachments unless you are expecting them from the sender. > > ATTENTION : Cet e-mail provient de l'extérieur de l'entreprise. Ne cliquez pas sur les liens ou n'ouvrez pas les pièces jointes à moins de connaitre l'expéditeur. > -------------------------------------------------------------------------------------------------------------- > > Hi Alexandre, > > On 3/19/25 3:58 AM, Alexandre Ferrieux wrote: >> When a Linux Wifi{4,5} device talks to a Wifi6 AP, if the AP proposes a Block >> Acknowledgement aggregation size (ADDBA) exceeding its expectations, the code in >> mac80211 just bails out, rejecting the aggregation. This yields a big >> performance penalty on the ack path, which is observable in comparison with >> other OSes (Windows and MacOS) which "play smarter" and accept the proposal with >> a "clipped" size. > Out of curiosity do you have any performance numbers for this, like > Linux vs Windows vs MacOS? We ran into a significant performance hit > after I added multicast RX support on ath10k (after ~30 clients were on > the same channel). After looking into the pcaps we saw many ADDBA > failures and ultimately had to disable multicast RX. I want to give this > patch a try either way, but I was curious if you had any data on > performance improvements. >> >> A typical scenario would be: >> >> AP -> Device : ADDBA_request(size=256) >> >> Current Linux reaction: >> >> Device -> AP : ADDBA_reply(failure) >> >> Other OSes reaction: >> >> Device -> AP : ADDBA_reply(size=64) >> >> Note that the IEEE802.11 standard allows for both reactions, but it sounds >> really suboptimal to be bailing out instead of clipping. The patch below does >> the latter. >> >> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> >> --- >> >> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c >> index f3fbe5a4395e..264dad847842 100644 >> --- a/net/mac80211/agg-rx.c >> +++ b/net/mac80211/agg-rx.c >> @@ -317,18 +317,20 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta, >> max_buf_size = IEEE80211_MAX_AMPDU_BUF_HT; >> >> /* sanity check for incoming parameters: >> - * check if configuration can support the BA policy >> - * and if buffer size does not exceeds max value */ >> + * check if configuration can support the BA policy */ >> /* XXX: check own ht delayed BA capability?? */ >> if (((ba_policy != 1) && >> - (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA))) || >> - (buf_size > max_buf_size)) { >> - status = WLAN_STATUS_INVALID_QOS_PARAM; >> + (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA)))) { >> + status = WLAN_STATUS_INVALID_QOS_PARAM; >> ht_dbg_ratelimited(sta->sdata, >> "AddBA Req with bad params from %pM on tid >> %u. policy %d, buffer size %d\n", >> sta->sta.addr, tid, ba_policy, buf_size); >> goto end; >> } >> + if (buf_size > max_buf_size) { >> + buf_size = max_buf_size ; // Clip instead of bailing out >> + } >> + >> /* determine default buffer size */ >> if (buf_size == 0) >> buf_size = max_buf_size; >> >>
On 3/19/25 7:18 AM, alexandre.ferrieux@orange.com wrote: > Hi James, > > There is roughly a 8x slowdown :} > I got these numbers from the colleagues who detected the issue > > - physical available bandwidth 1.733 Gbps (as per iwconfig) > - ADDBA offer size=256 > - effective bandwidth observed 1.2Gbps with accept-and-clip-ADDBA (size=64) > - vs. 150 Mbps with reject-ADDBA > > Note, as a Wifi rookie it is not immediately obvious to me how the semantics of > ack aggregation would interfere with broadcast actions, as ADDBA are supposedly > unicast. But you're the expert :) "expert", your giving me too much credit :) My running theory is that allowing multicast RX inadvertently caused the driver/firmware to receive more than just the action frames we registered for, and in turn bogged down the RX path, and introduced packet loss. We saw this externally as ADDBA retries in the pcaps. So it may have nothing to do with ADDBA rejections/failures, but when I saw this patch it got me thinking. Thanks for the numbers, either way this seems like a huge performance gain so I'm eager to test it out even if it has no effect on the multicast action RX performance. Thanks, James > > > > On 19/03/2025 14:21, James Prestwood wrote: >> -------------------------------------------------------------------------------------------------------------- >> CAUTION : This email originated outside the company. Do not click on any links or open attachments unless you are expecting them from the sender. >> >> ATTENTION : Cet e-mail provient de l'extérieur de l'entreprise. Ne cliquez pas sur les liens ou n'ouvrez pas les pièces jointes à moins de connaitre l'expéditeur. >> -------------------------------------------------------------------------------------------------------------- >> >> Hi Alexandre, >> >> On 3/19/25 3:58 AM, Alexandre Ferrieux wrote: >>> When a Linux Wifi{4,5} device talks to a Wifi6 AP, if the AP proposes a Block >>> Acknowledgement aggregation size (ADDBA) exceeding its expectations, the code in >>> mac80211 just bails out, rejecting the aggregation. This yields a big >>> performance penalty on the ack path, which is observable in comparison with >>> other OSes (Windows and MacOS) which "play smarter" and accept the proposal with >>> a "clipped" size. >> Out of curiosity do you have any performance numbers for this, like >> Linux vs Windows vs MacOS? We ran into a significant performance hit >> after I added multicast RX support on ath10k (after ~30 clients were on >> the same channel). After looking into the pcaps we saw many ADDBA >> failures and ultimately had to disable multicast RX. I want to give this >> patch a try either way, but I was curious if you had any data on >> performance improvements. >>> A typical scenario would be: >>> >>> AP -> Device : ADDBA_request(size=256) >>> >>> Current Linux reaction: >>> >>> Device -> AP : ADDBA_reply(failure) >>> >>> Other OSes reaction: >>> >>> Device -> AP : ADDBA_reply(size=64) >>> >>> Note that the IEEE802.11 standard allows for both reactions, but it sounds >>> really suboptimal to be bailing out instead of clipping. The patch below does >>> the latter. >>> >>> Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> >>> --- >>> >>> diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c >>> index f3fbe5a4395e..264dad847842 100644 >>> --- a/net/mac80211/agg-rx.c >>> +++ b/net/mac80211/agg-rx.c >>> @@ -317,18 +317,20 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta, >>> max_buf_size = IEEE80211_MAX_AMPDU_BUF_HT; >>> >>> /* sanity check for incoming parameters: >>> - * check if configuration can support the BA policy >>> - * and if buffer size does not exceeds max value */ >>> + * check if configuration can support the BA policy */ >>> /* XXX: check own ht delayed BA capability?? */ >>> if (((ba_policy != 1) && >>> - (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA))) || >>> - (buf_size > max_buf_size)) { >>> - status = WLAN_STATUS_INVALID_QOS_PARAM; >>> + (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA)))) { >>> + status = WLAN_STATUS_INVALID_QOS_PARAM; >>> ht_dbg_ratelimited(sta->sdata, >>> "AddBA Req with bad params from %pM on tid >>> %u. policy %d, buffer size %d\n", >>> sta->sta.addr, tid, ba_policy, buf_size); >>> goto end; >>> } >>> + if (buf_size > max_buf_size) { >>> + buf_size = max_buf_size ; // Clip instead of bailing out >>> + } >>> + >>> /* determine default buffer size */ >>> if (buf_size == 0) >>> buf_size = max_buf_size; >>> >>> > ____________________________________________________________________________________________________________ > Ce message et ses pieces jointes peuvent contenir des informations confidentielles ou privilegiees et ne doivent donc > pas etre diffuses, exploites ou copies sans autorisation. Si vous avez recu ce message par erreur, veuillez le signaler > a l'expediteur et le detruire ainsi que les pieces jointes. Les messages electroniques etant susceptibles d'alteration, > Orange decline toute responsabilite si ce message a ete altere, deforme ou falsifie. Merci. > > This message and its attachments may contain confidential or privileged information that may be protected by law; > they should not be distributed, used or copied without authorisation. > If you have received this email in error, please notify the sender and delete this message and its attachments. > As emails may be altered, Orange is not liable for messages that have been modified, changed or falsified. > Thank you.
diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c index f3fbe5a4395e..264dad847842 100644 --- a/net/mac80211/agg-rx.c +++ b/net/mac80211/agg-rx.c @@ -317,18 +317,20 @@ void __ieee80211_start_rx_ba_session(struct sta_info *sta, max_buf_size = IEEE80211_MAX_AMPDU_BUF_HT; /* sanity check for incoming parameters: - * check if configuration can support the BA policy - * and if buffer size does not exceeds max value */ + * check if configuration can support the BA policy */ /* XXX: check own ht delayed BA capability?? */ if (((ba_policy != 1) && - (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA))) || - (buf_size > max_buf_size)) { - status = WLAN_STATUS_INVALID_QOS_PARAM; + (!(sta->sta.deflink.ht_cap.cap & IEEE80211_HT_CAP_DELAY_BA)))) { + status = WLAN_STATUS_INVALID_QOS_PARAM; ht_dbg_ratelimited(sta->sdata, "AddBA Req with bad params from %pM on tid %u. policy %d, buffer size %d\n", sta->sta.addr, tid, ba_policy, buf_size); goto end; } + if (buf_size > max_buf_size) { + buf_size = max_buf_size ; // Clip instead of bailing out + } + /* determine default buffer size */ if (buf_size == 0)
When a Linux Wifi{4,5} device talks to a Wifi6 AP, if the AP proposes a Block Acknowledgement aggregation size (ADDBA) exceeding its expectations, the code in mac80211 just bails out, rejecting the aggregation. This yields a big performance penalty on the ack path, which is observable in comparison with other OSes (Windows and MacOS) which "play smarter" and accept the proposal with a "clipped" size. A typical scenario would be: AP -> Device : ADDBA_request(size=256) Current Linux reaction: Device -> AP : ADDBA_reply(failure) Other OSes reaction: Device -> AP : ADDBA_reply(size=64) Note that the IEEE802.11 standard allows for both reactions, but it sounds really suboptimal to be bailing out instead of clipping. The patch below does the latter. Signed-off-by: Alexandre Ferrieux <alexandre.ferrieux@gmail.com> --- buf_size = max_buf_size;