XrdCl: Use std::atomic when possible for the PostMaster pointer

[esindril]

Hi all,

I've created this pull request which deals with the atomicity of the sPostMaster pointer. This is only enforced when compiled with c++11 support. Let me know if this is acceptable for you or if you want the __sync_fetch* approach. The reason for this is that on x86_64 architectures this approach has no performance penalty for the load operation.

For example the following code:

#include <atomic>

std::atomic<int> a;
void val_set(int v) { a = v; }
int val_get() { return a; }

yields the following assembler code on x86_64 for the two functions:

Z7val_seti:
        .cfi_startproc
        movl    %edi, a(%rip)
        mfence
        ret
        .cfi_endproc

_Z7val_getv:
        .cfi_startproc
        movl    a(%rip), %eax
        ret
        .cfi_endproc

Therefore the get operation is no different on the atomic variable than on the non-atomic one.

For an ARM architecture, the the issue is totally different. The assembler code for the same functions is:

Z7val_seti:
        @ args = 0, pretend = 0, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        stmfd   sp!, {r4, r5, r6, lr}
        mov     r5, r0
        ldr     r4, .L2
        mov     r0, r4
        bl      __atomic_flag_for_address
        mov     r1, #5
        mov     r6, r0
        bl      __atomic_flag_wait_explicit
        mov     r0, r6
        mov     r1, #5
        str     r5, [r4, #0]
        ldmfd   sp!, {r4, r5, r6, lr}
        b       atomic_flag_clear_explicit

_Z7val_getv:
        @ args = 0, pretend = 0, frame = 0
        @ frame_needed = 0, uses_anonymous_args = 0
        stmfd   sp!, {r3, r4, r5, lr}
        ldr     r4, .L5
        mov     r0, r4
        bl      __atomic_flag_for_address
        mov     r1, #5
        mov     r5, r0
        bl      __atomic_flag_wait_explicit
        ldr     r4, [r4, #0]
        mov     r0, r5
        mov     r1, #5
        bl      atomic_flag_clear_explicit
        mov     r0, r4
        ldmfd   sp!, {r3, r4, r5, pc}

Therefore, the trade-off is that when you compile for ARM it will be a bit slower and you need to use c++11 which I assume is already the case for the CMSSW where c++11 is omnipresent.
Let me know your thoughts on this and if everything is fine, I will merge it tomorrow.

Cheers,
Elvin

[ljanyst]

You already hold explicit mutex when you do mutations, so memory_order_relaxed would be enough, you only need atomicity. This will likely just result with something like xchg [RAX], RCX on x86_64, no memory fence, same as __sync_fetch* would. If you solve the problem only for C++11, you leave out most clients without a fix.

[esindril]

Thanks @ljanyst, I've update the pull request to use memory_order_relaxed.

[ljanyst]

You're welcome. I would strongly recommend making a fix for non-c++11 clients as well.

[esindril]

There is already one. #266 :)

[bbockelm]

@esindril - did you push the update? It still seems to be using too-strong of memory ordering.

[bbockelm]

As-is, DefaultEnv::Finalize does not look thread-safe. Is there a mutex held by the caller? To make it thread-safe, you'd want to do something like:

PostMaster* post_master = sPostMaster.exchange(nullptr);
if (post_master) {
   ...
   delete post_master;
}

This guarantees only one thread enters the conditional block.

[esindril]

It uses memory_order_relaxed only inside the locked area. The rest uses strong memory ordering.
@bbockelm even with the exchange above this is not thread safe since a FileStateHandler might already hold a pointer to the PostMaster which is going to be deleted.

So, I don't know exactly what was the initial idea here ... maybe @ljanyst knows more?

[bbockelm]

@esindril - my point is that you could use memory_order_relaxed outside the lock too because the sPostMaster pointer is reloaded after the lock is taken.

[ljanyst]

DefaultEnv::Finalize is called from __cxa_finalize so if there are still user threads referring to us, then it's a user error.

@esindril @bbockelm is right.

[esindril]

DefaultEnv::Finalize is called as a result of destroying the EnvInitializer object which is static. This is called after the program's exit function and after all stack objects have been destroyed. Therefore, we don't need to worry about client stack objects still accessing the sPostMaster value.

Now, the dynamically allocated object should be properly destroyed by the client. If they fail to do this before the exit call then this is an application error. If they are automatically managed i.e. shared_ptr then they fall in the previous category of stack objects. So, how can an user object still refer to us in this case? Shouldn't all user objects be destroyed by the time we get to the Finalize method. Of course, except the user static ones ...

[ljanyst]

DefaultEnv::Finalize does not need modifications at all. @bbockelm is right about the memory order. You can use memory_order_relaxed everywhere, since you just need atomicity and nothing else.

[esindril]

Ok, it wasn't clear for me what you were referring to before. Given that this only covers C++11, I will drop this pull request in favour of #266 which uses the __sync_fetch* functions.