LKML Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH 1/1] ppc/crash: Skip spinlocks during crash
@ 2020-03-26 22:28 Leonardo Bras
  2020-03-26 23:26 ` Leonardo Bras
  2020-03-27  6:50 ` Christophe Leroy
  0 siblings, 2 replies; 8+ messages in thread
From: Leonardo Bras @ 2020-03-26 22:28 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Will Deacon, Benjamin Herrenschmidt,
	Paul Mackerras, Michael Ellerman, Enrico Weigelt, Leonardo Bras,
	Allison Randal, Christophe Leroy, Thomas Gleixner
  Cc: linux-kernel, linuxppc-dev

During a crash, there is chance that the cpus that handle the NMI IPI
are holding a spin_lock. If this spin_lock is needed by crashing_cpu it
will cause a deadlock. (rtas_lock and printk logbuf_log as of today)

This is a problem if the system has kdump set up, given if it crashes
for any reason kdump may not be saved for crash analysis.

Skip spinlocks after NMI IPI is sent to all other cpus.

Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
---
 arch/powerpc/include/asm/spinlock.h | 6 ++++++
 arch/powerpc/kexec/crash.c          | 3 +++
 2 files changed, 9 insertions(+)

diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
index 860228e917dc..a6381d110795 100644
--- a/arch/powerpc/include/asm/spinlock.h
+++ b/arch/powerpc/include/asm/spinlock.h
@@ -111,6 +111,8 @@ static inline void splpar_spin_yield(arch_spinlock_t *lock) {};
 static inline void splpar_rw_yield(arch_rwlock_t *lock) {};
 #endif
 
+extern bool crash_skip_spinlock __read_mostly;
+
 static inline bool is_shared_processor(void)
 {
 #ifdef CONFIG_PPC_SPLPAR
@@ -142,6 +144,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
 		if (likely(__arch_spin_trylock(lock) == 0))
 			break;
 		do {
+			if (unlikely(crash_skip_spinlock))
+				return;
 			HMT_low();
 			if (is_shared_processor())
 				splpar_spin_yield(lock);
@@ -161,6 +165,8 @@ void arch_spin_lock_flags(arch_spinlock_t *lock, unsigned long flags)
 		local_save_flags(flags_dis);
 		local_irq_restore(flags);
 		do {
+			if (unlikely(crash_skip_spinlock))
+				return;
 			HMT_low();
 			if (is_shared_processor())
 				splpar_spin_yield(lock);
diff --git a/arch/powerpc/kexec/crash.c b/arch/powerpc/kexec/crash.c
index d488311efab1..8a522380027d 100644
--- a/arch/powerpc/kexec/crash.c
+++ b/arch/powerpc/kexec/crash.c
@@ -66,6 +66,8 @@ static int handle_fault(struct pt_regs *regs)
 
 #ifdef CONFIG_SMP
 
+bool crash_skip_spinlock;
+
 static atomic_t cpus_in_crash;
 void crash_ipi_callback(struct pt_regs *regs)
 {
@@ -129,6 +131,7 @@ static void crash_kexec_prepare_cpus(int cpu)
 	/* Would it be better to replace the trap vector here? */
 
 	if (atomic_read(&cpus_in_crash) >= ncpus) {
+		crash_skip_spinlock = true;
 		printk(KERN_EMERG "IPI complete\n");
 		return;
 	}
-- 
2.24.1


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] ppc/crash: Skip spinlocks during crash
  2020-03-26 22:28 [PATCH 1/1] ppc/crash: Skip spinlocks during crash Leonardo Bras
@ 2020-03-26 23:26 ` Leonardo Bras
  2020-03-27  6:50 ` Christophe Leroy
  1 sibling, 0 replies; 8+ messages in thread
From: Leonardo Bras @ 2020-03-26 23:26 UTC (permalink / raw)
  To: Peter Zijlstra, Ingo Molnar, Will Deacon, Benjamin Herrenschmidt,
	Paul Mackerras, Michael Ellerman, Enrico Weigelt, Allison Randal,
	Christophe Leroy, Thomas Gleixner
  Cc: linux-kernel, linuxppc-dev


[-- Attachment #1: Type: text/plain, Size: 2613 bytes --]

oops, forgot to EXPORT_SYMBOL. 
arch_spin_lock*() is used on modules.

Sending v2.

On Thu, 2020-03-26 at 19:28 -0300, Leonardo Bras wrote:
> During a crash, there is chance that the cpus that handle the NMI IPI
> are holding a spin_lock. If this spin_lock is needed by crashing_cpu it
> will cause a deadlock. (rtas_lock and printk logbuf_log as of today)
> 
> This is a problem if the system has kdump set up, given if it crashes
> for any reason kdump may not be saved for crash analysis.
> 
> Skip spinlocks after NMI IPI is sent to all other cpus.
> 
> Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
> ---
>  arch/powerpc/include/asm/spinlock.h | 6 ++++++
>  arch/powerpc/kexec/crash.c          | 3 +++
>  2 files changed, 9 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
> index 860228e917dc..a6381d110795 100644
> --- a/arch/powerpc/include/asm/spinlock.h
> +++ b/arch/powerpc/include/asm/spinlock.h
> @@ -111,6 +111,8 @@ static inline void splpar_spin_yield(arch_spinlock_t *lock) {};
>  static inline void splpar_rw_yield(arch_rwlock_t *lock) {};
>  #endif
>  
> +extern bool crash_skip_spinlock __read_mostly;
> +
>  static inline bool is_shared_processor(void)
>  {
>  #ifdef CONFIG_PPC_SPLPAR
> @@ -142,6 +144,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
>  		if (likely(__arch_spin_trylock(lock) == 0))
>  			break;
>  		do {
> +			if (unlikely(crash_skip_spinlock))
> +				return;
>  			HMT_low();
>  			if (is_shared_processor())
>  				splpar_spin_yield(lock);
> @@ -161,6 +165,8 @@ void arch_spin_lock_flags(arch_spinlock_t *lock, unsigned long flags)
>  		local_save_flags(flags_dis);
>  		local_irq_restore(flags);
>  		do {
> +			if (unlikely(crash_skip_spinlock))
> +				return;
>  			HMT_low();
>  			if (is_shared_processor())
>  				splpar_spin_yield(lock);
> diff --git a/arch/powerpc/kexec/crash.c b/arch/powerpc/kexec/crash.c
> index d488311efab1..8a522380027d 100644
> --- a/arch/powerpc/kexec/crash.c
> +++ b/arch/powerpc/kexec/crash.c
> @@ -66,6 +66,8 @@ static int handle_fault(struct pt_regs *regs)
>  
>  #ifdef CONFIG_SMP
>  
> +bool crash_skip_spinlock;
> +
>  static atomic_t cpus_in_crash;
>  void crash_ipi_callback(struct pt_regs *regs)
>  {
> @@ -129,6 +131,7 @@ static void crash_kexec_prepare_cpus(int cpu)
>  	/* Would it be better to replace the trap vector here? */
>  
>  	if (atomic_read(&cpus_in_crash) >= ncpus) {
> +		crash_skip_spinlock = true;
>  		printk(KERN_EMERG "IPI complete\n");
>  		return;
>  	}

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] ppc/crash: Skip spinlocks during crash
  2020-03-26 22:28 [PATCH 1/1] ppc/crash: Skip spinlocks during crash Leonardo Bras
  2020-03-26 23:26 ` Leonardo Bras
@ 2020-03-27  6:50 ` Christophe Leroy
  2020-03-27 15:51   ` Leonardo Bras
  2020-03-30 11:02   ` Peter Zijlstra
  1 sibling, 2 replies; 8+ messages in thread
From: Christophe Leroy @ 2020-03-27  6:50 UTC (permalink / raw)
  To: Leonardo Bras, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Enrico Weigelt, Allison Randal, Thomas Gleixner
  Cc: linux-kernel, linuxppc-dev



Le 26/03/2020 à 23:28, Leonardo Bras a écrit :
> During a crash, there is chance that the cpus that handle the NMI IPI
> are holding a spin_lock. If this spin_lock is needed by crashing_cpu it
> will cause a deadlock. (rtas_lock and printk logbuf_log as of today)
> 
> This is a problem if the system has kdump set up, given if it crashes
> for any reason kdump may not be saved for crash analysis.
> 
> Skip spinlocks after NMI IPI is sent to all other cpus.
> 
> Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
> ---
>   arch/powerpc/include/asm/spinlock.h | 6 ++++++
>   arch/powerpc/kexec/crash.c          | 3 +++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
> index 860228e917dc..a6381d110795 100644
> --- a/arch/powerpc/include/asm/spinlock.h
> +++ b/arch/powerpc/include/asm/spinlock.h
> @@ -111,6 +111,8 @@ static inline void splpar_spin_yield(arch_spinlock_t *lock) {};
>   static inline void splpar_rw_yield(arch_rwlock_t *lock) {};
>   #endif
>   
> +extern bool crash_skip_spinlock __read_mostly;
> +
>   static inline bool is_shared_processor(void)
>   {
>   #ifdef CONFIG_PPC_SPLPAR
> @@ -142,6 +144,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
>   		if (likely(__arch_spin_trylock(lock) == 0))
>   			break;
>   		do {
> +			if (unlikely(crash_skip_spinlock))
> +				return;

You are adding a test that reads a global var in the middle of a so hot 
path ? That must kill performance. Can we do different ?

Christophe

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] ppc/crash: Skip spinlocks during crash
  2020-03-27  6:50 ` Christophe Leroy
@ 2020-03-27 15:51   ` Leonardo Bras
  2020-03-28 10:19     ` Christophe Leroy
  2020-03-30 11:02   ` Peter Zijlstra
  1 sibling, 1 reply; 8+ messages in thread
From: Leonardo Bras @ 2020-03-27 15:51 UTC (permalink / raw)
  To: Christophe Leroy, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Enrico Weigelt, Allison Randal, Thomas Gleixner
  Cc: linuxppc-dev, linux-kernel


[-- Attachment #1: Type: text/plain, Size: 1664 bytes --]

Hello Christophe, thanks for the feedback.

I noticed an error in this patch and sent a v2, that can be seen here:
http://patchwork.ozlabs.org/patch/1262468/

Comments inline::

On Fri, 2020-03-27 at 07:50 +0100, Christophe Leroy wrote:
> > @@ -142,6 +144,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
> >   		if (likely(__arch_spin_trylock(lock) == 0))
> >   			break;
> >   		do {
> > +			if (unlikely(crash_skip_spinlock))
> > +				return;

Complete function for reference:
static inline void arch_spin_lock(arch_spinlock_t *lock)
{
	while (1) {
		if (likely(__arch_spin_trylock(lock) == 0))
			break;
		do {
			if (unlikely(crash_skip_spinlock))
				return;
			HMT_low();
			if (is_shared_processor())
				splpar_spin_yield(lock);
		} while (unlikely(lock->slock != 0));
		HMT_medium();
	}
}

> You are adding a test that reads a global var in the middle of a so hot 
> path ? That must kill performance. 

I thought it would, in worst case scenario, increase a maximum delay of
an arch_spin_lock() call 1 spin cycle. Here is what I thought:

- If the lock is already free, it would change nothing, 
- Otherwise, the lock will wait.
- Waiting cycle just got bigger.
- Worst case scenario: running one more cycle, given lock->slock can
turn to 0 just after checking.

Could you please point where I failed to see the performance penalty?
(I need to get better at this :) )


> Can we do different ?

Sure, a less intrusive way of doing it would be to free the currently
needed locks before proceeding. I just thought it would be harder to
maintain.

> Christophe

Best regards,
Leonardo

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] ppc/crash: Skip spinlocks during crash
  2020-03-27 15:51   ` Leonardo Bras
@ 2020-03-28 10:19     ` Christophe Leroy
  2020-03-30 14:33       ` Leonardo Bras
  0 siblings, 1 reply; 8+ messages in thread
From: Christophe Leroy @ 2020-03-28 10:19 UTC (permalink / raw)
  To: Leonardo Bras, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Enrico Weigelt, Allison Randal, Thomas Gleixner
  Cc: linuxppc-dev, linux-kernel

Hi Leonardo,

On 03/27/2020 03:51 PM, Leonardo Bras wrote:
> Hello Christophe, thanks for the feedback.
> 
> I noticed an error in this patch and sent a v2, that can be seen here:
> http://patchwork.ozlabs.org/patch/1262468/
> 
> Comments inline::
> 
> On Fri, 2020-03-27 at 07:50 +0100, Christophe Leroy wrote:
>>> @@ -142,6 +144,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
>>>    		if (likely(__arch_spin_trylock(lock) == 0))
>>>    			break;
>>>    		do {
>>> +			if (unlikely(crash_skip_spinlock))
>>> +				return;
> 
> Complete function for reference:
> static inline void arch_spin_lock(arch_spinlock_t *lock)
> {
> 	while (1) {
> 		if (likely(__arch_spin_trylock(lock) == 0))
> 			break;
> 		do {
> 			if (unlikely(crash_skip_spinlock))
> 				return;
> 			HMT_low();
> 			if (is_shared_processor())
> 				splpar_spin_yield(lock);
> 		} while (unlikely(lock->slock != 0));
> 		HMT_medium();
> 	}
> }
> 
>> You are adding a test that reads a global var in the middle of a so hot
>> path ? That must kill performance.
> 
> I thought it would, in worst case scenario, increase a maximum delay of
> an arch_spin_lock() call 1 spin cycle. Here is what I thought:
> 
> - If the lock is already free, it would change nothing,
> - Otherwise, the lock will wait.
> - Waiting cycle just got bigger.
> - Worst case scenario: running one more cycle, given lock->slock can
> turn to 0 just after checking.
> 
> Could you please point where I failed to see the performance penalty?
> (I need to get better at this :) )

You are right that when the lock is free, it changes nothing. However 
when it is not, it is not just one cycle.

Here is arch_spin_lock() without your patch:

00000440 <my_lock>:
  440:	39 40 00 01 	li      r10,1
  444:	7d 20 18 28 	lwarx   r9,0,r3
  448:	2c 09 00 00 	cmpwi   r9,0
  44c:	40 82 00 10 	bne     45c <my_lock+0x1c>
  450:	7d 40 19 2d 	stwcx.  r10,0,r3
  454:	40 a2 ff f0 	bne     444 <my_lock+0x4>
  458:	4c 00 01 2c 	isync
  45c:	2f 89 00 00 	cmpwi   cr7,r9,0
  460:	4d be 00 20 	bclr+   12,4*cr7+eq
  464:	7c 21 0b 78 	mr      r1,r1
  468:	81 23 00 00 	lwz     r9,0(r3)
  46c:	2f 89 00 00 	cmpwi   cr7,r9,0
  470:	40 be ff f4 	bne     cr7,464 <my_lock+0x24>
  474:	7c 42 13 78 	mr      r2,r2
  478:	7d 20 18 28 	lwarx   r9,0,r3
  47c:	2c 09 00 00 	cmpwi   r9,0
  480:	40 82 00 10 	bne     490 <my_lock+0x50>
  484:	7d 40 19 2d 	stwcx.  r10,0,r3
  488:	40 a2 ff f0 	bne     478 <my_lock+0x38>
  48c:	4c 00 01 2c 	isync
  490:	2f 89 00 00 	cmpwi   cr7,r9,0
  494:	40 be ff d0 	bne     cr7,464 <my_lock+0x24>
  498:	4e 80 00 20 	blr

Here is arch_spin_lock() with your patch. I enclose with === what comes 
in addition:

00000440 <my_lock>:
  440:	39 40 00 01 	li      r10,1
  444:	7d 20 18 28 	lwarx   r9,0,r3
  448:	2c 09 00 00 	cmpwi   r9,0
  44c:	40 82 00 10 	bne     45c <my_lock+0x1c>
  450:	7d 40 19 2d 	stwcx.  r10,0,r3
  454:	40 a2 ff f0 	bne     444 <my_lock+0x4>
  458:	4c 00 01 2c 	isync
  45c:	2f 89 00 00 	cmpwi   cr7,r9,0
  460:	4d be 00 20 	bclr+   12,4*cr7+eq
=====================================================
  464:	3d 40 00 00 	lis     r10,0
			466: R_PPC_ADDR16_HA	crash_skip_spinlock
  468:	39 4a 00 00 	addi    r10,r10,0
			46a: R_PPC_ADDR16_LO	crash_skip_spinlock
  46c:	39 00 00 01 	li      r8,1
  470:	89 2a 00 00 	lbz     r9,0(r10)
  474:	2f 89 00 00 	cmpwi   cr7,r9,0
  478:	4c 9e 00 20 	bnelr   cr7
=====================================================
  47c:	7c 21 0b 78 	mr      r1,r1
  480:	81 23 00 00 	lwz     r9,0(r3)
  484:	2f 89 00 00 	cmpwi   cr7,r9,0
  488:	40 be ff f4 	bne     cr7,47c <my_lock+0x3c>
  48c:	7c 42 13 78 	mr      r2,r2
  490:	7d 20 18 28 	lwarx   r9,0,r3
  494:	2c 09 00 00 	cmpwi   r9,0
  498:	40 82 00 10 	bne     4a8 <my_lock+0x68>
  49c:	7d 00 19 2d 	stwcx.  r8,0,r3
  4a0:	40 a2 ff f0 	bne     490 <my_lock+0x50>
  4a4:	4c 00 01 2c 	isync
  4a8:	2f 89 00 00 	cmpwi   cr7,r9,0
  4ac:	40 be ff c4 	bne     cr7,470 <my_lock+0x30>
  4b0:	4e 80 00 20 	blr


Christophe

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] ppc/crash: Skip spinlocks during crash
  2020-03-27  6:50 ` Christophe Leroy
  2020-03-27 15:51   ` Leonardo Bras
@ 2020-03-30 11:02   ` Peter Zijlstra
  2020-03-30 14:12     ` Leonardo Bras
  1 sibling, 1 reply; 8+ messages in thread
From: Peter Zijlstra @ 2020-03-30 11:02 UTC (permalink / raw)
  To: Christophe Leroy
  Cc: Leonardo Bras, Ingo Molnar, Will Deacon, Benjamin Herrenschmidt,
	Paul Mackerras, Michael Ellerman, Enrico Weigelt, Allison Randal,
	Thomas Gleixner, linux-kernel, linuxppc-dev

On Fri, Mar 27, 2020 at 07:50:20AM +0100, Christophe Leroy wrote:
> 
> 
> Le 26/03/2020 à 23:28, Leonardo Bras a écrit :
> > During a crash, there is chance that the cpus that handle the NMI IPI
> > are holding a spin_lock. If this spin_lock is needed by crashing_cpu it
> > will cause a deadlock. (rtas_lock and printk logbuf_log as of today)
> > 
> > This is a problem if the system has kdump set up, given if it crashes
> > for any reason kdump may not be saved for crash analysis.
> > 
> > Skip spinlocks after NMI IPI is sent to all other cpus.
> > 
> > Signed-off-by: Leonardo Bras <leonardo@linux.ibm.com>
> > ---
> >   arch/powerpc/include/asm/spinlock.h | 6 ++++++
> >   arch/powerpc/kexec/crash.c          | 3 +++
> >   2 files changed, 9 insertions(+)
> > 
> > diff --git a/arch/powerpc/include/asm/spinlock.h b/arch/powerpc/include/asm/spinlock.h
> > index 860228e917dc..a6381d110795 100644
> > --- a/arch/powerpc/include/asm/spinlock.h
> > +++ b/arch/powerpc/include/asm/spinlock.h
> > @@ -111,6 +111,8 @@ static inline void splpar_spin_yield(arch_spinlock_t *lock) {};
> >   static inline void splpar_rw_yield(arch_rwlock_t *lock) {};
> >   #endif
> > +extern bool crash_skip_spinlock __read_mostly;
> > +
> >   static inline bool is_shared_processor(void)
> >   {
> >   #ifdef CONFIG_PPC_SPLPAR
> > @@ -142,6 +144,8 @@ static inline void arch_spin_lock(arch_spinlock_t *lock)
> >   		if (likely(__arch_spin_trylock(lock) == 0))
> >   			break;
> >   		do {
> > +			if (unlikely(crash_skip_spinlock))
> > +				return;
> 
> You are adding a test that reads a global var in the middle of a so hot path
> ? That must kill performance. Can we do different ?

This; adding code to a super hot patch like this for an exceptional case
like the crash handling seems like a very very bad trade to me.

One possible solution is to simply write 0 to the affected spinlocks
after sending the NMI IPI thing, no?

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] ppc/crash: Skip spinlocks during crash
  2020-03-30 11:02   ` Peter Zijlstra
@ 2020-03-30 14:12     ` Leonardo Bras
  0 siblings, 0 replies; 8+ messages in thread
From: Leonardo Bras @ 2020-03-30 14:12 UTC (permalink / raw)
  To: Peter Zijlstra, Christophe Leroy
  Cc: Ingo Molnar, Will Deacon, Benjamin Herrenschmidt, Paul Mackerras,
	Michael Ellerman, Enrico Weigelt, Allison Randal,
	Thomas Gleixner, linux-kernel, linuxppc-dev


[-- Attachment #1: Type: text/plain, Size: 677 bytes --]

Hello Peter, 

On Mon, 2020-03-30 at 13:02 +0200, Peter Zijlstra wrote:
>  		do {
> > > +			if (unlikely(crash_skip_spinlock))
> > > +				return;
> > 
> > You are adding a test that reads a global var in the middle of a so hot path
> > ? That must kill performance. Can we do different ?
> 
> This; adding code to a super hot patch like this for an exceptional case
> like the crash handling seems like a very very bad trade to me.
> 
> One possible solution is to simply write 0 to the affected spinlocks
> after sending the NMI IPI thing, no?

Yes, I agree.
I suggested this on a comment in v2 of this patch:
http://patchwork.ozlabs.org/patch/1262468/



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/1] ppc/crash: Skip spinlocks during crash
  2020-03-28 10:19     ` Christophe Leroy
@ 2020-03-30 14:33       ` Leonardo Bras
  0 siblings, 0 replies; 8+ messages in thread
From: Leonardo Bras @ 2020-03-30 14:33 UTC (permalink / raw)
  To: Christophe Leroy, Peter Zijlstra, Ingo Molnar, Will Deacon,
	Benjamin Herrenschmidt, Paul Mackerras, Michael Ellerman,
	Enrico Weigelt, Allison Randal, Thomas Gleixner
  Cc: linuxppc-dev, linux-kernel


[-- Attachment #1: Type: text/plain, Size: 3551 bytes --]

Hello Christophe,

On Sat, 2020-03-28 at 10:19 +0000, Christophe Leroy wrote:
> Hi Leonardo,
> 
> 
> > On 03/27/2020 03:51 PM, Leonardo Bras wrote:
> > > 
> > [SNIP]
> > - If the lock is already free, it would change nothing,
> > - Otherwise, the lock will wait.
> > - Waiting cycle just got bigger.
> > - Worst case scenario: running one more cycle, given lock->slock can
> > turn to 0 just after checking.Could you please point where I failed to see the performance penalty?
> > (I need to get better at this :) )
> 
> You are right that when the lock is free, it changes nothing. However 
> when it is not, it is not just one cycle.

Sorry, what I meant here is one "waiting cycle", meaning that in WCS
there would be 1 extra iteration on that while. Or it would 'spin' one
more time.

> 
> Here is arch_spin_lock() without your patch:
> 
> 00000440 <my_lock>:
>   440:	39 40 00 01 	li      r10,1
>   444:	7d 20 18 28 	lwarx   r9,0,r3
>   448:	2c 09 00 00 	cmpwi   r9,0
>   44c:	40 82 00 10 	bne     45c <my_lock+0x1c>
>   450:	7d 40 19 2d 	stwcx.  r10,0,r3
>   454:	40 a2 ff f0 	bne     444 <my_lock+0x4>
>   458:	4c 00 01 2c 	isync
>   45c:	2f 89 00 00 	cmpwi   cr7,r9,0
>   460:	4d be 00 20 	bclr+   12,4*cr7+eq
>   464:	7c 21 0b 78 	mr      r1,r1
>   468:	81 23 00 00 	lwz     r9,0(r3)
>   46c:	2f 89 00 00 	cmpwi   cr7,r9,0
>   470:	40 be ff f4 	bne     cr7,464 <my_lock+0x24>
>   474:	7c 42 13 78 	mr      r2,r2
>   478:	7d 20 18 28 	lwarx   r9,0,r3
>   47c:	2c 09 00 00 	cmpwi   r9,0
>   480:	40 82 00 10 	bne     490 <my_lock+0x50>
>   484:	7d 40 19 2d 	stwcx.  r10,0,r3
>   488:	40 a2 ff f0 	bne     478 <my_lock+0x38>
>   48c:	4c 00 01 2c 	isync
>   490:	2f 89 00 00 	cmpwi   cr7,r9,0
>   494:	40 be ff d0 	bne     cr7,464 <my_lock+0x24>
>   498:	4e 80 00 20 	blr
> 
> Here is arch_spin_lock() with your patch. I enclose with === what comes 
> in addition:
> 
> 00000440 <my_lock>:
>   440:	39 40 00 01 	li      r10,1
>   444:	7d 20 18 28 	lwarx   r9,0,r3
>   448:	2c 09 00 00 	cmpwi   r9,0
>   44c:	40 82 00 10 	bne     45c <my_lock+0x1c>
>   450:	7d 40 19 2d 	stwcx.  r10,0,r3
>   454:	40 a2 ff f0 	bne     444 <my_lock+0x4>
>   458:	4c 00 01 2c 	isync
>   45c:	2f 89 00 00 	cmpwi   cr7,r9,0
>   460:	4d be 00 20 	bclr+   12,4*cr7+eq
> =====================================================
>   464:	3d 40 00 00 	lis     r10,0
> 			466: R_PPC_ADDR16_HA	crash_skip_spinlock
>   468:	39 4a 00 00 	addi    r10,r10,0
> 			46a: R_PPC_ADDR16_LO	crash_skip_spinlock
>   46c:	39 00 00 01 	li      r8,1
>   470:	89 2a 00 00 	lbz     r9,0(r10)
>   474:	2f 89 00 00 	cmpwi   cr7,r9,0
>   478:	4c 9e 00 20 	bnelr   cr7
> =====================================================
>   47c:	7c 21 0b 78 	mr      r1,r1
>   480:	81 23 00 00 	lwz     r9,0(r3)
>   484:	2f 89 00 00 	cmpwi   cr7,r9,0
>   488:	40 be ff f4 	bne     cr7,47c <my_lock+0x3c>
>   48c:	7c 42 13 78 	mr      r2,r2
>   490:	7d 20 18 28 	lwarx   r9,0,r3
>   494:	2c 09 00 00 	cmpwi   r9,0
>   498:	40 82 00 10 	bne     4a8 <my_lock+0x68>
>   49c:	7d 00 19 2d 	stwcx.  r8,0,r3
>   4a0:	40 a2 ff f0 	bne     490 <my_lock+0x50>
>   4a4:	4c 00 01 2c 	isync
>   4a8:	2f 89 00 00 	cmpwi   cr7,r9,0
>   4ac:	40 be ff c4 	bne     cr7,470 <my_lock+0x30>
>   4b0:	4e 80 00 20 	blr
> 
> 
> Christophe

I agree. When there is waiting, it will usually add some time to it.
Accounting that spinlocks are widely used, it will cause a slowdown in
the whole system.

Thanks for the feedback,
Best regards,

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, back to index

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-26 22:28 [PATCH 1/1] ppc/crash: Skip spinlocks during crash Leonardo Bras
2020-03-26 23:26 ` Leonardo Bras
2020-03-27  6:50 ` Christophe Leroy
2020-03-27 15:51   ` Leonardo Bras
2020-03-28 10:19     ` Christophe Leroy
2020-03-30 14:33       ` Leonardo Bras
2020-03-30 11:02   ` Peter Zijlstra
2020-03-30 14:12     ` Leonardo Bras

LKML Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lkml.kernel.org/lkml/0 lkml/git/0.git
	git clone --mirror https://lkml.kernel.org/lkml/1 lkml/git/1.git
	git clone --mirror https://lkml.kernel.org/lkml/2 lkml/git/2.git
	git clone --mirror https://lkml.kernel.org/lkml/3 lkml/git/3.git
	git clone --mirror https://lkml.kernel.org/lkml/4 lkml/git/4.git
	git clone --mirror https://lkml.kernel.org/lkml/5 lkml/git/5.git
	git clone --mirror https://lkml.kernel.org/lkml/6 lkml/git/6.git
	git clone --mirror https://lkml.kernel.org/lkml/7 lkml/git/7.git
	git clone --mirror https://lkml.kernel.org/lkml/8 lkml/git/8.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 lkml lkml/ https://lkml.kernel.org/lkml \
		linux-kernel@vger.kernel.org
	public-inbox-index lkml

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-kernel


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git