| Message ID | 1383617296-20273-1-git-send-email-subramanian.vijay@gmail.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Vijay Subramanian <subramanian.vijay@gmail.com> Date: Mon, 4 Nov 2013 18:08:16 -0800 > Make sure timer does not fire once qdisc is destroyed. > > Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com> I do not understand how this problem can trigger. del_timer_sync() waits for all instances of the timer handler which are running to complete, and then it deletes the timer from any of the scheduled timer lists. Your patch is making the mod_timer() in the timer handler conditional, and this really shouldn't be necessary. The whole point of del_timer_sync() is to address these kinds of situations. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> del_timer_sync() waits for all instances of the timer handler which > are running to complete, and then it deletes the timer from any of > the scheduled timer lists. > > Your patch is making the mod_timer() in the timer handler conditional, > and this really shouldn't be necessary. > > The whole point of del_timer_sync() is to address these kinds of > situations. Thanks for the explanation Dave. I am trying to reconcile your explanation with the text above del_timer_sync() vim +1013 kernel/timer.c " * This function only differs from del_timer() on SMP: besides deactivating * the timer it also makes sure the handler has finished executing on other * CPUs. * * Synchronization rules: Callers must prevent restarting of the timer, * otherwise this function is meaningless. ..." This seems to suggest simply calling del_timer_sync() is not enough. In fact, for the same reason I was recently pointed towards commit 980c478ddbb72 (sch_sfq: use del_timer_sync() in sfq_destroy() ) and I based the patch on how q->perturb_period is reset in sfq_destroy() and used in sfq_perturbation(). Based on your explanation , I think we do not need (in sched/sch_sfq.c) q->perturb_period = 0; in sfq_destroy() and just del_timer_sync() should be enough. Thanks, Vijay -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sched/sch_red.c b/net/sched/sch_red.c index 633e32d..380507e 100644 --- a/net/sched/sch_red.c +++ b/net/sched/sch_red.c @@ -39,6 +39,7 @@ struct red_sched_data { u32 limit; /* HARD maximal queue length */ unsigned char flags; + bool timeron; /* to prevent race on destroy*/ struct timer_list adapt_timer; struct red_parms parms; struct red_vars vars; @@ -166,6 +167,7 @@ static void red_destroy(struct Qdisc *sch) { struct red_sched_data *q = qdisc_priv(sch); + q->timeron = false; del_timer_sync(&q->adapt_timer); qdisc_destroy(q->qdisc); } @@ -241,7 +243,8 @@ static inline void red_adaptative_timer(unsigned long arg) spin_lock(root_lock); red_adaptative_algo(&q->parms, &q->vars); - mod_timer(&q->adapt_timer, jiffies + HZ/2); + if (q->timeron) + mod_timer(&q->adapt_timer, jiffies + HZ/2); spin_unlock(root_lock); } @@ -250,6 +253,7 @@ static int red_init(struct Qdisc *sch, struct nlattr *opt) struct red_sched_data *q = qdisc_priv(sch); q->qdisc = &noop_qdisc; + q->timeron = true; setup_timer(&q->adapt_timer, red_adaptative_timer, (unsigned long)sch); return red_change(sch, opt); }
Make sure timer does not fire once qdisc is destroyed. Signed-off-by: Vijay Subramanian <subramanian.vijay@gmail.com> --- net/sched/sch_red.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)