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
===================================================================
--- bu3sch-wireless-dev.orig/net/mac80211/ieee80211.c 2007-03-06 15:55:53.000000000 +0100
+++ bu3sch-wireless-dev/net/mac80211/ieee80211.c 2007-03-06 15:58:10.000000000 +0100
@@ -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;
--
Greetings Michael.
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.
On Tuesday 06 March 2007 19:01, Michael Buesch wrote:
> 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
> ===================================================================
> --- bu3sch-wireless-dev.orig/net/mac80211/ieee80211.c 2007-03-06 15:55:53.000000000 +0100
> +++ bu3sch-wireless-dev/net/mac80211/ieee80211.c 2007-03-06 15:58:10.000000000 +0100
> @@ -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;
>
--
Greetings Michael.
On Tue, 2007-03-06 at 19:01 +0100, Michael Buesch wrote:
> This fixes the tasklet related crash that was reported some time ago.
I agree with that :)
> - 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
On Fri, 23 Mar 2007 18:22:50 +0100, Michael Buesch wrote:
> 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
--
Jiri Benc
SUSE Labs
On Thursday 08 March 2007 02:26, Johannes Berg wrote:
> On Tue, 2007-03-06 at 19:01 +0100, Michael Buesch wrote:
>
> > This fixes the tasklet related crash that was reported some time ago.
>
> I agree with that :)
>
> > - 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.
> 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.
--
Greetings Michael.
On Thu, 2007-03-08 at 16:39 +0100, Michael Buesch wrote:
> 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.
> 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
On Friday 23 March 2007 18:32, Jiri Benc wrote:
> On Fri, 23 Mar 2007 18:22:50 +0100, Michael Buesch wrote:
> > 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).
--
Greetings Michael.
On Fri, 23 Mar 2007 19:36:42 +0100, Michael Buesch wrote:
> 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
--
Jiri Benc
SUSE Labs
On Tue, 6 Mar 2007 19:01:59 +0100, Michael Buesch wrote:
> 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
> ===================================================================
> --- bu3sch-wireless-dev.orig/net/mac80211/ieee80211.c 2007-03-06 15:55:53.000000000 +0100
> +++ bu3sch-wireless-dev/net/mac80211/ieee80211.c 2007-03-06 15:58:10.000000000 +0100
> @@ -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.
> - /* 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
--
Jiri Benc
SUSE Labs
On Friday 23 March 2007 18:17, Jiri Benc wrote:
> On Tue, 6 Mar 2007 19:01:59 +0100, Michael Buesch wrote:
> > 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
> > ===================================================================
> > --- bu3sch-wireless-dev.orig/net/mac80211/ieee80211.c 2007-03-06 15:55:53.000000000 +0100
> > +++ bu3sch-wireless-dev/net/mac80211/ieee80211.c 2007-03-06 15:58:10.000000000 +0100
> > @@ -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.
Why?...
> > - /* 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
--
Greetings Michael.