Messages in this thread | | | Date | Thu, 7 Apr 2022 17:58:09 -0700 | From | Guenter Roeck <> | Subject | Re: [RFC][PATCH] timers: Add del_time_free() to be called before freeing timers |
| |
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
| |