Message ID | 20221115202117.560506554@linutronix.de |
---|---|
State | Superseded |
Headers | show |
Series | timers: Provide timer_shutdown[_sync]() | expand |
On Tue, 15 Nov 2022 21:28:49 +0100 (CET) Thomas Gleixner <tglx@linutronix.de> wrote: > @@ -1128,6 +1144,9 @@ static inline int > * mod_timer_pending() is the same for pending timers as mod_timer(), but > * will not activate inactive timers. > * > + * If @timer->function == NULL then the start operation is silently > + * discarded. > + * > * Return: > * * %0 - The timer was inactive and not modified > * * %1 - The timer was active and requeued to expire at @expires > @@ -1154,6 +1173,9 @@ EXPORT_SYMBOL(mod_timer_pending); > * same timer, then mod_timer() is the only safe way to modify the timeout, > * since add_timer() cannot modify an already running timer. > * > + * If @timer->function == NULL then the start operation is silently > + * discarded, the return value is 0 and meaningless. > + * > * Return: > * * %0 - The timer was inactive and started For those that only read the "Return" portion of kernel-doc, perhaps add here: "or the timer is in the shutdown state and was not started". > * * %1 - The timer was active and requeued to expire at @expires or > @@ -1175,6 +1197,9 @@ EXPORT_SYMBOL(mod_timer); > * modify an enqueued timer if that would reduce the expiration time. If > * @timer is not enqueued it starts the timer. > * > + * If @timer->function == NULL then the start operation is silently > + * discarded. > + * > * Return: > * * %0 - The timer was inactive and started > * * %1 - The timer was active and requeued to expire at @expires or > @@ -1201,6 +1226,9 @@ EXPORT_SYMBOL(timer_reduce); > * > * If @timer->expires is already in the past @timer will be queued to > * expire at the next timer tick. > + * > + * If @timer->function == NULL then the start operation is silently > + * discarded. > */ > void add_timer(struct timer_list *timer) > { > @@ -1217,13 +1245,18 @@ EXPORT_SYMBOL(add_timer); > * > * This can only operate on an inactive timer. Attempts to invoke this on > * an active timer are rejected with a warning. > + * > + * If @timer->function == NULL then the start operation is silently > + * discarded. > */ > void add_timer_on(struct timer_list *timer, int cpu) > { > struct timer_base *new_base, *base; > unsigned long flags; > > - if (WARN_ON_ONCE(timer_pending(timer) || !timer->function)) > + debug_assert_init(timer); > + > + if (WARN_ON_ONCE(timer_pending(timer))) > return; > > new_base = get_timer_cpu_base(timer->flags, cpu); > @@ -1234,6 +1267,13 @@ void add_timer_on(struct timer_list *tim > * wrong base locked. See lock_timer_base(). > */ > base = lock_timer_base(timer, &flags); > + /* > + * Has @timer been shutdown? This needs to be evaluated while > + * holding base lock to prevent a race against the shutdown code. > + */ > + if (!timer->function) > + goto out_unlock; > + > if (base != new_base) { > timer->flags |= TIMER_MIGRATING; > > @@ -1247,6 +1287,7 @@ void add_timer_on(struct timer_list *tim > > debug_timer_activate(timer); > internal_add_timer(base, timer); > +out_unlock: > raw_spin_unlock_irqrestore(&base->lock, flags); > } > EXPORT_SYMBOL_GPL(add_timer_on); > @@ -1532,6 +1573,12 @@ static void expire_timers(struct timer_b > > fn = timer->function; > > + if (WARN_ON_ONCE(!fn)) { > + /* Should never happen. Emphasis on should! */ > + base->running_timer = NULL; > + return; Why return and not continue? Wont this drop the other timers in the queue? -- Steve > + } > + > if (timer->flags & TIMER_IRQSAFE) { > raw_spin_unlock(&base->lock); > call_timer_fn(timer, fn, baseclk);
On Mon, Nov 21 2022 at 16:35, Steven Rostedt wrote: > On Tue, 15 Nov 2022 21:28:49 +0100 (CET) > Thomas Gleixner <tglx@linutronix.de> wrote: >> @@ -1532,6 +1573,12 @@ static void expire_timers(struct timer_b >> >> fn = timer->function; >> >> + if (WARN_ON_ONCE(!fn)) { >> + /* Should never happen. Emphasis on should! */ >> + base->running_timer = NULL; >> + return; > > Why return and not continue? > > Wont this drop the other timers in the queue? Duh. Yes. Thanks for catching that!
--- a/kernel/time/timer.c +++ b/kernel/time/timer.c @@ -1017,7 +1017,7 @@ static inline int unsigned int idx = UINT_MAX; int ret = 0; - BUG_ON(!timer->function); + debug_assert_init(timer); /* * This is a common optimization triggered by the networking code - if @@ -1044,6 +1044,14 @@ static inline int * dequeue/enqueue dance. */ base = lock_timer_base(timer, &flags); + /* + * Has @timer been shutdown? This needs to be evaluated + * while holding base lock to prevent a race against the + * shutdown code. + */ + if (!timer->function) + goto out_unlock; + forward_timer_base(base); if (timer_pending(timer) && (options & MOD_TIMER_REDUCE) && @@ -1070,6 +1078,14 @@ static inline int } } else { base = lock_timer_base(timer, &flags); + /* + * Has @timer been shutdown? This needs to be evaluated + * while holding base lock to prevent a race against the + * shutdown code. + */ + if (!timer->function) + goto out_unlock; + forward_timer_base(base); } @@ -1128,6 +1144,9 @@ static inline int * mod_timer_pending() is the same for pending timers as mod_timer(), but * will not activate inactive timers. * + * If @timer->function == NULL then the start operation is silently + * discarded. + * * Return: * * %0 - The timer was inactive and not modified * * %1 - The timer was active and requeued to expire at @expires @@ -1154,6 +1173,9 @@ EXPORT_SYMBOL(mod_timer_pending); * same timer, then mod_timer() is the only safe way to modify the timeout, * since add_timer() cannot modify an already running timer. * + * If @timer->function == NULL then the start operation is silently + * discarded, the return value is 0 and meaningless. + * * Return: * * %0 - The timer was inactive and started * * %1 - The timer was active and requeued to expire at @expires or @@ -1175,6 +1197,9 @@ EXPORT_SYMBOL(mod_timer); * modify an enqueued timer if that would reduce the expiration time. If * @timer is not enqueued it starts the timer. * + * If @timer->function == NULL then the start operation is silently + * discarded. + * * Return: * * %0 - The timer was inactive and started * * %1 - The timer was active and requeued to expire at @expires or @@ -1201,6 +1226,9 @@ EXPORT_SYMBOL(timer_reduce); * * If @timer->expires is already in the past @timer will be queued to * expire at the next timer tick. + * + * If @timer->function == NULL then the start operation is silently + * discarded. */ void add_timer(struct timer_list *timer) { @@ -1217,13 +1245,18 @@ EXPORT_SYMBOL(add_timer); * * This can only operate on an inactive timer. Attempts to invoke this on * an active timer are rejected with a warning. + * + * If @timer->function == NULL then the start operation is silently + * discarded. */ void add_timer_on(struct timer_list *timer, int cpu) { struct timer_base *new_base, *base; unsigned long flags; - if (WARN_ON_ONCE(timer_pending(timer) || !timer->function)) + debug_assert_init(timer); + + if (WARN_ON_ONCE(timer_pending(timer))) return; new_base = get_timer_cpu_base(timer->flags, cpu); @@ -1234,6 +1267,13 @@ void add_timer_on(struct timer_list *tim * wrong base locked. See lock_timer_base(). */ base = lock_timer_base(timer, &flags); + /* + * Has @timer been shutdown? This needs to be evaluated while + * holding base lock to prevent a race against the shutdown code. + */ + if (!timer->function) + goto out_unlock; + if (base != new_base) { timer->flags |= TIMER_MIGRATING; @@ -1247,6 +1287,7 @@ void add_timer_on(struct timer_list *tim debug_timer_activate(timer); internal_add_timer(base, timer); +out_unlock: raw_spin_unlock_irqrestore(&base->lock, flags); } EXPORT_SYMBOL_GPL(add_timer_on); @@ -1532,6 +1573,12 @@ static void expire_timers(struct timer_b fn = timer->function; + if (WARN_ON_ONCE(!fn)) { + /* Should never happen. Emphasis on should! */ + base->running_timer = NULL; + return; + } + if (timer->flags & TIMER_IRQSAFE) { raw_spin_unlock(&base->lock); call_timer_fn(timer, fn, baseclk);