Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1472254 > unrolled thread
| Started by | Kenneth Lee <liguozhu@hisilicon.com> |
|---|---|
| First post | 2016-08-30 08:20 +0200 |
| Last post | 2016-09-01 12:30 +0200 |
| Articles | 4 — 3 participants |
Back to article view | Back to linux.kernel
[PATCH] [bugfix] replace unnessary ldax with common ldr Kenneth Lee <liguozhu@hisilicon.com> - 2016-08-30 08:20 +0200
Re: [PATCH] [bugfix] replace unnessary ldax with common ldr Catalin Marinas <catalin.marinas@arm.com> - 2016-08-30 11:10 +0200
Re: [PATCH] [bugfix] replace unnessary ldax with common ldr Vladimir Murzin <vladimir.murzin@arm.com> - 2016-08-31 15:40 +0200
Re: [PATCH] [bugfix] replace unnessary ldax with common ldr Catalin Marinas <catalin.marinas@arm.com> - 2016-09-01 12:30 +0200
| From | Kenneth Lee <liguozhu@hisilicon.com> |
|---|---|
| Date | 2016-08-30 08:20 +0200 |
| Subject | [PATCH] [bugfix] replace unnessary ldax with common ldr |
| Message-ID | <sbKyR-15N-3@gated-at.bofh.it> |
(add comment for the previous mail, sorry for the duplication) There is no store_ex pairing with this load_ex. It is not necessary and gave wrong hint to the cache system. Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com> --- arch/arm64/include/asm/spinlock.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h index c85e96d..3334c4f 100644 --- a/arch/arm64/include/asm/spinlock.h +++ b/arch/arm64/include/asm/spinlock.h @@ -63,7 +63,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) */ " sevl\n" "2: wfe\n" -" ldaxrh %w2, %4\n" +" ldrh %w2, %4\n" " eor %w1, %w2, %w0, lsr #16\n" " cbnz %w1, 2b\n" /* We got the lock. Critical section starts here. */ -- 1.9.1
[toc] | [next] | [standalone]
| From | Catalin Marinas <catalin.marinas@arm.com> |
|---|---|
| Date | 2016-08-30 11:10 +0200 |
| Message-ID | <sbNdo-2Ps-27@gated-at.bofh.it> |
| In reply to | #1472254 |
On Tue, Aug 30, 2016 at 02:35:31PM +0800, Kenneth Lee wrote: > (add comment for the previous mail, sorry for the duplication) > > There is no store_ex pairing with this load_ex. It is not necessary and > gave wrong hint to the cache system. > > Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com> > --- > arch/arm64/include/asm/spinlock.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h > index c85e96d..3334c4f 100644 > --- a/arch/arm64/include/asm/spinlock.h > +++ b/arch/arm64/include/asm/spinlock.h > @@ -63,7 +63,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) > */ > " sevl\n" > "2: wfe\n" > -" ldaxrh %w2, %4\n" > +" ldrh %w2, %4\n" > " eor %w1, %w2, %w0, lsr #16\n" > " cbnz %w1, 2b\n" > /* We got the lock. Critical section starts here. */ This is needed because the arch_spin_unlock() code only uses an STLR without an explicit SEV (like we have on AArch32). An event is automatically generated when the exclusive monitor is cleared by STLR. But without setting it with a load exclusive in arch_spin_lock() (even though it does not acquire the lock), there won't be anything to clear, hence no event to be generated. In this case, the WFE would wait indefinitely. -- Catalin
[toc] | [prev] | [next] | [standalone]
| From | Vladimir Murzin <vladimir.murzin@arm.com> |
|---|---|
| Date | 2016-08-31 15:40 +0200 |
| Message-ID | <scdUd-2XV-7@gated-at.bofh.it> |
| In reply to | #1472341 |
On 30/08/16 10:07, Catalin Marinas wrote: > On Tue, Aug 30, 2016 at 02:35:31PM +0800, Kenneth Lee wrote: >> (add comment for the previous mail, sorry for the duplication) >> >> There is no store_ex pairing with this load_ex. It is not necessary and >> gave wrong hint to the cache system. >> >> Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com> >> --- >> arch/arm64/include/asm/spinlock.h | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h >> index c85e96d..3334c4f 100644 >> --- a/arch/arm64/include/asm/spinlock.h >> +++ b/arch/arm64/include/asm/spinlock.h >> @@ -63,7 +63,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) >> */ >> " sevl\n" >> "2: wfe\n" >> -" ldaxrh %w2, %4\n" >> +" ldrh %w2, %4\n" >> " eor %w1, %w2, %w0, lsr #16\n" >> " cbnz %w1, 2b\n" >> /* We got the lock. Critical section starts here. */ > > This is needed because the arch_spin_unlock() code only uses an STLR > without an explicit SEV (like we have on AArch32). An event is > automatically generated when the exclusive monitor is cleared by STLR. > But without setting it with a load exclusive in arch_spin_lock() (even > though it does not acquire the lock), there won't be anything to clear, > hence no event to be generated. In this case, the WFE would wait > indefinitely. > Maybe worth to add this as a comment, no? Cheers Vladimir
[toc] | [prev] | [next] | [standalone]
| From | Catalin Marinas <catalin.marinas@arm.com> |
|---|---|
| Date | 2016-09-01 12:30 +0200 |
| Message-ID | <scxpU-vd-33@gated-at.bofh.it> |
| In reply to | #1473476 |
On Wed, Aug 31, 2016 at 02:30:40PM +0100, Vladimir Murzin wrote: > On 30/08/16 10:07, Catalin Marinas wrote: > > On Tue, Aug 30, 2016 at 02:35:31PM +0800, Kenneth Lee wrote: > >> (add comment for the previous mail, sorry for the duplication) > >> > >> There is no store_ex pairing with this load_ex. It is not necessary and > >> gave wrong hint to the cache system. > >> > >> Signed-off-by: Kenneth Lee <liguozhu@hisilicon.com> > >> --- > >> arch/arm64/include/asm/spinlock.h | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/arch/arm64/include/asm/spinlock.h b/arch/arm64/include/asm/spinlock.h > >> index c85e96d..3334c4f 100644 > >> --- a/arch/arm64/include/asm/spinlock.h > >> +++ b/arch/arm64/include/asm/spinlock.h > >> @@ -63,7 +63,7 @@ static inline void arch_spin_lock(arch_spinlock_t *lock) > >> */ > >> " sevl\n" > >> "2: wfe\n" > >> -" ldaxrh %w2, %4\n" > >> +" ldrh %w2, %4\n" > >> " eor %w1, %w2, %w0, lsr #16\n" > >> " cbnz %w1, 2b\n" > >> /* We got the lock. Critical section starts here. */ > > > > This is needed because the arch_spin_unlock() code only uses an STLR > > without an explicit SEV (like we have on AArch32). An event is > > automatically generated when the exclusive monitor is cleared by STLR. > > But without setting it with a load exclusive in arch_spin_lock() (even > > though it does not acquire the lock), there won't be anything to clear, > > hence no event to be generated. In this case, the WFE would wait > > indefinitely. > > Maybe worth to add this as a comment, no? Yes, we just need to find someone to send a patch ;). -- Catalin
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web