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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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@xxxxxxxxxxxxx

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@xxxxxxxxxxxxxxxxxx

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@xxxxxxxxxxx

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@xxxxxxxxx>



[Index of Archives]     [Bluez Devel]     [Linux Wireless Networking]     [Linux Wireless Personal Area Networking]     [Linux ATH6KL]     [Linux USB Devel]     [Linux Media Drivers]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Big List of Linux Books]

  Powered by Linux