All of lore.kernel.org
 help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Linus Torvalds <torvalds@linux-foundation.org>
Cc: Steven Rostedt <rostedt@goodmis.org>,
	linux-kernel@vger.kernel.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>,
	Eric Dumazet <edumazet@google.com>,
	Marcel Holtmann <marcel@holtmann.org>
Subject: Re: [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called before freeing timers
Date: Mon, 14 Nov 2022 20:45:29 +0100	[thread overview]
Message-ID: <875yfhs4km.ffs@tglx> (raw)
In-Reply-To: <CAHk-=wj7DtViDctAzV3PqdYBEh5vcQnRJPtFFB=uaAP=W-VG4A@mail.gmail.com>

Linus!

On Mon, Nov 14 2022 at 09:16, Linus Torvalds wrote:
> On Mon, Nov 14, 2022 at 7:42 AM Thomas Gleixner <tglx@linutronix.de> wrote:
>>
>> So if we want to make this solid and make the life of driver writers
>> easier, then we cannot issue a warning as I said in the original thread
>> already.
>
> So I think that there are two issues at play:
>
>  (a) do we want to *find* problem places after the conversion
>
>  (b) do we want to make driver writing easier
>
> and (a) argues for warning on timer re-arming, but (b) just says
> "don't warn, just ignore it, the driver is being shut down".
>
> I'm personally ok with either of those approaches, and it's literally
> just a question of mindset.

Correct. I'm very much for (b). Look at the bluetooth example. The "fix"
was obviously right and then introduced a new subtle bug which will only
happen every 7th half-moon.

But if you turn it around then:

    timer_shutdown();
    destroy_workqueue();

will trigger the warning in mod_timer() every 6.5th half-moon.

And then you have to go and sprinkle 'if (mydev->inshutdown)'
conditionals all over the place with a high probability that they will
not cut it completely. Or you end up with the reverse order of shutdown
calls which is wrong too.

So I rather have the very simple semantics that attempts to arm a
shutdown timer are silently ignored. As I said to Steven in the other
mail, I'm sure that the vast majority of teardown sites will not depend
on the timer(s) being functional. The two other esoteric cases will have
to be treated special.

>> The semantics of timer_shutdown_sync() have to be:
>>
>>    After return:
>>      - the timer is not queued
>>      - the timer callbacks is not running
>>      - the timer cannot be enqueued again
>
> Yes, but that last case is literally a "do we expect the *driver* to
> not enqueue it and warn if it tries, or do we just silently enforce
> it"?
>
> I agree with all three points. I'm just not sure about who we expect
> to do the "don't enqueue again".
>
> There's a big argument for "make it easy for driver writers" in just
> saying "make mod_timer() silently just ignore a re-arming". Making
> things easier for driver writers is a good thing.
>
> But maybe it's a "you shouldn't have done that in the first place"
> thing, and merits a warning?

See above.

> I have no strong opinions on that.
>
> What I *do* still want to happen is for subsystems to be able to start
> doing the conversion one by one. Which is why I'd still prefer to have
> the new names available just so that we don't have to have one
> 50-patch series, but we can have subsystems apply the obvious cases.
>
> And I'd still like the mindless "let's get the non-semantic changes
> out of the way" as one single patch, to get rid of mindless noise.
>
> And honestly, for that to happen I'd be perfectly happy with something like
>
>   #define timer_shutdown(t) del_timer(t)
>   #define timer_shutdown_sync(t) del_timer_sync(t)
>
> (obviously with the patches that first remove the existing
> 'timer_shutdown()' uses first). That wouldn't introduce the *new*
> semantics, but it would at least allow the different subsystems to do
> the obvious cases, and let the networking people wonder about the much
> less obvious ones.

As we are at -rc5 now and the core code is not yet ready, I suggest that
we get the core changes done for the next merge window and have some
obvious fixes which demonstrate the usage, e.g. the borked BT fix
replacement, and then subsystem people can queue their stuff for 6.3 or
send in the obvious bugfixes during the 6.2-rc series.

I'm not a fan of having

   #define timer_shutdown_sync(t) del_timer_sync(t)

as a gap measure right now. That's just going to make things worse
because the semantical difference between the both functions is
significant and I don't want people to run around and replace their
'if (mydev->in_shutdown)' conditionals prematurely or do any other fancy
"fixes" which cause more problems than they solve.

This problem exists for ever so there is no need to rush this just
because.

If we all agree that the semantics of timer_shutdown_sync() are:

    After return:
      - the timer is not queued
      - the timer callback is not running
      - the timer cannot be enqueued again. Any attempts to do
        so are silently ignored (needs some more explanation...)

and the semantics of timer_shutdown() are:

    After return:
      - the timer is not queued
      - the timer cannot be enqueued again. Any attempts to do
        so are silently ignored (needs some more explanation...)
      - the timer callback might be still running

then we can definitly get this in shape for 6.2.

Thanks,

        tglx

  parent reply	other threads:[~2022-11-14 19:45 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-11-10  6:41 [PATCH v6 0/6] timers: Use timer_shutdown*() before freeing timers Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 1/6] ARM: spear: Do not use timer namespace for timer_shutdown() function Steven Rostedt
2022-11-10  6:41   ` Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 2/6] clocksource/drivers/arm_arch_timer: " Steven Rostedt
2022-11-10  6:41   ` Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 3/6] clocksource/drivers/sp804: " Steven Rostedt
2022-11-10  6:41 ` [PATCH v6 4/6] timers: Add timer_shutdown_sync() to be called before freeing timers Steven Rostedt
2022-11-13 21:52   ` Thomas Gleixner
2022-11-14  0:11     ` Steven Rostedt
2022-11-14  1:04       ` Thomas Gleixner
2022-11-14 14:08         ` Steven Rostedt
2022-11-14 18:53           ` Thomas Gleixner
2022-11-14 19:14             ` Steven Rostedt
2022-11-13 23:18   ` Thomas Gleixner
2022-11-14  0:15     ` Steven Rostedt
2022-11-14  0:33       ` Thomas Gleixner
2022-11-14 13:36         ` Steven Rostedt
2022-11-14 19:13           ` Thomas Gleixner
2022-11-14 19:28             ` Steven Rostedt
2022-11-14 19:54               ` Thomas Gleixner
2022-11-14 15:42         ` Thomas Gleixner
2022-11-14 16:04           ` Steven Rostedt
2022-11-14 17:16           ` Linus Torvalds
2022-11-14 17:50             ` Steven Rostedt
2022-11-14 17:54               ` Linus Torvalds
2022-11-14 19:45             ` Thomas Gleixner [this message]
2022-11-24 14:15           ` [tip: timers/core] Bluetooth: hci_qca: Fix the teardown problem for real tip-bot2 for Thomas Gleixner
2022-11-10  6:41 ` [PATCH v6 5/6] timers: Add timer_shutdown() to be called before freeing timers Steven Rostedt
2022-11-13 22:20   ` Thomas Gleixner
2022-11-10  6:41 ` [PATCH v6 6/6] timers: Update the documentation to reflect on the new timer_shutdown() API Steven Rostedt
2022-11-24 14:16   ` [tip: timers/core] " tip-bot2 for Steven Rostedt (Google)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=875yfhs4km.ffs@tglx \
    --to=tglx@linutronix.de \
    --cc=Julia.Lawall@inria.fr \
    --cc=akpm@linux-foundation.org \
    --cc=anna-maria@linutronix.de \
    --cc=edumazet@google.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@roeck-us.net \
    --cc=marcel@holtmann.org \
    --cc=rostedt@goodmis.org \
    --cc=sboyd@kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.