Signal Fetch Missing Context

[jdinan]

Issue

Should the shmem_signal_fetch operation have a context argument?

Discussion is still open on this.

Possible Solution

Add shmem_ctx_signal_fetch C/C++ binding and C11 generic selection API.

[jdinan]

@shamisp @manjugv @naveen-rn Can you please check if this can be fixed before the RC2 cutoff? There is no need to change the existing signal fetch function, we can add the context version and add a C11 binding as we have done with the other signaling operations.

[naveen-rn]

I thought signal fetch is a simple load operation - do we need a context for performing this operation? Isn't this analogous to shmem_signal_wait_until() without the conditional check?

[naveen-rn]

Well, I shouldn't say "simple load" - but some sort of the load, basically a local operation. I haven't thought about going through the network to read the local buffer.

[naveen-rn]

Side note: @nspark @BryantLam If we make this change, should we also update the memory ordering table - it now saya blocking and nbi put-with-signal. We should add shmem_signal_fetch.

[BryantLam]

The analogy to shmem_signal_wait_until (or even shmem_wait_until) seems apt. The atomics have ctx arguments, but shmem_wait_until doesn't.

Is shmem_signal_fetch closer to shmem_atomic_fetch and why the ctx argument is needed?

@naveen-rn -- The previous question should answer whether or not we put it in the memory ordering table. The atomics are already in the table.

[wrrobin]

We have a statement in the corresponding section:

Access to sig_addr signal object at the calling PE is expected to satisfy the atomicity guarantees as described in Section 9.8.1.

Based on this, I feel shmem_signal_fetch should be similar to a shmem_atomic_fetch rather than a local load operation. In such case, we might need a context argument as well.

[naveen-rn]

Incase, if I didn't make it clear:

1. I prefer to not add context argument, I don't see any need for converting the shmem_signal_fetch as a context-based operation. To me it is similar to shmem_signal_wait_until and not shmem_atomic_fetch.
2. Adding context as part of shmem_signal_fetch and not on shmem_signal_wait_until is also confusing.
3. @wrrobin: the exact statement refers to signaling atomicity and not OpenSHMEM atomics. Access to sig_addr signal object at the calling PE is expected to satisfy the atomicity guarantees as described in Section 9.10.1..
4. In case, if we prefer to add context objects, then we should add shmem_signal_fetch as part of the memory ordering table.

[wrrobin]

Thanks @naveen-rn for the clarification. Trying to understand, shmem_signal_fetch does not really wait for any event, right? It just returns the current value of the signal. Please correct me if I am wrong on this.

[naveen-rn]

Yes, it just reads the value of a local symmetric region without breaking the signaling atomicity guarantees.

[BryantLam]

@jdinan -- What are the arguments for adding the ctx argument to shmem_signal_fetch?

Fortunately, this change can be made after v1.5 in a non-breaking way. I don't mind deferring this change if we don't come to a consensus on its need.

[jdinan]

@BryantLam That's a good point with regard to urgency. The thing that triggered me posting this issue was @wrrobin's implementation that performs a network atomic fetch to read the data. The atomic fetch is needed for compatibility with signal atomic-add. Since it's a network operation, it requires a context within the implementation.

I appreciate @naveen-rn's point, as well, that this should be analogous to whatever is done within wait/test operations, and that does not require a context. I think I'm ok with deferring this discussion to 1.6 unless anyone objects.

[BryantLam]

I'm not sure if we want to solve this now, but I do want to point out that if we do decide to pursue adding a ctx argument to shmem_signal_fetch, the function is currently specified for only C/C++. We would have to deprecate the current function for C/C++ and introduce it again for C11.

This is similar to what happened with C/C++ shmem_wait_until(long) that now resides in the deprecation table.

In this way, adding a ctx argument would cause a similar split with how shmem_wait_until is currently defined where there are two versions of the routine, one for C/C++ and another using C11 generics.

[jdinan]

@BryantLam I don't think there will be an issue. We would have bindings like this:

uint64_t shmem_signal_fetch(uint64_t *signal); /* uses default context */
uint64_t shmem_ctx_signal_fetch(shmem_ctx_t ctx, uint64_t *signal);

#define FIRST(first_thing, ...) first_thing
#define shmem_signal_fetch(...) _Generic(FIRST(__VA_ARGS__), \
  shmem_ctx_t: shmem_ctx_signal_fetch, default: shmem_signal_fetch)(__VA_ARGS__)

You have to be careful about where and when the macro is defined, but we have used this workaround for the wait functions without issue.

[BKP]

@jdinan - Please mark me as an assignee on this issue.