[PATCH v2] mac80211: temporarily disable reorder release timer

[PATCH v2] mac80211: temporarily disable reorder release timer

[Christian Lamparter]

Several serve threading problems in the current
release reorder timer implementation have been
discovered.

A lengthy discussion - which lists some of the
pitfalls and possible solutions - can be found at:
 http://marc.info/?t=128635927000001

But due to the complicated nature of the subject and
the imminent advent of a new -rc cycle, it was
decided to disable the feature for the time being.

Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
---
v1 -> v2:
	fix compiling error
	net/mac80211/rx.c: In function ‘ieee80211_sta_reorder_release’:
	net/mac80211/rx.c:665: error: label at end of compound statement
---
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index b67221d..aa9b08d 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -622,6 +622,26 @@ static void ieee80211_sta_reorder_release(struct ieee80211_hw *hw,
 							tid_agg_rx->buf_size;
 	}
 
+	/*
+	 * Disable the reorder release timer for now.
+	 *
+	 * The current implementation lacks a proper locking scheme
+	 * which would protect vital statistic and debug counters
+	 * from being updated by two different but concurrent BHs.
+	 *
+	 * More information about the topic is available from:
+	 * - thread: http://marc.info/?t=128635927000001
+	 *
+	 * What was wrong:
+	 * =>  http://marc.info/?l=linux-wireless&m=128636170811964
+	 * "Basically the thing is that until your patch, the data
+	 *  in the struct didn't actually need locking because it
+	 *  was accessed by the RX path only which is not concurrent."
+	 *
+	 * List of what needs to be fixed:
+	 * => http://marc.info/?l=linux-wireless&m=128656352920957
+	 *
+
 	if (tid_agg_rx->stored_mpdu_num) {
 		j = index = seq_sub(tid_agg_rx->head_seq_num,
 				    tid_agg_rx->ssn) % tid_agg_rx->buf_size;
@@ -640,6 +660,10 @@ static void ieee80211_sta_reorder_release(struct ieee80211_hw *hw,
 	} else {
 		del_timer(&tid_agg_rx->reorder_timer);
 	}
+	*/
+
+set_release_timer:
+	return;
 }
 
 /*

Re: [PATCH v2] mac80211: temporarily disable reorder release timer

[Luis R. Rodriguez]

On Fri, Oct 8, 2010 at 1:35 PM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> Several serve threading problems in the current
> release reorder timer implementation have been
> discovered.
>
> A lengthy discussion - which lists some of the
> pitfalls and possible solutions - can be found at:
>  http://marc.info/?t=128635927000001
>
> But due to the complicated nature of the subject and
> the imminent advent of a new -rc cycle, it was
> decided to disable the feature for the time being.
>
> Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>

This patch lacks documentation of the impact of this patch, while it
may fix a lock issue, the does not address what happens when the patch
is actually applied.

  Luis

Re: [PATCH v2] mac80211: temporarily disable reorder release timer

[Christian Lamparter]

On Friday 08 October 2010 22:40:39 Luis R. Rodriguez wrote:
> On Fri, Oct 8, 2010 at 1:35 PM, Christian Lamparter
> <chunkeey@googlemail.com> wrote:
> > Several serve threading problems in the current
> > release reorder timer implementation have been
> > discovered.
> >
> > A lengthy discussion - which lists some of the
> > pitfalls and possible solutions - can be found at:
> >  http://marc.info/?t=128635927000001
> >
> > But due to the complicated nature of the subject and
> > the imminent advent of a new -rc cycle, it was
> > decided to disable the feature for the time being.
> >
> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> 
> This patch lacks documentation of the impact of this patch, while it
> may fix a lock issue, the does not address what happens when the patch
> is actually applied.
? It just disables the timer. The actual reorder code
is left untouched, everything is the same as in Linus'
2.6.36-rcX tree.

Or what impact are you talking about?

Re: [PATCH v2] mac80211: temporarily disable reorder release timer

[Luis R. Rodriguez]

On Fri, Oct 8, 2010 at 1:49 PM, Christian Lamparter
<chunkeey@googlemail.com> wrote:
> On Friday 08 October 2010 22:40:39 Luis R. Rodriguez wrote:
>> On Fri, Oct 8, 2010 at 1:35 PM, Christian Lamparter
>> <chunkeey@googlemail.com> wrote:
>> > Several serve threading problems in the current
>> > release reorder timer implementation have been
>> > discovered.
>> >
>> > A lengthy discussion - which lists some of the
>> > pitfalls and possible solutions - can be found at:
>> >  http://marc.info/?t=128635927000001
>> >
>> > But due to the complicated nature of the subject and
>> > the imminent advent of a new -rc cycle, it was
>> > decided to disable the feature for the time being.
>> >
>> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
>>
>> This patch lacks documentation of the impact of this patch, while it
>> may fix a lock issue, the does not address what happens when the patch
>> is actually applied.
> ? It just disables the timer. The actual reorder code
> is left untouched, everything is the same as in Linus'
> 2.6.36-rcX tree.
>
> Or what impact are you talking about?

The timer is what I'm talking about, we don't run then
ieee80211_release_reorder_timeout() right so we don't clear stale
frames on aggregates. What impact does this have?

  Luis

Re: [PATCH v2] mac80211: temporarily disable reorder release timer

[Christian Lamparter]

On Friday 08 October 2010 23:22:57 Luis R. Rodriguez wrote:
> On Fri, Oct 8, 2010 at 1:49 PM, Christian Lamparter
> <chunkeey@googlemail.com> wrote:
> > On Friday 08 October 2010 22:40:39 Luis R. Rodriguez wrote:
> >> On Fri, Oct 8, 2010 at 1:35 PM, Christian Lamparter
> >> <chunkeey@googlemail.com> wrote:
> >> > Several serve threading problems in the current
> >> > release reorder timer implementation have been
> >> > discovered.
> >> >
> >> > A lengthy discussion - which lists some of the
> >> > pitfalls and possible solutions - can be found at:
> >> >  http://marc.info/?t=128635927000001
> >> >
> >> > But due to the complicated nature of the subject and
> >> > the imminent advent of a new -rc cycle, it was
> >> > decided to disable the feature for the time being.
> >> >
> >> > Signed-off-by: Christian Lamparter <chunkeey@googlemail.com>
> >>
> >> This patch lacks documentation of the impact of this patch, while it
> >> may fix a lock issue, the does not address what happens when the patch
> >> is actually applied.
> > ? It just disables the timer. The actual reorder code
> > is left untouched, everything is the same as in Linus'
> > 2.6.36-rcX tree.
> >
> > Or what impact are you talking about?
> 
> The timer is what I'm talking about, we don't run then
> ieee80211_release_reorder_timeout() right so we don't clear stale
> frames on aggregates.
What? Who told you that? Because that's just misinformation.

This patch doesn't touch/disable the reorder release mechanism
(ieee80211_sta_reorder_release) at all.
It only disarms the timer which would clean out frames after
approx. 100ms.

And even without the timer, there are still three other ways
for cleaning out "stale" frame aggregates.

1. BAR from peer (ieee80211_rx_h_ctrl)

2. BAW window overflow (ieee80211_sta_manage_reorder_buf,
   part of the aMPDU rx path - executed for every received
   MPDU)

3. ieee80211_sta_reorder_release (automatically called by
   ieee80211_sta_manage_reorder_buf when a reorder gaps
   are opening up.)

(I repeat, the rx path logic & behavior is identical to
vanilla 2.6.36-rcX)

> What impact does this have?
everything is back to stock, at least for now?

see:
http://marc.info/?l=linux-wireless&m=128656352920957

"I don't like reverting these patches, but maybe we should
simply comment out the code that arms the timer, thereby
disabling all of it, while we work on this?"
 -- Johannes

Best regards,
	Christian