From e81ce1f7ecdaed2844c75313b09af791d44e6373 Mon Sep 17 00:00:00 2001 From: Heiko Carstens Date: Mon, 5 Mar 2007 00:30:51 -0800 Subject: [PATCH] [PATCH] timer/hrtimer: take per cpu locks in sane order Doing something like this on a two cpu system # echo 0 > /sys/devices/system/cpu/cpu0/online # echo 1 > /sys/devices/system/cpu/cpu0/online # echo 0 > /sys/devices/system/cpu/cpu1/online will give me this: ======================================================= [ INFO: possible circular locking dependency detected ] 2.6.21-rc2-g562aa1d4-dirty #7 ------------------------------------------------------- bash/1282 is trying to acquire lock: (&cpu_base->lock_key){.+..}, at: [<000000000005f17e>] hrtimer_cpu_notify+0xc6/0x240 but task is already holding lock: (&cpu_base->lock_key#2){.+..}, at: [<000000000005f174>] hrtimer_cpu_notify+0xbc/0x240 which lock already depends on the new lock. This happens because we have the following code in kernel/hrtimer.c: migrate_hrtimers(int cpu) [...] old_base = &per_cpu(hrtimer_bases, cpu); new_base = &get_cpu_var(hrtimer_bases); [...] spin_lock(&new_base->lock); spin_lock(&old_base->lock); Which means the spinlocks are taken in an order which depends on which cpu gets shut down from which other cpu. Therefore lockdep complains that there might be an ABBA deadlock. Since migrate_hrtimers() gets only called on cpu hotplug it's safe to assume that it isn't executed concurrently on a The same problem exists in kernel/timer.c: migrate_timers(). As pointed out by Christian Borntraeger one possible solution to avoid the locking order complaints would be to make sure that the locks are always taken in the same order. E.g. by taking the lock of the cpu with the lower number first. To achieve this we introduce two new spinlock functions double_spin_lock and double_spin_unlock which lock or unlock two locks in a given order. Cc: Ingo Molnar Cc: Thomas Gleixner Cc: Roman Zippel Cc: John Stultz Cc: Christian Borntraeger Cc: Martin Schwidefsky Signed-off-by: Heiko Carstens Signed-off-by: Andrew Morton Signed-off-by: Linus Torvalds --- include/linux/spinlock.h | 37 +++++++++++++++++++++++++++++++++++++ kernel/hrtimer.c | 9 ++++----- kernel/timer.c | 8 ++++---- 3 files changed, 45 insertions(+), 9 deletions(-) diff --git a/include/linux/spinlock.h b/include/linux/spinlock.h index 61fef376ed..a946176db6 100644 --- a/include/linux/spinlock.h +++ b/include/linux/spinlock.h @@ -282,6 +282,43 @@ do { \ 1 : ({ local_irq_restore(flags); 0; }); \ }) +/* + * Locks two spinlocks l1 and l2. + * l1_first indicates if spinlock l1 should be taken first. + */ +static inline void double_spin_lock(spinlock_t *l1, spinlock_t *l2, + bool l1_first) + __acquires(l1) + __acquires(l2) +{ + if (l1_first) { + spin_lock(l1); + spin_lock(l2); + } else { + spin_lock(l2); + spin_lock(l1); + } +} + +/* + * Unlocks two spinlocks l1 and l2. + * l1_taken_first indicates if spinlock l1 was taken first and therefore + * should be released after spinlock l2. + */ +static inline void double_spin_unlock(spinlock_t *l1, spinlock_t *l2, + bool l1_taken_first) + __releases(l1) + __releases(l2) +{ + if (l1_taken_first) { + spin_unlock(l2); + spin_unlock(l1); + } else { + spin_unlock(l1); + spin_unlock(l2); + } +} + /* * Pull the atomic_t declaration: * (asm-mips/atomic.h needs above definitions) diff --git a/kernel/hrtimer.c b/kernel/hrtimer.c index 476cb0c0b4..de93a8176c 100644 --- a/kernel/hrtimer.c +++ b/kernel/hrtimer.c @@ -1355,17 +1355,16 @@ static void migrate_hrtimers(int cpu) tick_cancel_sched_timer(cpu); local_irq_disable(); - - spin_lock(&new_base->lock); - spin_lock(&old_base->lock); + double_spin_lock(&new_base->lock, &old_base->lock, + smp_processor_id() < cpu); for (i = 0; i < HRTIMER_MAX_CLOCK_BASES; i++) { migrate_hrtimer_list(&old_base->clock_base[i], &new_base->clock_base[i]); } - spin_unlock(&old_base->lock); - spin_unlock(&new_base->lock); + double_spin_unlock(&new_base->lock, &old_base->lock, + smp_processor_id() < cpu); local_irq_enable(); put_cpu_var(hrtimer_bases); } diff --git a/kernel/timer.c b/kernel/timer.c index 6663a87f73..8ad384253e 100644 --- a/kernel/timer.c +++ b/kernel/timer.c @@ -1651,8 +1651,8 @@ static void __devinit migrate_timers(int cpu) new_base = get_cpu_var(tvec_bases); local_irq_disable(); - spin_lock(&new_base->lock); - spin_lock(&old_base->lock); + double_spin_lock(&new_base->lock, &old_base->lock, + smp_processor_id() < cpu); BUG_ON(old_base->running_timer); @@ -1665,8 +1665,8 @@ static void __devinit migrate_timers(int cpu) migrate_timer_list(new_base, old_base->tv5.vec + i); } - spin_unlock(&old_base->lock); - spin_unlock(&new_base->lock); + double_spin_unlock(&new_base->lock, &old_base->lock, + smp_processor_id() < cpu); local_irq_enable(); put_cpu_var(tvec_bases); } -- 2.39.5