[PATCH] mac80211: fix comment regarding aggregation buf_size

[PATCH] mac80211: fix comment regarding aggregation buf_size

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

The description for buf_size was misleading and
just said you couldn't TX larger aggregates, but
of course you can't TX aggregates in a way that
would exceed the window either, which is possible
even if the aggregates are shorter than that.

Expand the description, thanks to Emmanuel for
explaining this to me.

Cc: Emmanuel Grumbach <egrumbach@gmail.com>
Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/net/mac80211.h |   13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

--- a/include/net/mac80211.h	2011-03-16 10:30:26.000000000 +0100
+++ b/include/net/mac80211.h	2011-03-16 10:37:49.000000000 +0100
@@ -1753,8 +1753,17 @@ enum ieee80211_ampdu_mlme_action {
  * 	that TX/RX_STOP can pass NULL for this parameter.
  *	The @buf_size parameter is only valid when the action is set to
  *	%IEEE80211_AMPDU_TX_OPERATIONAL and indicates the peer's reorder
- *	buffer size (number of subframes) for this session -- aggregates
- *	containing more subframes than this may not be transmitted to the peer.
+ *	buffer size (number of subframes) for this session -- the driver
+ *	may neither send aggregates containing more subframes than this
+ *	nor send aggregates in a way that lost frames would exceed the
+ *	buffer size. If just limiting the aggregate size, this would be
+ *	possible with a buf_size of 8:
+ *	 - TX: 1......8
+ *	 - RX:  2.....8 (lost frame #1)
+ *	 - TX:         9...1...
+ *	which is invalid since #1 was now re-transmitted well past the
+ *	buffer size of 8. The correct way to retransmit #1 would be:
+ *	 - TX:         19......
  *	Returns a negative error code on failure.
  *	The callback can sleep.
  *

Re: [PATCH] mac80211: fix comment regarding aggregation buf_size

[Daniel Halperin]

Hmm, this seems to not quite convey some complexities. See some inlines below.

On Wed, Mar 16, 2011 at 2:39 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> The description for buf_size was misleading and
> just said you couldn't TX larger aggregates, but
> of course you can't TX aggregates in a way that
> would exceed the window either, which is possible
> even if the aggregates are shorter than that.
>
> Expand the description, thanks to Emmanuel for
> explaining this to me.
>
> Cc: Emmanuel Grumbach <egrumbach@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/net/mac80211.h |   13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> --- a/include/net/mac80211.h    2011-03-16 10:30:26.000000000 +0100
> +++ b/include/net/mac80211.h    2011-03-16 10:37:49.000000000 +0100
> @@ -1753,8 +1753,17 @@ enum ieee80211_ampdu_mlme_action {
>  *     that TX/RX_STOP can pass NULL for this parameter.
>  *     The @buf_size parameter is only valid when the action is set to
>  *     %IEEE80211_AMPDU_TX_OPERATIONAL and indicates the peer's reorder
> - *     buffer size (number of subframes) for this session -- aggregates
> - *     containing more subframes than this may not be transmitted to the peer.
> + *     buffer size (number of subframes) for this session -- the driver
> + *     may neither send aggregates containing more subframes than this
> + *     nor send aggregates in a way that lost frames would exceed the
> + *     buffer size. If just limiting the aggregate size, this would be

<dan starts here>

> + *     possible with a buf_size of 8:
> + *      - TX: 1.....7
> + *      - RX: 2.....7 (lost frame #1),
> + *      - TX:         89...1...
> + *     which is invalid since #1 was now re-transmitted well past the
> + *     buffer size of 7. The correct way to retransmit #1 would be:
> + *      - TX:         1       or
> + *      - TX:         18
> + *     though the standard allows:
> + *      - TX:         81
> + *     The following might happen, but will violate buffering assumptions as above if frame 1 is lost again
> + *      - TX:         189.....

Dan

Re: [PATCH] mac80211: fix comment regarding aggregation buf_size

[Daniel Halperin]

That last line should really be:

>> + *     Note that the following is also illegal because it will violate buffering assumptions if frame 1 is lost again
>> + *      - TX:         189.....
>
> Dan
>

Re: [PATCH] mac80211: fix comment regarding aggregation buf_size

[Emmanuel Grumbach]

On Wed, Mar 16, 2011 at 11:51, Daniel Halperin
<dhalperi@cs.washington.edu> wrote:
> That last line should really be:
>
>>> + *     Note that the following is also illegal because it will violate buffering assumptions if frame 1 is lost again

agreed - the Txer cannot send any frame with seqNum greater than SSN
from last BA reponse + window size. In this case, the SSN of last BA
is 0, so you cannot send 8 and any further frame until you get the BA
response for 1.

>>> + *      - TX:         189.....
>>
>> Dan
>>
>

Re: [PATCH] mac80211: fix comment regarding aggregation buf_size

[Johannes Berg]

On Wed, 2011-03-16 at 02:47 -0700, Daniel Halperin wrote:

> <dan starts here>
> 
> > + *     possible with a buf_size of 8:
> > + *      - TX: 1.....7
> > + *      - RX: 2.....7 (lost frame #1),
> > + *      - TX:         89...1...
> > + *     which is invalid since #1 was now re-transmitted well past the
> > + *     buffer size of 7. The correct way to retransmit #1 would be:
> > + *      - TX:         1       or
> > + *      - TX:         18
> > + *     though the standard allows:
> > + *      - TX:         81
> > + *     The following might happen, but will violate buffering assumptions as above if frame 1 is lost again
> > + *      - TX:         189.....

Oh, oops, yes you're right. Will respin the patch.

johannes