Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: Thomas Gleixner <tglx@linutronix.de>,
	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>
Subject: Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers
Date: Fri, 8 Apr 2022 13:18:27 -1000	[thread overview]
Message-ID: <CAHk-=whPMLcdy0P6WGP4nbrn-e1ksr35sBM+k=kziWikzmQ31A@mail.gmail.com> (raw)
In-Reply-To: <20220408161025.5842a663@gandalf.local.home>

On Fri, Apr 8, 2022 at 10:10 AM Steven Rostedt <rostedt@goodmis.org> wrote:
>
> We have that already, it's called "del_timer_sync()". And that's not used
> when it should be, and does not catch the case of the timer being rearmed.

THAT'S MY POINT.

It doesn't do the "no rearming".

So my point is that people who actually use "del_timer_sync()"
generally *want* that "no re-arming", and right now they go to extra
work to do so.

Because calling del_timer_sync() while the timer can get re-armed is
almost always a bug _anyway_.

So :

 - don't use the name "free" - because that's not what people always are about

 - aim to *replace* the current del_timer_sync() with the new functionality

A new name is good. But "free" is wrong. The function doesn't free
anything, and it's not necessarily even the case that all users would
want to free it.

I do like the "timer_shutdown()" naming that Thomas suggested.

Imagine having something that just guarantees that the timer isn't
running, and you have to explicitly restart it.

No, del_timer_sync() doesn't do that. It only guarantees that any
previously started timer has been finished.

And no, currently we don't actually have a "timer_restart()". You have
to re-init it completely with timer_setup(), and then do a
"mod_timer()". But that's because we haven't had that
"timer_shutdown()" functionality in the past.

Now, most timers don't necessarily re-arm themselves at all, so those
people don't care, and del_timer_sync() works for them as-is.

 But even if you don't have a self-rearming timer, maybe you want to
make sure nobody else restarts it either.

As mentioned, right now people actually do things like this manually
using some flag of their own.

               Linus