From: Leonardo Bras <leonardo@linux.ibm.com>
To: Christophe Leroy <christophe.leroy@c-s.fr>,
Peter Zijlstra <peterz@infradead.org>,
Ingo Molnar <mingo@redhat.com>, Will Deacon <will@kernel.org>,
Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Paul Mackerras <paulus@samba.org>,
Michael Ellerman <mpe@ellerman.id.au>,
Enrico Weigelt <info@metux.net>,
Allison Randal <allison@lohutok.net>,
Thomas Gleixner <tglx@linutronix.de>
Cc: linuxppc-dev@lists.ozlabs.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/1] ppc/crash: Skip spinlocks during crash
Date: Mon, 30 Mar 2020 11:33:15 -0300 [thread overview]
Message-ID: <e1d8c41bb5d4f4a66be4edc8e2c80534f4abe2b4.camel@linux.ibm.com> (raw)
In-Reply-To: <4759f5e9-24a6-7710-86a0-c8e45f5decb7@c-s.fr>
[-- 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 --]
next prev parent reply other threads:[~2020-03-30 14:34 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
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 [this message]
2020-03-30 11:02 ` Peter Zijlstra
2020-03-30 14:12 ` Leonardo Bras
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=e1d8c41bb5d4f4a66be4edc8e2c80534f4abe2b4.camel@linux.ibm.com \
--to=leonardo@linux.ibm.com \
--cc=allison@lohutok.net \
--cc=benh@kernel.crashing.org \
--cc=christophe.leroy@c-s.fr \
--cc=info@metux.net \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=mingo@redhat.com \
--cc=mpe@ellerman.id.au \
--cc=paulus@samba.org \
--cc=peterz@infradead.org \
--cc=tglx@linutronix.de \
--cc=will@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).