2013-03-19 11:08:28

by Luis Chamberlain

Subject: [RFC] mac80211: avoid rearming BA session timers unnecessarily

From: "Luis R. Rodriguez" <[email protected]>

If mac80211 tears down the BA sessions with del_timer_sync()
technically mod_timer() can still rearm them. If mac80211
tears the BA sessions down we don't want to rearm the timers
at a later time so avoid this possibility.

Signed-off-by: Luis R. Rodriguez <[email protected]>
---

Figure I'd start reviewing mod_timer()/mod_timer_pending() as
no one caught on. Likely not an issue on mac80211 but I suspect
drivers will all be poo.

net/mac80211/agg-rx.c | 2 +-
net/mac80211/agg-tx.c | 2 +-
net/mac80211/rx.c | 4 ++--
3 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/agg-rx.c b/net/mac80211/agg-rx.c
index 31bf258..47d7468 100644
--- a/net/mac80211/agg-rx.c
+++ b/net/mac80211/agg-rx.c
@@ -153,7 +153,7 @@ static void sta_rx_agg_session_timer_expired(unsigned long data)

timeout = tid_rx->last_rx + TU_TO_JIFFIES(tid_rx->timeout);
if (time_is_after_jiffies(timeout)) {
- mod_timer(&tid_rx->session_timer, timeout);
+ mod_timer_pending(&tid_rx->session_timer, timeout);
rcu_read_unlock();
return;
}
diff --git a/net/mac80211/agg-tx.c b/net/mac80211/agg-tx.c
index 13b7683..b15b629 100644
--- a/net/mac80211/agg-tx.c
+++ b/net/mac80211/agg-tx.c
@@ -488,7 +488,7 @@ static void sta_tx_agg_session_timer_expired(unsigned long data)

timeout = tid_tx->last_tx + TU_TO_JIFFIES(tid_tx->timeout);
if (time_is_after_jiffies(timeout)) {
- mod_timer(&tid_tx->session_timer, timeout);
+ mod_timer_pending(&tid_tx->session_timer, timeout);
rcu_read_unlock();
return;
}
diff --git a/net/mac80211/rx.c b/net/mac80211/rx.c
index 5b4492a..0b81653 100644
--- a/net/mac80211/rx.c
+++ b/net/mac80211/rx.c
@@ -2216,8 +2216,8 @@ ieee80211_rx_h_ctrl(struct ieee80211_rx_data *rx, struct sk_buff_head *frames)

/* reset session timer */
if (tid_agg_rx->timeout)
- mod_timer(&tid_agg_rx->session_timer,
- TU_TO_EXP_TIME(tid_agg_rx->timeout));
+ mod_timer_pending(&tid_agg_rx->session_timer,
+ TU_TO_EXP_TIME(tid_agg_rx->timeout));

spin_lock(&tid_agg_rx->reorder_lock);
/* release stored frames up to start of BAR */
--
1.7.10.4

2013-03-19 20:44:12

by Johannes Berg

Subject: Re: [RFC] mac80211: avoid rearming BA session timers unnecessarily

On Tue, 2013-03-19 at 04:08 -0700, Luis R. Rodriguez wrote:
> From: "Luis R. Rodriguez" <[email protected]>
>
> If mac80211 tears down the BA sessions with del_timer_sync()
> technically mod_timer() can still rearm them. If mac80211
> tears the BA sessions down we don't want to rearm the timers
> at a later time so avoid this possibility.

Hmm, you may be onto something, but I'm not sure I want to apply this.
You don't seem to actually understand what you're doing -- two of the
three changes you're making don't really seem to be needed and the third
one should probably be fixed differently?

johannes

2013-03-20 09:45:52

by Luis Chamberlain

Subject: Re: [RFC] mac80211: avoid rearming BA session timers unnecessarily

On Tue, Mar 19, 2013 at 1:44 PM, Johannes Berg
<[email protected]> wrote:
> On Tue, 2013-03-19 at 04:08 -0700, Luis R. Rodriguez wrote:
>> From: "Luis R. Rodriguez" <[email protected]>
>>
>> If mac80211 tears down the BA sessions with del_timer_sync()
>> technically mod_timer() can still rearm them. If mac80211
>> tears the BA sessions down we don't want to rearm the timers
>> at a later time so avoid this possibility.
>
> Hmm, you may be onto something, but I'm not sure I want to apply this.
> You don't seem to actually understand what you're doing -- two of the
> three changes you're making don't really seem to be needed and the third
> one should probably be fixed differently?

If I get to review it further to the point I don't disappoint, sure,
otherwise at least you now know of my suspicions.

Luis