[PATCH] x86/spinlocks: Avoid a deadlock when someone unlock a zapped ticked spinlock

[PATCH] x86/spinlocks: Avoid a deadlock when someone unlock a zapped ticked spinlock

[Petr Mladek]

There are few situations when we reinitialize (zap) ticket spinlocks. It
typically happens when the system is going down after an error and we
want to avoid deadlock in some important services. For example,
zap_locks() in printk.c and ioapic_zap_locks().

Peter pointed out that partial deadlock was still possible. It happens
when someone owns a ticket spinlock, we reinitialize it, and the old
owner releases it. Then the head is above the tail and the following
spin_lock() will never[*] succeed.

We could detect this situation in arch_spin_lock() and simply ignore
the superfluous head increment.

We need to do it in the lock() side because the unlock() side works
only with the head to avoid an overflow. Therefore we do not see
the consistent state of the head and the tail there.

Note that we could not check for (head == TICKET_LOCK_INC && !tail)
because the reinitialized lock might be taken several times before
the old owner releases the lock. By other words, the superfluous
head increment might happen at any time.

The change looks quite harmless. It should not affect the fast path
when the lock is taken immediately. It does not make worse the
situation when two processes might own the lock after zapping.
It just avoids the partial deadlock.

[*] unless the ticket number overflows.

Reported-by: Peter Zijlstra <peterz@infradead.org>
Signed-off-by: Petr Mladek <pmladek@suse.com>
---
 arch/x86/include/asm/spinlock.h | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/include/asm/spinlock.h b/arch/x86/include/asm/spinlock.h
index be0a05913b91..f732abf57c6f 100644
--- a/arch/x86/include/asm/spinlock.h
+++ b/arch/x86/include/asm/spinlock.h
@@ -105,12 +105,21 @@ static __always_inline int arch_spin_value_unlocked(arch_spinlock_t lock)
  */
 static __always_inline void arch_spin_lock(arch_spinlock_t *lock)
 {
-	register struct __raw_tickets inc = { .tail = TICKET_LOCK_INC };
+	register struct __raw_tickets inc;
 
+again:
+	inc = (struct __raw_tickets){ .tail = TICKET_LOCK_INC };
 	inc = xadd(&lock->tickets, inc);
 	if (likely(inc.head == inc.tail))
 		goto out;
 
+	/*
+	 * Avoid a stall when an old owner unlocked a reinitialized spinlock.
+	 * Simply ignore the superfluous increment of the head.
+	 */
+	if (unlikely(inc.head == inc.tail + TICKET_LOCK_INC))
+		goto again;
+
 	for (;;) {
 		unsigned count = SPIN_THRESHOLD;
 
-- 
1.8.5.6

Re: [PATCH] x86/spinlocks: Avoid a deadlock when someone unlock a zapped ticked spinlock

[Peter Zijlstra]

On Wed, Oct 21, 2015 at 11:18:09AM +0200, Petr Mladek wrote:
> There are few situations when we reinitialize (zap) ticket spinlocks. It
> typically happens when the system is going down after an error and we
> want to avoid deadlock in some important services. For example,
> zap_locks() in printk.c and ioapic_zap_locks().

So there's a few problems here. On x86 the code you patch is dead code,
x86 no longer uses ticket locks. Other archs might still.

And I entirely detest adding instructions to any lock path, be it the
utmost fast path or not, for something that will _never_ happen (on a
healthy system).

I would still very much recommend getting rid of the need for
zap_locks() in the first place.

What I did back when is punt on the whole printk buffer madness and dump
things to early_printk() without any locking.

I think that as long as the printk buffer has locks you have to accept
to loose some data when really bad stuff goes down.

Re: [PATCH] x86/spinlocks: Avoid a deadlock when someone unlock a zapped ticked spinlock

[Petr Mladek]

On Wed 2015-10-21 13:11:20, Peter Zijlstra wrote:
> On Wed, Oct 21, 2015 at 11:18:09AM +0200, Petr Mladek wrote:
> > There are few situations when we reinitialize (zap) ticket spinlocks. It
> > typically happens when the system is going down after an error and we
> > want to avoid deadlock in some important services. For example,
> > zap_locks() in printk.c and ioapic_zap_locks().
> 
> So there's a few problems here. On x86 the code you patch is dead code,
> x86 no longer uses ticket locks. Other archs might still.
>
> And I entirely detest adding instructions to any lock path, be it the
> utmost fast path or not, for something that will _never_ happen (on a
> healthy system).

OK

> I would still very much recommend getting rid of the need for
> zap_locks() in the first place.
>
> What I did back when is punt on the whole printk buffer madness and dump
> things to early_printk() without any locking.

My problem with this approach is that the early console is a black hole if
people do not watch it on a remote machine. We would stop using the
really used console even when it would work in most cases. IMHO,
zap_locks() is far from ideal but it would give better results in most
cases.

Maybe, we could add support to use early console in case of Oops and
panic. But we should not do this by default. It would help, especially
when the problem is reproducible and we get stacked with the normal
console.


> I think that as long as the printk buffer has locks you have to accept
> to loose some data when really bad stuff goes down.

Yup, but we could try a bit harder. I am afraid that lockless buffer,
e.g. ring_buffer is not an option here because it would be too complex,
rather slow, and hard to handle in a crash dump.

Thanks a lot for feedback.


Best Regards,
Petr

Re: [PATCH] x86/spinlocks: Avoid a deadlock when someone unlock a zapped ticked spinlock

[Peter Zijlstra]

On Thu, Oct 22, 2015 at 11:39:38AM +0200, Petr Mladek wrote:
> Maybe, we could add support to use early console in case of Oops and
> panic. But we should not do this by default. It would help, especially
> when the problem is reproducible and we get stacked with the normal
> console.

What I typically do is route everything to 8250 early console and rip
out everything printk buffer nonsense.

That gets you a fairly reliable console, no locking, only vscnprintf()
and OUTB banging.

No printk() wanting to do wakeups and other such nonsense that doesn't
work.

I appreciate that might not be for everybody, but it also shouldn't be
this hard to get something reliable.