Hi Johannes,
When testing with iperf I've been observing very significant problems
with throughput and packet loss during software scans. Throughput often
drops to the point where iperf is reporting 0 bits/sec for several
seconds, and packet loss commonly gets over 20%. I started investigating
and discovered the following problems:
1. It seems that drivers are responsible for ensuring that PM is set in
frame control when powersave is enabled. However drivers are unaware
of off-channel powersave, so when the scan is suspended frames may
be transmitted that cause the AP to think we've exited powersave. As
a result the AP may attempt to deliver frames while we are
off-channel.
2. There's no flushing of the hardware queues after queueing the
nullfunc frame to enable off-channel powersave before going
off-channel, so it's possible to go off-channel before the frame is
transmitted.
3. There's no flush of pending frames prior to queueing the nullfunc
frame to enable powersave. That frame is sent at a high priority,
and drivers supporting QoS may have lower-priority frames queued
with PM cleared. Those frames will be sent after the nullfunc, and
the AP may try to deliver frames while we are off-channel.
4. During a scan we won't process beacon frames from our associated AP,
which prevents PS-Poll from starting when the scan is suspended.
Even if we process the beacons we won't start PS-Poll unless
powersave is actually enabled, which isn't the case during a scan.
5. The SCAN_SUSPEND state uses a fixed timeout, so if we do start
PS-Poll there's no guarantee that it will receive all buffered
frames from the AP before going back off-channel.
The following RFC patches contain fixes for problems 1, 2, and 3. I
toyed around with fixing 4 and 5, but the results really aren't much
better than before. Even with PS-Poll working properly during scans I
was seeing poor throughput and high packet loss with iperf. So instead I
tried disabling off-channel powersave when the scan is suspended, and
the results were fantastic. The implementation is trivial, packet loss
is completely eliminated, and the performance drops during scans are
modest. Therefore I've included a patch which does this rather than
fixing PS-Poll during scans.
These changes aren't quite complete, although they currently work very
well with ath9k and brcmsmac. I'm hoping to get some feedback on the
changes and answers to some questions before I proceed.
It turns out that fixing problem #1 (i.e. patch 2) probably isn't
necessary with the other changes, as no frames should be sent while
off-channel PS is enabled. Does this still seem like a problem worth
fixing?
If so, there's one other change I had considered making. Currently patch
2 will require all drivers to have some amount of powersave support for
off-channel PS to make sure PM is set. Would it make sense to set PM in
mac80211 when IEEE80211_HW_PS_NULLFUNC_STACK is set?
Thanks,
Seth
Seth Forshee (3):
mac80211: Add flushes to ensure off-channel PS is enabled during sw
scans
mac80211: Convey information about off-channel powersave to drivers
mac80211: Disable off-channel powersave when software scans are
suspended
drivers/net/wireless/ath/ath9k/main.c | 2 +-
.../net/wireless/brcm80211/brcmsmac/mac80211_if.c | 16 +++++--
drivers/net/wireless/brcm80211/brcmsmac/main.c | 10 ++++
drivers/net/wireless/brcm80211/brcmsmac/pub.h | 3 ++
include/net/mac80211.h | 23 ++++++++++
net/mac80211/mlme.c | 19 +++++---
net/mac80211/offchannel.c | 48 +++++++++++++++-----
net/mac80211/scan.c | 20 +++++---
net/mac80211/util.c | 2 +-
9 files changed, 113 insertions(+), 30 deletions(-)
Hi Seth, all,
> [points 1-5, go to the previous email to read them]
All of these issues, btw, are, I think, shortcomings of the way
powersave and scanning is currently implemented in mac80211.
First, I think it is instructive to compare it to HW scan/powersave:
mac80211 can transmit/receive quite normally, process everything pretty
much just as it normally would, etc. Queues might be stopped by the
driver while it's scanning or become full quicker while, but
fundamentally that's not really different from regular operation.
Now let's go back to the software case: the real cause of all of these
problems is that mac80211 does the scan/PS TX via the regular TX
mechanism. This is *wrong*, it's not a regular TX that should be
subjected to stopped queues etc., it's *special*. For this reason we
can't properly stop the queues, and thus the saga of all these bugs
begins, with flushing, etc.
I actually think that our (your) time would be much better spent
implementing a new powersave/scan/offchannel module that, in a way, sits
between the driver and mac80211. Using that module, drivers could
implement the HW scan and HW offchannel callbacks, and powersave.
Obviously, all of this could be implemented directly in mac80211, with
if()s etc. However, I think splitting it out would actually create a
cleaner and easier to understand solution -- I'm hoping you might agree
by the end of this email :-)
So let's see what such a module would look like. I'm going to call it
"swpso" for "software Powersave/Scan/Offchannel" for now.
Obviously, it has to provide helper functions to implement
* hw_scan
* cancel_hw_scan
* remain_on_channel
* cancel_remain_on_channel
If we allow this module to have a single pointer in "struct
ieee80211_hw" (which we can, it should be logically part of mac80211)
then we can even have the module export functions that can directly be
assigned, so let's say it does that:
int ieee80211_swpso_scan(...) // prototype matching hw_scan
// etc.
Then drivers can just do
.hw_scan = ieee80211_swpso_scan,
.remain_on_channel = ieee80211_swpso_roc,
etc.
The module also needs to provide a function that the driver can call in
the TX path, for dynamic powersave. This would adjust the timers in the
TX path etc.
Additionally, the module might require queue control wrappers, so
drivers would call ieee80211_swpso_stop_queue() and
ieee80211_swpso_wake_queue() instead of the normal versions, obviously
it would forward there, but only after some own internal checking.
So how would this work? Let's consider remain-on-channel, instead of
scan, since scan is really just a bunch of remain-on-channels one after
another.
On receiving ROC, it would first call mac80211 to stop all queues
(except the, possibly entirely virtual, offchannel queue!), and
internally mark the queues as SWPSO-stopped. It also has to continually
maintain driver-stopped, hence the wrappers I talked about earlier. If
the queues are SWPSO-stopped, and the driver wakes it, this wake doesn't
propagate to mac80211. This really works much like
__ieee80211_wake_queue() and it could actually be combined into that
function since SWPSO would be a part of mac80211. Then drivers also
wouldn't need the wrappers here, that seems sensible.
Then, it would flush all queues in the driver, to make sure that nothing
is there. (1) After that, it would call ->tx() to send out any nulldata
frame(s). Since the queues are empty, this must succeed (2). If the
flush caused the driver to wake a queue because it was stopped, this
won't propagate to mac80211, where it would cause all kinds of issues.
Now switch channel, obviously.
This is ROC, so SWPSO has done its job. Any packets that now get
transmitted on the offchannel queue by mac80211 are transmitted
normally, on the normal queue, by the driver (it needs a bit of extra
logic for that). If it was scanning, it would now also transmit the
probe request.
Upon finishing with the channel, it just has to switch the channel back,
send nulldata frames and re-enable the queues.
Ok and now that I've typed it all up, I think it points to how we can do
it much more easily without a separate module and without really
changing the drivers.
Do a few things:
* introduce IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL
* stop all queues, using the new reason, before going offchannel
* then flush
* then *directly* tell the driver to transmit the nulldata packets,
checking only if the driver has the queue stopped, in which case we
have IEEE80211_QUEUE_STOP_REASON_DRIVER set; if it does, fail going
off channel
* similarly, *directly* tell the driver to transmit probe requests and
offchannel packets from cfg80211, if it fails (stop reason driver)
then do nothing (probe request) or return a failure to userspace
(offchannel TX)
In fact the last two bullets should probably be implemented by passing
some sort of "offchan_tx_ok" to ieee80211_tx_skb() and friends, and
checking queue_stop_reasons more carefully in ieee80211_tx_frags().
I'll address powersave another day, but it also needs major rework. I
also think that the framework should somehow better integrate with
multi-channel, although I'm not sure anyone really wants to implement
that in software. It should be possible with some HW though, if it has
enough queues etc.
johannes
(1) Actually ... this whole queue stopping business could be improved
even further, if the driver has a hardware queue it can dedicate to the
transmissions we need here. Then, the driver could stop the *hardware*
queues, possibly leaving frames sitting on them. However, for ROC/scan
this really isn't interesting. It becomes interesting only for SW
multi-channel. It also cannot be implemented with all drivers, certainly
not with USB devices I think.
(2) I think there's a single driver that only accepts one packet at a
time, but that's not a big deal, since it also only support a single
virtual interface
On Thu, Jan 17, 2013 at 12:31:18AM +0100, Johannes Berg wrote:
> Hi Seth,
>
> > When testing with iperf I've been observing very significant problems
> > with throughput and packet loss during software scans. Throughput often
> > drops to the point where iperf is reporting 0 bits/sec for several
> > seconds, and packet loss commonly gets over 20%.
>
> Ouch.
>
> One caveat up front: While I'm somewhat familiar with the SW scan code,
> we have "HW" scan, so I'm not really completely into the details.
Actually most of my machines have Intel wireless so I don't have all
that many test cases :-(
> > I started investigating
> > and discovered the following problems:
> >
> > 1. It seems that drivers are responsible for ensuring that PM is set in
> > frame control when powersave is enabled. However drivers are unaware
> > of off-channel powersave, so when the scan is suspended frames may
> > be transmitted that cause the AP to think we've exited powersave. As
> > a result the AP may attempt to deliver frames while we are
> > off-channel.
>
> Hm, yeah, this is a problem. I've kinda always known this... 802.11mb
> (or -2012) clarified that in (most?) management frames at least it
> doesn't matter.
>
> > 2. There's no flushing of the hardware queues after queueing the
> > nullfunc frame to enable off-channel powersave before going
> > off-channel, so it's possible to go off-channel before the frame is
> > transmitted.
>
> Yep. And to make matters worse, flush() is broken. If the driver has
> queues stopped while being asked to flush, it will probably enable
> queues and I'm pretty sure we'll give it more frames while it's
> flushing.
I've seen this. Although as long as PM is getting set correctly it
doesn't cause problems with getting the AP to buffer frames.
> > 3. There's no flush of pending frames prior to queueing the nullfunc
> > frame to enable powersave. That frame is sent at a high priority,
> > and drivers supporting QoS may have lower-priority frames queued
> > with PM cleared. Those frames will be sent after the nullfunc, and
> > the AP may try to deliver frames while we are off-channel.
>
> Yeah...
>
> > 4. During a scan we won't process beacon frames from our associated AP,
> > which prevents PS-Poll from starting when the scan is suspended.
> > Even if we process the beacons we won't start PS-Poll unless
> > powersave is actually enabled, which isn't the case during a scan.
>
> Uh oh.. yeah, ok, but this gets really really complicated. I truly hate
> the powersave code. One of these days I'm just going to rip it out ;-)
Yeah. I had it working, but as I indicated it's better to disable PS
when the scan is suspended anyway. You just merged Stanislaw's patch
which does that, so from my perspective there's no pressing need to fix
this.
> > It turns out that fixing problem #1 (i.e. patch 2) probably isn't
> > necessary with the other changes, as no frames should be sent while
> > off-channel PS is enabled. Does this still seem like a problem worth
> > fixing?
>
> Hmm. Probably not then. It just makes the API and driver implementation
> more complex, no?
I'll address this in response to one of your other emails.
Seth
On Thu, Jan 17, 2013 at 12:32:43AM +0100, Johannes Berg wrote:
> On Wed, 2013-01-09 at 07:40 -0600, Seth Forshee wrote:
>
> > > It turns out that fixing problem #1 (i.e. patch 2) probably isn't
> > > necessary with the other changes, as no frames should be sent while
> > > off-channel PS is enabled. Does this still seem like a problem worth
> > > fixing?
> >
> > This is incorrect. We actually do need patch 2 for some hardware. I
> > forgot that when I was testing with BCM43224 I found that PM gets
> > actively set or cleared based on the device configuration. It's
> > impossible to enable PS at the AP without informing the driver.
>
> Hm, don't understand. If we're not sending any packets to the AP, why
> does this matter?
>
> Or are you saying it wants nullfunc frames generated in software, but
> then changes the PM bit in them?
Exactly. At least that's what it looks like to me. I lack detailed
knowledge of how to handle powersave on Broadcom, but I do know that the
PM bit is under the control of the MCTL_HPS field. Experimentally it
appears that the hardware actively clears PM when this field is 0 and
actively sets it when this field is 1, for all frames including
nullfuncs.
Seth
On Wed, 2013-01-16 at 23:34 -0600, Seth Forshee wrote:
> On Thu, Jan 17, 2013 at 12:32:43AM +0100, Johannes Berg wrote:
> > On Wed, 2013-01-09 at 07:40 -0600, Seth Forshee wrote:
> >
> > > > It turns out that fixing problem #1 (i.e. patch 2) probably isn't
> > > > necessary with the other changes, as no frames should be sent while
> > > > off-channel PS is enabled. Does this still seem like a problem worth
> > > > fixing?
> > >
> > > This is incorrect. We actually do need patch 2 for some hardware. I
> > > forgot that when I was testing with BCM43224 I found that PM gets
> > > actively set or cleared based on the device configuration. It's
> > > impossible to enable PS at the AP without informing the driver.
> >
> > Hm, don't understand. If we're not sending any packets to the AP, why
> > does this matter?
> >
> > Or are you saying it wants nullfunc frames generated in software, but
> > then changes the PM bit in them?
>
> Exactly. At least that's what it looks like to me. I lack detailed
> knowledge of how to handle powersave on Broadcom, but I do know that the
> PM bit is under the control of the MCTL_HPS field. Experimentally it
> appears that the hardware actively clears PM when this field is 0 and
> actively sets it when this field is 1, for all frames including
> nullfuncs.
Hm maybe that also answers my other question :-)
Maybe for the benefit of such drivers instead of splitting the powersave
state it would be easier to introduce a "PM bit state"? I don't know ...
johannes
On Wed, 2013-01-16 at 23:35 -0600, Seth Forshee wrote:
> > I'll address powersave another day, but it also needs major rework.
>
> I can't completely ignore powersave if I want to get brcmsmac working
> properly. The hardware seems to be actively setting or clearing PM based
> on its configuration, so I need a way to tell the driver that PM should
> be set. Making off-channel an explicit powersave state lets me do this,
> and while it doesn't fix any of the other powersave problems I do think
> it make the code clearer than the psuedo-state used now for off-channel.
>
> What I've got locally now for this is code to expand the PS config flag
> to two bits to support three PS states: disabled, enabled, and
> off-channel. Doing this is working well enough with the two machines
> I've been testing with (brcmsmac and ath9k). There's an odd side effect
> on brcmsmac of having PM set in probe request frames during scans, but
> according to the spec PM should be reserved in those frames anyway.
I'm a little confused by this. What frames are we sending to the AP
while we are in powersave? Doesn't the scan code flush etc., and then
the stop-reason thing would prevent other frames from going out?
> I pushed the changes to the following if you're interested in taking a
> look:
>
> git://kernel.ubuntu.com/sforshee/linux.git mac80211-offchannel
>
> I'm possibly still missing some driver updates that would need to be
> done, but otherwise the changes should be fairly complete. A major
> rework of the powersave code is likely to take some time, so is there
> any chance of getting something like this in place to fix things in the
> short term?
Totally, sure. I can hardly lay powersave rewrite on your shoulders :-)
(Nor would I reject it if you did it though, somebody has to do it
eventually and given how we don't use this code at all for our device I
can't really justify doing it myself)
johannes
On Tue, 2013-01-08 at 12:10 -0600, Seth Forshee wrote:
> +++ b/net/mac80211/offchannel.c
> @@ -136,8 +136,23 @@ void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local,
> netif_tx_stop_all_queues(sdata->dev);
> if (offchannel_ps_enable &&
> (sdata->vif.type == NL80211_IFTYPE_STATION) &&
> - sdata->u.mgd.associated)
> + sdata->u.mgd.associated) {
> + /*
> + * Need to flush frames in driver queues
> + * before sending nullfunc. Otherwise
> + * devices which support QoS may send the
> + * nullfunc before these queued frames, and
> + * those frames may not have PM set.
> + *
> + * XXX: Would be nice to not flush for each
> + * vif, however I don't see that there's any
> + * protection to prevent frames being handed
> + * to the driver before stopping the netdev
> + * queue.
> + */
> + drv_flush(local, false);
> ieee80211_offchannel_ps_enable(sdata);
Could we split the loop, and send the frames in a second loop, to
combine the flushes into a single one?
johannes
Responsibility for setting PM in frame control when powersave is enabled
falls to the drivers/hardware. This is a problem during off-channel
operation, when the hardware isn't actually in powersave but PM still
needs to be set so that the AP will buffer frames while the STA is
off-channel, as the drivers don't currently receive enough information
to know of this state.
To rememdy this, convert the powersave information in ieee80211_conf
from a binary flag to a set of states. This patch is the first step
towards doing this. A new set of powersave states is defined and added
to ieee80211_conf, and mac80211 is updated to set and use these states.
Once all drivers have been moved over to using the new states
IEEE80211_CONF_PS can be removed.
Currnetly this patch also has changes to a couple of drivers to use the
new states for testing purposes. The driver changes should be split out
into separate commits.
Signed-off-by: Seth Forshee <[email protected]>
---
Currently this patch includes changes to the drivers I've been testing
with. My plan is to split those out into separate patches, add patches
for other drivers to use the ps_mode field instead of IEEE80211_CONF_PS,
then remove IEEE80211_CONF_PS completely. But I'd like to get comments
on the mac80211 changes before doing the work to convert all the
drivers.
drivers/net/wireless/ath/ath9k/main.c | 2 +-
.../net/wireless/brcm80211/brcmsmac/mac80211_if.c | 16 +++++++---
drivers/net/wireless/brcm80211/brcmsmac/main.c | 10 +++++++
drivers/net/wireless/brcm80211/brcmsmac/pub.h | 3 ++
include/net/mac80211.h | 23 +++++++++++++++
net/mac80211/mlme.c | 19 ++++++++----
net/mac80211/offchannel.c | 31 +++++++++++++-------
net/mac80211/util.c | 2 +-
8 files changed, 83 insertions(+), 23 deletions(-)
diff --git a/drivers/net/wireless/ath/ath9k/main.c b/drivers/net/wireless/ath/ath9k/main.c
index be30a9a..a216142 100644
--- a/drivers/net/wireless/ath/ath9k/main.c
+++ b/drivers/net/wireless/ath/ath9k/main.c
@@ -689,7 +689,7 @@ static void ath9k_tx(struct ieee80211_hw *hw,
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *) skb->data;
unsigned long flags;
- if (sc->ps_enabled) {
+ if (hw->conf.ps_mode != IEEE80211_PS_DISABLED) {
/*
* mac80211 does not set PM field for normal data frames, so we
* need to update that based on the current PS mode.
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
index 1fbd8ec..dc317a5 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/mac80211_if.c
@@ -390,10 +390,18 @@ static int brcms_ops_config(struct ieee80211_hw *hw, u32 changed)
brcms_dbg_info(core, "%s: change monitor mode: %s\n",
__func__, conf->flags & IEEE80211_CONF_MONITOR ?
"true" : "false");
- if (changed & IEEE80211_CONF_CHANGE_PS)
- brcms_err(core, "%s: change power-save mode: %s (implement)\n",
- __func__, conf->flags & IEEE80211_CONF_PS ?
- "true" : "false");
+ if (changed & IEEE80211_CONF_CHANGE_PS) {
+ /*
+ * brcmsmac doesn't support powersave, but it does support
+ * setting the PM bit in frame control for off-channel PS
+ */
+ if (conf->ps_mode == IEEE80211_PS_ENABLED)
+ brcms_err(core, "%s: change power-save mode: %s (implement)\n",
+ __func__, conf->flags & IEEE80211_CONF_PS ?
+ "true" : "false");
+ else
+ brcms_c_set_ps(wl->wlc, conf->ps_mode);
+ }
if (changed & IEEE80211_CONF_CHANGE_POWER) {
err = brcms_c_set_tx_power(wl->wlc, conf->power_level);
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/main.c b/drivers/net/wireless/brcm80211/brcmsmac/main.c
index 17594de..0286305 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/main.c
+++ b/drivers/net/wireless/brcm80211/brcmsmac/main.c
@@ -3054,6 +3054,11 @@ static void brcms_b_antsel_set(struct brcms_hardware *wlc_hw, u32 antsel_avail)
static bool brcms_c_ps_allowed(struct brcms_c_info *wlc)
{
struct brcms_bss_cfg *cfg = wlc->bsscfg;
+ struct ieee80211_conf *hw_cfg = &wlc->pub->ieee_hw->conf;
+
+ /* allow PS when off-channel PS enabled */
+ if (hw_cfg->ps_mode == IEEE80211_PS_OFFCHANNEL)
+ return true;
/* disallow PS when one of the following global conditions meets */
if (!wlc->pub->associated)
@@ -7546,6 +7551,11 @@ void brcms_c_set_beacon_listen_interval(struct brcms_c_info *wlc, u8 interval)
brcms_c_bcn_li_upd(wlc);
}
+void brcms_c_set_ps(struct brcms_c_info *wlc, enum ieee80211_ps_mode mode)
+{
+ brcms_c_set_ps_ctrl(wlc);
+}
+
int brcms_c_set_tx_power(struct brcms_c_info *wlc, int txpwr)
{
uint qdbm;
diff --git a/drivers/net/wireless/brcm80211/brcmsmac/pub.h b/drivers/net/wireless/brcm80211/brcmsmac/pub.h
index 4fb2834..40c05f9 100644
--- a/drivers/net/wireless/brcm80211/brcmsmac/pub.h
+++ b/drivers/net/wireless/brcm80211/brcmsmac/pub.h
@@ -18,6 +18,7 @@
#define _BRCM_PUB_H_
#include <linux/bcma/bcma.h>
+#include <net/mac80211.h>
#include <brcmu_wifi.h>
#include "types.h"
#include "defs.h"
@@ -328,6 +329,8 @@ extern void brcms_c_set_shortslot_override(struct brcms_c_info *wlc,
s8 sslot_override);
extern void brcms_c_set_beacon_listen_interval(struct brcms_c_info *wlc,
u8 interval);
+extern void brcms_c_set_ps(struct brcms_c_info *wlc,
+ enum ieee80211_ps_mode mode);
extern int brcms_c_set_tx_power(struct brcms_c_info *wlc, int txpwr);
extern int brcms_c_get_tx_power(struct brcms_c_info *wlc);
extern bool brcms_c_check_radio_disabled(struct brcms_c_info *wlc);
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index ee50c5e..237cf16 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -910,6 +910,27 @@ enum ieee80211_conf_changed {
};
/**
+ * enum ieee80211_ps_mode - power save mode
+ *
+ * @IEEE80211_PS_DISABLED: disabled
+ * @IEEE80211_PS_ENABLED: enabled
+ * @IEEE80211_PS_OFFCHANNEL: Off-channel power save
+ * For off-channel power save drivers need to ensure that PM is set
+ * in frame control during STA operation for all frames which will be
+ * acked by the associated AP. In all other respects power save should
+ * remain disabled.
+ * @IEEE80211_PS_NUM_MODES: internal, don't use
+ */
+enum ieee80211_ps_mode {
+ IEEE80211_PS_DISABLED,
+ IEEE80211_PS_ENABLED,
+ IEEE80211_PS_OFFCHANNEL,
+
+ /* keep last */
+ IEEE80211_PS_NUM_MODES,
+};
+
+/**
* enum ieee80211_smps_mode - spatial multiplexing power save mode
*
* @IEEE80211_SMPS_AUTOMATIC: automatic
@@ -935,6 +956,7 @@ enum ieee80211_smps_mode {
*
* @flags: configuration flags defined above
*
+ * @ps_mode: current powersave mode
* @listen_interval: listen interval in units of beacon interval
* @max_sleep_period: the maximum number of beacon intervals to sleep for
* before checking the beacon for a TIM bit (managed mode only); this
@@ -969,6 +991,7 @@ enum ieee80211_smps_mode {
*/
struct ieee80211_conf {
u32 flags;
+ enum ieee80211_ps_mode ps_mode;
int power_level, dynamic_ps_timeout;
int max_sleep_period;
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index 7753a9c..c4b1243 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -965,6 +965,7 @@ static void ieee80211_enable_ps(struct ieee80211_local *local,
(local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS))
return;
+ conf->ps_mode = IEEE80211_PS_ENABLED;
conf->flags |= IEEE80211_CONF_PS;
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
@@ -976,7 +977,8 @@ static void ieee80211_change_ps(struct ieee80211_local *local)
if (local->ps_sdata) {
ieee80211_enable_ps(local, local->ps_sdata);
- } else if (conf->flags & IEEE80211_CONF_PS) {
+ } else if (conf->ps_mode == IEEE80211_PS_ENABLED) {
+ conf->ps_mode = IEEE80211_PS_DISABLED;
conf->flags &= ~IEEE80211_CONF_PS;
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
del_timer_sync(&local->dynamic_ps_timer);
@@ -1115,7 +1117,8 @@ void ieee80211_dynamic_ps_disable_work(struct work_struct *work)
container_of(work, struct ieee80211_local,
dynamic_ps_disable_work);
- if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ if (local->hw.conf.ps_mode == IEEE80211_PS_ENABLED) {
+ local->hw.conf.ps_mode = IEEE80211_PS_DISABLED;
local->hw.conf.flags &= ~IEEE80211_CONF_PS;
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
@@ -1140,7 +1143,7 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
ifmgd = &sdata->u.mgd;
- if (local->hw.conf.flags & IEEE80211_CONF_PS)
+ if (local->hw.conf.ps_mode == IEEE80211_PS_ENABLED)
return;
if (!local->disable_dynamic_ps &&
@@ -1191,6 +1194,7 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
(local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK)) ||
(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
ifmgd->flags &= ~IEEE80211_STA_NULLFUNC_ACKED;
+ local->hw.conf.ps_mode = IEEE80211_PS_ENABLED;
local->hw.conf.flags |= IEEE80211_CONF_PS;
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
@@ -1492,7 +1496,8 @@ static void ieee80211_set_disassoc(struct ieee80211_sub_if_data *sdata,
* to do it before sending disassoc, as otherwise the null-packet
* won't be valid.
*/
- if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ if (local->hw.conf.ps_mode != IEEE80211_PS_DISABLED) {
+ local->hw.conf.ps_mode = IEEE80211_PS_DISABLED;
local->hw.conf.flags &= ~IEEE80211_CONF_PS;
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
@@ -2622,13 +2627,15 @@ static void ieee80211_rx_mgmt_beacon(struct ieee80211_sub_if_data *sdata,
ifmgd->aid);
if (directed_tim) {
if (local->hw.conf.dynamic_ps_timeout > 0) {
- if (local->hw.conf.flags & IEEE80211_CONF_PS) {
+ if (local->hw.conf.ps_mode == IEEE80211_PS_ENABLED) {
+ local->hw.conf.ps_mode = IEEE80211_PS_DISABLED;
local->hw.conf.flags &= ~IEEE80211_CONF_PS;
ieee80211_hw_config(local,
IEEE80211_CONF_CHANGE_PS);
}
ieee80211_send_nullfunc(local, sdata, 0);
- } else if (!local->pspolling && sdata->u.mgd.powersave) {
+ } else if (!local->pspolling &&
+ local->hw.conf.ps_mode != IEEE80211_PS_DISABLED) {
local->pspolling = true;
/*
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index ccb91a2..4b759b2 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -42,9 +42,16 @@ static void ieee80211_offchannel_ps_enable(struct ieee80211_sub_if_data *sdata)
if (local->hw.conf.flags & IEEE80211_CONF_PS) {
local->offchannel_ps_enabled = true;
local->hw.conf.flags &= ~IEEE80211_CONF_PS;
- ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
}
+ /*
+ * Change powersave state even if driver doesn't support
+ * powersave, as drivers are responsible for setting PM in
+ * frame control.
+ */
+ local->hw.conf.ps_mode = IEEE80211_PS_OFFCHANNEL;
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
+
if (!local->offchannel_ps_enabled ||
!(local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK))
/*
@@ -65,9 +72,7 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
{
struct ieee80211_local *local = sdata->local;
- if (!local->ps_sdata)
- ieee80211_send_nullfunc(local, sdata, 0);
- else if (local->offchannel_ps_enabled) {
+ if (local->offchannel_ps_enabled) {
/*
* In !IEEE80211_HW_PS_NULLFUNC_STACK case the hardware
* will send a nullfunc frame with the powersave bit set
@@ -84,18 +89,22 @@ static void ieee80211_offchannel_ps_disable(struct ieee80211_sub_if_data *sdata)
/* TODO: Only set hardware if CONF_PS changed?
* TODO: Should we set offchannel_ps_enabled to false?
*/
+ local->hw.conf.ps_mode = IEEE80211_PS_ENABLED;
local->hw.conf.flags |= IEEE80211_CONF_PS;
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
- } else if (local->hw.conf.dynamic_ps_timeout > 0) {
+ } else {
/*
- * If IEEE80211_CONF_PS was not set and the dynamic_ps_timer
- * had been running before leaving the operating channel,
- * restart the timer now and send a nullfunc frame to inform
- * the AP that we are awake.
+ * For all other cases we need to return powersave to the
+ * disabled state and send a nullfunc frame to inform the
+ * AP that we are awake. Also restart dynamic_ps_timer if
+ * it had been running before leaving the operating channel.
*/
+ local->hw.conf.ps_mode = IEEE80211_PS_DISABLED;
+ ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_PS);
ieee80211_send_nullfunc(local, sdata, 0);
- mod_timer(&local->dynamic_ps_timer, jiffies +
- msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
+ if (local->hw.conf.dynamic_ps_timeout > 0)
+ mod_timer(&local->dynamic_ps_timer, jiffies +
+ msecs_to_jiffies(local->hw.conf.dynamic_ps_timeout));
}
ieee80211_sta_reset_beacon_monitor(sdata);
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index f11e8c5..4dc85af 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -1580,7 +1580,7 @@ int ieee80211_reconfig(struct ieee80211_local *local)
* explicitly send a null packet in order to make sure
* it'll sync against the ap (and get out of psm).
*/
- if (!(local->hw.conf.flags & IEEE80211_CONF_PS)) {
+ if (local->hw.conf.ps_mode == IEEE80211_PS_DISABLED) {
list_for_each_entry(sdata, &local->interfaces, list) {
if (sdata->vif.type != NL80211_IFTYPE_STATION)
continue;
--
1.7.9.5
On Thu, Jan 17, 2013 at 01:29:02AM +0100, Johannes Berg wrote:
> Ok and now that I've typed it all up, I think it points to how we can do
> it much more easily without a separate module and without really
> changing the drivers.
>
> Do a few things:
> * introduce IEEE80211_QUEUE_STOP_REASON_OFFCHANNEL
> * stop all queues, using the new reason, before going offchannel
> * then flush
> * then *directly* tell the driver to transmit the nulldata packets,
> checking only if the driver has the queue stopped, in which case we
> have IEEE80211_QUEUE_STOP_REASON_DRIVER set; if it does, fail going
> off channel
> * similarly, *directly* tell the driver to transmit probe requests and
> offchannel packets from cfg80211, if it fails (stop reason driver)
> then do nothing (probe request) or return a failure to userspace
> (offchannel TX)
>
> In fact the last two bullets should probably be implemented by passing
> some sort of "offchan_tx_ok" to ieee80211_tx_skb() and friends, and
> checking queue_stop_reasons more carefully in ieee80211_tx_frags().
That looks like it would work well. I'll start working on these changes.
> I'll address powersave another day, but it also needs major rework.
I can't completely ignore powersave if I want to get brcmsmac working
properly. The hardware seems to be actively setting or clearing PM based
on its configuration, so I need a way to tell the driver that PM should
be set. Making off-channel an explicit powersave state lets me do this,
and while it doesn't fix any of the other powersave problems I do think
it make the code clearer than the psuedo-state used now for off-channel.
What I've got locally now for this is code to expand the PS config flag
to two bits to support three PS states: disabled, enabled, and
off-channel. Doing this is working well enough with the two machines
I've been testing with (brcmsmac and ath9k). There's an odd side effect
on brcmsmac of having PM set in probe request frames during scans, but
according to the spec PM should be reserved in those frames anyway.
I pushed the changes to the following if you're interested in taking a
look:
git://kernel.ubuntu.com/sforshee/linux.git mac80211-offchannel
I'm possibly still missing some driver updates that would need to be
done, but otherwise the changes should be fairly complete. A major
rework of the powersave code is likely to take some time, so is there
any chance of getting something like this in place to fix things in the
short term?
Seth
Leaving powersave enabled while the scan is suspended requires the use
of PS-Poll to retrieve frames buffered at the AP. PS-Poll isn't a very
efficient means of data transfer and may not even work with drivers not
supporting powersave. These problems can largely be avoided by simply
disabling off-channel powersave when the scan is suspended and enabling
it again when returning to off-channel operation.
The results of this change are fantastic. Previously when using iperf I
would see greatly decreased throughput (often dropping to 0 bits/sec for
several seconds) and very high packet loss during software scans. This
patch completely eliminates the packet loss and reduces the loss in
throughput to very reasonable levels.
Signed-off-by: Seth Forshee <[email protected]>
---
net/mac80211/scan.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index a875f74..f982dda 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -699,11 +699,11 @@ static void ieee80211_scan_state_suspend(struct ieee80211_local *local,
ieee80211_hw_config(local, IEEE80211_CONF_CHANGE_CHANNEL);
/*
- * Re-enable vifs and beaconing. Leave PS
- * in off-channel state..will put that back
- * on-channel at the end of scanning.
+ * Re-enable vifs and beaconing. Disable PS
+ * while the scan is suspended for more
+ * efficient frame rx than with PS-Poll.
*/
- ieee80211_offchannel_return(local, false);
+ ieee80211_offchannel_return(local, true);
*next_delay = HZ / 5;
/* afterwards, resume scan & go to next channel */
@@ -713,8 +713,8 @@ static void ieee80211_scan_state_suspend(struct ieee80211_local *local,
static void ieee80211_scan_state_resume(struct ieee80211_local *local,
unsigned long *next_delay)
{
- /* PS already is in off-channel mode */
- ieee80211_offchannel_stop_vifs(local, false);
+ /* Put PS back in off-channel mode */
+ ieee80211_offchannel_stop_vifs(local, true);
if (local->ops->flush) {
drv_flush(local, false);
--
1.7.9.5
Hi
On Tue, Jan 08, 2013 at 12:10:45PM -0600, Seth Forshee wrote:
> Leaving powersave enabled while the scan is suspended requires the use
> of PS-Poll to retrieve frames buffered at the AP. PS-Poll isn't a very
> efficient means of data transfer and may not even work with drivers not
> supporting powersave. These problems can largely be avoided by simply
> disabling off-channel powersave when the scan is suspended and enabling
> it again when returning to off-channel operation.
>
> The results of this change are fantastic. Previously when using iperf I
> would see greatly decreased throughput (often dropping to 0 bits/sec for
> several seconds) and very high packet loss during software scans. This
> patch completely eliminates the packet loss and reduces the loss in
> throughput to very reasonable levels.
I posted a patch, which remove offchannel_ps_disable argument from
ieee80211_offchannel_return, since there were possible AP and STA power
save state mismash:
http://marc.info/?l=linux-wireless&m=135601089321822&w=2
It should give the same effect on performance as this patch.
Thanks
Stanislaw
So, strangely enough (!) FreeBSD's net80211 code has similar problems
with TX and flushing.
Luckily (again, !) FreeBSD has a "raw" transmit path for things like
this, which is used for things such as probe requests and such. But
it's all a bit hairy and unclear.
There's a double problem - if you have frames in the raw transmit path
that have a sequence number or a CCMP IV number, those need to stay in
order, or things also go boom. Linux works around this by holding a
lock across the whole TX path, so things are "naturally" serialised
this way. I'm fixing this particular issue differently in FreeBSD.
Anyway, the main problem is making sure frames go out "right". Right
now, there's strange issues where probe requests that are queued get
flushed when the scan code goes to set a new channel.
So (after a few things like IBSS QoS, IBSS 11n and the last bits of
power save / ps-poll handling that i'm fixing up) go into FreeBSD's
wireless code, I'm likely going to revamp the scan and off-channel
code. At the very least I'm going to make it so a VAP doesn't change a
channel immediately - it marks the VAP as not accepting further
frames, then it waits for the TX queue to flush before it changes code
(with a _hard_ timeout..) and then a notice will go back to the scan
code tos ay that the channel change has completed.
Anyway, fun times..
adrian
Hi Seth,
> When testing with iperf I've been observing very significant problems
> with throughput and packet loss during software scans. Throughput often
> drops to the point where iperf is reporting 0 bits/sec for several
> seconds, and packet loss commonly gets over 20%.
Ouch.
One caveat up front: While I'm somewhat familiar with the SW scan code,
we have "HW" scan, so I'm not really completely into the details.
> I started investigating
> and discovered the following problems:
>
> 1. It seems that drivers are responsible for ensuring that PM is set in
> frame control when powersave is enabled. However drivers are unaware
> of off-channel powersave, so when the scan is suspended frames may
> be transmitted that cause the AP to think we've exited powersave. As
> a result the AP may attempt to deliver frames while we are
> off-channel.
Hm, yeah, this is a problem. I've kinda always known this... 802.11mb
(or -2012) clarified that in (most?) management frames at least it
doesn't matter.
> 2. There's no flushing of the hardware queues after queueing the
> nullfunc frame to enable off-channel powersave before going
> off-channel, so it's possible to go off-channel before the frame is
> transmitted.
Yep. And to make matters worse, flush() is broken. If the driver has
queues stopped while being asked to flush, it will probably enable
queues and I'm pretty sure we'll give it more frames while it's
flushing.
> 3. There's no flush of pending frames prior to queueing the nullfunc
> frame to enable powersave. That frame is sent at a high priority,
> and drivers supporting QoS may have lower-priority frames queued
> with PM cleared. Those frames will be sent after the nullfunc, and
> the AP may try to deliver frames while we are off-channel.
Yeah...
> 4. During a scan we won't process beacon frames from our associated AP,
> which prevents PS-Poll from starting when the scan is suspended.
> Even if we process the beacons we won't start PS-Poll unless
> powersave is actually enabled, which isn't the case during a scan.
Uh oh.. yeah, ok, but this gets really really complicated. I truly hate
the powersave code. One of these days I'm just going to rip it out ;-)
> It turns out that fixing problem #1 (i.e. patch 2) probably isn't
> necessary with the other changes, as no frames should be sent while
> off-channel PS is enabled. Does this still seem like a problem worth
> fixing?
Hmm. Probably not then. It just makes the API and driver implementation
more complex, no?
johannes
On Wed, Jan 09, 2013 at 12:03:44PM +0100, Stanislaw Gruszka wrote:
> Hi
>
> On Tue, Jan 08, 2013 at 12:10:45PM -0600, Seth Forshee wrote:
> > Leaving powersave enabled while the scan is suspended requires the use
> > of PS-Poll to retrieve frames buffered at the AP. PS-Poll isn't a very
> > efficient means of data transfer and may not even work with drivers not
> > supporting powersave. These problems can largely be avoided by simply
> > disabling off-channel powersave when the scan is suspended and enabling
> > it again when returning to off-channel operation.
> >
> > The results of this change are fantastic. Previously when using iperf I
> > would see greatly decreased throughput (often dropping to 0 bits/sec for
> > several seconds) and very high packet loss during software scans. This
> > patch completely eliminates the packet loss and reduces the loss in
> > throughput to very reasonable levels.
>
> I posted a patch, which remove offchannel_ps_disable argument from
> ieee80211_offchannel_return, since there were possible AP and STA power
> save state mismash:
>
> http://marc.info/?l=linux-wireless&m=135601089321822&w=2
>
> It should give the same effect on performance as this patch.
I guess I missed that one in my pile of post-holiday emails. I agree, it
ought to have the same effect as this patch.
Seth
We've got a couple of races when enabling powersave with an AP for
off-channel operation. The first is fairly simple. If we go off-channel
prior to the nullfunc frame to enable PS is actually transmitted then it
may not be received by the AP. Add a flush after enabling off-channel PS
to prevent this from happening.
The second race is a bit more subtle. If the driver supports QoS and has
frames queued when the nullfunc frame is queued, those frames may get
transmitted after the nullfunc frame. If PM is not set then the AP is
being told that we've exited PS before we go off-channel and may try to
deliver frames. To prevent this, add a flush after stopping the netdev
queues but before passing the nullfunc frame to the driver.
Signed-off-by: Seth Forshee <[email protected]>
---
net/mac80211/offchannel.c | 17 ++++++++++++++++-
net/mac80211/scan.c | 8 ++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
index a5379ae..ccb91a2 100644
--- a/net/mac80211/offchannel.c
+++ b/net/mac80211/offchannel.c
@@ -136,8 +136,23 @@ void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local,
netif_tx_stop_all_queues(sdata->dev);
if (offchannel_ps_enable &&
(sdata->vif.type == NL80211_IFTYPE_STATION) &&
- sdata->u.mgd.associated)
+ sdata->u.mgd.associated) {
+ /*
+ * Need to flush frames in driver queues
+ * before sending nullfunc. Otherwise
+ * devices which support QoS may send the
+ * nullfunc before these queued frames, and
+ * those frames may not have PM set.
+ *
+ * XXX: Would be nice to not flush for each
+ * vif, however I don't see that there's any
+ * protection to prevent frames being handed
+ * to the driver before stopping the netdev
+ * queue.
+ */
+ drv_flush(local, false);
ieee80211_offchannel_ps_enable(sdata);
+ }
}
}
mutex_unlock(&local->iflist_mtx);
diff --git a/net/mac80211/scan.c b/net/mac80211/scan.c
index 8ed83dc..a875f74 100644
--- a/net/mac80211/scan.c
+++ b/net/mac80211/scan.c
@@ -355,6 +355,14 @@ static int ieee80211_start_sw_scan(struct ieee80211_local *local)
ieee80211_offchannel_stop_vifs(local, true);
+ /*
+ * Flush hw queues to ensure that the nullfunc to enable powersave
+ * gets sent before going off-channel.
+ *
+ * XXX: Delay for drivers not supporting flush?
+ */
+ drv_flush(local, false);
+
ieee80211_configure_filter(local);
/* We need to set power level at maximum rate for scanning. */
--
1.7.9.5
On Wed, 2013-01-09 at 07:40 -0600, Seth Forshee wrote:
> > It turns out that fixing problem #1 (i.e. patch 2) probably isn't
> > necessary with the other changes, as no frames should be sent while
> > off-channel PS is enabled. Does this still seem like a problem worth
> > fixing?
>
> This is incorrect. We actually do need patch 2 for some hardware. I
> forgot that when I was testing with BCM43224 I found that PM gets
> actively set or cleared based on the device configuration. It's
> impossible to enable PS at the AP without informing the driver.
Hm, don't understand. If we're not sending any packets to the AP, why
does this matter?
Or are you saying it wants nullfunc frames generated in software, but
then changes the PM bit in them?
johannes
On Thu, Jan 17, 2013 at 12:34:14AM +0100, Johannes Berg wrote:
> On Tue, 2013-01-08 at 12:10 -0600, Seth Forshee wrote:
>
> > +++ b/net/mac80211/offchannel.c
> > @@ -136,8 +136,23 @@ void ieee80211_offchannel_stop_vifs(struct ieee80211_local *local,
> > netif_tx_stop_all_queues(sdata->dev);
> > if (offchannel_ps_enable &&
> > (sdata->vif.type == NL80211_IFTYPE_STATION) &&
> > - sdata->u.mgd.associated)
> > + sdata->u.mgd.associated) {
> > + /*
> > + * Need to flush frames in driver queues
> > + * before sending nullfunc. Otherwise
> > + * devices which support QoS may send the
> > + * nullfunc before these queued frames, and
> > + * those frames may not have PM set.
> > + *
> > + * XXX: Would be nice to not flush for each
> > + * vif, however I don't see that there's any
> > + * protection to prevent frames being handed
> > + * to the driver before stopping the netdev
> > + * queue.
> > + */
> > + drv_flush(local, false);
> > ieee80211_offchannel_ps_enable(sdata);
>
> Could we split the loop, and send the frames in a second loop, to
> combine the flushes into a single one?
I started thinking about just that earlier today. Seems like it ought to
be possible.
Seth
On Fri, Jan 25, 2013 at 10:34:56PM +0100, Johannes Berg wrote:
> On Wed, 2013-01-16 at 23:35 -0600, Seth Forshee wrote:
>
> > > I'll address powersave another day, but it also needs major rework.
> >
> > I can't completely ignore powersave if I want to get brcmsmac working
> > properly. The hardware seems to be actively setting or clearing PM based
> > on its configuration, so I need a way to tell the driver that PM should
> > be set. Making off-channel an explicit powersave state lets me do this,
> > and while it doesn't fix any of the other powersave problems I do think
> > it make the code clearer than the psuedo-state used now for off-channel.
> >
> > What I've got locally now for this is code to expand the PS config flag
> > to two bits to support three PS states: disabled, enabled, and
> > off-channel. Doing this is working well enough with the two machines
> > I've been testing with (brcmsmac and ath9k). There's an odd side effect
> > on brcmsmac of having PM set in probe request frames during scans, but
> > according to the spec PM should be reserved in those frames anyway.
>
> I'm a little confused by this. What frames are we sending to the AP
> while we are in powersave? Doesn't the scan code flush etc., and then
> the stop-reason thing would prevent other frames from going out?
That's true, but with Broadcom some driver configuration is needed to
even get it to transmit a nullfunc frame with PM set. Maybe adding a
powersave state is a bit extreme to cover this one corner case. I'm open
to other suggestions ;-)
I'm finishing up testing on my patches to implement the changes you
suggested, and I expect to send them out early next week (probably
Monday). I'll include the powersave changes along with those, so I guess
we can discuss this in more detail then.
> > I pushed the changes to the following if you're interested in taking a
> > look:
> >
> > git://kernel.ubuntu.com/sforshee/linux.git mac80211-offchannel
> >
> > I'm possibly still missing some driver updates that would need to be
> > done, but otherwise the changes should be fairly complete. A major
> > rework of the powersave code is likely to take some time, so is there
> > any chance of getting something like this in place to fix things in the
> > short term?
>
> Totally, sure. I can hardly lay powersave rewrite on your shoulders :-)
>
> (Nor would I reject it if you did it though, somebody has to do it
> eventually and given how we don't use this code at all for our device I
> can't really justify doing it myself)
I'll keep it in mind for when I have some extra time. So far I haven't
familarized myself much with the powersave code outside of the pieces
involved with off-channel stuff, and whatever I've had to touch for the
changes I've been making.
Seth
On Tue, Jan 08, 2013 at 12:10:42PM -0600, Seth Forshee wrote:
> Hi Johannes,
>
> When testing with iperf I've been observing very significant problems
> with throughput and packet loss during software scans. Throughput often
> drops to the point where iperf is reporting 0 bits/sec for several
> seconds, and packet loss commonly gets over 20%. I started investigating
> and discovered the following problems:
>
> 1. It seems that drivers are responsible for ensuring that PM is set in
> frame control when powersave is enabled. However drivers are unaware
> of off-channel powersave, so when the scan is suspended frames may
> be transmitted that cause the AP to think we've exited powersave. As
> a result the AP may attempt to deliver frames while we are
> off-channel.
>
> 2. There's no flushing of the hardware queues after queueing the
> nullfunc frame to enable off-channel powersave before going
> off-channel, so it's possible to go off-channel before the frame is
> transmitted.
>
> 3. There's no flush of pending frames prior to queueing the nullfunc
> frame to enable powersave. That frame is sent at a high priority,
> and drivers supporting QoS may have lower-priority frames queued
> with PM cleared. Those frames will be sent after the nullfunc, and
> the AP may try to deliver frames while we are off-channel.
>
> 4. During a scan we won't process beacon frames from our associated AP,
> which prevents PS-Poll from starting when the scan is suspended.
> Even if we process the beacons we won't start PS-Poll unless
> powersave is actually enabled, which isn't the case during a scan.
>
> 5. The SCAN_SUSPEND state uses a fixed timeout, so if we do start
> PS-Poll there's no guarantee that it will receive all buffered
> frames from the AP before going back off-channel.
>
> The following RFC patches contain fixes for problems 1, 2, and 3. I
> toyed around with fixing 4 and 5, but the results really aren't much
> better than before. Even with PS-Poll working properly during scans I
> was seeing poor throughput and high packet loss with iperf. So instead I
> tried disabling off-channel powersave when the scan is suspended, and
> the results were fantastic. The implementation is trivial, packet loss
> is completely eliminated, and the performance drops during scans are
> modest. Therefore I've included a patch which does this rather than
> fixing PS-Poll during scans.
>
> These changes aren't quite complete, although they currently work very
> well with ath9k and brcmsmac. I'm hoping to get some feedback on the
> changes and answers to some questions before I proceed.
>
> It turns out that fixing problem #1 (i.e. patch 2) probably isn't
> necessary with the other changes, as no frames should be sent while
> off-channel PS is enabled. Does this still seem like a problem worth
> fixing?
This is incorrect. We actually do need patch 2 for some hardware. I
forgot that when I was testing with BCM43224 I found that PM gets
actively set or cleared based on the device configuration. It's
impossible to enable PS at the AP without informing the driver.
> If so, there's one other change I had considered making. Currently patch
> 2 will require all drivers to have some amount of powersave support for
> off-channel PS to make sure PM is set. Would it make sense to set PM in
> mac80211 when IEEE80211_HW_PS_NULLFUNC_STACK is set?
>
> Thanks,
> Seth
>
>
> Seth Forshee (3):
> mac80211: Add flushes to ensure off-channel PS is enabled during sw
> scans
> mac80211: Convey information about off-channel powersave to drivers
> mac80211: Disable off-channel powersave when software scans are
> suspended
>
> drivers/net/wireless/ath/ath9k/main.c | 2 +-
> .../net/wireless/brcm80211/brcmsmac/mac80211_if.c | 16 +++++--
> drivers/net/wireless/brcm80211/brcmsmac/main.c | 10 ++++
> drivers/net/wireless/brcm80211/brcmsmac/pub.h | 3 ++
> include/net/mac80211.h | 23 ++++++++++
> net/mac80211/mlme.c | 19 +++++---
> net/mac80211/offchannel.c | 48 +++++++++++++++-----
> net/mac80211/scan.c | 20 +++++---
> net/mac80211/util.c | 2 +-
> 9 files changed, 113 insertions(+), 30 deletions(-)
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html