From: Guenter Roeck <linux@roeck-us.net>
To: Steven Rostedt <rostedt@goodmis.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
jstultz@google.com, Stephen Boyd <sboyd@kernel.org>,
Linus Torvalds <torvalds@linux-foundation.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>
Subject: Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers
Date: Thu, 7 Apr 2022 17:58:09 -0700 [thread overview]
Message-ID: <63d4c8d8-89c6-33ed-8178-be9ea86e53b9@roeck-us.net> (raw)
In-Reply-To: <20220407185144.5ac036b7@gandalf.local.home>
On 4/7/22 15:51, Steven Rostedt wrote:
> On Thu, 7 Apr 2022 14:58:02 -0700
> Guenter Roeck <linux@roeck-us.net> wrote:
>
>>> Add a del_timer_free() that not only does a del_timer_sync() but will mark
>>
>> This limits the use case to situations where del_timer_sync() can actually
>> be called. There is, however, code where this is not possible.
>> Specifically, it doesn't work if the code triggered with the timer uses a
>> lock, and del_timer() is also called under that same lock. An example for
>> that is the code in sound/synth/emux/emux.c. How do you suggest to handle
>> that situation ?
>
> Easy. Tell me how that situation is not a bug?
>
Sure, fixing the problem is of course the right thing to do. But replacing
del_timer() with your suggested version of del_timer_free() doesn't work
with this code because it would deadlock. Sure, that would not fix the
underlying problem anyway, but that isn't the point I was trying to make:
I think it would be beneficial to be able to replace del_timer() with a
version that can not result in deadlocks but would still identify problems
such as the one in the code in emux.c.
Can we have del_timer_free() and del_timer_sync_free() ? Or am I missing
something and that doesn't really make sense ?
Thanks,
Guenter
> That code you point out at emux.c looks extremely buggy as well. In other
> words, if you can't call del_timer_free() for the reason you mention above,
> the code is very likely to have race conditions. I cannot think of a
> situation that it is safe to do this.
>
> In fact, I think that just replacing that with del_timer_free() may be good
> enough. At least to show why it blows up later. I'm confused in what they
> are doing by taking that lock. I can see:
>
> CPU1 CPU2
> ---- ----
> snd_emux_timer_callback()
> spin_lock(voice_lock)
> if (timer_active)
> del_timer()
> spin_unlock(voice_lock)
>
> [..]
> do_again++
> [..]
> if (do_again) {
> mod_timer()
> timer_active = 1;
> }
>
> [..]
> free(emu);
> >
> BOOM!!
>
> Hmm, perhaps I should change the code in __mod_timer() to:
>
> if (WARN_ON(timer->flags & TIMER_FREED))
> return;
>
> To not rearm it.
>
> -- Steve
next prev parent reply other threads:[~2022-04-08 0:58 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-07 20:17 [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers Steven Rostedt
2022-04-07 21:58 ` Guenter Roeck
2022-04-07 22:51 ` Steven Rostedt
2022-04-08 0:58 ` Guenter Roeck [this message]
2022-04-08 1:36 ` Steven Rostedt
2022-04-08 10:37 ` Thomas Gleixner
2022-04-08 12:33 ` Steven Rostedt
2022-04-08 15:55 ` Steven Rostedt
2022-04-08 17:33 ` Linus Torvalds
2022-04-08 20:10 ` Steven Rostedt
2022-04-08 20:26 ` Steven Rostedt
2022-04-08 23:18 ` Linus Torvalds
2022-04-08 20:29 ` Thomas Gleixner
2022-04-08 20:58 ` Steven Rostedt
2022-04-08 21:46 ` Thomas Gleixner
2022-04-08 21:59 ` Steven Rostedt
2022-04-09 0:22 ` Steven Rostedt
2022-04-09 0:30 ` Linus Torvalds
2022-04-09 0:49 ` Steven Rostedt
2022-04-09 1:00 ` Linus Torvalds
2022-04-09 1:14 ` Steven Rostedt
2022-11-24 14:16 ` [tip: timers/core] timers: Provide timer_shutdown[_sync]() tip-bot2 for Thomas Gleixner
2022-11-24 14:16 ` [tip: timers/core] timers: Add shutdown mechanism to the internal functions tip-bot2 for Thomas Gleixner
2022-11-24 14:16 ` [tip: timers/core] timers: Split [try_to_]del_timer[_sync]() to prepare for shutdown mode tip-bot2 for Thomas Gleixner
2022-11-24 14:16 ` [tip: timers/core] timers: Silently ignore timers with a NULL function tip-bot2 for Thomas Gleixner
2022-11-24 14:16 ` [tip: timers/core] timers: Use del_timer_sync() even on UP tip-bot2 for Thomas Gleixner
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=63d4c8d8-89c6-33ed-8178-be9ea86e53b9@roeck-us.net \
--to=linux@roeck-us.net \
--cc=akpm@linux-foundation.org \
--cc=edumazet@google.com \
--cc=johan.hedberg@gmail.com \
--cc=jstultz@google.com \
--cc=linux-kernel@vger.kernel.org \
--cc=luiz.dentz@gmail.com \
--cc=marcel@holtmann.org \
--cc=peterz@infradead.org \
--cc=rostedt@goodmis.org \
--cc=sboyd@kernel.org \
--cc=tglx@linutronix.de \
--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 a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).