* Steven Rostedt | 2014-04-22 14:16:50 [-0400]: >On Tue, 22 Apr 2014 13:48:02 -0400 >Steven Rostedt <[email protected]> wrote: > >> I need to take a deeper look into the actual code. But as trylocks on >> UP are nops (always succeed), and if it expects to be able to do >> something in a critical section that is protected by spinlocks (again >> nops on UP), this would be broken for UP. > >Reading the code, I see it's broken. We should add something like this: > >Signed-off-by: Steven Rostedt <[email protected]> >--- >diff --git a/kernel/timer.c b/kernel/timer.c >index cc34e42..a03164a 100644 >--- a/kernel/timer.c >+++ b/kernel/timer.c >@@ -1447,6 +1447,12 @@ static void run_timer_softirq(struct softirq_action *h) > __run_timers(base); > } > >+#ifdef CONFIG_SMP >+#define timer_should_raise_softirq(lock) !spin_do_trylock(lock) >+#else >+#define timer_should_raise_softirq(lock) 1 >+#endif >+ > /* > * Called by the local, per-CPU timer interrupt on SMP. > */ >@@ -1467,7 +1473,7 @@ void run_local_timers(void) > return; > } > >- if (!spin_do_trylock(&base->lock)) { >+ if (timer_should_raise_softirq(&base->lock)) { > raise_softirq(TIMER_SOFTIRQ); > return; > }
Okay. So Peter said that it is okay to apply this since FULL_NO_HZ users wouldn't complain on UP. I still wouldn't say it is broken but that is a different story. We have two users of this trylock. run_local_timers() which pops up quite often (and you patched here) and the other is get_next_timer_interrupt(). What do you suggest we do here? It is basically the same thing. Sebastian -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
