[RFC PATCH] mac80211: clip ADDBA instead of bailing out

[RFC PATCH] mac80211: clip ADDBA instead of bailing out

[Alexandre Ferrieux]

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>
---

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;

Re: [RFC PATCH] mac80211: clip ADDBA instead of bailing out

[James Prestwood]

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;
>
>

Re: [RFC PATCH] mac80211: clip ADDBA instead of bailing out

[alexandre.ferrieux]

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;
>>
>>

____________________________________________________________________________________________________________
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.

Re: [RFC PATCH] mac80211: clip ADDBA instead of bailing out

[James Prestwood]

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.

Re: [RFC PATCH] mac80211: clip ADDBA instead of bailing out

[James Prestwood]

Hi Alexandre,

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

Fwiw I didn't see any difference in throughput with and without this 
patch. We do use wifi 6/6e APs and wifi 5 clients (ath10k), but I 
suspect it has something to do with our configuration not taking full 
advantage of 6/6e speeds, so the APs may not be proposing an ADDBA size 
that the client rejects. But I'll keep this in mind if we ever notice 
the behavior.

Overall though I didn't observe and regression-type behavior.

Thanks,

James

>
> 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;
>>>
>>>
> ____________________________________________________________________________________________________________
> 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.