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 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.