* [PATCH] parisc: don't enable irqs unconditionally in handle_interruption()
@ 2021-10-15 19:56 Sven Schnelle
[not found] ` <969e8f20-d27c-77cd-62c1-ddb86213ddec@gmx.de>
0 siblings, 1 reply; 6+ messages in thread
From: Sven Schnelle @ 2021-10-15 19:56 UTC (permalink / raw)
To: Helge Deller; +Cc: linux-parisc
If the previous context had interrupts disabled, we should better
keep them disabled. This was noticed in the unwinding code where
a copy_from_kernel_nofault() triggered a page fault, and after
the fixup by the page fault handler interrupts where suddenly
enabled.
Signed-off-by: Sven Schnelle <svens@stackframe.org>
---
arch/parisc/kernel/traps.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/arch/parisc/kernel/traps.c b/arch/parisc/kernel/traps.c
index 690e6abcaf22..3c5d968da415 100644
--- a/arch/parisc/kernel/traps.c
+++ b/arch/parisc/kernel/traps.c
@@ -480,9 +480,9 @@ void notrace handle_interruption(int code, struct pt_regs *regs)
int si_code;
if (code == 1)
- pdc_console_restart(); /* switch back to pdc if HPMC */
- else
- local_irq_enable();
+ pdc_console_restart(); /* switch back to pdc if HPMC */
+ else if (!irqs_disabled_flags(regs->gr[0]))
+ local_irq_enable();
/* Security check:
* If the priority level is still user, and the
--
2.33.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH] parisc: Don't disable interrupts in cmpxchg and futex operations
[not found] ` <48780780-0f8c-5aa9-d362-a9b9ddeaeedb@gmx.de>
@ 2021-10-20 18:32 ` John David Anglin
2021-10-20 19:05 ` Sven Schnelle
0 siblings, 1 reply; 6+ messages in thread
From: John David Anglin @ 2021-10-20 18:32 UTC (permalink / raw)
To: Helge Deller, Sven Schnelle; +Cc: linux-parisc
On 2021-10-17 12:30 p.m., Helge Deller wrote:
> It seems the warnings are gone if I remove the irq masking.
> I see the options:
> a) revert the irq masking in syscall.S. Not sure if it really hurts performance.
> b) revert the patch from Sven.
> c) insert code to turn back irq on in the fault handler if we are on the gateway page.
>
> What is your thought?
After some thought, I believe option a) is the best. I no longer think interrupts can be
disabled in the futex and cmpxchg operations because of COW breaks. This not ideal but
I suspect it's the best we can do.
For the cmpxchg operations in syscall.S, we rely on the code to not schedule off the gateway
page. For the futex, I added code to disable preemption.
So far, I haven't seen the warnings with the attached change but the change is only lightly tested.
Signed-off-by: Dave Anglin <dave.anglin@bell.net>
---
diff --git a/arch/parisc/include/asm/futex.h b/arch/parisc/include/asm/futex.h
index fceb9cf02fb3..c170cbe2c6d6 100644
--- a/arch/parisc/include/asm/futex.h
+++ b/arch/parisc/include/asm/futex.h
@@ -13,35 +13,37 @@
sixteen four-word locks. */
static inline void
-_futex_spin_lock_irqsave(u32 __user *uaddr, unsigned long int *flags)
+_futex_spin_lock(u32 __user *uaddr)
{
extern u32 lws_lock_start[];
long index = ((long)uaddr & 0x3f8) >> 1;
arch_spinlock_t *s = (arch_spinlock_t *)&lws_lock_start[index];
- local_irq_save(*flags);
+ preempt_disable();
arch_spin_lock(s);
}
static inline void
-_futex_spin_unlock_irqrestore(u32 __user *uaddr, unsigned long int *flags)
+_futex_spin_unlock(u32 __user *uaddr)
{
extern u32 lws_lock_start[];
long index = ((long)uaddr & 0x3f8) >> 1;
arch_spinlock_t *s = (arch_spinlock_t *)&lws_lock_start[index];
arch_spin_unlock(s);
- local_irq_restore(*flags);
+ preempt_enable();
}
static inline int
arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
{
- unsigned long int flags;
int oldval, ret;
u32 tmp;
- _futex_spin_lock_irqsave(uaddr, &flags);
-
ret = -EFAULT;
+ if (unlikely(get_user(oldval, uaddr) != 0))
+ goto out_pagefault_enable;
+
+ _futex_spin_lock(uaddr);
+
if (unlikely(get_user(oldval, uaddr) != 0))
goto out_pagefault_enable;
@@ -72,7 +74,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
ret = -EFAULT;
out_pagefault_enable:
- _futex_spin_unlock_irqrestore(uaddr, &flags);
+ _futex_spin_unlock(uaddr);
if (!ret)
*oval = oldval;
@@ -85,7 +87,6 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
u32 oldval, u32 newval)
{
u32 val;
- unsigned long flags;
/* futex.c wants to do a cmpxchg_inatomic on kernel NULL, which is
* our gateway page, and causes no end of trouble...
@@ -102,19 +103,19 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
* address. This should scale to a couple of CPUs.
*/
- _futex_spin_lock_irqsave(uaddr, &flags);
+ _futex_spin_lock(uaddr);
if (unlikely(get_user(val, uaddr) != 0)) {
- _futex_spin_unlock_irqrestore(uaddr, &flags);
+ _futex_spin_unlock(uaddr);
return -EFAULT;
}
if (val == oldval && unlikely(put_user(newval, uaddr) != 0)) {
- _futex_spin_unlock_irqrestore(uaddr, &flags);
+ _futex_spin_unlock(uaddr);
return -EFAULT;
}
*uval = val;
- _futex_spin_unlock_irqrestore(uaddr, &flags);
+ _futex_spin_unlock(uaddr);
return 0;
}
diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
index 3f24a0af1e04..5b8feeeedf40 100644
--- a/arch/parisc/kernel/syscall.S
+++ b/arch/parisc/kernel/syscall.S
@@ -603,13 +603,11 @@ cas_nocontend:
# endif
/* ENABLE_LWS_DEBUG */
- rsm PSW_SM_I, %r0 /* Disable interrupts */
/* COW breaks can cause contention on UP systems */
LDCW 0(%sr2,%r20), %r28 /* Try to acquire the lock */
cmpb,<>,n %r0, %r28, cas_action /* Did we get it? */
cas_wouldblock:
ldo 2(%r0), %r28 /* 2nd case */
- ssm PSW_SM_I, %r0
b lws_exit /* Contended... */
ldo -EAGAIN(%r0), %r21 /* Spin in userspace */
@@ -645,8 +643,6 @@ cas_action:
/* Clear thread register indicator */
stw %r0, 4(%sr2,%r20)
#endif
- /* Enable interrupts */
- ssm PSW_SM_I, %r0
/* Return to userspace, set no error */
b lws_exit
copy %r0, %r21
@@ -658,7 +654,6 @@ cas_action:
#if ENABLE_LWS_DEBUG
stw %r0, 4(%sr2,%r20)
#endif
- ssm PSW_SM_I, %r0
b lws_exit
ldo -EFAULT(%r0),%r21 /* set errno */
nop
@@ -770,13 +765,11 @@ cas2_lock_start:
shlw %r20, 4, %r20
add %r20, %r28, %r20
- rsm PSW_SM_I, %r0 /* Disable interrupts */
/* COW breaks can cause contention on UP systems */
LDCW 0(%sr2,%r20), %r28 /* Try to acquire the lock */
cmpb,<>,n %r0, %r28, cas2_action /* Did we get it? */
cas2_wouldblock:
ldo 2(%r0), %r28 /* 2nd case */
- ssm PSW_SM_I, %r0
b lws_exit /* Contended... */
ldo -EAGAIN(%r0), %r21 /* Spin in userspace */
@@ -856,8 +849,6 @@ cas2_action:
cas2_end:
/* Free lock */
stw,ma %r20, 0(%sr2,%r20)
- /* Enable interrupts */
- ssm PSW_SM_I, %r0
/* Return to userspace, set no error */
b lws_exit
copy %r0, %r21
@@ -866,7 +857,6 @@ cas2_end:
/* Error occurred on load or store */
/* Free lock */
stw,ma %r20, 0(%sr2,%r20)
- ssm PSW_SM_I, %r0
ldo 1(%r0),%r28
b lws_exit
ldo -EFAULT(%r0),%r21 /* set errno */
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] parisc: Don't disable interrupts in cmpxchg and futex operations
2021-10-20 18:32 ` [PATCH] parisc: Don't disable interrupts in cmpxchg and futex operations John David Anglin
@ 2021-10-20 19:05 ` Sven Schnelle
2021-11-03 14:42 ` [PATCH v2] " Helge Deller
0 siblings, 1 reply; 6+ messages in thread
From: Sven Schnelle @ 2021-10-20 19:05 UTC (permalink / raw)
To: John David Anglin; +Cc: Helge Deller, linux-parisc
John David Anglin <dave.anglin@bell.net> writes:
> On 2021-10-17 12:30 p.m., Helge Deller wrote:
>> It seems the warnings are gone if I remove the irq masking.
>> I see the options:
>> a) revert the irq masking in syscall.S. Not sure if it really hurts performance.
>> b) revert the patch from Sven.
>> c) insert code to turn back irq on in the fault handler if we are on the gateway page.
>> What is your thought?
>
> After some thought, I believe option a) is the best. I no longer think interrupts can be
> disabled in the futex and cmpxchg operations because of COW breaks. This not ideal but
> I suspect it's the best we can do.
>
> For the cmpxchg operations in syscall.S, we rely on the code to not schedule off the gateway
> page. For the futex, I added code to disable preemption.
>
> So far, I haven't seen the warnings with the attached change but the change is only lightly tested.
Thanks Dave. I just applied it to my tree and will give it a spin.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] parisc: Don't disable interrupts in cmpxchg and futex operations
2021-10-20 19:05 ` Sven Schnelle
@ 2021-11-03 14:42 ` Helge Deller
2021-11-03 15:41 ` John David Anglin
2021-11-03 17:10 ` Sven Schnelle
0 siblings, 2 replies; 6+ messages in thread
From: Helge Deller @ 2021-11-03 14:42 UTC (permalink / raw)
To: Sven Schnelle; +Cc: John David Anglin, Helge Deller, linux-parisc
* Sven Schnelle <svens@stackframe.org>:
> John David Anglin <dave.anglin@bell.net> writes:
>
> > On 2021-10-17 12:30 p.m., Helge Deller wrote:
> >> It seems the warnings are gone if I remove the irq masking.
> >> I see the options:
> >> a) revert the irq masking in syscall.S. Not sure if it really hurts performance.
> >> b) revert the patch from Sven.
> >> c) insert code to turn back irq on in the fault handler if we are on the gateway page.
> >> What is your thought?
> >
> > After some thought, I believe option a) is the best. I no longer think interrupts can be
> > disabled in the futex and cmpxchg operations because of COW breaks. This not ideal but
> > I suspect it's the best we can do.
> >
> > For the cmpxchg operations in syscall.S, we rely on the code to not schedule off the gateway
> > page. For the futex, I added code to disable preemption.
> >
> > So far, I haven't seen the warnings with the attached change but the change is only lightly tested.
>
> Thanks Dave. I just applied it to my tree and will give it a spin.
Sven, did you had some outcome?
I've cleaned up Daves patch and applied it to my for-next tree:
https://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git/commit/?h=for-next&id=b7cd8a368c986abf184e609772b083f43e5d522d
together with this patch from Sven ("parisc: don't enable irqs
unconditionally in handle_interruption()"):
https://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git/commit/?h=for-next&id=493f84fcb3a791350ffaa2fa2984a0815a924e39
When not applying to master branch from Linus, this patch is needed as well:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1030d681319b43869e0d5b568b9d0226652d1a6f
All is in my for-next tree.
Testing seems that those are OK and I don't see any
"BUG: using __this_cpu_add() in preemptible [00000000] code" warnings
any more.
Any objections if I push those patches to Linus soon?
Helge
From 381599d3eb726623948c6b4adb2ea49a7359232b Mon Sep 17 00:00:00 2001
From: Dave Anglin <dave.anglin@bell.net>
Date: Wed, 3 Nov 2021 12:49:32 +0100
Subject: [PATCH] parisc: Don't disable interrupts in cmpxchg and futex
operations
I no longer think interrupts can be disabled in the futex and cmpxchg
operations because of COW breaks. This not ideal but I suspect it's the
best we can do.
For the cmpxchg operations in syscall.S, we rely on the code to not
schedule off the gateway page. For the futex, I added code to disable
preemption.
So far, I haven't seen the warnings with the attached change but the
change is only lightly tested.
Signed-off-by: Dave Anglin <dave.anglin@bell.net>
Signed-off-by: Helge Deller <deller@gmx.de>
diff --git a/arch/parisc/include/asm/futex.h b/arch/parisc/include/asm/futex.h
index fceb9cf02fb3..05fe92453b29 100644
--- a/arch/parisc/include/asm/futex.h
+++ b/arch/parisc/include/asm/futex.h
@@ -13,35 +13,34 @@
sixteen four-word locks. */
static inline void
-_futex_spin_lock_irqsave(u32 __user *uaddr, unsigned long int *flags)
+_futex_spin_lock(u32 __user *uaddr)
{
extern u32 lws_lock_start[];
long index = ((long)uaddr & 0x3f8) >> 1;
arch_spinlock_t *s = (arch_spinlock_t *)&lws_lock_start[index];
- local_irq_save(*flags);
+ preempt_disable();
arch_spin_lock(s);
}
static inline void
-_futex_spin_unlock_irqrestore(u32 __user *uaddr, unsigned long int *flags)
+_futex_spin_unlock(u32 __user *uaddr)
{
extern u32 lws_lock_start[];
long index = ((long)uaddr & 0x3f8) >> 1;
arch_spinlock_t *s = (arch_spinlock_t *)&lws_lock_start[index];
arch_spin_unlock(s);
- local_irq_restore(*flags);
+ preempt_enable();
}
static inline int
arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
{
- unsigned long int flags;
int oldval, ret;
u32 tmp;
- _futex_spin_lock_irqsave(uaddr, &flags);
-
ret = -EFAULT;
+
+ _futex_spin_lock(uaddr);
if (unlikely(get_user(oldval, uaddr) != 0))
goto out_pagefault_enable;
@@ -72,7 +71,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
ret = -EFAULT;
out_pagefault_enable:
- _futex_spin_unlock_irqrestore(uaddr, &flags);
+ _futex_spin_unlock(uaddr);
if (!ret)
*oval = oldval;
@@ -85,7 +84,6 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
u32 oldval, u32 newval)
{
u32 val;
- unsigned long flags;
/* futex.c wants to do a cmpxchg_inatomic on kernel NULL, which is
* our gateway page, and causes no end of trouble...
@@ -102,19 +100,19 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
* address. This should scale to a couple of CPUs.
*/
- _futex_spin_lock_irqsave(uaddr, &flags);
+ _futex_spin_lock(uaddr);
if (unlikely(get_user(val, uaddr) != 0)) {
- _futex_spin_unlock_irqrestore(uaddr, &flags);
+ _futex_spin_unlock(uaddr);
return -EFAULT;
}
if (val == oldval && unlikely(put_user(newval, uaddr) != 0)) {
- _futex_spin_unlock_irqrestore(uaddr, &flags);
+ _futex_spin_unlock(uaddr);
return -EFAULT;
}
*uval = val;
- _futex_spin_unlock_irqrestore(uaddr, &flags);
+ _futex_spin_unlock(uaddr);
return 0;
}
diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
index 3f24a0af1e04..5b8feeeedf40 100644
--- a/arch/parisc/kernel/syscall.S
+++ b/arch/parisc/kernel/syscall.S
@@ -603,13 +603,11 @@ cas_nocontend:
# endif
/* ENABLE_LWS_DEBUG */
- rsm PSW_SM_I, %r0 /* Disable interrupts */
/* COW breaks can cause contention on UP systems */
LDCW 0(%sr2,%r20), %r28 /* Try to acquire the lock */
cmpb,<>,n %r0, %r28, cas_action /* Did we get it? */
cas_wouldblock:
ldo 2(%r0), %r28 /* 2nd case */
- ssm PSW_SM_I, %r0
b lws_exit /* Contended... */
ldo -EAGAIN(%r0), %r21 /* Spin in userspace */
@@ -645,8 +643,6 @@ cas_action:
/* Clear thread register indicator */
stw %r0, 4(%sr2,%r20)
#endif
- /* Enable interrupts */
- ssm PSW_SM_I, %r0
/* Return to userspace, set no error */
b lws_exit
copy %r0, %r21
@@ -658,7 +654,6 @@ cas_action:
#if ENABLE_LWS_DEBUG
stw %r0, 4(%sr2,%r20)
#endif
- ssm PSW_SM_I, %r0
b lws_exit
ldo -EFAULT(%r0),%r21 /* set errno */
nop
@@ -770,13 +765,11 @@ cas2_lock_start:
shlw %r20, 4, %r20
add %r20, %r28, %r20
- rsm PSW_SM_I, %r0 /* Disable interrupts */
/* COW breaks can cause contention on UP systems */
LDCW 0(%sr2,%r20), %r28 /* Try to acquire the lock */
cmpb,<>,n %r0, %r28, cas2_action /* Did we get it? */
cas2_wouldblock:
ldo 2(%r0), %r28 /* 2nd case */
- ssm PSW_SM_I, %r0
b lws_exit /* Contended... */
ldo -EAGAIN(%r0), %r21 /* Spin in userspace */
@@ -856,8 +849,6 @@ cas2_action:
cas2_end:
/* Free lock */
stw,ma %r20, 0(%sr2,%r20)
- /* Enable interrupts */
- ssm PSW_SM_I, %r0
/* Return to userspace, set no error */
b lws_exit
copy %r0, %r21
@@ -866,7 +857,6 @@ cas2_end:
/* Error occurred on load or store */
/* Free lock */
stw,ma %r20, 0(%sr2,%r20)
- ssm PSW_SM_I, %r0
ldo 1(%r0),%r28
b lws_exit
ldo -EFAULT(%r0),%r21 /* set errno */
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] parisc: Don't disable interrupts in cmpxchg and futex operations
2021-11-03 14:42 ` [PATCH v2] " Helge Deller
@ 2021-11-03 15:41 ` John David Anglin
2021-11-03 17:10 ` Sven Schnelle
1 sibling, 0 replies; 6+ messages in thread
From: John David Anglin @ 2021-11-03 15:41 UTC (permalink / raw)
To: Helge Deller, Sven Schnelle; +Cc: linux-parisc
On 2021-11-03 10:42 a.m., Helge Deller wrote:
> * Sven Schnelle <svens@stackframe.org>:
>> John David Anglin <dave.anglin@bell.net> writes:
>>
>>> On 2021-10-17 12:30 p.m., Helge Deller wrote:
>>>> It seems the warnings are gone if I remove the irq masking.
>>>> I see the options:
>>>> a) revert the irq masking in syscall.S. Not sure if it really hurts performance.
>>>> b) revert the patch from Sven.
>>>> c) insert code to turn back irq on in the fault handler if we are on the gateway page.
>>>> What is your thought?
>>> After some thought, I believe option a) is the best. I no longer think interrupts can be
>>> disabled in the futex and cmpxchg operations because of COW breaks. This not ideal but
>>> I suspect it's the best we can do.
>>>
>>> For the cmpxchg operations in syscall.S, we rely on the code to not schedule off the gateway
>>> page. For the futex, I added code to disable preemption.
>>>
>>> So far, I haven't seen the warnings with the attached change but the change is only lightly tested.
>> Thanks Dave. I just applied it to my tree and will give it a spin.
> Sven, did you had some outcome?
>
> I've cleaned up Daves patch and applied it to my for-next tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git/commit/?h=for-next&id=b7cd8a368c986abf184e609772b083f43e5d522d
> together with this patch from Sven ("parisc: don't enable irqs
> unconditionally in handle_interruption()"):
> https://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git/commit/?h=for-next&id=493f84fcb3a791350ffaa2fa2984a0815a924e39
>
> When not applying to master branch from Linus, this patch is needed as well:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1030d681319b43869e0d5b568b9d0226652d1a6f
>
> All is in my for-next tree.
> Testing seems that those are OK and I don't see any
> "BUG: using __this_cpu_add() in preemptible [00000000] code" warnings
> any more.
>
> Any objections if I push those patches to Linus soon?
No.
I now have about 2 weeks running time on mx3210 buildd with these changes. I haven't seen the warnings.
Overall stability has been good. Many packages that fail to build on other buildds build successfully.
Dave
>
> Helge
>
>
> From 381599d3eb726623948c6b4adb2ea49a7359232b Mon Sep 17 00:00:00 2001
> From: Dave Anglin <dave.anglin@bell.net>
> Date: Wed, 3 Nov 2021 12:49:32 +0100
> Subject: [PATCH] parisc: Don't disable interrupts in cmpxchg and futex
> operations
>
> I no longer think interrupts can be disabled in the futex and cmpxchg
> operations because of COW breaks. This not ideal but I suspect it's the
> best we can do.
>
> For the cmpxchg operations in syscall.S, we rely on the code to not
> schedule off the gateway page. For the futex, I added code to disable
> preemption.
>
> So far, I haven't seen the warnings with the attached change but the
> change is only lightly tested.
>
> Signed-off-by: Dave Anglin <dave.anglin@bell.net>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/arch/parisc/include/asm/futex.h b/arch/parisc/include/asm/futex.h
> index fceb9cf02fb3..05fe92453b29 100644
> --- a/arch/parisc/include/asm/futex.h
> +++ b/arch/parisc/include/asm/futex.h
> @@ -13,35 +13,34 @@
> sixteen four-word locks. */
>
> static inline void
> -_futex_spin_lock_irqsave(u32 __user *uaddr, unsigned long int *flags)
> +_futex_spin_lock(u32 __user *uaddr)
> {
> extern u32 lws_lock_start[];
> long index = ((long)uaddr & 0x3f8) >> 1;
> arch_spinlock_t *s = (arch_spinlock_t *)&lws_lock_start[index];
> - local_irq_save(*flags);
> + preempt_disable();
> arch_spin_lock(s);
> }
>
> static inline void
> -_futex_spin_unlock_irqrestore(u32 __user *uaddr, unsigned long int *flags)
> +_futex_spin_unlock(u32 __user *uaddr)
> {
> extern u32 lws_lock_start[];
> long index = ((long)uaddr & 0x3f8) >> 1;
> arch_spinlock_t *s = (arch_spinlock_t *)&lws_lock_start[index];
> arch_spin_unlock(s);
> - local_irq_restore(*flags);
> + preempt_enable();
> }
>
> static inline int
> arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
> {
> - unsigned long int flags;
> int oldval, ret;
> u32 tmp;
>
> - _futex_spin_lock_irqsave(uaddr, &flags);
> -
> ret = -EFAULT;
> +
> + _futex_spin_lock(uaddr);
> if (unlikely(get_user(oldval, uaddr) != 0))
> goto out_pagefault_enable;
>
> @@ -72,7 +71,7 @@ arch_futex_atomic_op_inuser(int op, int oparg, int *oval, u32 __user *uaddr)
> ret = -EFAULT;
>
> out_pagefault_enable:
> - _futex_spin_unlock_irqrestore(uaddr, &flags);
> + _futex_spin_unlock(uaddr);
>
> if (!ret)
> *oval = oldval;
> @@ -85,7 +84,6 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> u32 oldval, u32 newval)
> {
> u32 val;
> - unsigned long flags;
>
> /* futex.c wants to do a cmpxchg_inatomic on kernel NULL, which is
> * our gateway page, and causes no end of trouble...
> @@ -102,19 +100,19 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr,
> * address. This should scale to a couple of CPUs.
> */
>
> - _futex_spin_lock_irqsave(uaddr, &flags);
> + _futex_spin_lock(uaddr);
> if (unlikely(get_user(val, uaddr) != 0)) {
> - _futex_spin_unlock_irqrestore(uaddr, &flags);
> + _futex_spin_unlock(uaddr);
> return -EFAULT;
> }
>
> if (val == oldval && unlikely(put_user(newval, uaddr) != 0)) {
> - _futex_spin_unlock_irqrestore(uaddr, &flags);
> + _futex_spin_unlock(uaddr);
> return -EFAULT;
> }
>
> *uval = val;
> - _futex_spin_unlock_irqrestore(uaddr, &flags);
> + _futex_spin_unlock(uaddr);
>
> return 0;
> }
> diff --git a/arch/parisc/kernel/syscall.S b/arch/parisc/kernel/syscall.S
> index 3f24a0af1e04..5b8feeeedf40 100644
> --- a/arch/parisc/kernel/syscall.S
> +++ b/arch/parisc/kernel/syscall.S
> @@ -603,13 +603,11 @@ cas_nocontend:
> # endif
> /* ENABLE_LWS_DEBUG */
>
> - rsm PSW_SM_I, %r0 /* Disable interrupts */
> /* COW breaks can cause contention on UP systems */
> LDCW 0(%sr2,%r20), %r28 /* Try to acquire the lock */
> cmpb,<>,n %r0, %r28, cas_action /* Did we get it? */
> cas_wouldblock:
> ldo 2(%r0), %r28 /* 2nd case */
> - ssm PSW_SM_I, %r0
> b lws_exit /* Contended... */
> ldo -EAGAIN(%r0), %r21 /* Spin in userspace */
>
> @@ -645,8 +643,6 @@ cas_action:
> /* Clear thread register indicator */
> stw %r0, 4(%sr2,%r20)
> #endif
> - /* Enable interrupts */
> - ssm PSW_SM_I, %r0
> /* Return to userspace, set no error */
> b lws_exit
> copy %r0, %r21
> @@ -658,7 +654,6 @@ cas_action:
> #if ENABLE_LWS_DEBUG
> stw %r0, 4(%sr2,%r20)
> #endif
> - ssm PSW_SM_I, %r0
> b lws_exit
> ldo -EFAULT(%r0),%r21 /* set errno */
> nop
> @@ -770,13 +765,11 @@ cas2_lock_start:
> shlw %r20, 4, %r20
> add %r20, %r28, %r20
>
> - rsm PSW_SM_I, %r0 /* Disable interrupts */
> /* COW breaks can cause contention on UP systems */
> LDCW 0(%sr2,%r20), %r28 /* Try to acquire the lock */
> cmpb,<>,n %r0, %r28, cas2_action /* Did we get it? */
> cas2_wouldblock:
> ldo 2(%r0), %r28 /* 2nd case */
> - ssm PSW_SM_I, %r0
> b lws_exit /* Contended... */
> ldo -EAGAIN(%r0), %r21 /* Spin in userspace */
>
> @@ -856,8 +849,6 @@ cas2_action:
> cas2_end:
> /* Free lock */
> stw,ma %r20, 0(%sr2,%r20)
> - /* Enable interrupts */
> - ssm PSW_SM_I, %r0
> /* Return to userspace, set no error */
> b lws_exit
> copy %r0, %r21
> @@ -866,7 +857,6 @@ cas2_end:
> /* Error occurred on load or store */
> /* Free lock */
> stw,ma %r20, 0(%sr2,%r20)
> - ssm PSW_SM_I, %r0
> ldo 1(%r0),%r28
> b lws_exit
> ldo -EFAULT(%r0),%r21 /* set errno */
--
John David Anglin dave.anglin@bell.net
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] parisc: Don't disable interrupts in cmpxchg and futex operations
2021-11-03 14:42 ` [PATCH v2] " Helge Deller
2021-11-03 15:41 ` John David Anglin
@ 2021-11-03 17:10 ` Sven Schnelle
1 sibling, 0 replies; 6+ messages in thread
From: Sven Schnelle @ 2021-11-03 17:10 UTC (permalink / raw)
To: Helge Deller; +Cc: John David Anglin, linux-parisc
Helge Deller <deller@gmx.de> writes:
> * Sven Schnelle <svens@stackframe.org>:
>> John David Anglin <dave.anglin@bell.net> writes:
>>
>> > On 2021-10-17 12:30 p.m., Helge Deller wrote:
>> >> It seems the warnings are gone if I remove the irq masking.
>> >> I see the options:
>> >> a) revert the irq masking in syscall.S. Not sure if it really hurts performance.
>> >> b) revert the patch from Sven.
>> >> c) insert code to turn back irq on in the fault handler if we are on the gateway page.
>> >> What is your thought?
>> >
>> > After some thought, I believe option a) is the best. I no longer think interrupts can be
>> > disabled in the futex and cmpxchg operations because of COW breaks. This not ideal but
>> > I suspect it's the best we can do.
>> >
>> > For the cmpxchg operations in syscall.S, we rely on the code to not schedule off the gateway
>> > page. For the futex, I added code to disable preemption.
>> >
>> > So far, I haven't seen the warnings with the attached change but the change is only lightly tested.
>>
>> Thanks Dave. I just applied it to my tree and will give it a spin.
>
> Sven, did you had some outcome?
The outcome is that i haven't seen any problems yet.
> I've cleaned up Daves patch and applied it to my for-next tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git/commit/?h=for-next&id=b7cd8a368c986abf184e609772b083f43e5d522d
> together with this patch from Sven ("parisc: don't enable irqs
> unconditionally in handle_interruption()"):
> https://git.kernel.org/pub/scm/linux/kernel/git/deller/parisc-linux.git/commit/?h=for-next&id=493f84fcb3a791350ffaa2fa2984a0815a924e39
>
> When not applying to master branch from Linus, this patch is needed as well:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=1030d681319b43869e0d5b568b9d0226652d1a6f
>
> All is in my for-next tree.
> Testing seems that those are OK and I don't see any
> "BUG: using __this_cpu_add() in preemptible [00000000] code" warnings
> any more.
>
> Any objections if I push those patches to Linus soon?
No objections.
Sven
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-03 17:26 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-15 19:56 [PATCH] parisc: don't enable irqs unconditionally in handle_interruption() Sven Schnelle
[not found] ` <969e8f20-d27c-77cd-62c1-ddb86213ddec@gmx.de>
[not found] ` <a5030b48-b6bc-639c-5360-0389103c228e@bell.net>
[not found] ` <ac539330-bdc2-1bba-e2c2-78d29614864f@bell.net>
[not found] ` <8bef26c2-daee-2a6c-1828-100a5b27e205@gmx.de>
[not found] ` <28325394-99de-67f4-dcca-b1caf1105df2@bell.net>
[not found] ` <87o87oxhhf.fsf@x1.stackframe.org>
[not found] ` <48780780-0f8c-5aa9-d362-a9b9ddeaeedb@gmx.de>
2021-10-20 18:32 ` [PATCH] parisc: Don't disable interrupts in cmpxchg and futex operations John David Anglin
2021-10-20 19:05 ` Sven Schnelle
2021-11-03 14:42 ` [PATCH v2] " Helge Deller
2021-11-03 15:41 ` John David Anglin
2021-11-03 17:10 ` Sven Schnelle
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.