Re: [PATCH] dump_stack: Avoid the livelock of the dump_lock

[Kevin Hao]

On Tue, Oct 29, 2019 at 01:46:40PM +0100, Linus Torvalds wrote:
> Sorry for html crud, I'm at a conference without my laptop, so reading email on
> my phone..
> 
> On Tue, Oct 29, 2019, 10:31 Kevin Hao <haokexin@xxxxxxxxx> wrote:
> 
>     In the current code, we uses the atomic_cmpxchg() to serialize the
>     output of the dump_stack(), but this implementation suffers the
>     thundering herd problem. Actually we can use a spinlock here and leverage
>     the
> 
>     implementation of the spinlock(either ticket or queued spinlock) to
>     mediate such kind of livelock. Since the dump_stack() runs with the
>     irq disabled, so use the raw_spinlock_t to make it safe for rt kernel.
> 
> 
> The problem with this is that I think it will deadlock easily with NMI's, won't
> it?
> 
> There is a window between getting the spin lock and setting 'dump_cpu' where an
> incoming NMI would then deadlock because it also tries to get the lock, because
> it hasn't seen the fact that this cpu already got it.

Indeed, sorry I missed that.

> 
> The cmpxchg is supposed to protect against that, and make the locking be atomic
> with the CPU setting.
> 
> Can you try instead to make the retry loop not jump right back to the cmpxchg,
> but start looping just reading the CPU value, and only jumping back when it
> becomes -1?

Do you mean something like this?
diff --git a/lib/dump_stack.c b/lib/dump_stack.c
index 5cff72f18c4a..a0f1293a3638 100644
--- a/lib/dump_stack.c
+++ b/lib/dump_stack.c
@@ -107,6 +107,7 @@ asmlinkage __visible void dump_stack(void)
 	} else {
 		local_irq_restore(flags);
 		cpu_relax();
+		while (atomic_read(&dump_lock) != -1);
 		goto retry;
 	}
 
It does help. I will send a v2 if it pass more stress test.

Thanks,
Kevin

> 
>       Linus

Attachment: signature.asc
Description: PGP signature