Re: [RFC v2] mac80211: add A-MSDU tx support

[Felix Fietkau <nbd@openwrt.org>]

On 2016-02-07 12:32, Emmanuel Grumbach wrote:
>>>>>>> +
>>>>>>> +static bool ieee80211_amsdu_aggregate(struct ieee80211_sub_if_data *sdata,
>>>>>>> +                                     struct sta_info *sta,
>>>>>>> +                                     struct ieee80211_fast_tx *fast_tx,
>>>>>>> +                                     struct sk_buff *skb)
>>>>>>> +{
>>>>>>> +       struct ieee80211_local *local = sdata->local;
>>>>>>> +       u8 tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
>>>>>>> +       struct ieee80211_txq *txq = sta->sta.txq[tid];
>>>>>>> +       struct txq_info *txqi;
>>>>>>> +       struct sk_buff **frag_tail, *head;
>>>>>>> +       int subframe_len = skb->len - ETH_ALEN;
>>>>>>> +       int max_amsdu_len;
>>>>>>> +       __be16 len;
>>>>>>> +       void *data;
>>>>>>> +       bool ret = false;
>>>>>>> +       int n = 1;
>>>>>>> +
>>>>>>> +       if (!ieee80211_hw_check(&local->hw, TX_AMSDU))
>>>>>>> +               return false;
>>>>>>> +
>>>>>>> +       if (!txq)
>>>>>>> +               return false;
>>>>>>> +
>>>>>>> +       txqi = to_txq_info(txq);
>>>>>>> +       if (test_bit(IEEE80211_TXQ_NO_AMSDU, &txqi->flags))
>>>>>>> +               return false;
>>>>>>> +
>>>>>>> +       /*
>>>>>>> +        * A-MPDU limits maximum MPDU size to 4095 bytes. Since aggregation
>>>>>>> +        * sessions are started/stopped without txq flush, use the limit here
>>>>>>> +        * to avoid having to de-aggregate later.
>>>>>>> +        */
>>>>>>> +       max_amsdu_len = min_t(int, sta->sta.max_amsdu_len, 4095);
>>>>>>
>>>>>> So you can't get 10K A-MSDUs? I don't see where you check that you
>>>>>> have an A-MPDU session here. You seem to be applying the 4095 limit
>>>>>> also for streams that are not an A-MPDU?
>>>>>> I guess you could check if the sta is a VHT peer, in that case, no
>>>>>> limit applies.
>>>>> The explanation for the missing A-MPDU change is in that comment -
>>>>> checking for an active A-MPDU session would make it unnecessarily complex.
>>>>> Good point about checking for VHT capabilities to remove this limit, I
>>>>> will add that.
>>>
>>> Yes - I read the comment, but it seemed very sub-optimal to limit all
>>> the A-MSDUs to 4K. With TSO I can get up to 10K and it really helps
>>> TPT.
>> This was built with the assumption that most scenarios use A-MPDU anyway
>> and thus don't need really large A-MSDUs.
> 
> Yes - so that's interesting. We can chose to have long A-MSDUs inside
> a short (in terms of number of MPDUs) A-MPDU, of with shorter A-MSDU
> and squeeze more of these into a single A-MDPU.
> The first intuition says that we'd better have more MPDUs because of
> the CRC check for each MPDU which doesn't exist in A-MSDU. OTOH, I
> remember I could clearly see that I get a higher TPT with longer
> A-MSDUs. Maybe I wasn't looking right at the size of the A-MPDU? I
> guess I'd need to go back to the table with all the values we had, but
> since we pretty much got what we wanted, I am not sure I will able to
> find time for this :)
I think it also depends on the environment. I'd guess that under very
ideal conditions with very few retransmissions, really long A-MSDU might
have some performance benefits, but I don't think that'll hold if the
conditions are less than ideal and you have rate fluctuation and
retransmissions.

>>> One more point. In VHT, there may be a limit on the numbers of
>>> subframes in the A-MSDU. I don't see you handle that. Maybe I missed
>>> it?
>> I haven't looked at that much yet. Right now the driver can only specify
>> a limit for the number of subframes.
> 
> I am talking about a limitation the peer can advertise. Check this out:
> https://git.kernel.org/cgit/linux/kernel/git/jberg/mac80211-next.git/tree/net/mac80211/cfg.c#n1134
> 
> I couldn't see the limit the driver can specify in your code. I may
> very well have missed it.
I missed that one. Will add it in the next patch.

>>> This is an order 4 allocation, for each A-MSDU. Note
>>> that iwlwifi (and probably other drivers) can handle gather DMA in Tx,
>>> but they have a limited number of frags they can handle. iwlwifi e.g.
>>> can handle up to 20 frags, but 3 are taken for "paperwork". You'll
>>> have 2 frags per subframe at least (assuming that each subframe's
>>> payload is nicely contiguous and not on a page boundary). I think that
>>> it may be worthwhile to ask the driver how many frags it is supposed
>>> to handle. I can't promise iwlwifi will use it, but I guess it will be
>>> useful for someone.
>> You mean an extra frag limit in addition to the driver subframe limit,
>> in case individual subframes are fragmented as well?
>>
> 
> well.. Yes, you can't assume that you'll have one descriptor for one
> MSDU payload  (unless the driver doesn't advertise SG to the
> netstack).
Okay, please make a suggestion describing the exact kinds of limits you
would need for iwlwifi.

- Felix