[PATCH] timers: fix synchronization rules in comments of del_timer_sync

[PATCH] timers: fix synchronization rules in comments of del_timer_sync

[Duoming Zhou]

The del_timer_sync() could stop the timer that restart itself
in the timer's handler. So the synchronization rules should be
changed to "Callers must prevent restarting of the timer in
other places except for the timer's handler".

The root cause is shown below which is a part of code in
del_timer_sync:

	do {
		ret = try_to_del_timer_sync(timer);

		if (unlikely(ret < 0)) {
			del_timer_wait_running(timer);
			cpu_relax();
		}
	} while (ret < 0);

If the timer's handler is running, the try_to_del_timer_sync will
return -1. Then, it will loop until the timer is not queued and
the timer's handler is not running on any CPU.

Although the timer may restart itself in timer's handler, the
del_timer_sync could also stop it.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Duoming Zhou <duoming@zju.edu.cn>
---
 kernel/time/timer.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 717fcb9fb14..823e45c1235 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1374,12 +1374,13 @@ static inline void del_timer_wait_running(struct timer_list *timer) { }
  * 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. 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
- * handler. 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.
+ * Synchronization rules: Callers must prevent restarting of the timer in
+ * other places except for the timer's handler, 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 handler. 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.
  *
  * Note: For !irqsafe timers, you must not hold locks that are held in
  *   interrupt context while calling this function. Even if the lock has
-- 
2.17.1

Re: [PATCH] timers: fix synchronization rules in comments of del_timer_sync

[duoming]

Hello maintainers,

In order to further prove the del_timer_sync() could stop the timer that
restart itself in its timer handler, I wrote the following kernel module
whoes part of code is shown below:

=================================================================

struct timer_list my_timer;
static void my_timer_callback(struct timer_list *timer);
static void start_timer(void);

static void start_timer(void){
    del_timer(&my_timer);
    my_timer.expires = jiffies+HZ;
    my_timer.function = my_timer_callback;
    add_timer(&my_timer);
}

static void my_timer_callback(struct timer_list *timer){
    printk("In my_timer_function");
    printk("the jiffies is %ld\n",jiffies);
    start_timer();
}

static int __init del_timer_sync_init(void)
{
    int result;
    printk("my_timer will be create.\n");
    printk("the jiffies is :%ld\n", jiffies);
    timer_setup(&my_timer,my_timer_callback,0);
    result = mod_timer(&my_timer,jiffies + SIXP_TXDELAY);
    printk("the mod_timer is :%d\n\n",result);
    return 0;
}

static void __exit del_timer_sync_exit(void)
{
    int result=del_timer_sync(&my_timer);
    printk("the del_timer_sync is :%d\n\n", result);
}

=================================================================

The timer handler is running from interrupts and del_timer_sync() could stop
the timer that rewind itself in its timer handler, the result is shown below:

# insmod del_timer_sync.ko 
[  103.505857] my_timer will be create.
[  103.505922] the jiffies is :4294770832
[  103.506845] the mod_timer is :0
[  103.506845] 
# [  103.532389] In my_timer_function
[  103.532452] the jiffies is 4294770859
[  104.576768] In my_timer_function
[  104.577096] the jiffies is 4294771904
[  105.600941] In my_timer_function
[  105.601072] the jiffies is 4294772928
[  106.625397] In my_timer_function
[  106.625573] the jiffies is 4294773952
[  107.648995] In my_timer_function
[  107.649212] the jiffies is 4294774976
[  108.673037] In my_timer_function
[  108.673787] the jiffies is 4294776001
rmmod del_timer_sync.ko
[  109.649482] the del_timer_sync is :1
[  109.649482] 
# 

The root cause is shown below:

	do {
		ret = try_to_del_timer_sync(timer);

		if (unlikely(ret < 0)) {
			del_timer_wait_running(timer);
			cpu_relax();
		}
	} while (ret < 0);

https://elixir.bootlin.com/linux/latest/source/kernel/time/timer.c#L1381

If we call another thread such as a work_queue or the code in other places
to restart the timer instead of in its timer handler, the del_timer_sync()
could not stop it. So, I think the comments should be changed to "Callers
must prevent restarting of the timer in other places except for the timer's
handler".

Best regards,
Duoming Zhou

Re: [PATCH] timers: fix synchronization rules in comments of del_timer_sync

[Thomas Gleixner]

On Fri, Jul 01 2022 at 16:55, Duoming Zhou wrote:
> - * 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
> - * handler. 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.
> + * Synchronization rules: Callers must prevent restarting of the timer in
> + * other places except for the timer's handler, 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 handler. The timer's handler must
> + * not call add_timer_on().

If we are making this correct: What's so special about add_timer_on()?

Thanks,

        tglx

Re: [PATCH] timers: fix synchronization rules in comments of del_timer_sync

[duoming]

Hello,

On Mon, 08 Aug 2022 16:01:57 +0200 Thomas Gleixner wrote:

> On Fri, Jul 01 2022 at 16:55, Duoming Zhou wrote:
> > - * 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
> > - * handler. 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.
> > + * Synchronization rules: Callers must prevent restarting of the timer in
> > + * other places except for the timer's handler, 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 handler. The timer's handler must
> > + * not call add_timer_on().
> 
> If we are making this correct: What's so special about add_timer_on()?

The del_timer_sync() could also stop add_timer_on(), there is nothing special
about add_timer_on(). I think change the annotation to the following is better.

diff --git a/kernel/time/timer.c b/kernel/time/timer.c
index 717fcb9fb14..dd623018dbc 100644
--- a/kernel/time/timer.c
+++ b/kernel/time/timer.c
@@ -1375,11 +1375,11 @@ static inline void del_timer_wait_running(struct timer_list *timer) { }
  * CPUs.
  *
  * 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
- * handler. 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.
+ * otherwise this function is meaningless. It could also stop periodic timer.
+ * 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 handler. Upon exit the timer is not queued and the handler is not
+ * running on any CPU.

Best regards,
Duoming Zhou