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

[Jacob Keller <jacob.e.keller@intel.com>]

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>