Re: [PATCH 2/9] mac80211: limit the A-MSDU Tx based on peer's capabilities

On 12/08/2015 07:49 PM, Krishna Chaitanya wrote:

On Dec 8, 2015 10:13 PM, "Grumbach, Emmanuel"
<emmanuel.grumbach@intel.com <mailto:emmanuel.grumbach@intel.com>> wrote:

quoted

On 12/08/2015 06:35 PM, Krishna Chaitanya wrote:

quoted

On Dec 8, 2015 10:01 PM, "Grumbach, Emmanuel"
<emmanuel.grumbach@intel.com <mailto:emmanuel.grumbach@intel.com>

<mailto:emmanuel.grumbach@intel.com
<mailto:emmanuel.grumbach@intel.com>>> wrote:

quoted

quoted

quoted

On 12/08/2015 06:03 PM, Krishna Chaitanya wrote:

quoted

On Tue, Dec 8, 2015 at 7:34 PM, Emmanuel Grumbach
<emmanuel.grumbach@intel.com

<mailto:emmanuel.grumbach@intel.com>
<mailto:emmanuel.grumbach@intel.com <mailto:emmanuel.grumbach@intel.com>>>

quoted

quoted

wrote:

quoted

quoted

quoted

index 7a76ce6..f4a5287 100644

--- a/net/mac80211/ht.c
+++ b/net/mac80211/ht.c

@@ -230,6 +230,11 @@ bool

ieee80211_ht_cap_ie_to_sta_ht_cap(struct ieee80211_sub_if_data *sdata,

quoted

quoted

quoted

        /* set Rx highest rate */
        ht_cap.mcs.rx_highest = ht_cap_ie->mcs.rx_highest;

+       if (ht_cap.cap & IEEE80211_HT_CAP_MAX_AMSDU)
+               sta->sta.max_amsdu_len =

IEEE80211_MAX_MPDU_LEN_HT_7935;

quoted

quoted

quoted

+       else
+               sta->sta.max_amsdu_len =

IEEE80211_MAX_MPDU_LEN_HT_3839;

quoted

quoted

quoted

+
  apply:
        changed = memcmp(&sta->sta.ht_cap, &ht_cap,

sizeof(ht_cap));

quoted

quoted

quoted

quoted

quoted

diff --git a/net/mac80211/vht.c b/net/mac80211/vht.c
index 92c9843..d2f2ff6 100644
--- a/net/mac80211/vht.c
+++ b/net/mac80211/vht.c

@@ -281,6 +281,24 @@ ieee80211_vht_cap_ie_to_sta_vht_cap(struct

ieee80211_sub_if_data *sdata,

quoted

quoted

quoted

        }

        sta->sta.bandwidth = ieee80211_sta_cur_vht_bw(sta);
+
+       /* If HT IE reported 3839 bytes only, stay with that

size. */

quoted

quoted

quoted

quoted

quoted

+       if (sta->sta.max_amsdu_len ==

IEEE80211_MAX_MPDU_LEN_HT_3839)

quoted

quoted

quoted

quoted

quoted

+               return;
+
+       switch (vht_cap->cap & IEEE80211_VHT_CAP_MAX_MPDU_MASK) {
+       case IEEE80211_VHT_CAP_MAX_MPDU_LENGTH_11454:
+               sta->sta.max_amsdu_len =

IEEE80211_MAX_MPDU_LEN_VHT_11454;

quoted

quoted

quoted

+               break;
+       case IEEE80211_VHT_CAP_MAX_MPDU_LENGTH_7991:
+               sta->sta.max_amsdu_len =

IEEE80211_MAX_MPDU_LEN_VHT_7991;

quoted

quoted

quoted

+               break;
+       case IEEE80211_VHT_CAP_MAX_MPDU_LENGTH_3895:
+       default:
+               sta->sta.max_amsdu_len =

IEEE80211_MAX_MPDU_LEN_VHT_3895;

quoted

quoted

quoted

+               break;
+       }

Ideally speaking shouldn't we use different variables for HT

and VHT

quoted

quoted

quoted

quoted

and depending on
rate HT/VHT we should aggregate accordingly? of course that

will be

quoted

quoted

quoted

quoted

tricky as we dont
have the rate control info at the time of aggregation. Standard is
clear about usage of
independent lengths depending on whether its an VHT/HT PPDU.

Yes - but since it is tricky, I preferred to be on the safe side and
limit VHT amsdus for the very peculiar AP that would decide to have
large A-MSDUs in VHT and small ones in HT (?!).

Yes but wouldn't it be safer to just use min(ht , vht)? For a good AP
it shouldn't matter and bad AP it will still work.

This is the de-facto what this code does I think.
If the HT limit is 4K, then don't even take the VHT limit into account
and the VHT limit can't be smaller than the HT one in that case.
If the HT limit is 8K, then we can limit it further to 4K in case VHT
has a limit of 4K (which is another weird case), but we can also make it
larger for VHT frames?

So I don't really see the bug here, am I missing something?

If we take a maximum case HT=8K and VHT=12K then we endup sending 12K
frames using HT rates... Which is not expected...

Right, but it is explicitly mentioned in the API that if you use an HT
preamble you should limit the A-MSDU to 8K.
If the HT limit was to be 4K, then the VHT would be 4K as well (even if
a broken peer would advertise 8K in HT and 4K in VHT).

quoted

quoted

quoted

quoted

If we use the same variable to arrive at max_amsdu_len for

both VHT

quoted

quoted

quoted

quoted

and HT, we might
end up sending 11454 sized MSDU in a HT rate.

And since that can't be handled at the time of the A-MSDU building,
leave that to another entity :)