All of lore.kernel.org
 help / color / mirror / Atom feed
From: Steven Rostedt <rostedt@goodmis.org>
To: Guenter Roeck <linux@roeck-us.net>
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 21:36:44 -0400	[thread overview]
Message-ID: <20220407213644.767ae1c7@gandalf.local.home> (raw)
In-Reply-To: <63d4c8d8-89c6-33ed-8178-be9ea86e53b9@roeck-us.net>

On Thu, 7 Apr 2022 17:58:09 -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

I meant replacing the entire block with del_timer_free().

diff --git a/sound/synth/emux/emux.c b/sound/synth/emux/emux.c
index 5ed8e36d2e04..f631e090e074 100644
--- a/sound/synth/emux/emux.c
+++ b/sound/synth/emux/emux.c
@@ -131,10 +131,7 @@ int snd_emux_free(struct snd_emux *emu)
 	if (! emu)
 		return -EINVAL;
 
-	spin_lock_irqsave(&emu->voice_lock, flags);
-	if (emu->timer_active)
-		del_timer(&emu->tlist);
-	spin_unlock_irqrestore(&emu->voice_lock, flags);
+	del_timer_free(&emu->tlist);
 
 	snd_emux_proc_free(emu);
 	snd_emux_delete_virmidi(emu);

It doesn't hurt to delete it if it wasn't queued. I'm not sure what the
dance with spinlocks are all about.

The above may actually be enough. I don't see where the timer could be
enqueued again after that.

That code goes back to original git history, so it was probably trying to
do it's own del_timer_sync() albeit poorly.

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

No, that doesn't make sense.

-- Steve

  reply	other threads:[~2022-04-08  1:36 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
2022-04-08  1:36       ` Steven Rostedt [this message]
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=20220407213644.767ae1c7@gandalf.local.home \
    --to=rostedt@goodmis.org \
    --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=linux@roeck-us.net \
    --cc=luiz.dentz@gmail.com \
    --cc=marcel@holtmann.org \
    --cc=peterz@infradead.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.