lkml.org 
[lkml]   [2022]   [Nov]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch V2 12/17] timers: Silently ignore timers with a NULL function
    On Tue, 22 Nov 2022, Thomas Gleixner wrote:

    > 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.

    > 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 <tglx@linutronix.de>
    > 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(-)
    >
    > --- a/kernel/time/timer.c
    > +++ b/kernel/time/timer.c
    > @@ -1128,8 +1144,12 @@ static inline int
    > * mod_timer_pending() is the same for pending timers as mod_timer(), but
    > * will not activate inactive timers.
    > *
    > + * If @timer->function == NULL then the start operation is silently
    > + * discarded.
    > + *
    > * Return:
    > - * * %0 - The timer was inactive and not modified
    > + * * %0 - The timer was inactive and not modified or was is in
    > + * shutdown state and the operation was discarded

    Do you mean "was" or "is"? Please have also a look at the places where you
    use the same phrase.

    > * * %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

    \
     
     \ /
      Last update: 2022-11-23 10:24    [W:3.431 / U:0.084 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site