Reduce barrier/collective execution floor

[avinciguerra07]

Shrink the shmem_barrier_all/collective critical path, where the internal pSync reset was issued as a put-to-self over the fabric plus a wait for its own NIC writeback on every PE every collective.

• collectives.c: replace the self-loopback pSync clear (put_scalar to shmem_internal_my_pe + SHMEM_WAIT_UNTIL == 0) with a local store plus an acquire-release fence at all 22 sync/broadcast/reduce/scan/fcollect sites. Each clear is preceded by a fully-draining counting wait, so the incoming call-ins are already drained and a local reset before the next reuse is race-free; remote ack/signal puts are unchanged.

• transport_ofi.h: skip the progress-lock trylock/unlock in shmem_transport_probe() when running SHMEM_THREAD_SINGLE, and read the put error counter in shmem_transport_quiet() only when completions have stalled (success < cnt) instead of every spin, keeping error detection intact.

Barrier microbenchmark (osu_oshm_barrier) shows a ~15-40 percent reduction of the pure barrier.

[lstewart]

This changes a put to self with a CPU store. I do not understand why this should work. If other PEs modify the same location using atomics and the atomicity zone is using NIC atomics that might be cached in the NIC, then a CPU atomic will not work correctly. If the only remote operations are ordinary puts, then it could work.

[avinciguerra07]

This changes a put to self with a CPU store. I do not understand why this should work. If other PEs modify the same location using atomics and the atomicity zone is using NIC atomics that might be cached in the NIC, then a CPU atomic will not work correctly. If the only remote operations are ordinary puts, then it could work.

I was working on the assumption that the barrier makes the slot quiescent at clear time: the SHMEM_WAIT_UNTIL(pSync == N) above the clear has already drained all N call-ins for this round, and barrier semantics forbid any PE from issuing the next round's call-in until every PE has left this barrier (each waits for its ack). So between "count satisfied" and "next round's first atomic," no atomic is in flight against the cell, nothing races the store.

But if I understand what you are saying, a NIC-atomic provider, "drained all call-ins" (completion-visible) does not guarantee the NIC has written the value back to host memory?

This seems to work for ofi, but I might be breaking some other transport. Would it be alright if I gated it with something like this:

static inline void
shmem_internal_sync_clear(long *pSync)
{
if (shmem_transport_ofi_host_atomics) {
*pSync = 0;
shmem_internal_membar_acq_rel();
} else {
long zero = 0;
shmem_internal_put_scalar(SHMEM_CTX_DEFAULT, pSync, &zero,
sizeof(zero), shmem_internal_my_pe);
SHMEM_WAIT_UNTIL(pSync, SHMEM_CMP_EQ, 0);
}
}

Or something more conservative like an env variable and we have to manually enable it at config: --enable-host-atomic-psync-clear?

[lstewart]

That is indeed the issue I am worried about, you can't mix shmem atomics with loads, stores, or atomics in a different atomicity domain.
However, maybe there is another way? Rather than reset the psync to zero, why not just remember its starting value and do the WAIT_FOR with starting_value + num_pes -1 ?
Also, if there IS a problem mixing atomics and non-atomics, then it was already wrong to use shmem_put_internal anyway.

The first idea only works if the psync is only used for barriers with the same arguments...
It looks like you could do this hack only for the non-active-set versions, which use internal dedicated psync and you can make your own rules.

I'd argue the performance of the active set versions is uninteresting, and the spec is broken anyway, since it specifies initializing the user supplied psync with load/store and then uses them with atomics.

[lstewart]

Well the exact wording is
OpenSHMEM atomic operations do not guarantee exclusivity in the following scenarios, all of which result in unde-
fined behavior.

1. When concurrent accesses to the same location are performed using OpenSHMEM atomic operations using
   communication contexts in different atomicity domains.
2. When concurrent accesses to the same location are performed using OpenSHMEM atomic operations using
   different datatypes.
3. When atomic and non-atomic OpenSHMEM operations are used to access the same location concurrently.
4. When OpenSHMEM atomic operations and non-OpenSHMEM operations (e.g., load and store operations) are
   used to access the same location concurrently.

Which at least suggests that you can alternate atomic and non-atomic accesses provided you can prove the accesses are not concurrent, which I think means potentially occurring in different orders

In the case of barrier, the WAIT guarantees that clearing the psync doesn't occur until after all atomic updates are visible, and doing the clear before the downstream notifications guarantees that no future atomics will occur until the clear is done

So I guess the change is OK.

[lstewart]

This also suggests that NIC atomics are not allowed to cache, which I find surprising. I wonder if that is true.

[avinciguerra07]

Rather than reset the psync to zero, why not just remember its starting value and do the WAIT_FOR with starting_value + num_pes -1 ?

I like this idea much better than mine. I will test it a little bit and if all goes well you should see a push with this implementation soon.