Re: bind9 failures on mips

[James Cowgill <james410@cowgill.org.uk>]

On Mon, 2015-09-07 at 15:04 -0400, Robert Edmonds wrote:
> Hi,
> 
> I've been reviewing bugs filed against bind9, and I came across #778720
> ("Bin[d]9 hangs on mips jessie based machine"), which points to a forum
> thread [0] where multiple people have reported problems running our
> bind9 package on mips-based networking devices.
[...]
> That leaves the most complicated operation, isc_atomic_cmpxchg(), which
> unfortunately isn't exercised by the "t_atomic" test program.  Does this
> code [5] look correct?
> 
>     /*
>      * This routine atomically replaces the value in 'p' with 'val', if the
>      * original value is equal to 'cmpval'.  The original value is returned in any
>      * case.
>      */
>     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);
>     }

The tmp register constraint should be earlyclobber ("=&r") because the
other two registers (%3 and %4) will be read again after tmp (%2) is
written if the code loops around. At the moment, GCC could make tmp and
cmpval/val refer to the same register.

But to be honest, I would just replace all the functions in that file
with calls to the C11 atomic functions which GCC implements itself -
this way you can be sure they're right:

#include <stdatomic.h>

static inline isc_int32_t
isc_atomic_xadd(isc_int32_t *p, int val) {
	return atomic_fetch_add(p, val);
}

static inline void
isc_atomic_store(isc_int32_t *p, isc_int32_t val) {
	atomic_store(p, val);
}

static inline isc_int32_t
isc_atomic_cmpxchg(isc_int32_t *p, int cmpval, int val) {
        atomic_compare_exchange_strong(p, &cmpval, val);
        return cmpval;
}

Thanks,
James

Attachment: signature.asc
Description: This is a digitally signed message part