Use the tx_frames_pending() driver callback to determine if Tx frames are
pending for its internal queues. If so postpone the dynamic PS timeout
to avoid interrupting Tx traffic.
The commit e8306f989483e4b97a8b37dd268de6c8c6f35e75 enabled this
behavior for drivers with IEEE80211_HW_PS_NULLFUNC_STACK. We enable this
for all drivers supporting dynamic PS.
This patch helps improve performance in noisy environments.
Signed-off-by: Arik Nemtsov <[email protected]>
---
net/mac80211/mlme.c | 22 +++++++++++++---------
1 files changed, 13 insertions(+), 9 deletions(-)
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index a41f234..3a1699b 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -758,19 +758,23 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
if (local->hw.conf.flags & IEEE80211_CONF_PS)
return;
+ /* don't enter PS if dynamic PS is enabled and TX frames are pending */
+ if (local->hw.conf.dynamic_ps_timeout > 0 &&
+ !local->disable_dynamic_ps && drv_tx_frames_pending(local)) {
+ mod_timer(&local->dynamic_ps_timer, jiffies +
+ msecs_to_jiffies(
+ local->hw.conf.dynamic_ps_timeout));
+ return;
+ }
+
if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
(!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
netif_tx_stop_all_queues(sdata->dev);
- if (drv_tx_frames_pending(local))
- mod_timer(&local->dynamic_ps_timer, jiffies +
- msecs_to_jiffies(
- local->hw.conf.dynamic_ps_timeout));
- else {
- ieee80211_send_nullfunc(local, sdata, 1);
- /* Flush to get the tx status of nullfunc frame */
- drv_flush(local, false);
- }
+ ieee80211_send_nullfunc(local, sdata, 1);
+
+ /* Flush to get the tx status of nullfunc frame */
+ drv_flush(local, false);
}
if (!((local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
--
1.7.4.1
On Sun, Jun 19, 2011 at 10:11:16AM +0200, Johannes Berg wrote:
> On Sat, 2011-06-18 at 22:32 +0300, Arik Nemtsov wrote:
> > Use the tx_frames_pending() driver callback to determine if Tx frames are
> > pending for its internal queues. If so postpone the dynamic PS timeout
> > to avoid interrupting Tx traffic.
> >
> > The commit e8306f989483e4b97a8b37dd268de6c8c6f35e75 enabled this
> > behavior for drivers with IEEE80211_HW_PS_NULLFUNC_STACK. We enable this
> > for all drivers supporting dynamic PS.
> >
> > This patch helps improve performance in noisy environments.
>
> FWIW, I don't really consider myself maintaining the PS
> implementation ;-) I don't understand it and think the code is way too
> complex.
Perhaps Kalle can comment? Or...?
John
--
John W. Linville Someday the world will need a hero, and you
[email protected] might be all we have. Be ready.
On Tue, Jun 21, 2011 at 07:55, Vivek Natarajan <[email protected]> wrote:
> On Sun, Jun 19, 2011 at 1:02 AM, Arik Nemtsov <[email protected]> wrote:
>> Use the tx_frames_pending() driver callback to determine if Tx frames are
>> pending for its internal queues. If so postpone the dynamic PS timeout
>> to avoid interrupting Tx traffic.
>>
>> The commit e8306f989483e4b97a8b37dd268de6c8c6f35e75 enabled this
>> behavior for drivers with IEEE80211_HW_PS_NULLFUNC_STACK. We enable this
>> for all drivers supporting dynamic PS.
>>
>> This patch helps improve performance in noisy environments.
>>
>> Signed-off-by: Arik Nemtsov <[email protected]>
>> ---
>> ?net/mac80211/mlme.c | ? 22 +++++++++++++---------
>> ?1 files changed, 13 insertions(+), 9 deletions(-)
>>
>> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
>> index a41f234..3a1699b 100644
>> --- a/net/mac80211/mlme.c
>> +++ b/net/mac80211/mlme.c
>> @@ -758,19 +758,23 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
>> ? ? ? ?if (local->hw.conf.flags & IEEE80211_CONF_PS)
>> ? ? ? ? ? ? ? ?return;
>>
>> + ? ? ? /* don't enter PS if dynamic PS is enabled and TX frames are pending */
>> + ? ? ? if (local->hw.conf.dynamic_ps_timeout > 0 &&
>> + ? ? ? ? ? !local->disable_dynamic_ps && drv_tx_frames_pending(local)) {
>
> netif_tx_stop_all_queues has to be called before checking
> frames_pending(), so that we can make sure no data is transmitted in
> the time between checking frames_pending and transmitting null data
> frames. If it is not done, this might cause some power save states
> mismatch between the AP and the station in some corner cases.
>
> Vivek.
>
That's a good point. But I guess its only relevant to drivers using
IEEE80211_HW_PS_NULLFUNC_STACK.
Still seems racy to me - even if queues are stopped it doesn't mean
mac80211 is not in the middle of ieee80211_tx(). Is there a way to
make sure?
Arik
On Mon, 2011-06-20 at 09:35 -0400, John W. Linville wrote:
> On Sun, Jun 19, 2011 at 10:11:16AM +0200, Johannes Berg wrote:
> > On Sat, 2011-06-18 at 22:32 +0300, Arik Nemtsov wrote:
> > > Use the tx_frames_pending() driver callback to determine if Tx frames are
> > > pending for its internal queues. If so postpone the dynamic PS timeout
> > > to avoid interrupting Tx traffic.
> > >
> > > The commit e8306f989483e4b97a8b37dd268de6c8c6f35e75 enabled this
> > > behavior for drivers with IEEE80211_HW_PS_NULLFUNC_STACK. We enable this
> > > for all drivers supporting dynamic PS.
> > >
> > > This patch helps improve performance in noisy environments.
> >
> > FWIW, I don't really consider myself maintaining the PS
> > implementation ;-) I don't understand it and think the code is way too
> > complex.
>
> Perhaps Kalle can comment? Or...?
Yeah he's a good candidate :)
johannes
Johannes Berg <[email protected]> writes:
>> > FWIW, I don't really consider myself maintaining the PS
>> > implementation ;-) I don't understand it and think the code is way too
>> > complex.
>>
>> Perhaps Kalle can comment? Or...?
>
> Yeah he's a good candidate :)
Hehe :)
But I agree with Johannes, the powersave in mac80211 is pretty
complex. I lost track of it a long time ago so it's pretty difficult
for me to comment about the patch.
--
Kalle Valo
On Tue, Jun 21, 2011 at 09:33, Vivek Natarajan <[email protected]> wrote:
>
> Well, I can't find any way to stop this race. :)
> Atleast the other race could be fixed with stop_queues.
Sure I'll add a stop_queues for for the IEEE80211_HW_PS_NULLFUNC_STACK
case. If you can't hit the other race I guess it doesn't matter. It's
a corner case anyway.
> How about the below patch? I am not sure about the disable_dynamic_ps
> and its BT/WLAN antenna sharing. Please add that if you see it as
> appropriate.
I think the disable_dynamic_ps and peers are needed. Otherwise we risk
getting inconsistent PS notifications from mac80211 when dyn_ps is
disabled.
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index faca503..67ecad1 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -778,15 +778,14 @@ void ieee80211_dynamic_ps_enable_work(struct
> work_struct *work)
> ? ? ? ?}
> ? ? ? ?spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
>
> - ? ? ? if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
> - ? ? ? ? ? (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
> + ? ? ? if (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
> ? ? ? ? ? ? ? ?netif_tx_stop_all_queues(sdata->dev);
>
> ? ? ? ? ? ? ? ?if (drv_tx_frames_pending(local))
> ? ? ? ? ? ? ? ? ? ? ? ?mod_timer(&local->dynamic_ps_timer, jiffies +
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?msecs_to_jiffies(
> ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?local->hw.conf.dynamic_ps_timeout));
> - ? ? ? ? ? ? ? else {
> + ? ? ? ? ? ? ? else if (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) {
> ? ? ? ? ? ? ? ? ? ? ? ?ieee80211_send_nullfunc(local, sdata, 1);
> ? ? ? ? ? ? ? ? ? ? ? ?/* Flush to get the tx status of nullfunc frame */
> ? ? ? ? ? ? ? ? ? ? ? ?drv_flush(local, false);
>
Well drivers without IEEE80211_HW_PS_NULLFUNC_STACK need to leave the
function after mod_timer(). Otherwise we end up changing the PS state
anyway.
But yea, basically its very similar to my patch.
I'll submit a v2 with the stop_queues pending Sam's review.
Arik
On Tue, Jun 21, 2011 at 11:13 AM, Arik Nemtsov <[email protected]> wrote:
> On Tue, Jun 21, 2011 at 07:55, Vivek Natarajan <[email protected]> wrote:
>>>
>>> + ? ? ? /* don't enter PS if dynamic PS is enabled and TX frames are pending */
>>> + ? ? ? if (local->hw.conf.dynamic_ps_timeout > 0 &&
>>> + ? ? ? ? ? !local->disable_dynamic_ps && drv_tx_frames_pending(local)) {
>>
>> netif_tx_stop_all_queues has to be called before checking
>> frames_pending(), so that we can make sure no data is transmitted in
>> the time between checking frames_pending and transmitting null data
>> frames. If it is not done, this might cause some power save states
>> mismatch between the AP and the station in some corner cases.
>
> That's a good point. But I guess its only relevant to drivers using
> IEEE80211_HW_PS_NULLFUNC_STACK.
> Still seems racy to me - even if queues are stopped it doesn't mean
> mac80211 is not in the middle of ieee80211_tx(). Is there a way to
> make sure?
Well, I can't find any way to stop this race. :)
Atleast the other race could be fixed with stop_queues.
How about the below patch? I am not sure about the disable_dynamic_ps
and its BT/WLAN antenna sharing. Please add that if you see it as
appropriate.
diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
index faca503..67ecad1 100644
--- a/net/mac80211/mlme.c
+++ b/net/mac80211/mlme.c
@@ -778,15 +778,14 @@ void ieee80211_dynamic_ps_enable_work(struct
work_struct *work)
}
spin_unlock_irqrestore(&local->queue_stop_reason_lock, flags);
- if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
- (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
+ if (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED)) {
netif_tx_stop_all_queues(sdata->dev);
if (drv_tx_frames_pending(local))
mod_timer(&local->dynamic_ps_timer, jiffies +
msecs_to_jiffies(
local->hw.conf.dynamic_ps_timeout));
- else {
+ else if (local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) {
ieee80211_send_nullfunc(local, sdata, 1);
/* Flush to get the tx status of nullfunc frame */
drv_flush(local, false);
Vivek.
On Mon, Jun 20, 2011 at 7:42 AM, Kalle Valo <[email protected]> wrote:
> Johannes Berg <[email protected]> writes:
>
>>> > FWIW, I don't really consider myself maintaining the PS
>>> > implementation ;-) I don't understand it and think the code is way too
>>> > complex.
>>>
>>> Perhaps Kalle can comment? ?Or...?
>>
>> Yeah he's a good candidate :)
>
> Hehe :)
>
> But I agree with Johannes, the powersave in mac80211 is pretty
> complex. I lost track of it a long time ago so it's pretty difficult
> for me to comment about the patch.
I believe I've hit this situation (and have changes of my own to do
something similar but different); will review.
-Sam
On Sat, 2011-06-18 at 22:32 +0300, Arik Nemtsov wrote:
> Use the tx_frames_pending() driver callback to determine if Tx frames are
> pending for its internal queues. If so postpone the dynamic PS timeout
> to avoid interrupting Tx traffic.
>
> The commit e8306f989483e4b97a8b37dd268de6c8c6f35e75 enabled this
> behavior for drivers with IEEE80211_HW_PS_NULLFUNC_STACK. We enable this
> for all drivers supporting dynamic PS.
>
> This patch helps improve performance in noisy environments.
FWIW, I don't really consider myself maintaining the PS
implementation ;-) I don't understand it and think the code is way too
complex.
I'm still not convinced I like the drv_tx_frames_pending() thing, but
then again I don't really care much anyway about PS stuff.
johannes
> Signed-off-by: Arik Nemtsov <[email protected]>
> ---
> net/mac80211/mlme.c | 22 +++++++++++++---------
> 1 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index a41f234..3a1699b 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -758,19 +758,23 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
> if (local->hw.conf.flags & IEEE80211_CONF_PS)
> return;
>
> + /* don't enter PS if dynamic PS is enabled and TX frames are pending */
> + if (local->hw.conf.dynamic_ps_timeout > 0 &&
> + !local->disable_dynamic_ps && drv_tx_frames_pending(local)) {
> + mod_timer(&local->dynamic_ps_timer, jiffies +
> + msecs_to_jiffies(
> + local->hw.conf.dynamic_ps_timeout));
> + return;
> + }
> +
> if ((local->hw.flags & IEEE80211_HW_PS_NULLFUNC_STACK) &&
> (!(ifmgd->flags & IEEE80211_STA_NULLFUNC_ACKED))) {
> netif_tx_stop_all_queues(sdata->dev);
>
> - if (drv_tx_frames_pending(local))
> - mod_timer(&local->dynamic_ps_timer, jiffies +
> - msecs_to_jiffies(
> - local->hw.conf.dynamic_ps_timeout));
> - else {
> - ieee80211_send_nullfunc(local, sdata, 1);
> - /* Flush to get the tx status of nullfunc frame */
> - drv_flush(local, false);
> - }
> + ieee80211_send_nullfunc(local, sdata, 1);
> +
> + /* Flush to get the tx status of nullfunc frame */
> + drv_flush(local, false);
> }
>
> if (!((local->hw.flags & IEEE80211_HW_REPORTS_TX_ACK_STATUS) &&
On Sun, Jun 19, 2011 at 1:02 AM, Arik Nemtsov <[email protected]> wrote:
> Use the tx_frames_pending() driver callback to determine if Tx frames are
> pending for its internal queues. If so postpone the dynamic PS timeout
> to avoid interrupting Tx traffic.
>
> The commit e8306f989483e4b97a8b37dd268de6c8c6f35e75 enabled this
> behavior for drivers with IEEE80211_HW_PS_NULLFUNC_STACK. We enable this
> for all drivers supporting dynamic PS.
>
> This patch helps improve performance in noisy environments.
>
> Signed-off-by: Arik Nemtsov <[email protected]>
> ---
> ?net/mac80211/mlme.c | ? 22 +++++++++++++---------
> ?1 files changed, 13 insertions(+), 9 deletions(-)
>
> diff --git a/net/mac80211/mlme.c b/net/mac80211/mlme.c
> index a41f234..3a1699b 100644
> --- a/net/mac80211/mlme.c
> +++ b/net/mac80211/mlme.c
> @@ -758,19 +758,23 @@ void ieee80211_dynamic_ps_enable_work(struct work_struct *work)
> ? ? ? ?if (local->hw.conf.flags & IEEE80211_CONF_PS)
> ? ? ? ? ? ? ? ?return;
>
> + ? ? ? /* don't enter PS if dynamic PS is enabled and TX frames are pending */
> + ? ? ? if (local->hw.conf.dynamic_ps_timeout > 0 &&
> + ? ? ? ? ? !local->disable_dynamic_ps && drv_tx_frames_pending(local)) {
netif_tx_stop_all_queues has to be called before checking
frames_pending(), so that we can make sure no data is transmitted in
the time between checking frames_pending and transmitting null data
frames. If it is not done, this might cause some power save states
mismatch between the AP and the station in some corner cases.
Vivek.