Re: [patch V2 00/17] timers: Provide timer_shutdown[_sync]()

Jacob Keller
2022-11-22T23:10:00Z

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>