* [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).