* [RFC PATCH] mac80211: sta_cleanup timer handling
@ 2010-04-16 7:35 Juuso Oikarinen
2010-04-16 7:35 ` [RFC PATCH] mac80211: Prevent running sta_cleanup timer unnecessarily Juuso Oikarinen
0 siblings, 1 reply; 6+ messages in thread
From: Juuso Oikarinen @ 2010-04-16 7:35 UTC (permalink / raw)
To: linux-wireless
The sta_cleanup timer is used to expire frames in the sta structure ps_tx_buf
-list. To perform this, the timer is executed periodically.
The timer is executed periodically, regardless if there are frames in the
buffer or not. As a result, the system gets periodically woken up for no
reason at all, wasting precious resources.
This patch proposes a simple fix. It prevent rescheduling the sta_cleanup
timer if on the particular invocation the ps_tx_buf buffers for all sta's
were empty. On the TX path, the timer is restarted upon queueing frames to
those buffers.
Due to the way checking is performed, this approach will "leak" a few
unnecessary invocations of the timer handler, but in the big picture those
do not matter.
Comments are appreciated.
Juuso Oikarinen (1):
mac80211: Prevent running sta_cleanup timer unnecessarily
net/mac80211/sta_info.c | 13 ++++++++++---
net/mac80211/tx.c | 7 +++++++
2 files changed, 17 insertions(+), 3 deletions(-)
^ permalink raw reply [flat|nested] 6+ messages in thread
* [RFC PATCH] mac80211: Prevent running sta_cleanup timer unnecessarily
2010-04-16 7:35 [RFC PATCH] mac80211: sta_cleanup timer handling Juuso Oikarinen
@ 2010-04-16 7:35 ` Juuso Oikarinen
2010-04-16 13:41 ` John W. Linville
2010-04-16 20:17 ` Johannes Berg
0 siblings, 2 replies; 6+ messages in thread
From: Juuso Oikarinen @ 2010-04-16 7:35 UTC (permalink / raw)
To: linux-wireless
The sta_cleanup timer is used to periodically expire buffered frames from the
tx buf. The timer is executing periodically, regardless of the need for it.
This is wasting resources.
Fix this simply by not restarting the sta_cleanup timer if the tx buffer was
empty. Restart the timer when there is some more tx-traffic.
Cc: Janne Ylälehto <janne.ylalehto@nokia.com>
Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
---
net/mac80211/sta_info.c | 13 ++++++++++---
net/mac80211/tx.c | 7 +++++++
2 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
index ff0eb94..3de7a22 100644
--- a/net/mac80211/sta_info.c
+++ b/net/mac80211/sta_info.c
@@ -575,7 +575,7 @@ static int sta_info_buffer_expired(struct sta_info *sta,
}
-static void sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
+static bool sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
struct sta_info *sta)
{
unsigned long flags;
@@ -583,7 +583,7 @@ static void sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
struct ieee80211_sub_if_data *sdata;
if (skb_queue_empty(&sta->ps_tx_buf))
- return;
+ return false;
for (;;) {
spin_lock_irqsave(&sta->ps_tx_buf.lock, flags);
@@ -608,6 +608,8 @@ static void sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
if (skb_queue_empty(&sta->ps_tx_buf))
sta_info_clear_tim_bit(sta);
}
+
+ return true;
}
static int __must_check __sta_info_destroy(struct sta_info *sta)
@@ -755,15 +757,20 @@ static void sta_info_cleanup(unsigned long data)
{
struct ieee80211_local *local = (struct ieee80211_local *) data;
struct sta_info *sta;
+ bool timer_needed = false;
rcu_read_lock();
list_for_each_entry_rcu(sta, &local->sta_list, list)
- sta_info_cleanup_expire_buffered(local, sta);
+ if (sta_info_cleanup_expire_buffered(local, sta))
+ timer_needed = true;
rcu_read_unlock();
if (local->quiescing)
return;
+ if (!timer_needed)
+ return;
+
local->sta_cleanup.expires =
round_jiffies(jiffies + STA_INFO_CLEANUP_INTERVAL);
add_timer(&local->sta_cleanup);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 2cb7726..e2aa972 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -429,6 +429,7 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
struct sta_info *sta = tx->sta;
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
+ struct ieee80211_local *local = tx->local;
u32 staflags;
if (unlikely(!sta ||
@@ -476,6 +477,12 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
info->control.vif = &tx->sdata->vif;
info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
skb_queue_tail(&sta->ps_tx_buf, tx->skb);
+
+ if (!timer_pending(&local->sta_cleanup))
+ mod_timer(&local->sta_cleanup,
+ round_jiffies(jiffies +
+ STA_INFO_CLEANUP_INTERVAL));
+
return TX_QUEUED;
}
#ifdef CONFIG_MAC80211_VERBOSE_PS_DEBUG
--
1.6.3.3
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mac80211: Prevent running sta_cleanup timer unnecessarily
2010-04-16 7:35 ` [RFC PATCH] mac80211: Prevent running sta_cleanup timer unnecessarily Juuso Oikarinen
@ 2010-04-16 13:41 ` John W. Linville
2010-04-16 20:17 ` Johannes Berg
1 sibling, 0 replies; 6+ messages in thread
From: John W. Linville @ 2010-04-16 13:41 UTC (permalink / raw)
To: Juuso Oikarinen; +Cc: linux-wireless
On Fri, Apr 16, 2010 at 10:35:08AM +0300, Juuso Oikarinen wrote:
> The sta_cleanup timer is used to periodically expire buffered frames from the
> tx buf. The timer is executing periodically, regardless of the need for it.
> This is wasting resources.
>
> Fix this simply by not restarting the sta_cleanup timer if the tx buffer was
> empty. Restart the timer when there is some more tx-traffic.
>
> Cc: Janne Ylälehto <janne.ylalehto@nokia.com>
> Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
Seems reasonable to me...
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mac80211: Prevent running sta_cleanup timer unnecessarily
2010-04-16 7:35 ` [RFC PATCH] mac80211: Prevent running sta_cleanup timer unnecessarily Juuso Oikarinen
2010-04-16 13:41 ` John W. Linville
@ 2010-04-16 20:17 ` Johannes Berg
2010-04-19 4:56 ` Juuso Oikarinen
1 sibling, 1 reply; 6+ messages in thread
From: Johannes Berg @ 2010-04-16 20:17 UTC (permalink / raw)
To: Juuso Oikarinen; +Cc: linux-wireless
On Fri, 2010-04-16 at 10:35 +0300, Juuso Oikarinen wrote:
> The sta_cleanup timer is used to periodically expire buffered frames from the
> tx buf. The timer is executing periodically, regardless of the need for it.
> This is wasting resources.
>
> Fix this simply by not restarting the sta_cleanup timer if the tx buffer was
> empty. Restart the timer when there is some more tx-traffic.
>
> Cc: Janne Ylälehto <janne.ylalehto@nokia.com>
> Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> ---
> net/mac80211/sta_info.c | 13 ++++++++++---
> net/mac80211/tx.c | 7 +++++++
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> index ff0eb94..3de7a22 100644
> --- a/net/mac80211/sta_info.c
> +++ b/net/mac80211/sta_info.c
> @@ -575,7 +575,7 @@ static int sta_info_buffer_expired(struct sta_info *sta,
> }
>
>
> -static void sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
> +static bool sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
> struct sta_info *sta)
> {
> unsigned long flags;
> @@ -583,7 +583,7 @@ static void sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
> struct ieee80211_sub_if_data *sdata;
>
> if (skb_queue_empty(&sta->ps_tx_buf))
> - return;
> + return false;
>
> for (;;) {
> spin_lock_irqsave(&sta->ps_tx_buf.lock, flags);
> @@ -608,6 +608,8 @@ static void sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
> if (skb_queue_empty(&sta->ps_tx_buf))
> sta_info_clear_tim_bit(sta);
> }
> +
> + return true;
> }
>
> static int __must_check __sta_info_destroy(struct sta_info *sta)
> @@ -755,15 +757,20 @@ static void sta_info_cleanup(unsigned long data)
> {
> struct ieee80211_local *local = (struct ieee80211_local *) data;
> struct sta_info *sta;
> + bool timer_needed = false;
>
> rcu_read_lock();
> list_for_each_entry_rcu(sta, &local->sta_list, list)
> - sta_info_cleanup_expire_buffered(local, sta);
> + if (sta_info_cleanup_expire_buffered(local, sta))
> + timer_needed = true;
> rcu_read_unlock();
>
> if (local->quiescing)
> return;
>
> + if (!timer_needed)
> + return;
> +
> local->sta_cleanup.expires =
> round_jiffies(jiffies + STA_INFO_CLEANUP_INTERVAL);
> add_timer(&local->sta_cleanup);
> diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> index 2cb7726..e2aa972 100644
> --- a/net/mac80211/tx.c
> +++ b/net/mac80211/tx.c
> @@ -429,6 +429,7 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
> struct sta_info *sta = tx->sta;
> struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
> struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
> + struct ieee80211_local *local = tx->local;
> u32 staflags;
>
> if (unlikely(!sta ||
> @@ -476,6 +477,12 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
> info->control.vif = &tx->sdata->vif;
> info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> skb_queue_tail(&sta->ps_tx_buf, tx->skb);
> +
> + if (!timer_pending(&local->sta_cleanup))
> + mod_timer(&local->sta_cleanup,
> + round_jiffies(jiffies +
> + STA_INFO_CLEANUP_INTERVAL));
> +
Why bother to check !timer_pending?
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mac80211: Prevent running sta_cleanup timer unnecessarily
2010-04-16 20:17 ` Johannes Berg
@ 2010-04-19 4:56 ` Juuso Oikarinen
2010-04-19 5:57 ` Johannes Berg
0 siblings, 1 reply; 6+ messages in thread
From: Juuso Oikarinen @ 2010-04-19 4:56 UTC (permalink / raw)
To: ext Johannes Berg; +Cc: linux-wireless
On Fri, 2010-04-16 at 22:17 +0200, ext Johannes Berg wrote:
> On Fri, 2010-04-16 at 10:35 +0300, Juuso Oikarinen wrote:
> > The sta_cleanup timer is used to periodically expire buffered frames from the
> > tx buf. The timer is executing periodically, regardless of the need for it.
> > This is wasting resources.
> >
> > Fix this simply by not restarting the sta_cleanup timer if the tx buffer was
> > empty. Restart the timer when there is some more tx-traffic.
> >
> > Cc: Janne Ylälehto <janne.ylalehto@nokia.com>
> > Signed-off-by: Juuso Oikarinen <juuso.oikarinen@nokia.com>
> > ---
> > net/mac80211/sta_info.c | 13 ++++++++++---
> > net/mac80211/tx.c | 7 +++++++
> > 2 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/net/mac80211/sta_info.c b/net/mac80211/sta_info.c
> > index ff0eb94..3de7a22 100644
> > --- a/net/mac80211/sta_info.c
> > +++ b/net/mac80211/sta_info.c
> > @@ -575,7 +575,7 @@ static int sta_info_buffer_expired(struct sta_info *sta,
> > }
> >
> >
> > -static void sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
> > +static bool sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
> > struct sta_info *sta)
> > {
> > unsigned long flags;
> > @@ -583,7 +583,7 @@ static void sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
> > struct ieee80211_sub_if_data *sdata;
> >
> > if (skb_queue_empty(&sta->ps_tx_buf))
> > - return;
> > + return false;
> >
> > for (;;) {
> > spin_lock_irqsave(&sta->ps_tx_buf.lock, flags);
> > @@ -608,6 +608,8 @@ static void sta_info_cleanup_expire_buffered(struct ieee80211_local *local,
> > if (skb_queue_empty(&sta->ps_tx_buf))
> > sta_info_clear_tim_bit(sta);
> > }
> > +
> > + return true;
> > }
> >
> > static int __must_check __sta_info_destroy(struct sta_info *sta)
> > @@ -755,15 +757,20 @@ static void sta_info_cleanup(unsigned long data)
> > {
> > struct ieee80211_local *local = (struct ieee80211_local *) data;
> > struct sta_info *sta;
> > + bool timer_needed = false;
> >
> > rcu_read_lock();
> > list_for_each_entry_rcu(sta, &local->sta_list, list)
> > - sta_info_cleanup_expire_buffered(local, sta);
> > + if (sta_info_cleanup_expire_buffered(local, sta))
> > + timer_needed = true;
> > rcu_read_unlock();
> >
> > if (local->quiescing)
> > return;
> >
> > + if (!timer_needed)
> > + return;
> > +
> > local->sta_cleanup.expires =
> > round_jiffies(jiffies + STA_INFO_CLEANUP_INTERVAL);
> > add_timer(&local->sta_cleanup);
> > diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
> > index 2cb7726..e2aa972 100644
> > --- a/net/mac80211/tx.c
> > +++ b/net/mac80211/tx.c
> > @@ -429,6 +429,7 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
> > struct sta_info *sta = tx->sta;
> > struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
> > struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
> > + struct ieee80211_local *local = tx->local;
> > u32 staflags;
> >
> > if (unlikely(!sta ||
> > @@ -476,6 +477,12 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
> > info->control.vif = &tx->sdata->vif;
> > info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> > skb_queue_tail(&sta->ps_tx_buf, tx->skb);
> > +
> > + if (!timer_pending(&local->sta_cleanup))
> > + mod_timer(&local->sta_cleanup,
> > + round_jiffies(jiffies +
> > + STA_INFO_CLEANUP_INTERVAL));
> > +
>
> Why bother to check !timer_pending?
>
Otherwise, if we have some periodic TX going on with an interval less
than STA_INFO_CLEANUP_INTERVAL, the sta_cleanup could be delayed
considerably.
-Juuso
> johannes
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC PATCH] mac80211: Prevent running sta_cleanup timer unnecessarily
2010-04-19 4:56 ` Juuso Oikarinen
@ 2010-04-19 5:57 ` Johannes Berg
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Berg @ 2010-04-19 5:57 UTC (permalink / raw)
To: Juuso Oikarinen; +Cc: linux-wireless
On Mon, 2010-04-19 at 07:56 +0300, Juuso Oikarinen wrote:
> > > --- a/net/mac80211/tx.c
> > > +++ b/net/mac80211/tx.c
> > > @@ -429,6 +429,7 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
> > > struct sta_info *sta = tx->sta;
> > > struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
> > > struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)tx->skb->data;
> > > + struct ieee80211_local *local = tx->local;
> > > u32 staflags;
> > >
> > > if (unlikely(!sta ||
> > > @@ -476,6 +477,12 @@ ieee80211_tx_h_unicast_ps_buf(struct ieee80211_tx_data *tx)
> > > info->control.vif = &tx->sdata->vif;
> > > info->flags |= IEEE80211_TX_INTFL_NEED_TXPROCESSING;
> > > skb_queue_tail(&sta->ps_tx_buf, tx->skb);
> > > +
> > > + if (!timer_pending(&local->sta_cleanup))
> > > + mod_timer(&local->sta_cleanup,
> > > + round_jiffies(jiffies +
> > > + STA_INFO_CLEANUP_INTERVAL));
> > > +
> >
> > Why bother to check !timer_pending?
> >
>
> Otherwise, if we have some periodic TX going on with an interval less
> than STA_INFO_CLEANUP_INTERVAL, the sta_cleanup could be delayed
> considerably.
Ok, makes sense, I didn't check the context there.
johannes
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2010-04-19 5:57 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-04-16 7:35 [RFC PATCH] mac80211: sta_cleanup timer handling Juuso Oikarinen
2010-04-16 7:35 ` [RFC PATCH] mac80211: Prevent running sta_cleanup timer unnecessarily Juuso Oikarinen
2010-04-16 13:41 ` John W. Linville
2010-04-16 20:17 ` Johannes Berg
2010-04-19 4:56 ` Juuso Oikarinen
2010-04-19 5:57 ` Johannes Berg
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).