diff mbox series

[V2,13/17] timers: Split [try_to_]del_timer[_sync]() to prepare for shutdown mode

Message ID 20221122173648.849454220@linutronix.de (mailing list archive)
State Superseded
Headers show
Series timers: Provide timer_shutdown[_sync]() | expand

Checks

Context Check Description
tedd_an/pre-ci_am success Success
tedd_an/CheckPatch success CheckPatch PASS
tedd_an/GitLint fail 1: T1 Title exceeds max length (81>80): "[V2,13/17] timers: Split [try_to_]del_timer[_sync]() to prepare for shutdown mode"
tedd_an/SubjectPrefix fail "Bluetooth: " prefix is not specified in the subject
tedd_an/IncrementalBuild success Incremental Build PASS

Commit Message

Thomas Gleixner Nov. 22, 2022, 5:45 p.m. UTC
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 it to set timer->function to
NULL and use this as an indicator for the timer arming functions to ignore
the (re)arm request.

Split the inner workings of try_do_del_timer_sync(), del_timer_sync() and
del_timer() into helper functions to prepare for implementing the shutdown
functionality.

No functional change.

Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home
Link: https://lore.kernel.org/all/20221110064101.429013735@goodmis.org
---
 kernel/time/timer.c |  143 +++++++++++++++++++++++++++++++++-------------------
 1 file changed, 92 insertions(+), 51 deletions(-)

Comments

Jacob Keller Nov. 22, 2022, 11:04 p.m. UTC | #1
On 11/22/2022 9:45 AM, Thomas Gleixner wrote:
> +/**
> + * try_to_del_timer_sync - Try to deactivate a timer
> + * @timer:	Timer to deactivate
> + *
> + * This function tries to deactivate a timer. On success the timer is not
> + * queued and the timer callback function is not running on any CPU.
> + *
> + * This function does not guarantee that the timer cannot be rearmed right
> + * after dropping the base lock. That needs to be prevented by the calling
> + * code if necessary.
> + *
> + * Return:
> + * * %0  - The timer was not pending
> + * * %1  - The timer was pending and deactivated
> + * * %-1 - The timer callback function is running on a different CPU
> + */
> +int try_to_del_timer_sync(struct timer_list *timer)
> +{
> +	return __try_to_del_timer_sync(timer);
> +}
>   EXPORT_SYMBOL(try_to_del_timer_sync);
>   


Its a bit odd to me that some patches refactor and replace functions 
with new variants all under timer_* namespace, but then we've left some 
of them available without that.

Any reasoning behind this? I guess "try_*" is pretty clear and unlikely 
to get stolen by other code..?

Thanks,
Jake
Anna-Maria Behnsen Nov. 23, 2022, 11:23 a.m. UTC | #2
On Tue, 22 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.

Comma missing (same as in previous commit message)

> In those cases it is desired to shutdown the timer in a way which prevents
> rearming of the timer. The mechanism to do so it to set timer->function to

s/it/is (same as in previous commit message)


Thanks,

	Anna-Maria
Anna-Maria Behnsen Nov. 23, 2022, 11:24 a.m. UTC | #3
On Wed, 23 Nov 2022, Anna-Maria Behnsen wrote:

> On Tue, 22 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.
> 
> Comma missing (same as in previous commit message)
> 
> > In those cases it is desired to shutdown the timer in a way which prevents
> > rearming of the timer. The mechanism to do so it to set timer->function to
> 
> s/it/is (same as in previous commit message)
> 
> 

Same for patch 14 and 15...
Thomas Gleixner Nov. 23, 2022, 5:05 p.m. UTC | #4
On Tue, Nov 22 2022 at 15:04, Jacob Keller wrote:
> On 11/22/2022 9:45 AM, Thomas Gleixner wrote:
>> +int try_to_del_timer_sync(struct timer_list *timer)
>> +{
>> +	return __try_to_del_timer_sync(timer);
>> +}
>>   EXPORT_SYMBOL(try_to_del_timer_sync);
>>   
>
>
> Its a bit odd to me that some patches refactor and replace functions 
> with new variants all under timer_* namespace, but then we've left some 
> of them available without that.
>
> Any reasoning behind this? I guess "try_*" is pretty clear and unlikely 
> to get stolen by other code..?

Kinda. I renamed del_timer*() because that's the ones which we want to
substitute with timer_shutdown*() where possible and reasonable.

A larger timer namespace cleanup is subject to a follow up series.

Thanks,

        tglx
Jacob Keller Nov. 23, 2022, 6:45 p.m. UTC | #5
On 11/23/2022 9:05 AM, Thomas Gleixner wrote:
> On Tue, Nov 22 2022 at 15:04, Jacob Keller wrote:
>> On 11/22/2022 9:45 AM, Thomas Gleixner wrote:
>>> +int try_to_del_timer_sync(struct timer_list *timer)
>>> +{
>>> +	return __try_to_del_timer_sync(timer);
>>> +}
>>>    EXPORT_SYMBOL(try_to_del_timer_sync);
>>>    
>>
>>
>> Its a bit odd to me that some patches refactor and replace functions
>> with new variants all under timer_* namespace, but then we've left some
>> of them available without that.
>>
>> Any reasoning behind this? I guess "try_*" is pretty clear and unlikely
>> to get stolen by other code..?
> 
> Kinda. I renamed del_timer*() because that's the ones which we want to
> substitute with timer_shutdown*() where possible and reasonable.
> 
> A larger timer namespace cleanup is subject to a follow up series.
> 
> Thanks,
> 
>          tglx

Yep thats what I figured once I got to the end of the series. Thanks!

Regards,
Jake
diff mbox series

Patch

--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1297,20 +1297,14 @@  void add_timer_on(struct timer_list *tim
 EXPORT_SYMBOL_GPL(add_timer_on);
 
 /**
- * timer_delete - Deactivate a timer.
+ * __timer_delete - Internal function: Deactivate a timer.
  * @timer:	The timer to be deactivated
  *
- * The function only deactivates a pending timer, but contrary to
- * timer_delete_sync() it does not take into account whether the timers
- * callback function is concurrently executed on a different CPU or not.
- * It neither prevents rearming of the timer.  If @timer can be rearmed
- * concurrently then the return value of this function is meaningless.
- *
  * Return:
  * * %0 - The timer was not pending
  * * %1 - The timer was pending and deactivated
  */
-int timer_delete(struct timer_list *timer)
+static int __timer_delete(struct timer_list *timer)
 {
 	struct timer_base *base;
 	unsigned long flags;
@@ -1326,25 +1320,37 @@  int timer_delete(struct timer_list *time
 
 	return ret;
 }
-EXPORT_SYMBOL(timer_delete);
 
 /**
- * try_to_del_timer_sync - Try to deactivate a timer
- * @timer:	Timer to deactivate
+ * timer_delete - Deactivate a timer.
+ * @timer:	The timer to be deactivated
  *
- * This function tries to deactivate a timer. On success the timer is not
- * queued and the timer callback function is not running on any CPU.
+ * The function only deactivates a pending timer, but contrary to
+ * timer_delete_sync() it does not take into account whether the timers
+ * callback function is concurrently executed on a different CPU or not.
+ * It neither prevents rearming of the timer.  If @timer can be rearmed
+ * concurrently then the return value of this function is meaningless.
  *
- * This function does not guarantee that the timer cannot be rearmed right
- * after dropping the base lock. That needs to be prevented by the calling
- * code if necessary.
+ * Return:
+ * * %0 - The timer was not pending
+ * * %1 - The timer was pending and deactivated
+ */
+int timer_delete(struct timer_list *timer)
+{
+	return __timer_delete(timer);
+}
+EXPORT_SYMBOL(timer_delete);
+
+/**
+ * __try_to_del_timer_sync - Internal function: Try to deactivate a timer
+ * @timer:	Timer to deactivate
  *
  * Return:
  * * %0  - The timer was not pending
  * * %1  - The timer was pending and deactivated
  * * %-1 - The timer callback function is running on a different CPU
  */
-int try_to_del_timer_sync(struct timer_list *timer)
+static int __try_to_del_timer_sync(struct timer_list *timer)
 {
 	struct timer_base *base;
 	unsigned long flags;
@@ -1361,6 +1367,27 @@  int try_to_del_timer_sync(struct timer_l
 
 	return ret;
 }
+
+/**
+ * try_to_del_timer_sync - Try to deactivate a timer
+ * @timer:	Timer to deactivate
+ *
+ * This function tries to deactivate a timer. On success the timer is not
+ * queued and the timer callback function is not running on any CPU.
+ *
+ * This function does not guarantee that the timer cannot be rearmed right
+ * after dropping the base lock. That needs to be prevented by the calling
+ * code if necessary.
+ *
+ * Return:
+ * * %0  - The timer was not pending
+ * * %1  - The timer was pending and deactivated
+ * * %-1 - The timer callback function is running on a different CPU
+ */
+int try_to_del_timer_sync(struct timer_list *timer)
+{
+	return __try_to_del_timer_sync(timer);
+}
 EXPORT_SYMBOL(try_to_del_timer_sync);
 
 #ifdef CONFIG_PREEMPT_RT
@@ -1437,45 +1464,15 @@  static inline void del_timer_wait_runnin
 #endif
 
 /**
- * timer_delete_sync - Deactivate a timer and wait for the handler to finish.
+ * __timer_delete_sync - Internal function: Deactivate a timer and wait
+ *			 for the handler to finish.
  * @timer:	The timer to be deactivated
  *
- * Synchronization rules: Callers must prevent restarting of the timer,
- * otherwise this function is meaningless. It must not be called from
- * interrupt contexts unless the timer is an irqsafe one. The caller must
- * not hold locks which would prevent completion of the timer's callback
- * function. The timer's handler must not call add_timer_on(). Upon exit
- * the timer is not queued and the handler is not running on any CPU.
- *
- * For !irqsafe timers, the caller must not hold locks that are held in
- * interrupt context. Even if the lock has nothing to do with the timer in
- * question.  Here's why::
- *
- *    CPU0                             CPU1
- *    ----                             ----
- *                                     <SOFTIRQ>
- *                                       call_timer_fn();
- *                                       base->running_timer = mytimer;
- *    spin_lock_irq(somelock);
- *                                     <IRQ>
- *                                        spin_lock(somelock);
- *    timer_delete_sync(mytimer);
- *    while (base->running_timer == mytimer);
- *
- * Now timer_delete_sync() will never return and never release somelock.
- * The interrupt on the other CPU is waiting to grab somelock but it has
- * interrupted the softirq that CPU0 is waiting to finish.
- *
- * This function cannot guarantee that the timer is not rearmed again by
- * some concurrent or preempting code, right after it dropped the base
- * lock. If there is the possibility of a concurrent rearm then the return
- * value of the function is meaningless.
- *
  * Return:
  * * %0	- The timer was not pending
  * * %1	- The timer was pending and deactivated
  */
-int timer_delete_sync(struct timer_list *timer)
+static int __timer_delete_sync(struct timer_list *timer)
 {
 	int ret;
 
@@ -1505,7 +1502,7 @@  int timer_delete_sync(struct timer_list
 		lockdep_assert_preemption_enabled();
 
 	do {
-		ret = try_to_del_timer_sync(timer);
+		ret = __try_to_del_timer_sync(timer);
 
 		if (unlikely(ret < 0)) {
 			del_timer_wait_running(timer);
@@ -1515,6 +1512,50 @@  int timer_delete_sync(struct timer_list
 
 	return ret;
 }
+
+/**
+ * timer_delete_sync - Deactivate a timer and wait for the handler to finish.
+ * @timer:	The timer to be deactivated
+ *
+ * Synchronization rules: Callers must prevent restarting of the timer,
+ * otherwise this function is meaningless. It must not be called from
+ * interrupt contexts unless the timer is an irqsafe one. The caller must
+ * not hold locks which would prevent completion of the timer's callback
+ * function. The timer's handler must not call add_timer_on(). Upon exit
+ * the timer is not queued and the handler is not running on any CPU.
+ *
+ * For !irqsafe timers, the caller must not hold locks that are held in
+ * interrupt context. Even if the lock has nothing to do with the timer in
+ * question.  Here's why::
+ *
+ *    CPU0                             CPU1
+ *    ----                             ----
+ *                                     <SOFTIRQ>
+ *                                       call_timer_fn();
+ *                                       base->running_timer = mytimer;
+ *    spin_lock_irq(somelock);
+ *                                     <IRQ>
+ *                                        spin_lock(somelock);
+ *    timer_delete_sync(mytimer);
+ *    while (base->running_timer == mytimer);
+ *
+ * Now timer_delete_sync() will never return and never release somelock.
+ * The interrupt on the other CPU is waiting to grab somelock but it has
+ * interrupted the softirq that CPU0 is waiting to finish.
+ *
+ * This function cannot guarantee that the timer is not rearmed again by
+ * some concurrent or preempting code, right after it dropped the base
+ * lock. If there is the possibility of a concurrent rearm then the return
+ * value of the function is meaningless.
+ *
+ * Return:
+ * * %0	- The timer was not pending
+ * * %1	- The timer was pending and deactivated
+ */
+int timer_delete_sync(struct timer_list *timer)
+{
+	return __timer_delete_sync(timer);
+}
 EXPORT_SYMBOL(timer_delete_sync);
 
 static void call_timer_fn(struct timer_list *timer,