Re: [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called before freeing timers

From: Steven Rostedt <rostedt@goodmis.org>
To: Thomas Gleixner <tglx@linutronix.de>
Cc: linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Stephen Boyd <sboyd@kernel.org>,
	Guenter Roeck <linux@roeck-us.net>,
	Anna-Maria Gleixner <anna-maria@linutronix.de>,
	Andrew Morton <akpm@linux-foundation.org>,
	Julia Lawall <Julia.Lawall@inria.fr>
Subject: Re: [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called before freeing timers
Date: Mon, 14 Nov 2022 14:28:58 -0500	[thread overview]
Message-ID: <20221114142858.59682a42@gandalf.local.home> (raw)
In-Reply-To: <87bkp9s61z.ffs@tglx>

On Mon, 14 Nov 2022 20:13:28 +0100
Thomas Gleixner <tglx@linutronix.de> wrote:

> On Mon, Nov 14 2022 at 08:36, Steven Rostedt wrote:
> > On Mon, 14 Nov 2022 01:33:25 +0100
> > Thomas Gleixner <tglx@linutronix.de> wrote:  
> >>    https://lore.kernel.org/all/87v8vjiaih.ffs@tglx/
> >>   
> > I'm not sure what you mean by that. The idea is that once timer_shutdown()
> > is called, we still warn on re-arming the timer.  
> 
> That's the whole point. As Linus and I discussed in that thread:
> 
>    "That would mean, that we still check the function pointer for NULL
>     without warning and just return. That would indeed be a good argument
>     for not having the warning at all."
> 
> and as I demonstrated you on the example of the BT driver which you
> "fixed" this is the only sensible way to handle this.

I agree that it wasn't a complete fix, but as I mentioned before, I was
pulled off before I could do more.

> 
> The warning does not buy us anything, unless you want to go and amend
> all the usage sites which trigger it with 'if (mystruct->shutdown)'
> conditionals.

The rationale for the warning was that it would let us know what drivers
need to be fixed for older kernels without the shutdown state. I'm
perfectly fine in removing the warning. We may just add it to the field
kernels so that we can know if there's any drivers that have issues that we
need to look at.

> 
> It's very similar to the work->canceling logic for kthreads that Linus
> mentioned in this thread which prevents that the work timer is rearmed
> concurrently. The difference is that timer_shutdown() is a final
> decision which renders the timer unusable unless it is explicitely
> reinitialized.
> 
> But that's mostly a matter of documentation and it has to be made clear
> that nothing in a shutdown path which has the BT pattern:
> 
>      timer_shutdown();
>      destroy_workqueue();
> 
> relies on the timer being functional after the shutdown point. I'm
> pretty sure that the vast majority of such use cases do not care, but
> given the size of the driver zoo I'm also sure that you'll find at least
> one which depends on the timer working accross teardown.
>

Agreed.

-- Steve