Re: [PATCH v2] parisc: Don't disable interrupts in cmpxchg and futex operations

[Helge Deller <deller@gmx.de>]

* 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 */