Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220407161745.7d6754b3@gandalf.local.home> <87pmlrkgi3.ffs@tglx>
In-Reply-To: <87pmlrkgi3.ffs@tglx>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri, 8 Apr 2022 07:33:53 -1000
Message-ID: <CAHk-=whbsLXy85XpKRQmBXr=GqWbMoi+wVjFY_V22=BOE=dHog@mail.gmail.com>
Subject: Re: [RFC][PATCH] timers: Add del_time_free() to be called before
 freeing timers
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     Steven Rostedt <rostedt@goodmis.org>,
        LKML <linux-kernel@vger.kernel.org>, jstultz@google.com,
        Stephen Boyd <sboyd@kernel.org>,
        Andrew Morton <akpm@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>,
        Marcel Holtmann <marcel@holtmann.org>,
        Johan Hedberg <johan.hedberg@gmail.com>,
        Luiz Augusto von Dentz <luiz.dentz@gmail.com>,
        Eric Dumazet <edumazet@google.com>,
        Guenter Roeck <linux@roeck-us.net>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Fri, Apr 8, 2022 at 12:37 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> So this would become:
>
> -       BUG_ON(!timer->function);
> +       if (WARN_ON(!timer->function))
> +               return -EBROKEN;

Yes. But please make it a WARN_ON_ONCE(), just on basic principles. I
can't imagine this happening a lot, but at the same time I don't think
there's any reason _not_ to just always use WARN_ON_ONCE() for these
kinds of "serious bug, but should never happen" situations.

Because we don't want some "user can trigger this and spam the logs"
situation either.

That said, I would actually prefer a name-change: instead of making
this about "del_timer_free()", can we please just make this about it
being "final". Or maybe "del_timer_cancel()" or something like that?

Because the actual _freeing_ will obviously happen later, and the
function does nothing of the sort. In fact, there may be situations
where you don't free it at all, but just want to be in the situation
where you want to make sure there are no pending timers until after
you explicitly re-arm it, even if the timer would otherwise be
self-arming.

(That use-case would actually mean removing the WARN_ON_ONCE(), but I
think that would be a "future use" issue, I'm *not* suggesting it not
be done initially).

I also suspect that 99% of all del_timer_sync() users actually want
that kind of explicit "del_timer_final()" behavior. Some may not
_need_ it (because their re-arming already checks for "have I shut
down?"), but I have this suspicion that we could convert a lot - maybe
all - of the current del_timer_sync() users to this and try to see if
we could just make it the rule.

And then we could actually maybe start removing the explicit "shut
down timer" code. See for example "work->canceling" logic for
kthreads, which now does double duty (it disables re-arming the timer,
_and_ it does some "double cancel work avoidance")

              Linus