From: Thomas Gleixner <hidden> Date: 2022-11-22 17:44:52
This is the second version of the timer shutdown work. The first version
can be found here:
https://lore.kernel.org/all/20221115195802.415956561@linutronix.de
Tearing down timers can be tedious when there are circular dependencies to
other things which need to be torn down. A prime example is timer and
workqueue where the timer schedules work and the work arms the timer.
Steven and the Google Chromebook team ran into such an issue in the
Bluetooth HCI code.
Steven suggested to create a new function del_timer_free() which marks the
timer as shutdown. Rearm attempts of shutdown timers are discarded and he
wanted to emit a warning for that case:
https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home
This resulted in a lengthy discussion and suggestions how this should be
implemented. The patch series went through several iterations and during
the review of the last version it turned out that this approach is
suboptimal:
https://lore.kernel.org/all/20221110064101.429013735@goodmis.org
The warning is not really helpful because it's entirely unclear how it
should be acted upon. The only way to address such a case is to add 'if
(in_shutdown)' conditionals all over the place. This is error prone and in
most cases of teardown like the HCI one which started this discussion not
required all.
What needs to prevented is that pending work which is drained via
destroy_workqueue() does not rearm the previously shutdown timer. Nothing
in that shutdown sequence relies on the timer being functional.
The conclusion was that the semantics of timer_shutdown_sync() should be:
- timer is not enqueued
- timer callback is not running
- timer cannot be rearmed
Preventing the rearming of shutdown timers is done by discarding rearm
attempts silently.
As Steven is short of cycles, I made some spare cycles available and
reworked the patch series to follow the new semantics and plugged the races
which were discovered during review.
The patches have been split up into small pieces to make review easier and
I took the liberty to throw a bunch of overdue cleanups into the picture
instead of proliferating the existing state further.
The last patch in the series addresses the HCI teardown issue for real.
The series is also available from git:
git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git timers
Changes vs. V1:
- Fixed the return vs. continue bug in the timer expiration code (Steven)
- Addressed the review vs. function documentation (Steven)
- Fixed up the del_timer*() references in documentation (Steven)
- Split out the 'remove bogus claims about del_timer_sync()' change
- Picked up Reviewed/Tested-by tags where appropriate
Thanks,
tglx
---
Documentation/RCU/Design/Requirements/Requirements.rst | 2
Documentation/core-api/local_ops.rst | 2
Documentation/kernel-hacking/locking.rst | 17
Documentation/timers/hrtimers.rst | 2
Documentation/translations/it_IT/kernel-hacking/locking.rst | 14
Documentation/translations/zh_CN/core-api/local_ops.rst | 2
arch/arm/mach-spear/time.c | 8
drivers/bluetooth/hci_qca.c | 10
drivers/char/tpm/tpm-dev-common.c | 4
drivers/clocksource/arm_arch_timer.c | 12
drivers/clocksource/timer-sp804.c | 6
drivers/staging/wlan-ng/hfa384x_usb.c | 4
drivers/staging/wlan-ng/prism2usb.c | 6
include/linux/timer.h | 35
kernel/time/timer.c | 424 +++++++++---
net/sunrpc/xprt.c | 2
16 files changed, 404 insertions(+), 146 deletions(-)
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:45:00
del_timer_sync() does not return the number of times it tried to delete the
timer which rearms itself. It's clearly documented:
The function returns whether it has deactivated a pending timer or not.
This part of the documentation is from 2003 where del_timer_sync() really
returned the number of deletion attempts for unknown reasons. This code
was rewritten in 2005, but the documentation was not updated.
Signed-off-by: Thomas Gleixner <redacted>
---
Documentation/kernel-hacking/locking.rst | 3 +--
Documentation/translations/it_IT/kernel-hacking/locking.rst | 4 +---
2 files changed, 2 insertions(+), 5 deletions(-)
@@ -1006,8 +1006,7 @@ Another common problem is deleting timer calling add_timer() at the end of their timer function). Because this is a fairly common case which is prone to races, you should use del_timer_sync() (``include/linux/timer.h``) to-handle this case. It returns the number of times the timer had to be-deleted before we finally stopped it from adding itself back in.+handle this case. Locking Speed =============--- a/Documentation/translations/it_IT/kernel-hacking/locking.rst+++ b/Documentation/translations/it_IT/kernel-hacking/locking.rst
@@ -1027,9 +1027,7 @@ Un altro problema è l'eliminazione dei da soli (chiamando add_timer() alla fine della loro esecuzione). Dato che questo è un problema abbastanza comune con una propensione alle corse critiche, dovreste usare del_timer_sync()-(``include/linux/timer.h``) per gestire questo caso. Questa ritorna il-numero di volte che il temporizzatore è stato interrotto prima che-fosse in grado di fermarlo senza che si riavviasse.+(``include/linux/timer.h``) per gestire questo caso. Velocità della sincronizzazione ===============================
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:45:03
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
A new "shutdown" timer state is being added to the generic timer code. One
of the functions to change the timer into the state is called
"timer_shutdown()". This means that there can not be other functions called
"timer_shutdown()" as the timer code owns the "timer_*" name space.
Rename timer_shutdown() to spear_timer_shutdown() to avoid this conflict.
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Link: https://lkml.kernel.org/r/20221106212701.822440504@goodmis.org
Link: https://lore.kernel.org/all/20221105060155.228348078@goodmis.org/
Link: https://lore.kernel.org/r/20221110064146.810953418@goodmis.org
---
arch/arm/mach-spear/time.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:45:05
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
A new "shutdown" timer state is being added to the generic timer code. One
of the functions to change the timer into the state is called
"timer_shutdown()". This means that there can not be other functions
called "timer_shutdown()" as the timer code owns the "timer_*" name space.
Rename timer_shutdown() to arch_timer_shutdown() to avoid this conflict.
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Marc Zyngier <maz@kernel.org>
Link: https://lkml.kernel.org/r/20221106212702.002251651@goodmis.org
Link: https://lore.kernel.org/all/20221105060155.409832154@goodmis.org/
Link: https://lore.kernel.org/r/20221110064146.981725531@goodmis.org
---
drivers/clocksource/arm_arch_timer.c | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)
@@ -701,22 +701,22 @@ static __always_inline int timer_shutdown(const int access,staticintarch_timer_shutdown_virt(structclock_event_device*clk){-returntimer_shutdown(ARCH_TIMER_VIRT_ACCESS,clk);+returnarch_timer_shutdown(ARCH_TIMER_VIRT_ACCESS,clk);}staticintarch_timer_shutdown_phys(structclock_event_device*clk){-returntimer_shutdown(ARCH_TIMER_PHYS_ACCESS,clk);+returnarch_timer_shutdown(ARCH_TIMER_PHYS_ACCESS,clk);}staticintarch_timer_shutdown_virt_mem(structclock_event_device*clk){-returntimer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS,clk);+returnarch_timer_shutdown(ARCH_TIMER_MEM_VIRT_ACCESS,clk);}staticintarch_timer_shutdown_phys_mem(structclock_event_device*clk){-returntimer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS,clk);+returnarch_timer_shutdown(ARCH_TIMER_MEM_PHYS_ACCESS,clk);}static__always_inlinevoidset_next_event(constintaccess,unsignedlongevt,
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:45:25
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
A new "shutdown" timer state is being added to the generic timer code. One
of the functions to change the timer into the state is called
"timer_shutdown()". This means that there can not be other functions
called "timer_shutdown()" as the timer code owns the "timer_*" name space.
Rename timer_shutdown() to evt_timer_shutdown() to avoid this conflict.
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lkml.kernel.org/r/20221106212702.182883323@goodmis.org
Link: https://lore.kernel.org/all/20221105060155.592778858@goodmis.org/
Link: https://lore.kernel.org/r/20221110064147.158230501@goodmis.org
---
drivers/clocksource/timer-sp804.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:45:28
del_singleshot_timer_sync() used to be an optimization for deleting timers
which are not rearmed from the timer callback function.
This optimization turned out to be broken and got mapped to
del_timer_sync() about 17 years ago.
Get rid of the undocumented indirection and use del_timer_sync() directly.
No functional change.
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
drivers/char/tpm/tpm-dev-common.c | 4 ++--
drivers/staging/wlan-ng/hfa384x_usb.c | 4 ++--
drivers/staging/wlan-ng/prism2usb.c | 6 +++---
include/linux/timer.h | 2 --
kernel/time/timer.c | 2 +-
net/sunrpc/xprt.c | 2 +-
6 files changed, 9 insertions(+), 11 deletions(-)
@@ -1116,8 +1116,8 @@ static int hfa384x_usbctlx_complete_syncif(ctlx==get_active_ctlx(hw)){spin_unlock_irqrestore(&hw->ctlxq.lock,flags);-del_singleshot_timer_sync(&hw->reqtimer);-del_singleshot_timer_sync(&hw->resptimer);+del_timer_sync(&hw->reqtimer);+del_timer_sync(&hw->resptimer);hw->req_timer_done=1;hw->resp_timer_done=1;usb_kill_urb(&hw->ctlx_urb);---a/drivers/staging/wlan-ng/prism2usb.c+++b/drivers/staging/wlan-ng/prism2usb.c
@@ -170,9 +170,9 @@ static void prism2sta_disconnect_usb(str*/prism2sta_ifstate(wlandev,P80211ENUM_ifstate_disable);-del_singleshot_timer_sync(&hw->throttle);-del_singleshot_timer_sync(&hw->reqtimer);-del_singleshot_timer_sync(&hw->resptimer);+del_timer_sync(&hw->throttle);+del_timer_sync(&hw->reqtimer);+del_timer_sync(&hw->resptimer);/* Unlink all the URBs. This "removes the wheels"*fromtheentireCTLXhandlingmechanism.---a/include/linux/timer.h+++b/include/linux/timer.h
@@ -1933,7 +1933,7 @@ signed long __sched schedule_timeout(sigtimer_setup_on_stack(&timer.timer,process_timeout,0);__mod_timer(&timer.timer,expire,MOD_TIMER_NOTPENDING);schedule();-del_singleshot_timer_sync(&timer.timer);+del_timer_sync(&timer.timer);/* Remove the timer from the object tracker */destroy_timer_on_stack(&timer.timer);---a/net/sunrpc/xprt.c+++b/net/sunrpc/xprt.c
@@ -1164,7 +1164,7 @@ xprt_request_enqueue_receive(struct rpc_spin_unlock(&xprt->queue_lock);/* Turn off autodisconnect */-del_singleshot_timer_sync(&xprt->timer);+del_timer_sync(&xprt->timer);return0;}
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:45:33
The timer code still has a few BUG_ON()s left which are crashing the kernel
in situations where it still can recover or simply refuse to take an
action.
Remove the one in the hotplug callback which checks for the CPU being
offline. If that happens then the whole hotplug machinery will explode in
colourful ways.
Replace the rest with WARN_ON_ONCE() and conditional returns where
appropriate.
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
kernel/time/timer.c | 11 ++++++-----
1 file changed, 6 insertions(+), 5 deletions(-)
@@ -2017,8 +2019,6 @@ int timers_dead_cpu(unsigned int cpu)structtimer_base*new_base;intb,i;-BUG_ON(cpu_online(cpu));-for(b=0;b<NR_BASES;b++){old_base=per_cpu_ptr(&timer_bases[b],cpu);new_base=get_cpu_ptr(&timer_bases[b]);
@@ -2035,7 +2035,8 @@ int timers_dead_cpu(unsigned int cpu)*/forward_timer_base(new_base);-BUG_ON(old_base->running_timer);+WARN_ON_ONCE(old_base->running_timer);+old_base->running_timer=NULL;for(i=0;i<WHEEL_SIZE;i++)migrate_timer_list(new_base,old_base->vectors+i);
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:45:46
The kernel-doc of timer related functions is partially uncomprehensible
word salad. Rewrite it to make it useful.
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
V2: Refined comments (Steven)
---
kernel/time/timer.c | 148 ++++++++++++++++++++++++++++++----------------------
1 file changed, 88 insertions(+), 60 deletions(-)
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:45:51
del_timer_sync() is assumed to be pointless on uniprocessor systems and can
be mapped to del_timer() because in theory del_timer() can never be invoked
while the timer callback function is executed.
This is not entirely true because del_timer() can be invoked from interrupt
context and therefore hit in the middle of a running timer callback.
Contrary to that del_timer_sync() is not allowed to be invoked from
interrupt context unless the affected timer is marked with TIMER_IRQSAFE.
del_timer_sync() has proper checks in place to detect such a situation.
Give up on the UP optimization and make del_timer_sync() unconditionally
available.
Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
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
---
include/linux/timer.h | 7 +------
kernel/time/timer.c | 2 --
2 files changed, 1 insertion(+), 8 deletions(-)
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:45:56
The timer related functions do not have a strict timer_ prefixed namespace
which is really annoying.
Rename del_timer() to timer_delete() and provide del_timer()
as a wrapper. Document that del_timer() is not for new code.
Signed-off-by: Thomas Gleixner <redacted>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
include/linux/timer.h | 15 ++++++++++++++-
kernel/time/timer.c | 6 +++---
2 files changed, 17 insertions(+), 4 deletions(-)
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:46:01
The timer related functions do not have a strict timer_ prefixed namespace
which is really annoying.
Rename del_timer_sync() to timer_delete_sync() and provide del_timer_sync()
as a wrapper. Document that del_timer_sync() is not for new code.
Signed-off-by: Thomas Gleixner <redacted>
Reviewed-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
include/linux/timer.h | 15 ++++++++++++++-
kernel/time/timer.c | 18 +++++++++---------
2 files changed, 23 insertions(+), 10 deletions(-)
@@ -1858,7 +1858,7 @@ unloaded. After a given module has been one of its functions results in a segmentation fault. The module-unload functions must therefore cancel any delayed calls to loadable-module functions, for example, any outstanding mod_timer() must be dealt-with via del_timer_sync() or similar.+with via timer_delete_sync() or similar. Unfortunately, there is no way to cancel an RCU callback; once you invoke call_rcu(), the callback function is eventually going to be--- a/Documentation/core-api/local_ops.rst+++ b/Documentation/core-api/local_ops.rst
@@ -191,7 +191,7 @@ Here is a sample module which implements static void __exit test_exit(void) {- del_timer_sync(&test_timer);+ timer_delete_sync(&test_timer); } module_init(test_init);--- a/Documentation/kernel-hacking/locking.rst+++ b/Documentation/kernel-hacking/locking.rst
@@ -967,7 +967,7 @@ If you want to destroy the entire collec while (list) { struct foo *next = list->next;- del_timer(&list->timer);+ timer_delete(&list->timer); kfree(list); list = next; }
@@ -981,7 +981,7 @@ the lock after we spin_unlock_bh(), and the element (which has already been freed!). This can be avoided by checking the result of-del_timer(): if it returns 1, the timer has been deleted.+timer_delete(): if it returns 1, the timer has been deleted. If 0, it means (in this case) that it is currently running, so we can do::
@@ -990,7 +990,7 @@ If 0, it means (in this case) that it is while (list) { struct foo *next = list->next;- if (!del_timer(&list->timer)) {+ if (!timer_delete(&list->timer)) { /* Give timer a chance to delete this */ spin_unlock_bh(&list_lock); goto retry;
@@ -1005,8 +1005,7 @@ If 0, it means (in this case) that it is Another common problem is deleting timers which restart themselves (by calling add_timer() at the end of their timer function). Because this is a fairly common case which is prone to races, you should-use del_timer_sync() (``include/linux/timer.h``) to-handle this case.+use timer_delete_sync() (``include/linux/timer.h``) to handle this case. Locking Speed =============
@@ -1334,7 +1333,7 @@ lock.- kfree()-- add_timer() and del_timer()+- add_timer() and timer_delete() Mutex API reference ===================--- a/Documentation/timers/hrtimers.rst+++ b/Documentation/timers/hrtimers.rst
@@ -118,7 +118,7 @@ existing timer wheel code, as it is matu was not really a win, due to the different data structures. Also, the hrtimer functions now have clearer behavior and clearer names - such as hrtimer_try_to_cancel() and hrtimer_cancel() [which are roughly-equivalent to del_timer() and del_timer_sync()] - so there's no direct+equivalent to timer_delete() and timer_delete_sync()] - so there's no direct 1:1 mapping between them on the algorithmic level, and thus no real potential for code sharing either.--- a/Documentation/translations/it_IT/kernel-hacking/locking.rst+++ b/Documentation/translations/it_IT/kernel-hacking/locking.rst
@@ -990,7 +990,7 @@ Se volete eliminare l'intera collezione while (list) { struct foo *next = list->next;- del_timer(&list->timer);+ timer_delete(&list->timer); kfree(list); list = next; }
@@ -1003,7 +1003,7 @@ e prenderà il *lock* solo dopo spin_unl di eliminare il suo oggetto (che però è già stato eliminato). Questo può essere evitato controllando il valore di ritorno di-del_timer(): se ritorna 1, il temporizzatore è stato già+timer_delete(): se ritorna 1, il temporizzatore è stato già rimosso. Se 0, significa (in questo caso) che il temporizzatore è in esecuzione, quindi possiamo fare come segue::
@@ -1012,7 +1012,7 @@ rimosso. Se 0, significa (in questo caso while (list) { struct foo *next = list->next;- if (!del_timer(&list->timer)) {+ if (!timer_delete(&list->timer)) { /* Give timer a chance to delete this */ spin_unlock_bh(&list_lock); goto retry;
@@ -1026,7 +1026,7 @@ rimosso. Se 0, significa (in questo caso Un altro problema è l'eliminazione dei temporizzatori che si riavviano da soli (chiamando add_timer() alla fine della loro esecuzione). Dato che questo è un problema abbastanza comune con una propensione-alle corse critiche, dovreste usare del_timer_sync()+alle corse critiche, dovreste usare timer_delete_sync() (``include/linux/timer.h``) per gestire questo caso. Velocità della sincronizzazione
@@ -1372,7 +1372,7 @@ contesto, o trattenendo un qualsiasi *lo- kfree()-- add_timer() e del_timer()+- add_timer() e timer_delete() Riferimento per l'API dei Mutex ===============================--- a/Documentation/translations/zh_CN/core-api/local_ops.rst+++ b/Documentation/translations/zh_CN/core-api/local_ops.rst
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:46:21
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.
In preparation for that replace the warnings in the relevant code pathes
with checks for timer->function == NULL and 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.
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.
Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
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
---
V2: Use continue instead of return and amend the return value docs (Steven)
---
kernel/time/timer.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 55 insertions(+), 5 deletions(-)
@@ -1537,6 +1581,12 @@ static void expire_timers(struct timer_bfn=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);
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:46:31
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 <redacted>
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(-)
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:47:03
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.
Add a shutdown argument to the relevant internal functions which makes the
actual deactivation code set timer->function to NULL which in turn prevents
rearming of the timer.
Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
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
---
V2: Add missing commata (Steven)
---
kernel/time/timer.c | 64 ++++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 55 insertions(+), 9 deletions(-)
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:47:12
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.
Expose new interfaces for this: timer_shutdown_sync() and timer_shutdown().
timer_shutdown_sync() has the same functionality as timer_delete_sync()
plus the NULL-ification of the timer function.
timer_shutdown() has the same functionality as timer_delete() plus the
NULL-ification of the timer function.
In both cases the rearming of the timer is prevented by silently discarding
rearm attempts due to timer->function being NULL.
Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
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
---
include/linux/timer.h | 2 +
kernel/time/timer.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 68 insertions(+)
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:47:18
From: "Steven Rostedt (Google)" <rostedt@goodmis.org>
In order to make sure that a timer is not re-armed after it is stopped
before freeing, a new shutdown state is added to the timer code. The API
timer_shutdown_sync() and timer_shutdown() must be called before the
object that holds the timer can be freed.
Update the documentation to reflect this new workflow.
[ tglx: Updated to the new semantics and updated the zh_CN version ]
Signed-off-by: Steven Rostedt (Google) <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Link: https://lore.kernel.org/r/20221110064147.712934793@goodmis.org
---
Documentation/RCU/Design/Requirements/Requirements.rst | 2 +-
Documentation/core-api/local_ops.rst | 2 +-
Documentation/kernel-hacking/locking.rst | 5 +++++
Documentation/translations/zh_CN/core-api/local_ops.rst | 2 +-
4 files changed, 8 insertions(+), 3 deletions(-)
@@ -1858,7 +1858,7 @@ unloaded. After a given module has been one of its functions results in a segmentation fault. The module-unload functions must therefore cancel any delayed calls to loadable-module functions, for example, any outstanding mod_timer() must be dealt-with via timer_delete_sync() or similar.+with via timer_shutdown_sync() or similar. Unfortunately, there is no way to cancel an RCU callback; once you invoke call_rcu(), the callback function is eventually going to be--- a/Documentation/core-api/local_ops.rst+++ b/Documentation/core-api/local_ops.rst
@@ -191,7 +191,7 @@ Here is a sample module which implements static void __exit test_exit(void) {- timer_delete_sync(&test_timer);+ timer_shutdown_sync(&test_timer); } module_init(test_init);--- a/Documentation/kernel-hacking/locking.rst+++ b/Documentation/kernel-hacking/locking.rst
@@ -1007,6 +1007,11 @@ calling add_timer() at the end of their Because this is a fairly common case which is prone to races, you should use timer_delete_sync() (``include/linux/timer.h``) to handle this case.+Before freeing a timer, timer_shutdown() or timer_shutdown_sync() should be+called which will keep it from being rearmed. Any subsequent attempt to+rearm the timer will be silently ignored by the core code.++ Locking Speed =============--- a/Documentation/translations/zh_CN/core-api/local_ops.rst+++ b/Documentation/translations/zh_CN/core-api/local_ops.rst
From: Thomas Gleixner <hidden> Date: 2022-11-22 17:47:46
While discussing solutions for the teardown problem which results from
circular dependencies between timers and workqueues, where timers schedule
work from their timer callback and workqueues arm the timers from work
items, it was discovered that the recent fix to the QCA code is incorrect.
That commit fixes the obvious problem of using del_timer() instead of
del_timer_sync() and reorders the teardown calls to
destroy_workqueue(wq);
del_timer_sync(t);
This makes it less likely to explode, but it's still broken:
destroy_workqueue(wq);
/* After this point @wq cannot be touched anymore */
---> timer expires
queue_work(wq) <---- Results in a NULl pointer dereference
deep in the work queue core code.
del_timer_sync(t);
Use the new timer_shutdown_sync() function to ensure that the timers are
disarmed, no timer callbacks are running and the timers cannot be armed
again. This restores the original teardown sequence:
timer_shutdown_sync(t);
destroy_workqueue(wq);
which is now correct because the timer core silently ignores potential
rearming attempts which can happen when destroy_workqueue() drains pending
work before mopping up the workqueue.
Fixes: 72ef98445aca ("Bluetooth: hci_qca: Use del_timer_sync() before freeing")
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Guenter Roeck <linux@roeck-us.net>
Acked-by: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Johan Hedberg <redacted>
Cc: linux-bluetooth@vger.kernel.org
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org
Link: https://lore.kernel.org/all/87iljhsftt.ffs@tglx
---
drivers/bluetooth/hci_qca.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
From: David Laight <hidden> Date: 2022-11-22 22:23:20
From: Thomas Gleixner
Sent: 22 November 2022 17:45
The timer related functions do not have a strict timer_ prefixed namespace
which is really annoying.
Rename del_timer_sync() to timer_delete_sync() and provide del_timer_sync()
as a wrapper. Document that del_timer_sync() is not for new code.
To change the colo[u]r of the bikeshed, would it be better to
name the functions timer_start() and timer_stop[_sync]().
And, as I found out for a local driver, adding items to work queues
from timer callbacks really isn't a good idea at all!
The delayed_work functions handle it a lot better.
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2022-11-22 23:04:57
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
From: Jacob Keller <jacob.e.keller@intel.com> Date: 2022-11-22 23:10:00
On 11/22/2022 9:44 AM, Thomas Gleixner wrote:
This is the second version of the timer shutdown work. The first version
can be found here:
https://lore.kernel.org/all/20221115195802.415956561@linutronix.de
Tearing down timers can be tedious when there are circular dependencies to
other things which need to be torn down. A prime example is timer and
workqueue where the timer schedules work and the work arms the timer.
Steven and the Google Chromebook team ran into such an issue in the
Bluetooth HCI code.
Steven suggested to create a new function del_timer_free() which marks the
timer as shutdown. Rearm attempts of shutdown timers are discarded and he
wanted to emit a warning for that case:
https://lore.kernel.org/all/20220407161745.7d6754b3@gandalf.local.home
This resulted in a lengthy discussion and suggestions how this should be
implemented. The patch series went through several iterations and during
the review of the last version it turned out that this approach is
suboptimal:
https://lore.kernel.org/all/20221110064101.429013735@goodmis.org
The warning is not really helpful because it's entirely unclear how it
should be acted upon. The only way to address such a case is to add 'if
(in_shutdown)' conditionals all over the place. This is error prone and in
most cases of teardown like the HCI one which started this discussion not
required all.
What needs to prevented is that pending work which is drained via
destroy_workqueue() does not rearm the previously shutdown timer. Nothing
in that shutdown sequence relies on the timer being functional.
The conclusion was that the semantics of timer_shutdown_sync() should be:
- timer is not enqueued
- timer callback is not running
- timer cannot be rearmed
Preventing the rearming of shutdown timers is done by discarding rearm
attempts silently.
As Steven is short of cycles, I made some spare cycles available and
reworked the patch series to follow the new semantics and plugged the races
which were discovered during review.
The patches have been split up into small pieces to make review easier and
I took the liberty to throw a bunch of overdue cleanups into the picture
instead of proliferating the existing state further.
The last patch in the series addresses the HCI teardown issue for real.
The series is also available from git:
git://git.kernel.org/pub/scm/linux/kernel/git/tglx/devel.git timers
Changes vs. V1:
- Fixed the return vs. continue bug in the timer expiration code (Steven)
- Addressed the review vs. function documentation (Steven)
- Fixed up the del_timer*() references in documentation (Steven)
- Split out the 'remove bogus claims about del_timer_sync()' change
- Picked up Reviewed/Tested-by tags where appropriate
Thanks,
tglx
---
Documentation/RCU/Design/Requirements/Requirements.rst | 2
Documentation/core-api/local_ops.rst | 2
Documentation/kernel-hacking/locking.rst | 17
Documentation/timers/hrtimers.rst | 2
Documentation/translations/it_IT/kernel-hacking/locking.rst | 14
Documentation/translations/zh_CN/core-api/local_ops.rst | 2
arch/arm/mach-spear/time.c | 8
drivers/bluetooth/hci_qca.c | 10
drivers/char/tpm/tpm-dev-common.c | 4
drivers/clocksource/arm_arch_timer.c | 12
drivers/clocksource/timer-sp804.c | 6
drivers/staging/wlan-ng/hfa384x_usb.c | 4
drivers/staging/wlan-ng/prism2usb.c | 6
include/linux/timer.h | 35
kernel/time/timer.c | 424 +++++++++---
net/sunrpc/xprt.c | 2
16 files changed, 404 insertions(+), 146 deletions(-)
I read through the series! Really appreciate breaking things up and
cleaning up a bunch of the docs. A thorough explanation of the problem
is great.
I noticed that we still left some timer functions outside the timer_*
namespace, but nothing was made worse as these functions already existed.
Reviewed-by: Jacob Keller <jacob.e.keller@intel.com>
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.
NIT (comma is missing): can arm timer, 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
s/to do so it/to do so is/
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 pathes
with checks for timer->function == NULL and discard the rearm request
silently.
Here is a verb missing that this is a grammatically correct (and
understandable) sentence.
quoted hunk
Add debug_assert_init() instead of the WARN_ON_ONCE(!timer->function)
checks so that debug objects can warn about non-initialized timers.
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.
Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
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
---
V2: Use continue instead of return and amend the return value docs (Steven)
---
kernel/time/timer.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 55 insertions(+), 5 deletions(-)
Do you mean "was" or "is"? Please have also a look at the places where you
use the same phrase.
quoted hunk
* * %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, the return value is 0 and meaningless.
It's easier to read, if you make a dot instead of comma.
Thanks,
Anna-Maria
The kernel-doc of timer related functions is partially uncomprehensible
word salad. Rewrite it to make it useful.
Signed-off-by: Thomas Gleixner <redacted>
Tested-by: Guenter Roeck <linux@roeck-us.net>
---
V2: Refined comments (Steven)
---
kernel/time/timer.c | 148 ++++++++++++++++++++++++++++++----------------------
1 file changed, 88 insertions(+), 60 deletions(-)
This is also true for add_timer(). Is it possible to add this to
add_timer() function description and just referencing to add_timer()
function description in add_timer_on()? They behave the same, only
difference is the CPU where the timer is enqueued.
quoted hunk
*/
void add_timer_on(struct timer_list *timer, int cpu)
{
@@ -1240,15 +1253,18 @@ void add_timer_on(struct timer_list *tim EXPORT_SYMBOL_GPL(add_timer_on); /**- * del_timer - deactivate a timer.- * @timer: the timer to be deactivated- *- * del_timer() deactivates a timer - this works on both active and inactive- * timers.+ * del_timer - Deactivate a timer.+ * @timer: The timer to be deactivated *- * The function returns whether it has deactivated a pending timer or not.- * (ie. del_timer() of an inactive timer returns 0, del_timer() of an- * active timer returns 1.)+ * The function only deactivates a pending timer, but contrary to+ * del_timer_sync() it does not take into account whether the timers
timer's callback function or timer callback function (if the latter one is
used, please replace it in description for del_timer_sync() as well).
+ * callback function is concurrently executed on a different CPU or not.
+ * It neither prevents rearming of the timer. If @timer can be rearmed
NIT ^ two whitespaces
+ * 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 del_timer(struct timer_list *timer)
{
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.
In preparation for that replace the warnings in the relevant code pathes
with checks for timer->function == NULL and 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.
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.
Co-developed-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <redacted>
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
---
V2: Use continue instead of return and amend the return value docs (Steven)
---
kernel/time/timer.c | 60 +++++++++++++++++++++++++++++++++++++++++++++++-----
1 file changed, 55 insertions(+), 5 deletions(-)
Could you move the new paragraph after the paragraph where is is mentioned,
that timer->function has to be set prior calling add_timer()?
Thanks,
Anna-Maria
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.
In preparation for that replace the warnings in the relevant code pathes
with checks for timer->function == NULL and 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.
Could you expand this paragraph, so that is is not missleading when a
reader is not aware of the details of debug objects? Otherwise it seems to
the reader that debug objects will warn when timer->function == NULL.
The warning of debug objects does not cover the original
WARN_ON_ONCE(!timer->function). 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.
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
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)
quoted
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
@@ -1605,6 +1629,48 @@ int timer_delete_sync(struct timer_list } EXPORT_SYMBOL(timer_delete_sync);+/**+ * timer_shutdown_sync - Shutdown a timer and prevent rearming+ * @timer: The timer to be shutdown+ *+ * When the function returns it is guaranteed that:+ * - @timer is not queued+ * - The callback function of @timer is not running+ * - @timer cannot be enqueued again. Any attempt to rearm+ * @timer is silently ignored.+ *+ * See timer_delete_sync() for synchronization rules.+ *+ * This function is useful for final teardown of an infrastructure where+ * the timer is subject to a circular dependency problem.+ *+ * A common pattern for this is a timer and a workqueue where the timer can+ * schedule work and work can arm the timer. On shutdown the workqueue must+ * be destroyed and the timer must be prevented from rearming. Unless the+ * code has conditionals like 'if (mything->in_shutdown)' to prevent that+ * there is no way to get this correct with timer_delete_sync().+ *+ * timer_shutdown_sync() is solving the problem. The correct ordering of+ * calls in this case is:+ *+ * timer_shutdown_sync(&mything->timer);+ * workqueue_destroy(&mything->workqueue);+ *+ * After this 'mything' can be safely freed.+ *+ * This obviously requires that the timer is not required to be functional+ * for the rest of the shutdown operation.
NIT... Maybe the first requires could be replaced by
assumes/expects/presupposes to prevent double use of required?
Thanks,
Anna-Maria
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
From: Thomas Gleixner <hidden> Date: 2022-11-23 17:09:33
On Wed, Nov 23 2022 at 12:06, Anna-Maria Behnsen wrote:
On Tue, 22 Nov 2022, Thomas Gleixner wrote:
quoted
Add debug_assert_init() instead of the WARN_ON_ONCE(!timer->function)
checks so that debug objects can warn about non-initialized timers.
Could you expand this paragraph, so that is is not missleading when a
reader is not aware of the details of debug objects? Otherwise it seems to
the reader that debug objects will warn when timer->function == NULL.
The warning of debug objects does not cover the original
WARN_ON_ONCE(!timer->function). It warns when timer was not initialized
using timer_setup[_on_stack]() or via DEFINE_TIMER().
From: Thomas Gleixner <hidden> Date: 2022-11-23 17:10:05
On Wed, Nov 23 2022 at 11:23, Anna-Maria Behnsen wrote:
quoted
/**
- * add_timer_on - start a timer on a particular CPU
- * @timer: the timer to be added
- * @cpu: the CPU to start it on
+ * add_timer_on - Start a timer on a particular CPU
+ * @timer: The timer to be started
+ * @cpu: The CPU to start it on
*
- * This is not very scalable on SMP. Double adds are not possible.
+ * This can only operate on an inactive timer. Attempts to invoke this on
+ * an active timer are rejected with a warning.
This is also true for add_timer(). Is it possible to add this to
add_timer() function description and just referencing to add_timer()
function description in add_timer_on()? They behave the same, only
difference is the CPU where the timer is enqueued.
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