Tearing down timers which have circular dependencies to other
functionality, e.g. workqueues, where the timer can schedule work and work
can arm timers, is not trivial.
In those cases it is desired to shutdown the timer in a way which prevents
rearming of the timer. The mechanism to do so is to set timer->function to
NULL and use this as an indicator for the timer arming functions to ignore
the (re)arm request.
In preparation for that replace the warnings in the relevant code paths
with checks for timer->function == NULL. If the pointer is NULLL, then
discard the rearm request silently.
Add debug_assert_init() instead of the WARN_ON_ONCE(!timer->function)
checks so that debug objects can warn about non-initialized timers.
The warning of debug objects does warn if timer->function == NULL. It
warns when timer was not initialized using timer_setup[_on_stack]() or via
DEFINE_TIMER(). If developers fail to enable debug objects and then waste
lots of time to figure out why their non-initialized timer is not firing,
they deserve it. Same for initializing a timer with a NULL function.
Co-developed-by: Steven Rostedt <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Tested-by: Guenter Roeck <[email protected]>
Reviewed-by: Jacob Keller <[email protected]>
Link: https://lore.kernel.org/all/[email protected]
Link: https://lore.kernel.org/all/[email protected]
---
V2: Use continue instead of return and amend the return value docs (Steven)
V3: Changelog and comment updates (Anna-Maria)
---
kernel/time/timer.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 52 insertions(+), 5 deletions(-)
--- 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,8 +1144,12 @@ 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
+ * * %0 - The timer was inactive and not modified or was is in
+ * shutdown state and the operation was discarded
* * %1 - The timer was active and requeued to expire at @expires
*/
int mod_timer_pending(struct timer_list *timer, unsigned long expires)
@@ -1155,8 +1175,12 @@ 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. In this case the return value is 0 and meaningless.
+ *
* Return:
- * * %0 - The timer was inactive and started
+ * * %0 - The timer was inactive and started or was in shutdown
+ * state and the operation was discarded
* * %1 - The timer was active and requeued to expire at @expires or
* the timer was active and not modified because @expires did
* not change the effective expiry time
@@ -1176,8 +1200,12 @@ 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
+ * * %0 - The timer was inactive and started or was in shutdown
+ * state and the operation was discarded
* * %1 - The timer was active and requeued to expire at @expires or
* the timer was active and not modified because @expires
* did not change the effective expiry time such that the
@@ -1200,6 +1228,9 @@ EXPORT_SYMBOL(timer_reduce);
* The @timer->expires and @timer->function fields must be set prior
* to calling this function.
*
+ * If @timer->function == NULL then the start operation is silently
+ * discarded.
+ *
* If @timer->expires is already in the past @timer will be queued to
* expire at the next timer tick.
*
@@ -1228,7 +1259,9 @@ void add_timer_on(struct timer_list *tim
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);
@@ -1239,6 +1272,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;
@@ -1252,6 +1292,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);
@@ -1541,6 +1582,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;
+ continue;
+ }
+
if (timer->flags & TIMER_IRQSAFE) {
raw_spin_unlock(&base->lock);
call_timer_fn(timer, fn, baseclk);
On Wed, 23 Nov 2022, Thomas Gleixner wrote:
> Tearing down timers which have circular dependencies to other
> functionality, e.g. workqueues, where the timer can schedule work and work
> can arm timers, is not trivial.
>
> In those cases it is desired to shutdown the timer in a way which prevents
> rearming of the timer. The mechanism to do so is to set timer->function to
> NULL and use this as an indicator for the timer arming functions to ignore
> the (re)arm request.
>
> In preparation for that replace the warnings in the relevant code paths
> with checks for timer->function == NULL. If the pointer is NULLL, then
s/NULLL/NULL
> discard the rearm request silently.
>
> Add debug_assert_init() instead of the WARN_ON_ONCE(!timer->function)
> checks so that debug objects can warn about non-initialized timers.
>
> The warning of debug objects does warn if timer->function == NULL. It
does NOT warn
> warns when timer was not initialized using timer_setup[_on_stack]() or via
> DEFINE_TIMER(). If developers fail to enable debug objects and then waste
> lots of time to figure out why their non-initialized timer is not firing,
> they deserve it. Same for initializing a timer with a NULL function.
>
> Co-developed-by: Steven Rostedt <[email protected]>
> Signed-off-by: Steven Rostedt <[email protected]>
> Signed-off-by: Thomas Gleixner <[email protected]>
> Tested-by: Guenter Roeck <[email protected]>
> Reviewed-by: Jacob Keller <[email protected]>
> Link: https://lore.kernel.org/all/[email protected]
> Link: https://lore.kernel.org/all/[email protected]
> ---
> V2: Use continue instead of return and amend the return value docs (Steven)
> V3: Changelog and comment updates (Anna-Maria)
> ---
> kernel/time/timer.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-----
> 1 file changed, 52 insertions(+), 5 deletions(-)
>
> --- a/kernel/time/timer.c
> +++ b/kernel/time/timer.c
> @@ -1128,8 +1144,12 @@ 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
> + * * %0 - The timer was inactive and not modified or was is in
> + * shutdown state and the operation was discarded
You forgot to update this "was is" mistake. All other places are fine.
> * * %1 - The timer was active and requeued to expire at @expires
> */
> int mod_timer_pending(struct timer_list *timer, unsigned long expires)
Thanks,
Anna-Maria
On Thu, Nov 24 2022 at 08:37, Anna-Maria Behnsen wrote:
> On Wed, 23 Nov 2022, Thomas Gleixner wrote:
>
>> Tearing down timers which have circular dependencies to other
>> functionality, e.g. workqueues, where the timer can schedule work and work
>> can arm timers, is not trivial.
>>
>> In those cases it is desired to shutdown the timer in a way which prevents
>> rearming of the timer. The mechanism to do so is to set timer->function to
>> NULL and use this as an indicator for the timer arming functions to ignore
>> the (re)arm request.
>>
>> In preparation for that replace the warnings in the relevant code paths
>> with checks for timer->function == NULL. If the pointer is NULLL, then
>
> s/NULLL/NULL
Bah. I should have went to the bar instead of trying to fix this.
Tearing down timers which have circular dependencies to other
functionality, e.g. workqueues, where the timer can schedule work and work
can arm timers, is not trivial.
In those cases it is desired to shutdown the timer in a way which prevents
rearming of the timer. The mechanism to do so is to set timer->function to
NULL and use this as an indicator for the timer arming functions to ignore
the (re)arm request.
In preparation for that replace the warnings in the relevant code paths
with checks for timer->function == NULL. If the pointer is NULL, then
discard the rearm request silently.
Add debug_assert_init() instead of the WARN_ON_ONCE(!timer->function)
checks so that debug objects can warn about non-initialized timers.
The warning of debug objects does not warn if timer->function == NULL. It
warns when timer was not initialized using timer_setup[_on_stack]() or via
DEFINE_TIMER(). If developers fail to enable debug objects and then waste
lots of time to figure out why their non-initialized timer is not firing,
they deserve it. Same for initializing a timer with a NULL function.
Co-developed-by: Steven Rostedt <[email protected]>
Signed-off-by: Steven Rostedt <[email protected]>
Signed-off-by: Thomas Gleixner <[email protected]>
Tested-by: Guenter Roeck <[email protected]>
Reviewed-by: Jacob Keller <[email protected]>
Link: https://lore.kernel.org/all/[email protected]
Link: https://lore.kernel.org/all/[email protected]
---
V2: Use continue instead of return and amend the return value docs (Steven)
V3: Changelog and comment updates (Anna-Maria)
V3.1: Fix it for real
---
kernel/time/timer.c | 57 +++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 52 insertions(+), 5 deletions(-)
--- 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,8 +1144,12 @@ 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
+ * * %0 - The timer was inactive and not modified or was in
+ * shutdown state and the operation was discarded
* * %1 - The timer was active and requeued to expire at @expires
*/
int mod_timer_pending(struct timer_list *timer, unsigned long expires)
@@ -1155,8 +1175,12 @@ 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. In this case the return value is 0 and meaningless.
+ *
* Return:
- * * %0 - The timer was inactive and started
+ * * %0 - The timer was inactive and started or was in shutdown
+ * state and the operation was discarded
* * %1 - The timer was active and requeued to expire at @expires or
* the timer was active and not modified because @expires did
* not change the effective expiry time
@@ -1176,8 +1200,12 @@ 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
+ * * %0 - The timer was inactive and started or was in shutdown
+ * state and the operation was discarded
* * %1 - The timer was active and requeued to expire at @expires or
* the timer was active and not modified because @expires
* did not change the effective expiry time such that the
@@ -1200,6 +1228,9 @@ EXPORT_SYMBOL(timer_reduce);
* The @timer->expires and @timer->function fields must be set prior
* to calling this function.
*
+ * If @timer->function == NULL then the start operation is silently
+ * discarded.
+ *
* If @timer->expires is already in the past @timer will be queued to
* expire at the next timer tick.
*
@@ -1228,7 +1259,9 @@ void add_timer_on(struct timer_list *tim
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);
@@ -1239,6 +1272,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;
@@ -1252,6 +1292,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);
@@ -1541,6 +1582,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;
+ continue;
+ }
+
if (timer->flags & TIMER_IRQSAFE) {
raw_spin_unlock(&base->lock);
call_timer_fn(timer, fn, baseclk);
This is an automated email and please do not reply to this email.
Dear Submitter,
Thank you for submitting the patches to the linux bluetooth mailing list.
While preparing the CI tests, the patches you submitted couldn't be applied to the current HEAD of the repository.
----- Output -----
error: patch failed: kernel/time/timer.c:1128
error: kernel/time/timer.c: patch does not apply
hint: Use 'git am --show-current-patch' to see the failed patch
Please resolve the issue and submit the patches again.
---
Regards,
Linux Bluetooth