On ARM atomics

[Jon Masters]

Hi everyone,

I wrote the following little summary of ARM atomics:

http://www.jonmasters.org/blog/2012/11/13/arm-atomic-operations/

It's not terribly comprehensive, but you could use it to understand what
I did in OpenMPI, and how to poke at other packages.

Jon.

[Nicolas Pitre]

On Tue, 13 Nov 2012, Jon Masters wrote:

>  Hi everyone,
>  
>  I wrote the following little summary of ARM atomics:
>  
>  http://www.jonmasters.org/blog/2012/11/13/arm-atomic-operations/
>  
>  It's not terribly comprehensive, but you could use it to understand what
>  I did in OpenMPI, and how to poke at other packages. 
There are several serious mistakes in your assembly example.

|START_FUNC(opal_atomic_add_32)
|       push    {r4-r7}

Why r4 to r7?  You're using only r4 below.

|       mov     r4, r1
|       mov     r2, r0
|       mov     r12, lr

r12 is not a good choice since it is allowed for callees to trash 
it.  Didn't r12 almost kill you in your sleep already anyway?  :-)

A good thing to know is that r12 is also known as ip (which doesn't mean 
"instruction pointer" on ARM).  And the __kuser_cmpxchg is documented as 
clobbering ip aka r12.  The only reason why this might have worked for 
you is because the current implementation does not actually clobber r12, 
but that is not guaranteed to ever stay that way, especially when the 
documentation is explicit about it.

The best would have been to use "push {r4, lr}" initially.

|       ldr     r3, REFLSYM(5)
|       LSYM(4)
|       ldr     r0, [r2]
|       add     r1, r0, r4
|       blx     r3
|       cmp     r0, #0

The __kuser_cmpxchg documentation also defines the C flag as being set 
on success.  So it is possible to optimize this slightly by omiting the 
cmp here.

|       movne   r0, #1

Why?  You are going to reload the value to increment into r0 after the 
following instruction anyway.

|       bne     REFLSYM(4)

Calling into __kuser_cmpxchg also clobbers r3 so your "blx r3" will 
branch into lalaland the second time around.

|       mov     lr, r12
|       pop     {r4-r7}
|       bx      lr
|       LSYM(5)
|       .align  2
|       .word   -61504

You put the label before the .align statement.  If ever some padding is 
inserted to satisfy the requested alignment, the label will refer to the 
unaligned padding value and not to the desired value.

Also, what is the point of using -61504 when 0xffff0fc0 is much less 
obscur?

|END_FUNC(opal_atomic_add_32)

So... I'd suggest this version instead:

START_FUNC(opal_atomic_add_32)
        push    {r4, lr}
        mov     r4, r1
        mov     r2, r0
        LSYM(4)
        ldr     r0, [r2]
        ldr     r3, REFLSYM(5)
        add     r1, r0, r4
        blx     r3
	bcc     REFLSYM(4)
	pop     {r4, lr}
        bx      lr
        .align  2
        LSYM(5)
        .word   0xffff0fc0
END_FUNC(opal_atomic_add_32)


Nicolas

[Jon Masters]

On 11/13/2012 01:24 AM, Nicolas Pitre wrote:

>  There are several serious mistakes in your assembly example. 
Thanks for the feedback! Appreciated. Comments below...

>  |START_FUNC(opal_atomic_add_32)
>  |       push    {r4-r7}
>  
>  Why r4 to r7?  You're using only r4 below. 
Ah. Because in the rest of the OpenMPI code they're doing similar on
function entry (saving all 4 of the variable registers). You're right
that I only use one of those registers so I could have not bothered to
save all 4 of them. Similar reasoning applies to the way I referred to
the kernel address for the helpers using a negative number, but you're
right that I can fix that too. So let's do that while doing the below.

>  |       mov     r4, r1
>  |       mov     r2, r0
>  |       mov     r12, lr
>  
>  r12 is not a good choice since it is allowed for callees to trash 
>  it.  Didn't r12 almost kill you in your sleep already anyway?  :-) 
In this particular case it's not with the current kernel implementation,
but you're right. I even knew that, and yet did it anyway, which was
bad, sorry. I'll fix that and send an updated patch out, based on your
example, which is more efficient.

>  Calling into __kuser_cmpxchg also clobbers r3 so your "blx
> r3" will 
>  branch into lalaland the second time around. 
This and the r12 use are the ones that bother me the most as they're
actual bugs. This will teach me to do this at 4am.

>  So... I'd suggest this version instead:
>  
>  START_FUNC(opal_atomic_add_32)
>          push    {r4, lr}
>          mov     r4, r1
>          mov     r2, r0
>          LSYM(4)
>          ldr     r0, [r2]
>          ldr     r3, REFLSYM(5)
>          add     r1, r0, r4
>          blx     r3
>  	bcc     REFLSYM(4)
>  	pop     {r4, lr}
>          bx      lr
>          .align  2
>          LSYM(5)
>          .word   0xffff0fc0
>  END_FUNC(opal_atomic_add_32) 
Thanks. I'll fix the writeup, and send out an updated patch.

Jon.

[Michael Hope]

On 14 November 2012 06:21, Jon Masters <jcm(a)redhat.com&gt; wrote:
>  On 11/13/2012 01:24 AM, Nicolas Pitre wrote:
>
> > There are several serious mistakes in your assembly example.
>
>  Thanks for the feedback! Appreciated. Comments below...
>
> > |START_FUNC(opal_atomic_add_32)
> > |       push    {r4-r7}
> >
> > Why r4 to r7?  You're using only r4 below.
>
>  Ah. Because in the rest of the OpenMPI code they're doing similar on
>  function entry (saving all 4 of the variable registers). You're right
>  that I only use one of those registers so I could have not bothered to
>  save all 4 of them. 
Random note: you need to save an even number of registers to keep the
stack aligned to eight bytes.  Nico's code below is good.

-- Michael