Date:   Tue, 22 Nov 2022 15:09:47 -0800
From:   Jacob Keller <jacob.e.keller@...el.com>
To:     Thomas Gleixner <tglx@...utronix.de>,
        LKML <linux-kernel@...r.kernel.org>
CC:     Linus Torvalds <torvalds@...uxfoundation.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Anna-Maria Behnsen <anna-maria@...utronix.de>,
        "Peter Zijlstra" <peterz@...radead.org>,
        Stephen Boyd <sboyd@...nel.org>,
        "Guenter Roeck" <linux@...ck-us.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Julia Lawall <Julia.Lawall@...ia.fr>,
        Arnd Bergmann <arnd@...db.de>,
        "Viresh Kumar" <viresh.kumar@...aro.org>,
        Marc Zyngier <maz@...nel.org>,
        "Marcel Holtmann" <marcel@...tmann.org>,
        Johan Hedberg <johan.hedberg@...il.com>,
        Luiz Augusto von Dentz <luiz.dentz@...il.com>,
        <linux-bluetooth@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        "Paolo Abeni" <pabeni@...hat.com>, <netdev@...r.kernel.org>
Subject: Re: [patch V2 00/17] timers: Provide timer_shutdown[_sync]()



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@...el.com>

Powered by blists - more mailing lists