[RFT] mac80211: fix broadcast/multicast data drop on scan

[RFT] mac80211: fix broadcast/multicast data drop on scan

[Luis R. Rodriguez]

The new scan implementation only takes into consideration
the the listen interval which the driver itself sets. The AP
however will send all buffered broadcast and multicast data
every dtim_period which typically is less than the listen
interval. We are also currently not respecting the pm-qos
network latency. Since dynamic powersave work already computes
for us the minimum allowed sleep period reuse that work
and ensure we don't sleep longer than what we allowed for.

Without this we drop buffered broadcast and multicast traffic.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
Cc: Kalle Valo <kvalo@adurom.com>
Cc: Amod Bodas <Amod.Bodas@atheros.com>
---

The big question is if the max_sleep_period is synched with 
the DTIM count. If it is not, then we may need something else. 
As I see it only the DTIM period is used for the max_sleep_period
calculation along with the network latency pm-qos requirement.

If this requires a bit too many changes I am not sure how to handle
this for stable. We'll see.

Please test with different DTIM intervals.

 net/mac80211/scan.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 2c7e376..e105304 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -444,6 +444,8 @@ static int ieee80211_scan_state_decision(struct ieee80211_local *local,
 	bool tx_empty = true;
 	bool bad_latency;
 	bool listen_int_exceeded;
+	/* accounts for PM_QOS_NETWORK_LATENCY and dtim period */
+	bool latency_dtim_period_exceeded = false;
 	unsigned long min_beacon_int = 0;
 	struct ieee80211_sub_if_data *sdata;
 	struct ieee80211_channel *next_chan;
@@ -466,6 +468,7 @@ static int ieee80211_scan_state_decision(struct ieee80211_local *local,
 
 		if (sdata->vif.type == NL80211_IFTYPE_STATION) {
 			if (sdata->u.mgd.associated) {
+
 				associated = true;
 
 				if (sdata->vif.bss_conf.beacon_int <
@@ -511,8 +514,16 @@ static int ieee80211_scan_state_decision(struct ieee80211_local *local,
 				usecs_to_jiffies(min_beacon_int * 1024) *
 				local->hw.conf.listen_interval);
 
+		if (associated && local->hw.conf.max_sleep_period) {
+			latency_dtim_period_exceeded = time_after(jiffies +
+				ieee80211_scan_get_channel_time(next_chan),
+				local->leave_oper_channel_time +
+				usecs_to_jiffies(min_beacon_int * 1024) *
+				local->hw.conf.max_sleep_period);
+		}
+
 		if (associated && ( !tx_empty || bad_latency ||
-		    listen_int_exceeded))
+		    listen_int_exceeded || latency_dtim_period_exceeded))
 			local->next_scan_state = SCAN_ENTER_OPER_CHANNEL;
 		else
 			local->next_scan_state = SCAN_SET_CHANNEL;
-- 
1.7.0.4

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Jouni Malinen]

On Fri, Aug 27, 2010 at 01:38:47AM -0400, Luis R. Rodriguez wrote:
> The new scan implementation only takes into consideration
> the the listen interval which the driver itself sets. The AP
> however will send all buffered broadcast and multicast data
> every dtim_period which typically is less than the listen
> interval. We are also currently not respecting the pm-qos
> network latency. Since dynamic powersave work already computes
> for us the minimum allowed sleep period reuse that work
> and ensure we don't sleep longer than what we allowed for.
> 
> Without this we drop buffered broadcast and multicast traffic.

Did you test how much this patch would help? While reducing the length
of each off-channel phase in background scan is indeed needed to receive
the broadcast frames, I don't see how this by itself would help at all.
Quite the opposite, I would not be surprised if this actually makes us
drop even larger number of broadcast frames by lengthening the total
scan duration and by adding more channel changes..

In order for this to provide real help for receiving broadcast/multicast
frames, the start of each off-channel scan sequence needs to be
synchronized with DTIM Beacon + PS broadcast/multicast RX. In other
words, when we are on the operating channel, we need to start the next
scan sequence immediately after receiving the last buffered
broadcast/multicast frame from our current AP (or if no buffered frames
are indicated in the DTIM Beacon, immediately after that Beacon frame is
received). This is obviously assuming that we are associated with an AP
(and only one AP for that matter; multiple virtual interfaces will make
this quite a bit more complex, but we can leave that for future work
while handling the simpler case now).

> If this requires a bit too many changes I am not sure how to handle
> this for stable. We'll see.

I would not even think about stable for this at the moment.. I don't
really consider this as a simple fix, but rather a completely new
feature for the background scan which was not previously designed to
handle broadcast/multicast receiving.

-- 
Jouni Malinen                                            PGP id EFC895FA

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Luis R. Rodriguez]

On Fri, Aug 27, 2010 at 2:06 AM, Jouni Malinen <j@w1.fi> wrote:
> On Fri, Aug 27, 2010 at 01:38:47AM -0400, Luis R. Rodriguez wrote:
>> The new scan implementation only takes into consideration
>> the the listen interval which the driver itself sets. The AP
>> however will send all buffered broadcast and multicast data
>> every dtim_period which typically is less than the listen
>> interval. We are also currently not respecting the pm-qos
>> network latency. Since dynamic powersave work already computes
>> for us the minimum allowed sleep period reuse that work
>> and ensure we don't sleep longer than what we allowed for.
>>
>> Without this we drop buffered broadcast and multicast traffic.
>
> Did you test how much this patch would help? While reducing the length
> of each off-channel phase in background scan is indeed needed to receive
> the broadcast frames, I don't see how this by itself would help at all.
> Quite the opposite, I would not be surprised if this actually makes us
> drop even larger number of broadcast frames by lengthening the total
> scan duration and by adding more channel changes..

I only tested how often we go off the home channel compared to before.

> In order for this to provide real help for receiving broadcast/multicast
> frames, the start of each off-channel scan sequence needs to be
> synchronized with DTIM Beacon + PS broadcast/multicast RX. In other
> words, when we are on the operating channel, we need to start the next
> scan sequence immediately after receiving the last buffered
> broadcast/multicast frame from our current AP (or if no buffered frames
> are indicated in the DTIM Beacon, immediately after that Beacon frame is
> received). This is obviously assuming that we are associated with an AP
> (and only one AP for that matter; multiple virtual interfaces will make
> this quite a bit more complex, but we can leave that for future work
> while handling the simpler case now).

Indeed... I do not see where we keep track of the DTIM count and was
afraid this was not sufficient. I had not thought about the last
received multicast / broadcast frame, that will require some more
work. But I also noticed even ieee80211_recalc_ps() does not take this
into account when computing the max_sleep_period ieee80211_enable_ps()
for dynamic power save, it only considers the DTIM period. We send the
nullfunc frame for dynamic power save and it does not seem we take
into consideration the DTIM count and last RX's broadcast / multicast
data prior to sending the nullfunc to go into power save. So it seems
to me dynamic power save would also loses broadcast / multicast data
frames. It also means though that if we fix this through
hw.conf.max_sleep_period we could take care of both.

>> If this requires a bit too many changes I am not sure how to handle
>> this for stable. We'll see.
>
> I would not even think about stable for this at the moment.. I don't
> really consider this as a simple fix, but rather a completely new
> feature for the background scan which was not previously designed to
> handle broadcast/multicast receiving.

Fair enough, if we do get this work done I'm motivated enough to carry
these onto compat-wireless as linux-next-cherry-pick patches for a
compat-wireless-2.6.36 release (another release can be provided
without applying the linux-next-cherry-pick stuff which only carries
vanilla 2.6.36).

  Luis

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Johannes Berg]

On Fri, 2010-08-27 at 08:28 -0700, Luis R. Rodriguez wrote:

> Indeed... I do not see where we keep track of the DTIM count and was
> afraid this was not sufficient. I had not thought about the last
> received multicast / broadcast frame, that will require some more
> work. But I also noticed even ieee80211_recalc_ps() does not take this
> into account when computing the max_sleep_period ieee80211_enable_ps()
> for dynamic power save, it only considers the DTIM period. We send the
> nullfunc frame for dynamic power save and it does not seem we take
> into consideration the DTIM count and last RX's broadcast / multicast
> data prior to sending the nullfunc to go into power save. So it seems
> to me dynamic power save would also loses broadcast / multicast data
> frames.

No, the max sleep period is just a helper variable for device
implementation of sleep -- the actual alignment of it with DTIM beacons
etc. has to be done by the device. Therefore, it isn't so :-)

johannes

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Luis R. Rodriguez]

On Fri, Aug 27, 2010 at 8:37 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2010-08-27 at 08:28 -0700, Luis R. Rodriguez wrote:
>
>> Indeed... I do not see where we keep track of the DTIM count and was
>> afraid this was not sufficient. I had not thought about the last
>> received multicast / broadcast frame, that will require some more
>> work. But I also noticed even ieee80211_recalc_ps() does not take this
>> into account when computing the max_sleep_period ieee80211_enable_ps()
>> for dynamic power save, it only considers the DTIM period. We send the
>> nullfunc frame for dynamic power save and it does not seem we take
>> into consideration the DTIM count and last RX's broadcast / multicast
>> data prior to sending the nullfunc to go into power save. So it seems
>> to me dynamic power save would also loses broadcast / multicast data
>> frames.
>
> No, the max sleep period is just a helper variable for device
> implementation of sleep -- the actual alignment of it with DTIM beacons
> etc. has to be done by the device. Therefore, it isn't so :-)

I noticed the comment on the PS documentation that the reason for this
is that mac80211 is too slow, is that right?

  Luis

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Johannes Berg]

On Fri, 2010-08-27 at 08:40 -0700, Luis R. Rodriguez wrote:
> On Fri, Aug 27, 2010 at 8:37 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Fri, 2010-08-27 at 08:28 -0700, Luis R. Rodriguez wrote:
> >
> >> Indeed... I do not see where we keep track of the DTIM count and was
> >> afraid this was not sufficient. I had not thought about the last
> >> received multicast / broadcast frame, that will require some more
> >> work. But I also noticed even ieee80211_recalc_ps() does not take this
> >> into account when computing the max_sleep_period ieee80211_enable_ps()
> >> for dynamic power save, it only considers the DTIM period. We send the
> >> nullfunc frame for dynamic power save and it does not seem we take
> >> into consideration the DTIM count and last RX's broadcast / multicast
> >> data prior to sending the nullfunc to go into power save. So it seems
> >> to me dynamic power save would also loses broadcast / multicast data
> >> frames.
> >
> > No, the max sleep period is just a helper variable for device
> > implementation of sleep -- the actual alignment of it with DTIM beacons
> > etc. has to be done by the device. Therefore, it isn't so :-)
> 
> I noticed the comment on the PS documentation that the reason for this
> is that mac80211 is too slow, is that right?

Well, there's no way we can reliably wake up exactly before a beacon by
implementing that in software, hard real-time hasn't been achieved in
Linux yet :-)

johannes

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Luis R. Rodriguez]

On Fri, Aug 27, 2010 at 8:43 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2010-08-27 at 08:40 -0700, Luis R. Rodriguez wrote:
>> On Fri, Aug 27, 2010 at 8:37 AM, Johannes Berg
>> <johannes@sipsolutions.net> wrote:
>> > On Fri, 2010-08-27 at 08:28 -0700, Luis R. Rodriguez wrote:
>> >
>> >> Indeed... I do not see where we keep track of the DTIM count and was
>> >> afraid this was not sufficient. I had not thought about the last
>> >> received multicast / broadcast frame, that will require some more
>> >> work. But I also noticed even ieee80211_recalc_ps() does not take this
>> >> into account when computing the max_sleep_period ieee80211_enable_ps()
>> >> for dynamic power save, it only considers the DTIM period. We send the
>> >> nullfunc frame for dynamic power save and it does not seem we take
>> >> into consideration the DTIM count and last RX's broadcast / multicast
>> >> data prior to sending the nullfunc to go into power save. So it seems
>> >> to me dynamic power save would also loses broadcast / multicast data
>> >> frames.
>> >
>> > No, the max sleep period is just a helper variable for device
>> > implementation of sleep -- the actual alignment of it with DTIM beacons
>> > etc. has to be done by the device. Therefore, it isn't so :-)
>>
>> I noticed the comment on the PS documentation that the reason for this
>> is that mac80211 is too slow, is that right?
>
> Well, there's no way we can reliably wake up exactly before a beacon by
> implementing that in software, hard real-time hasn't been achieved in
> Linux yet :-)

Keyword here is *exactly*. I don't want to wake up *exactly* at DTIM,
only ensure we don't sleep during it and the send buffered frames and
multicast traffic data. Why can we not implement this on mac80211?

  Luis

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Johannes Berg]

On Fri, 2010-08-27 at 08:48 -0700, Luis R. Rodriguez wrote:

> > Well, there's no way we can reliably wake up exactly before a beacon by
> > implementing that in software, hard real-time hasn't been achieved in
> > Linux yet :-)
> 
> Keyword here is *exactly*. I don't want to wake up *exactly* at DTIM,
> only ensure we don't sleep during it and the send buffered frames and
> multicast traffic data. Why can we not implement this on mac80211?

Well, how long before the beacon do you want to wake up to catch it?
1ms? 10ms? 100ms? 1000ms? What if the system is busy?

johannes

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Luis R. Rodriguez]

On Fri, Aug 27, 2010 at 8:51 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Fri, 2010-08-27 at 08:48 -0700, Luis R. Rodriguez wrote:
>
>> > Well, there's no way we can reliably wake up exactly before a beacon by
>> > implementing that in software, hard real-time hasn't been achieved in
>> > Linux yet :-)
>>
>> Keyword here is *exactly*. I don't want to wake up *exactly* at DTIM,
>> only ensure we don't sleep during it and the send buffered frames and
>> multicast traffic data. Why can we not implement this on mac80211?
>
> Well, how long before the beacon do you want to wake up to catch it?
> 1ms? 10ms? 100ms? 1000ms? What if the system is busy?

I cannot say what would work best, how about half a beacon interval early?

  Luis

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Johannes Berg]

On Fri, 2010-08-27 at 08:55 -0700, Luis R. Rodriguez wrote:
> On Fri, Aug 27, 2010 at 8:51 AM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
> > On Fri, 2010-08-27 at 08:48 -0700, Luis R. Rodriguez wrote:
> >
> >> > Well, there's no way we can reliably wake up exactly before a beacon by
> >> > implementing that in software, hard real-time hasn't been achieved in
> >> > Linux yet :-)
> >>
> >> Keyword here is *exactly*. I don't want to wake up *exactly* at DTIM,
> >> only ensure we don't sleep during it and the send buffered frames and
> >> multicast traffic data. Why can we not implement this on mac80211?
> >
> > Well, how long before the beacon do you want to wake up to catch it?
> > 1ms? 10ms? 100ms? 1000ms? What if the system is busy?
> 
> I cannot say what would work best, how about half a beacon interval early?

Why are we even discussing this anyway? It doesn't matter for a power
save implementation since you better offload it to the device so you get
as exact timing as you can?

johannes

Re: [RFT] mac80211: fix broadcast/multicast data drop on scan

[Luis R. Rodriguez]

On Fri, Aug 27, 2010 at 08:58:42AM -0700, Johannes Berg wrote:
> On Fri, 2010-08-27 at 08:55 -0700, Luis R. Rodriguez wrote:
> > On Fri, Aug 27, 2010 at 8:51 AM, Johannes Berg
> > <johannes@sipsolutions.net> wrote:
> > > On Fri, 2010-08-27 at 08:48 -0700, Luis R. Rodriguez wrote:
> > >
> > >> > Well, there's no way we can reliably wake up exactly before a beacon by
> > >> > implementing that in software, hard real-time hasn't been achieved in
> > >> > Linux yet :-)
> > >>
> > >> Keyword here is *exactly*. I don't want to wake up *exactly* at DTIM,
> > >> only ensure we don't sleep during it and the send buffered frames and
> > >> multicast traffic data. Why can we not implement this on mac80211?
> > >
> > > Well, how long before the beacon do you want to wake up to catch it?
> > > 1ms? 10ms? 100ms? 1000ms? What if the system is busy?
> > 
> > I cannot say what would work best, how about half a beacon interval early?
> 
> Why are we even discussing this anyway? It doesn't matter for a power
> save implementation since you better offload it to the device so you get
> as exact timing as you can?

ath9k hardware does not have a CPU to offload this. I just noticed we do
however already wait for the dtim count to go to 0 and also all RX'd buffered
data through ath_rx_ps() but even if then it seems we need and API to enable
drivers to flag when they want to disallow scanning (dynamic power save can be
disabled already by the driver at will I think). Otherwise we would have to
use something like the patch I'm going to send, which is fine for now.

 Luis