mac80211: Properly kill tasklets before shutdown

PATCH In review Michael Buesch 3 participants 10 messages Mar 6, 2007 – Mar 23, 2007 mac80211

Share & tools

Michael Buesch

author

2007-03-06 18:03 UTC #0 lore raw

PATCH 1/1 mac80211: Properly kill tasklets before shutdown

We need to do tasklet_kill() on any tasklet on unregister to make sure the tasklet is not running _and_ scheduled anymore. (tasklet_disable() only ensures it's not running anymore).

This fixes the tasklet related crash that was reported some time ago.

Signed-off-by Michael Buesch <[email protected]>

net/mac80211/ieee80211.c +2 -2

Index: bu3sch-wireless-dev/net/mac80211/ieee80211.c
===================================================================
@@ -4761,8 +4761,8 @@
 	struct ieee80211_sub_if_data *sdata, *tmp;
 	int i;
 
-	tasklet_disable(&local->tasklet);
-	/* TODO: skb_queue should be empty here, no need to do anything? */
+	tasklet_kill(&local->tx_pending_tasklet);
+	tasklet_kill(&local->tasklet);
 
 	rtnl_lock();
 	local->reg_state = IEEE80211_DEV_UNREGISTERED;

Signature · 1 line

Greetings Michael.

Johannes Berg

maintainer

2007-03-08 15:32 UTC #1 lore raw

Quoted 1 line · Michael Buesch

This fixes the tasklet related crash that was reported some time ago.

I agree with that :)

net/mac80211/ieee80211.c line 4765

Quoted 4 lines

net/mac80211/ieee80211.c +2 -2

-	tasklet_disable(&local->tasklet);
-	/* TODO: skb_queue should be empty here, no need to do anything? */
+	tasklet_kill(&local->tx_pending_tasklet);
+	tasklet_kill(&local->tasklet);

But I'm not sure this is sufficient. I think we can then leak pending skbs. In my tests I've often gotten that warning message about the skb queue not being empty (right before the crash). Or is that handled elsewhere now?

johannes

Michael Buesch

author

2007-03-08 15:40 UTC #2 lore raw

Quoted 6 lines · Johannes Berg

Quoted 1 line · Michael Buesch
This fixes the tasklet related crash that was reported some time ago.
I agree with that :)

Quoted 7 lines

net/mac80211/ieee80211.c line 4765
Quoted 4 lines
net/mac80211/ieee80211.c +2 -2
-	tasklet_disable(&local->tasklet);
-	/* TODO: skb_queue should be empty here, no need to do anything? */
+	tasklet_kill(&local->tx_pending_tasklet);
+	tasklet_kill(&local->tasklet);
But I'm not sure this is sufficient. I think we can then leak pending skbs.

I don't think so. tasklet_kill does actually waiting, not killing. It waits until the (maybe) scheduled tasklet did run and then waits for it to finish running.
It doesn't rip off a scheduled tasklet without letting it run.

Quoted 3 lines

In my tests I've often gotten that warning message about the skb queue not being empty (right before the crash). Or is that handled elsewhere now?

Uh, didn't get that. The assertion for this is in the same function some lines down.

Signature · 1 line

Greetings Michael.

Johannes Berg

maintainer

2007-03-08 15:43 UTC #3 lore raw

Quoted 4 lines · Michael Buesch

I don't think so. tasklet_kill does actually waiting, not killing. It waits until the (maybe) scheduled tasklet did run and then waits for it to finish running.
It doesn't rip off a scheduled tasklet without letting it run.

Ah ok.

Quoted 2 lines

Uh, didn't get that. The assertion for this is in the same function some lines down.

I used to get that but probably using tasklet_kill for that tasklet fixes this then.

johannes

Michael Buesch

author

2007-03-16 18:22 UTC #4 lore raw

In reply to Michael Buesch

Just want to make sure this crash fix patch is not lost, as I didn't see it showing up in some git tree, yet.

Quoted 24 lines · Michael Buesch

We need to do tasklet_kill() on any tasklet on unregister to make sure the tasklet is not running _and_ scheduled anymore. (tasklet_disable() only ensures it's not running anymore).
This fixes the tasklet related crash that was reported some time ago.
Signed-off-by Michael Buesch <[email protected]>
Index: bu3sch-wireless-dev/net/mac80211/ieee80211.c
===================================================================
net/mac80211/ieee80211.c +2 -2
@@ -4761,8 +4761,8 @@
 	struct ieee80211_sub_if_data *sdata, *tmp;
 	int i;
 
-	tasklet_disable(&local->tasklet);
-	/* TODO: skb_queue should be empty here, no need to do anything? */
+	tasklet_kill(&local->tx_pending_tasklet);
+	tasklet_kill(&local->tasklet);
 
 	rtnl_lock();
 	local->reg_state = IEEE80211_DEV_UNREGISTERED;

Signature · 1 line

Greetings Michael.

Jiri Benc

2007-03-23 17:17 UTC #5 lore raw

In reply to Michael Buesch

Quoted 13 lines · Michael Buesch

We need to do tasklet_kill() on any tasklet on unregister to make sure the tasklet is not running _and_ scheduled anymore. (tasklet_disable() only ensures it's not running anymore).
This fixes the tasklet related crash that was reported some time ago.
Signed-off-by Michael Buesch <[email protected]>
Index: bu3sch-wireless-dev/net/mac80211/ieee80211.c
===================================================================

net/mac80211/ieee80211.c line 4764

Quoted 4 lines

net/mac80211/ieee80211.c +0 -1

@@ -4761,8 +4761,8 @@
 	struct ieee80211_sub_if_data *sdata, *tmp;
 	int i;
 
-	tasklet_disable(&local->tasklet);

I think this tasklet_disable should not be removed.

net/mac80211/ieee80211.c line 4768

Quoted 6 lines

net/mac80211/ieee80211.c +2 -1

-	/* TODO: skb_queue should be empty here, no need to do anything? */
+	tasklet_kill(&local->tx_pending_tasklet);
+	tasklet_kill(&local->tasklet);
 
 	rtnl_lock();
 	local->reg_state = IEEE80211_DEV_UNREGISTERED;

Thanks,

Jiri

Signature · 2 lines

Jiri Benc
SUSE Labs

Michael Buesch

author

2007-03-23 17:23 UTC #6 lore raw

Quoted 17 lines · Jiri Benc

Quoted 16 lines · Michael Buesch
We need to do tasklet_kill() on any tasklet on unregister to make sure the tasklet is not running _and_ scheduled anymore. (tasklet_disable() only ensures it's not running anymore).
This fixes the tasklet related crash that was reported some time ago.
Signed-off-by Michael Buesch <[email protected]>
Index: bu3sch-wireless-dev/net/mac80211/ieee80211.c
===================================================================
net/mac80211/ieee80211.c +0 -0
@@ -4761,8 +4761,8 @@
 	struct ieee80211_sub_if_data *sdata, *tmp;
 	int i;
 

Quoted 3 lines

net/mac80211/ieee80211.c line 4764
Quoted 1 line
net/mac80211/ieee80211.c +0 -1
-	tasklet_disable(&local->tasklet);
I think this tasklet_disable should not be removed.

Why?...

Quoted 3 lines

net/mac80211/ieee80211.c line 4765
Quoted 3 lines
net/mac80211/ieee80211.c +2 -1
-	/* TODO: skb_queue should be empty here, no need to do anything? */
+	tasklet_kill(&local->tx_pending_tasklet);
+	tasklet_kill(&local->tasklet);

...We kill it here.

kill == (make sure it's not scheduled anymore) && disable

Signature · 1 line

Greetings Michael.

Jiri Benc

2007-03-23 17:32 UTC #7 lore raw

Quoted 1 line · Michael Buesch

kill == (make sure it's not scheduled anymore) && disable

Are you sure? I cannot find anything in tasklet_kill which disables the tasklet.

Thanks,

Jiri

Signature · 2 lines

Jiri Benc
SUSE Labs

Michael Buesch

author

2007-03-23 18:36 UTC #8 lore raw

Quoted 5 lines · Jiri Benc

Quoted 1 line · Michael Buesch
kill == (make sure it's not scheduled anymore) && disable
Are you sure? I cannot find anything in tasklet_kill which disables the tasklet.

It doesn't bounce the count, but it waits for the tasklet to finish and makes sure it's not scheduled anymore.
Why do you want to inc the count? There is no point in that.

disable does: Wait for it to finish running && inc the count

kill does: make sure it's not scheduled, wait for it to finish.

Why disable it? If there's anything scheduling the tasklet while we have unregistered we _want_ it to crash, as that's a real bug in the first place. (And I don't think it's possible to schedule it after unregister).

Signature · 1 line

Greetings Michael.

Jiri Benc

2007-03-23 20:16 UTC #9 lore raw

Quoted 4 lines · Michael Buesch

Why disable it? If there's anything scheduling the tasklet while we have unregistered we _want_ it to crash, as that's a real bug in the first place. (And I don't think it's possible to schedule it after unregister).

Hm, yes, sorry. Applied.

Thanks,

Jiri

Signature · 2 lines

Jiri Benc
SUSE Labs