Hi Thomas/Peter,
Of late I've been debugging a seeming lost wakeup when running a specific
EEMBC Multibench workload (4M-check -w4) on a quad core HS38 config on FPGA
CONFIG_SMP, CONFIG_PREEMPT.
I've yet to nail that issue down, but in the process found some deficinecies
in ARC futex backend. Can you please take a quick look and shout if there's
something wrong there.
The futex backend for ARC, despite it's age, only recently started getting
real testing due to recent switch to NPTL based userland (uClibc).
Thx,
-Vineet
Vineet Gupta (5):
ARC: add barriers to futex code
ARC: futex cosmetics
ARC: make futex_atomic_cmpxchg_inatomic() return bimodal
ARC: Enable HAVE_FUTEX_CMPXCHG
ARC: ensure futex ops are atomic in !LLSC config
arch/arc/Kconfig | 1 +
arch/arc/include/asm/futex.h | 66 +++++++++++++++++++++++++++-----------------
2 files changed, 41 insertions(+), 26 deletions(-)
--
1.9.1
The atomic ops on futex need to provide the full barrier just like
regular atomics in kernel.
Also remove pagefault_enable/disable in futex_atomic_cmpxchg_inatomic()
as core code already does that
Cc: David Hildenbrand <[email protected]>
Cc: Peter Zijlstra (Intel) <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Signed-off-by: Vineet Gupta <[email protected]>
---
arch/arc/include/asm/futex.h | 21 ++++++++++-----------
1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/arch/arc/include/asm/futex.h b/arch/arc/include/asm/futex.h
index 70cfe16b742d..9de18a526aff 100644
--- a/arch/arc/include/asm/futex.h
+++ b/arch/arc/include/asm/futex.h
@@ -20,6 +20,7 @@
#define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
\
+ smp_mb(); \
__asm__ __volatile__( \
"1: llock %1, [%2] \n" \
insn "\n" \
@@ -40,12 +41,14 @@
\
: "=&r" (ret), "=&r" (oldval) \
: "r" (uaddr), "r" (oparg), "ir" (-EFAULT) \
- : "cc", "memory")
+ : "cc", "memory"); \
+ smp_mb() \
#else /* !CONFIG_ARC_HAS_LLSC */
#define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
\
+ smp_mb(); \
__asm__ __volatile__( \
"1: ld %1, [%2] \n" \
insn "\n" \
@@ -65,7 +68,8 @@
\
: "=&r" (ret), "=&r" (oldval) \
: "r" (uaddr), "r" (oparg), "ir" (-EFAULT) \
- : "cc", "memory")
+ : "cc", "memory"); \
+ smp_mb() \
#endif
@@ -134,13 +138,8 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
return ret;
}
-/* Compare-xchg with pagefaults disabled.
- * Notes:
- * -Best-Effort: Exchg happens only if compare succeeds.
- * If compare fails, returns; leaving retry/looping to upper layers
- * -successful cmp-xchg: return orig value in @addr (same as cmp val)
- * -Compare fails: return orig value in @addr
- * -user access r/w fails: return -EFAULT
+/*
+ * cmpxchg of futex (pagefaults disabled by caller)
*/
static inline int
futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
@@ -151,7 +150,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
return -EFAULT;
- pagefault_disable();
+ smp_mb();
__asm__ __volatile__(
#ifdef CONFIG_ARC_HAS_LLSC
@@ -178,7 +177,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
: "r"(oldval), "r"(newval), "r"(uaddr), "ir"(-EFAULT)
: "cc", "memory");
- pagefault_enable();
+ smp_mb();
*uval = val;
return val;
--
1.9.1
Cc: Peter Zijlstra (Intel) <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Signed-off-by: Vineet Gupta <[email protected]>
---
arch/arc/include/asm/futex.h | 17 +++++++++--------
1 file changed, 9 insertions(+), 8 deletions(-)
diff --git a/arch/arc/include/asm/futex.h b/arch/arc/include/asm/futex.h
index 9de18a526aff..14b1c9aaf455 100644
--- a/arch/arc/include/asm/futex.h
+++ b/arch/arc/include/asm/futex.h
@@ -94,6 +94,7 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
__futex_atomic_op("mov %0, %3", ret, oldval, uaddr, oparg);
break;
case FUTEX_OP_ADD:
+ /* oldval = *uaddr; *uaddr += oparg ; ret = *uaddr */
__futex_atomic_op("add %0, %1, %3", ret, oldval, uaddr, oparg);
break;
case FUTEX_OP_OR:
@@ -142,12 +143,12 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
* cmpxchg of futex (pagefaults disabled by caller)
*/
static inline int
-futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
- u32 newval)
+futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 expval,
+ u32 newval)
{
- u32 val;
+ u32 existval;
- if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
+ if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
return -EFAULT;
smp_mb();
@@ -173,14 +174,14 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
" .word 1b, 4b \n"
" .word 2b, 4b \n"
" .previous\n"
- : "=&r"(val)
- : "r"(oldval), "r"(newval), "r"(uaddr), "ir"(-EFAULT)
+ : "=&r"(existval)
+ : "r"(expval), "r"(newval), "r"(uaddr), "ir"(-EFAULT)
: "cc", "memory");
smp_mb();
- *uval = val;
- return val;
+ *uval = existval;
+ return existval;
}
#endif
--
1.9.1
Callers of cmpxchg_futex_value_locked() in futex code expect bimodal
return value:
!0 (essentially -EFAULT as failure)
0 (success)
Before this patch, the success return value was old value of futex,
which could very well be non zero, causing caller to possibly take the
failure path erroneously.
Fix that by returning 0 for success
(This fix was done back in 2011 for all upstream arches, which ARC
obviously missed)
Cc: Peter Zijlstra (Intel) <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Signed-off-by: Vineet Gupta <[email protected]>
---
arch/arc/include/asm/futex.h | 20 +++++++++++---------
1 file changed, 11 insertions(+), 9 deletions(-)
diff --git a/arch/arc/include/asm/futex.h b/arch/arc/include/asm/futex.h
index 14b1c9aaf455..0ea8bcc7b846 100644
--- a/arch/arc/include/asm/futex.h
+++ b/arch/arc/include/asm/futex.h
@@ -141,11 +141,13 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
/*
* cmpxchg of futex (pagefaults disabled by caller)
+ * Return 0 for success, -EFAULT otherwise
*/
static inline int
futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 expval,
u32 newval)
{
+ int ret = 0;
u32 existval;
if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
@@ -155,18 +157,18 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 expval,
__asm__ __volatile__(
#ifdef CONFIG_ARC_HAS_LLSC
- "1: llock %0, [%3] \n"
- " brne %0, %1, 3f \n"
- "2: scond %2, [%3] \n"
+ "1: llock %1, [%4] \n"
+ " brne %1, %2, 3f \n"
+ "2: scond %3, [%4] \n"
" bnz 1b \n"
#else
- "1: ld %0, [%3] \n"
- " brne %0, %1, 3f \n"
- "2: st %2, [%3] \n"
+ "1: ld %1, [%4] \n"
+ " brne %1, %2, 3f \n"
+ "2: st %3, [%4] \n"
#endif
"3: \n"
" .section .fixup,\"ax\" \n"
- "4: mov %0, %4 \n"
+ "4: mov %0, %5 \n"
" b 3b \n"
" .previous \n"
" .section __ex_table,\"a\" \n"
@@ -174,14 +176,14 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 expval,
" .word 1b, 4b \n"
" .word 2b, 4b \n"
" .previous\n"
- : "=&r"(existval)
+ : "+&r"(ret), "=&r"(existval)
: "r"(expval), "r"(newval), "r"(uaddr), "ir"(-EFAULT)
: "cc", "memory");
smp_mb();
*uval = existval;
- return existval;
+ return ret;
}
#endif
--
1.9.1
ARC doesn't need the runtime detection of futex cmpxchg op
Cc: Peter Zijlstra (Intel) <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Signed-off-by: Vineet Gupta <[email protected]>
---
arch/arc/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/arc/Kconfig b/arch/arc/Kconfig
index e119d42f92ce..78c0621d5819 100644
--- a/arch/arc/Kconfig
+++ b/arch/arc/Kconfig
@@ -23,6 +23,7 @@ config ARC
select GENERIC_SMP_IDLE_THREAD
select HAVE_ARCH_KGDB
select HAVE_ARCH_TRACEHOOK
+ select HAVE_FUTEX_CMPXCHG
select HAVE_IOREMAP_PROT
select HAVE_KPROBES
select HAVE_KRETPROBES
--
1.9.1
W/o hardware assisted atomic r-m-w the best we can do is to disable
preemption.
Cc: David Hildenbrand <[email protected]>
Cc: Peter Zijlstra (Intel) <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: Michel Lespinasse <[email protected]>
Signed-off-by: Vineet Gupta <[email protected]>
---
arch/arc/include/asm/futex.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/arch/arc/include/asm/futex.h b/arch/arc/include/asm/futex.h
index 0ea8bcc7b846..8f449982523b 100644
--- a/arch/arc/include/asm/futex.h
+++ b/arch/arc/include/asm/futex.h
@@ -87,6 +87,9 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
return -EFAULT;
+#ifndef CONFIG_ARC_HAS_LLSC
+ preempt_disable(); /* to guarantee atomic r-m-w of futex op */
+#endif
pagefault_disable();
switch (op) {
@@ -111,6 +114,9 @@ static inline int futex_atomic_op_inuser(int encoded_op, u32 __user *uaddr)
}
pagefault_enable();
+#ifndef CONFIG_ARC_HAS_LLSC
+ preempt_enable();
+#endif
if (!ret) {
switch (cmp) {
@@ -153,6 +159,9 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 expval,
if (!access_ok(VERIFY_WRITE, uaddr, sizeof(u32)))
return -EFAULT;
+#ifndef CONFIG_ARC_HAS_LLSC
+ preempt_disable(); /* to guarantee atomic r-m-w of futex op */
+#endif
smp_mb();
__asm__ __volatile__(
@@ -182,6 +191,9 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 expval,
smp_mb();
+#ifndef CONFIG_ARC_HAS_LLSC
+ preempt_enable();
+#endif
*uval = existval;
return ret;
}
--
1.9.1