Hi,
On Mon, 2015-09-07 at 18:34 -0400, Robert Edmonds wrote:
> James Cowgill wrote:
> > 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.
>
> Hi, James:
>
> Thanks! So, "=r" (tmp) should be "=&r" (tmp) ? (Sorry, I am not an
> assembly programmer.)
Yes. Hopefully that will fix the hangs - I don't think anything else is
wrong. This page has all the constraints if you're interested:
https://gcc.gnu.org/onlinedocs/gcc/Constraints.html
> > 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:
>
> What minimum version of gcc would be necessary? The last time I looked
> at this topic, gcc still did not have stdatomic.h, but I see that's
> changed now.
It was added in GCC 4.9 so it's good enough for jessie + sid, but not
for anything older.
> > #include
> >
> > 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;
> > }
>
> This would indeed be much nicer, but I suspect upstream would still want
> to keep the old implementations around for compatibility reasons,
> necessitating some configure-time detection macros and more #ifdef's.
Unfortunatly you're probably right. The code is there if you want to
use it though.
JamesAttachment:
signature.asc
Description: This is a digitally signed message part