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

[Felix Fietkau <nbd@openwrt.org>]

On 2016-02-07 08:25, Emmanuel Grumbach wrote:
> On Fri, Feb 5, 2016 at 12:41 PM, Felix Fietkau <nbd@openwrt.org> wrote:
>> Requires software tx queueing support. frag_list support (for zero-copy)
>> is optional.
>>
>> Signed-off-by: Felix Fietkau <nbd@openwrt.org>
> 
> Looks nice!
> This would allow us to create aggregates of TCP Acks, the problem is
> that when you are mostly receiving data, the hardware queues are
> pretty much empty (nothing besides the TCP Acks which should go out
> quickly) so that packets don't pile up in the software queues and
> hence you don't have enough material to build an A-MSDU.
> I guess that for AP oriented devices, this is ideal solution since you
> can't rely on TSO (packets are not locally generated) and this allows
> to build an A-MSDU without adding more latency since you build an
> A-MSDU with packets that are already in the queue waiting instead of
> delaying transmission of the first packet.
> IIRC, the latter was the approach chose by the new Marvell driver
> posted a few weeks ago. This approach is better in my eyes.
> For iwlwifi which is much more station oriented (of GO which is
> basically an AP with locally generated traffic), I took the TSO
> approach. I guess we could try to change iwlwifi to use your tx
> queues, and check how that works. This would allow us to have A-MSDU
> on bridged traffic as well, although this use case is much less common
> for Intel devices.
Can the iwlwifi firmware maintain per-sta per-tid queues? Because that
way you would get the most benefits from using that tx queueing
infrastructure.

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

- Felix