Control: notforwarded -1 Control: tags -1 pending [Only the old patch was forwarded]
On 17/04/17 20:00, James Cowgill wrote: > As discussed in #839010 and after looking at this a bit more, I think > this bug should be release-critical for stretch. Although I can get > bind9 to start working on MIPS, it usually crashes after a few minutes > with it more likely to crash under load. > > The bug is related to atomics issues in libisc on MIPS. There was a > patch in this bug to fix the MIPS assembly bits, but while that does fix > the hangs it isn't enough to fix the crashes. I created a test program > which tests the rwlock from libisc and managed to get a situation where > 2 threads were both inside the same exclusive lock which obviously > should not happen. I expect the weak memory ordering of MIPS has > something to do with it (which would explain why this doesn't happen on > x86 and other arches). Here's a patch to fix this. The problem was not enough sync instructions around the MIPS atomic operations (in addition to needing the patch from earlier in this bug). My patch just removes all the MIPS assembly and uses stdatomic functions which are implemented correctly. This should be fine for Debian - even jessie's GCC should support them. bind9-9.10.3.dfsg.P4-12.2-nmu.debdiff is the NMU which I have uploaded to DELAYED/2. The NMU was generated using combinediff on the old atomics patch and MIPS-replace-stdatomic.patch (also attached). Thanks, James
diff -Nru bind9-9.10.3.dfsg.P4/debian/changelog
bind9-9.10.3.dfsg.P4/debian/changelog
--- bind9-9.10.3.dfsg.P4/debian/changelog 2017-03-17 18:07:16.000000000
+0000
+++ bind9-9.10.3.dfsg.P4/debian/changelog 2017-04-18 16:42:50.000000000
+0100
@@ -1,3 +1,11 @@
+bind9 (1:9.10.3.dfsg.P4-12.2) unstable; urgency=medium
+
+ * Non-maintainer upload.
+ * Replace 32_mips_atomic.diff with a version that uses C11 atomics. Fixes
+ hangs and crashes on MIPS. (Closes: #778720)
+
+ -- James Cowgill <jcowg...@debian.org> Tue, 18 Apr 2017 16:42:50 +0100
+
bind9 (1:9.10.3.dfsg.P4-12.1) unstable; urgency=medium
* Non-maintainer upload.
diff -Nru bind9-9.10.3.dfsg.P4/debian/patches/32_mips_atomic.diff
bind9-9.10.3.dfsg.P4/debian/patches/32_mips_atomic.diff
--- bind9-9.10.3.dfsg.P4/debian/patches/32_mips_atomic.diff 2017-02-19
22:38:45.000000000 +0000
+++ bind9-9.10.3.dfsg.P4/debian/patches/32_mips_atomic.diff 2017-04-18
16:42:50.000000000 +0100
@@ -1,22 +1,29 @@
-Author: Thiemo Seufer <t...@networkno.de>
-Date: Thu Nov 8 15:11:48 2007 -0700
-Forwarded: yes RT#41965
-
- mips:atomic.h: improve implementation of atomic ops, fix mips{el,64}
-
- The appended patch extends the configure check to cover mips64 and
- mipsel, and improves the mips atomics implementation.
-
- See http://bugs.debian.org/406409 for more detail.
-
- Signed-off-by: LaMont Jones <lam...@debian.org>
+Description: Replace MIPS atomics assembly with calls to C11 atomic functions
+ This fixes various hangs and crashes on MIPS.
+Author: James Cowgill <jcowg...@debian.org>
+Forwarded: no
+Bug-Debian: https://bugs.debian.org/778720
--- a/lib/isc/mips/include/isc/atomic.h
+++ b/lib/isc/mips/include/isc/atomic.h
-@@ -31,18 +31,20 @@
- isc_atomic_xadd(isc_int32_t *p, int val) {
- isc_int32_t orig;
+@@ -19,32 +19,19 @@
+ #ifndef ISC_ATOMIC_H
+ #define ISC_ATOMIC_H 1
+
++#include <stdatomic.h>
++
+ #include <isc/platform.h>
+ #include <isc/types.h>
+-#ifdef ISC_PLATFORM_USEGCCASM
+ /*
+ * This routine atomically increments the value stored in 'p' by 'val', and
+ * returns the previous value.
+ */
+ static inline isc_int32_t
+ isc_atomic_xadd(isc_int32_t *p, int val) {
+- isc_int32_t orig;
+-
- /* add is a cheat, since MIPS has no mov instruction */
- __asm__ volatile (
- "1:"
@@ -29,24 +36,13 @@
- : "m"(*p), "r"(val)
- : "memory", "$3"
- );
-+ __asm__ __volatile__ (
-+ " .set push \n"
-+ " .set mips2 \n"
-+ " .set noreorder \n"
-+ " .set noat \n"
-+ "1: ll $1, %1 \n"
-+ " addu %0, $1, %2 \n"
-+ " sc %0, %1 \n"
-+ " beqz %0, 1b \n"
-+ " move %0, $1 \n"
-+ " .set pop \n"
-+ : "=&r" (orig), "+R" (*p)
-+ : "r" (val)
-+ : "memory");
- return (orig);
+- return (orig);
++ return atomic_fetch_add(p, val);
}
-@@ -52,16 +54,7 @@
+
+ /*
+@@ -52,16 +39,7 @@ isc_atomic_xadd(isc_int32_t *p, int val)
*/
static inline void
isc_atomic_store(isc_int32_t *p, isc_int32_t val) {
@@ -60,16 +56,16 @@
- : "m"(*p), "r"(val)
- : "memory", "$3"
- );
-+ *p = val;
++ atomic_store(p, val);
}
/*
-@@ -72,20 +65,23 @@
+@@ -71,28 +49,8 @@ isc_atomic_store(isc_int32_t *p, isc_int
+ */
static inline isc_int32_t
isc_atomic_cmpxchg(isc_int32_t *p, int cmpval, int val) {
- isc_int32_t orig;
-+ isc_int32_t tmp;
-
+- isc_int32_t orig;
+-
- __asm__ volatile(
- "1:"
- "ll $3, %1\n"
@@ -83,21 +79,15 @@
- : "m"(*p), "r"(cmpval), "r"(val)
- : "memory", "$3"
- );
-+ __asm__ __volatile__ (
-+ " .set push \n"
-+ " .set mips2 \n"
-+ " .set noreorder \n"
-+ " .set noat \n"
-+ "1: ll $1, %1 \n"
-+ " bne $1, %3, 2f \n"
-+ " move %2, %4 \n"
-+ " sc %2, %1 \n"
-+ " beqz %2, 1b \n"
-+ "2: move %0, $1 \n"
-+ " .set pop \n"
-+ : "=&r"(orig), "+R" (*p), "=r" (tmp)
-+ : "r"(cmpval), "r"(val)
-+ : "memory");
-
- return (orig);
+-
+- return (orig);
++ atomic_compare_exchange_strong(p, &cmpval, val);
++ return cmpval;
}
+
+-#else /* !ISC_PLATFORM_USEGCCASM */
+-
+-#error "unsupported compiler. disable atomic ops by --disable-atomic"
+-
+-#endif
+ #endif /* ISC_ATOMIC_H */
--- a/lib/isc/mips/include/isc/atomic.h
+++ b/lib/isc/mips/include/isc/atomic.h
@@ -19,34 +19,19 @@
#ifndef ISC_ATOMIC_H
#define ISC_ATOMIC_H 1
+#include <stdatomic.h>
+
#include <isc/platform.h>
#include <isc/types.h>
-#ifdef ISC_PLATFORM_USEGCCASM
/*
* This routine atomically increments the value stored in 'p' by 'val', and
* returns the previous value.
*/
static inline isc_int32_t
isc_atomic_xadd(isc_int32_t *p, int val) {
- isc_int32_t orig;
-
- __asm__ __volatile__ (
- " .set push \n"
- " .set mips2 \n"
- " .set noreorder \n"
- " .set noat \n"
- "1: ll $1, %1 \n"
- " addu %0, $1, %2 \n"
- " sc %0, %1 \n"
- " beqz %0, 1b \n"
- " move %0, $1 \n"
- " .set pop \n"
- : "=&r" (orig), "+R" (*p)
- : "r" (val)
- : "memory");
- return (orig);
+ return atomic_fetch_add(p, val);
}
/*
@@ -54,7 +39,7 @@ isc_atomic_xadd(isc_int32_t *p, int val)
*/
static inline void
isc_atomic_store(isc_int32_t *p, isc_int32_t val) {
- *p = val;
+ atomic_store(p, val);
}
/*
@@ -64,31 +49,8 @@ isc_atomic_store(isc_int32_t *p, isc_int
*/
static inline isc_int32_t
isc_atomic_cmpxchg(isc_int32_t *p, int cmpval, int val) {
- isc_int32_t orig;
- isc_int32_t tmp;
-
- __asm__ __volatile__ (
- " .set push \n"
- " .set mips2 \n"
- " .set noreorder \n"
- " .set noat \n"
- "1: ll $1, %1 \n"
- " bne $1, %3, 2f \n"
- " move %2, %4 \n"
- " sc %2, %1 \n"
- " beqz %2, 1b \n"
- "2: move %0, $1 \n"
- " .set pop \n"
- : "=&r"(orig), "+R" (*p), "=r" (tmp)
- : "r"(cmpval), "r"(val)
- : "memory");
-
- return (orig);
+ atomic_compare_exchange_strong(p, &cmpval, val);
+ return cmpval;
}
-#else /* !ISC_PLATFORM_USEGCCASM */
-
-#error "unsupported compiler. disable atomic ops by --disable-atomic"
-
-#endif
#endif /* ISC_ATOMIC_H */
signature.asc
Description: OpenPGP digital signature
