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

[James Prestwood <prestwoj@gmail.com>]

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.