[RFC] mac80211: fix release_reorder_timeout in scan

[RFC] mac80211: fix release_reorder_timeout in scan

[Johannes Berg]

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

Even if the reorder timeout timer fires while
scanning, the frames weren't received during
scanning and therefore shouldn't be dropped.
To implement this, changes to the passive scan
RX handler are necessary.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
Should we go for the frequency checks? Or just accept potential
off-channel RX like we used to for hw scan (we have to for hw scan since
hw scan might go back to operating channel on its own, and we don't even
stop queues)

 net/mac80211/rx.c |   28 ++++++++++++----------------
 1 file changed, 12 insertions(+), 16 deletions(-)

--- wireless-testing.orig/net/mac80211/rx.c	2010-09-23 16:13:40.000000000 +0200
+++ wireless-testing/net/mac80211/rx.c	2010-09-23 16:58:02.000000000 +0200
@@ -388,20 +388,20 @@ ieee80211_rx_h_passive_scan(struct ieee8
 {
 	struct ieee80211_local *local = rx->local;
 	struct sk_buff *skb = rx->skb;
+	struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
+	ieee80211_rx_result ret;
 
-	if (unlikely(test_bit(SCAN_HW_SCANNING, &local->scanning)))
-		return ieee80211_scan_rx(rx->sdata, skb);
-
-	if (unlikely(test_bit(SCAN_SW_SCANNING, &local->scanning) &&
-		     (rx->flags & IEEE80211_RX_IN_SCAN))) {
-		/* drop all the other packets during a software scan anyway */
-		if (ieee80211_scan_rx(rx->sdata, skb) != RX_QUEUED)
-			dev_kfree_skb(skb);
-		return RX_QUEUED;
-	}
+	if (likely(!(rx->flags & IEEE80211_RX_IN_SCAN)))
+		return RX_CONTINUE;
 
-	if (unlikely(rx->flags & IEEE80211_RX_IN_SCAN)) {
-		/* scanning finished during invoking of handlers */
+	ret = ieee80211_scan_rx(rx->sdata, skb);
+	if (ret != RX_CONTINUE)
+		return ret;
+
+	if (status->freq != local->oper_channel->center_freq &&
+	    (!local->tmp_channel ||
+	     status->freq != local->tmp_channel->center_freq)) {
+		/* ignore scan channel RX */
 		I802_DEBUG_INC(local->rx_handlers_drop_passive_scan);
 		return RX_DROP_UNUSABLE;
 	}
@@ -2500,10 +2500,6 @@ void ieee80211_release_reorder_timeout(s
 	rx.queue = tid;
 	rx.flags |= IEEE80211_RX_RA_MATCH;
 
-	if (unlikely(test_bit(SCAN_HW_SCANNING, &sta->local->scanning) ||
-		     test_bit(SCAN_OFF_CHANNEL, &sta->local->scanning)))
-		rx.flags |= IEEE80211_RX_IN_SCAN;
-
 	spin_lock(&tid_agg_rx->reorder_lock);
 	ieee80211_sta_reorder_release(&sta->local->hw, tid_agg_rx, &frames);
 	spin_unlock(&tid_agg_rx->reorder_lock);

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Luis R. Rodriguez]

On Thu, Sep 23, 2010 at 11:27 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> Even if the reorder timeout timer fires while
> scanning, the frames weren't received during
> scanning and therefore shouldn't be dropped.

I don't get what we are doing with these frames today, are you saying
we pegged some of these frames into the reorder buffer when starting a
sw scan, or that we were dropping them?

  Luis

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Johannes Berg]

On Mon, 2010-09-27 at 11:14 -0700, Luis R. Rodriguez wrote:
> On Thu, Sep 23, 2010 at 11:27 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > From: Johannes Berg <johannes.berg@intel.com>
> >
> > Even if the reorder timeout timer fires while
> > scanning, the frames weren't received during
> > scanning and therefore shouldn't be dropped.
> 
> I don't get what we are doing with these frames today, are you saying
> we pegged some of these frames into the reorder buffer when starting a
> sw scan, or that we were dropping them?

You keep confusing TX and RX ... ;)

This is all about RX only.

johannes

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Luis R. Rodriguez]

On Mon, Sep 27, 2010 at 11:20 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2010-09-27 at 11:14 -0700, Luis R. Rodriguez wrote:
>> On Thu, Sep 23, 2010 at 11:27 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > From: Johannes Berg <johannes.berg@intel.com>
>> >
>> > Even if the reorder timeout timer fires while
>> > scanning, the frames weren't received during
>> > scanning and therefore shouldn't be dropped.
>>
>> I don't get what we are doing with these frames today, are you saying
>> we pegged some of these frames into the reorder buffer when starting a
>> sw scan, or that we were dropping them?
>
> You keep confusing TX and RX ... ;)
>
> This is all about RX only.

yeah, I was thinking about the release reorder buffer and its
respective timeout, I thought that's what the subject of this patch
was about.

  Luis

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Johannes Berg]

On Mon, 2010-09-27 at 11:49 -0700, Luis R. Rodriguez wrote:

> >> I don't get what we are doing with these frames today, are you saying
> >> we pegged some of these frames into the reorder buffer when starting a
> >> sw scan, or that we were dropping them?

> yeah, I was thinking about the release reorder buffer and its
> respective timeout, I thought that's what the subject of this patch
> was about.

So I got confused by your question ...

We put them into the reorder buffer, but if we get a reorder timeout
while scanning we'll drop the frames which isn't necessary.

johannes

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Luis R. Rodriguez]

On Mon, Sep 27, 2010 at 11:54 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2010-09-27 at 11:49 -0700, Luis R. Rodriguez wrote:
>
>> >> I don't get what we are doing with these frames today, are you saying
>> >> we pegged some of these frames into the reorder buffer when starting a
>> >> sw scan, or that we were dropping them?
>
>> yeah, I was thinking about the release reorder buffer and its
>> respective timeout, I thought that's what the subject of this patch
>> was about.
>
> So I got confused by your question ...
>
> We put them into the reorder buffer, but if we get a reorder timeout
> while scanning we'll drop the frames which isn't necessary.

Ah, got it, thanks, good catch. Guess its not a terrible stable fix,
but at least important for those who care about VoIP and such.

  Luis

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Johannes Berg]

On Mon, 2010-09-27 at 12:03 -0700, Luis R. Rodriguez wrote:

> > We put them into the reorder buffer, but if we get a reorder timeout
> > while scanning we'll drop the frames which isn't necessary.
> 
> Ah, got it, thanks, good catch. Guess its not a terrible stable fix,
> but at least important for those who care about VoIP and such.

You wouldn't typically use aggregation on VO traffic, I think?

johannes

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Luis R. Rodriguez]

On Mon, Sep 27, 2010 at 12:06 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2010-09-27 at 12:03 -0700, Luis R. Rodriguez wrote:
>
>> > We put them into the reorder buffer, but if we get a reorder timeout
>> > while scanning we'll drop the frames which isn't necessary.
>>
>> Ah, got it, thanks, good catch. Guess its not a terrible stable fix,
>> but at least important for those who care about VoIP and such.
>
> You wouldn't typically use aggregation on VO traffic, I think?

ath9k uses aggregation for all data traffic.

  Luis

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Johannes Berg]

On Mon, 2010-09-27 at 12:09 -0700, Luis R. Rodriguez wrote:
> On Mon, Sep 27, 2010 at 12:06 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Mon, 2010-09-27 at 12:03 -0700, Luis R. Rodriguez wrote:
> >
> >> > We put them into the reorder buffer, but if we get a reorder timeout
> >> > while scanning we'll drop the frames which isn't necessary.
> >>
> >> Ah, got it, thanks, good catch. Guess its not a terrible stable fix,
> >> but at least important for those who care about VoIP and such.
> >
> > You wouldn't typically use aggregation on VO traffic, I think?
> 
> ath9k uses aggregation for all data traffic.

But it can only control TX :) Which is, of course, relevant if it's used
as an AP, but for typical VO traffic I would argue that using
aggregation is a bug since it'll just increase delays & jitter.

johannes

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Luis R. Rodriguez]

On Mon, Sep 27, 2010 at 12:10 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2010-09-27 at 12:09 -0700, Luis R. Rodriguez wrote:
>> On Mon, Sep 27, 2010 at 12:06 PM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Mon, 2010-09-27 at 12:03 -0700, Luis R. Rodriguez wrote:
>> >
>> >> > We put them into the reorder buffer, but if we get a reorder timeout
>> >> > while scanning we'll drop the frames which isn't necessary.
>> >>
>> >> Ah, got it, thanks, good catch. Guess its not a terrible stable fix,
>> >> but at least important for those who care about VoIP and such.
>> >
>> > You wouldn't typically use aggregation on VO traffic, I think?
>>
>> ath9k uses aggregation for all data traffic.
>
> But it can only control TX :)

True

> Which is, of course, relevant if it's used
> as an AP,

True

> but for typical VO traffic I would argue that using
> aggregation is a bug since it'll just increase delays & jitter.

This deviates from subject a little but while at it, I am curious,
Matt, Srini, what do you think?

  Luis

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Matt Smith]

I would not send voice data under a BA agreement.  If you have to give up on transmitting a frame, it will leave a hole at the receiver which will then require a BAR to move past.  Since you will typically only send one frame at a time, it (use of A-MPDU aggregation) doesn't buy you anything.  IIRC, there are industry certification tests that send more than one frame a second at AC_VO, however, which one needs to to pass.

Matt

On Sep 27, 2010, at 12:12 PM, Luis R. Rodriguez wrote:

> On Mon, Sep 27, 2010 at 12:10 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> On Mon, 2010-09-27 at 12:09 -0700, Luis R. Rodriguez wrote:
>>> On Mon, Sep 27, 2010 at 12:06 PM, Johannes Berg
>>> <johannes@sipsolutions.net> wrote:
>>>> On Mon, 2010-09-27 at 12:03 -0700, Luis R. Rodriguez wrote:
>>>> 
>>>>>> We put them into the reorder buffer, but if we get a reorder timeout
>>>>>> while scanning we'll drop the frames which isn't necessary.
>>>>> 
>>>>> Ah, got it, thanks, good catch. Guess its not a terrible stable fix,
>>>>> but at least important for those who care about VoIP and such.
>>>> 
>>>> You wouldn't typically use aggregation on VO traffic, I think?
>>> 
>>> ath9k uses aggregation for all data traffic.
>> 
>> But it can only control TX :)
> 
> True
> 
>> Which is, of course, relevant if it's used
>> as an AP,
> 
> True
> 
>> but for typical VO traffic I would argue that using
>> aggregation is a bug since it'll just increase delays & jitter.
> 
> This deviates from subject a little but while at it, I am curious,
> Matt, Srini, what do you think?
> 
>  Luis

[ath9k-devel] [RFC] mac80211: fix release_reorder_timeout in scan

[Luis R. Rodriguez]

On Mon, Sep 27, 2010 at 3:34 PM, Matt Smith <Matt.Smith@atheros.com> wrote:
> I would not send voice data under a BA agreement. ?If you have to give up on transmitting a frame, it will leave a hole at the receiver which will then require a BAR to move past. ?Since you will typically only send one frame at a time, it (use of A-MPDU aggregation) doesn't buy you anything. ?IIRC, there are industry certification tests that send more than one frame a second at AC_VO, however, which one needs to to pass.

Thanks Matt, I've stashed this under out TODO list:

http://wireless.kernel.org/en/users/Drivers/ath9k/todo

  Luis

Re: [RFC] mac80211: fix release_reorder_timeout in scan

[Luis R. Rodriguez]

On Mon, Sep 27, 2010 at 3:34 PM, Matt Smith <Matt.Smith@atheros.com> wrote:
> I would not send voice data under a BA agreement.  If you have to give up on transmitting a frame, it will leave a hole at the receiver which will then require a BAR to move past.  Since you will typically only send one frame at a time, it (use of A-MPDU aggregation) doesn't buy you anything.  IIRC, there are industry certification tests that send more than one frame a second at AC_VO, however, which one needs to to pass.

Thanks Matt, I've stashed this under out TODO list:

http://wireless.kernel.org/en/users/Drivers/ath9k/todo

  Luis