Adding shmem_malloc_with_hints interface

[manjugv]

Proposal to address the issue #258 .

[manjugv]

I think this is orthogonal to PR #256 and there is value in co-existing with the PR #256 .

[naveen-rn]

If I understand correctly in PR #256 we allocate the whole SHEAP using hints and then use shmem_kind_malloc for allocating SDO from the previously allocated SHEAP. In this PR, we are not controlling the SHEAP at all, it is just that during the shmem_malloc operation - we try to optimally place the SDO based on user hints.

[manjugv]

@naveen-rn Correct. Please refer #258 .

[hoopoepg]

@manjugv
question regarding Multiple hints are expressed as OR of hints.: how oshmem should react on case if supported less flags than requested? or supported all honts, but not at same buffer (and I believe this case - supported all but not at same buffer - is the most actual)?

it seems OR combination of traithints hints should be denied

[manjugv]

@hoopoepg That is correct. In the last sentence of this paragraph I say that. Some combination of hints does not make sense and some combination of hints might not be supported by a particular implementation. In either case, it can behave like shmem_malloc.

[naveen-rn]

On the core functionality of this proposal - do we need the hints to have both the storage traits and possible usage pattern? Can we just stick to the possible usage pattern, say SHMEM_HINT_AMO, SHMEM_HINT_SIGNAL? Something to discuss further.

[jamesaross]

When no action is performed, these routines return without performing a barrier.

What does this mean? That the function returns NULL for all PEs, no memory has been allocated, and no implicit barrier has occurred? Some applications may use shmem_malloc and friends as implicit barriers. This proposal is for an optimization which seems to be a drop in replacement for vanilla shmem_malloc, but dropping the implicit barrier on function exit would change the behavior of the application.

[manjugv]

When no allocation is done, “dropping the implicit barrier” is the behavior we have for shmem_malloc in OpenSHMEM 1.4 - please refer page 26 line 40-41. The proposal is aiming to maintain the same behavior for that case.

[jamesaross]

Oops. In OpenSHMEM 1.4, that was not the case. This behavior was changed between 1.4 and now in #201.

[manjugv]

Ah… Thanks for correcting that. It has been so long that we debated about this I did not realize it is relatively new. I was looking at my git copy. :)

Can we consider this issue resolved?

[jamesaross]

Must all PEs allocate from the same special memory? It's not clear if asymmetry can exist. Does this impose additional implicit synchronization for each subset configuration of hints if it cannot satisfy the entire hint list?

Also, what happens if you OR SHMEM_HINT_NONE with other hint behaviors?

[manjugv]

Good question.

All use cases that I have thought of requires the memory to be symmetric (same kind of memory).

Regarding extra synchronization, it depends on the implementation. If the implementations maintain asymmetric memory sizes (say each PE starts with different amount of special memory) on the PEs, you might need the extra synchronization for agreement. Otherwise, I do not see a need. In a way, it is similar to current DRAM allocations. Also, for the implementation we explored, we did not need extra synchronization.

I’m reluctant to add such a constraint. Without such constraint, the implementations are free to explore either approach (symmetric and asymmetric).

Do see value in specifying one way or other?

[jamesaross]

To clarify, what would using long hint = SHMEM_HINT_NONE | SHMEM_HINT_LOW_LAT_MEM | SHMEM_HINT_HIGH_BW_MEM do?

Dropping SHMEM_HINT_NONE, what if the platform could provide SHMEM_HINT_LOW_LAT_MEM or SHMEM_HINT_HIGH_BW_MEM but not both simultaneously? Will we see application code marked up like this because who doesn't want to use low latency and high bandwidth memory for their application? Does the "best effort" default to a platform-specific precedence? The only feedback is that an allocation succeeded or it did not.

[manjugv]

To clarify, what would using long hint = SHMEM_HINT_NONE | SHMEM_HINT_LOW_LAT_MEM | SHMEM_HINT_HIGH_BW_MEM do?

Though this is a legal usage, it does not make sense to use. The implementations are allowed to default to shmem_malloc in this case.

Dropping SHMEM_HINT_NONE, what if the platform could provide SHMEM_HINT_LOW_LAT_MEM or SHMEM_HINT_HIGH_BW_MEM but not both simultaneously? Will we see application code marked up like this because who doesn't want to use low latency and high bandwidth memory for their application? Does the "best effort" default to a platform-specific precedence? The only feedback is that an allocation succeeded or it did not.

“If more than one hint is provided, the implementation will make the best effort to use one or more hints to optimize performance. 
“

My intention with this statement was to provide flexibility for the implementations to optimize as they wish when the user provides multiple hints. Obviously, some combinations of hints might not make sense. In such cases, If the implementations want to give precedence of one hint over others, the proposal allows it. That (assigning priorities to hints) is one way to implement it, but not the only way.

[jamesaross]

Assuming that the #280 Teams Proposal makes it through for 1.5, won't all references to active set synchronization memory and reduction working memory be deprecated? If so, would SHMEM_HINT_PSYNC, SHMEM_HINT_PWORK also be deprecated before they become part of the specification? There seems to be a consistency problem with these two proposals.

Would SHMEM_HINT_SIGNAL (from #275) and SHMEM_HINT_ATOMIC have any different/distinct behavior in an implementation? It seems most code (and I believe all of the examples in the specification) use global or static variables for atomics and signals rather than shmem_malloc'ed memory. Should it be recommended to use shmem_malloc_with hints over static/global?

Often the lowest latency memory pools have the highest bandwidth (with some exceptions). Are we relying on users to know whether their application needs lower latency or higher bandwidth and to understand which pools the flags implicitly allocate from?

What about very large memory pools of slow memory or mapped storage when latency/bandwidth aren't issues (i.e. SHMEM_HINT_BIGMEM)?

[naveen-rn]

Would SHMEM_HINT_SIGNAL (from #275) and SHMEM_HINT_ATOMIC have any different/distinct behavior in an implementation?

Yes, depends on the implementation - there might be a difference.

[manjugv]

Assuming that the #280 Teams Proposal makes it through for 1.5, won't all references to active set synchronization memory and reduction working memory be deprecated? If so, would SHMEM_HINT_PSYNC, SHMEM_HINT_PWORK also be deprecated before they become part of the specification? There seems to be a consistency problem with these two proposals.

Yes, I agree with your observation. One way to approach such inconsistencies is via chapter committee. In this particular instance, I’m ok not to introduce it or resolve it via chapter committee.

[manjugv]

Would SHMEM_HINT_SIGNAL (from #275) and SHMEM_HINT_ATOMIC have any different/distinct behavior in an implementation?

Since the signal has a separate interface to fetch the signal value and it is atomic only with itself, it provides more\different opportunities for optimization. I can envision implementing them in different ways based on the capabilities of a particular network. On some systems, it might be the same.

[manjugv]

What about very large memory pools of slow memory or mapped storage when latency/bandwidth aren't issues (i.e. SHMEM_HINT_BIGMEM)?

In the usage scenarios that I have come across, we have not mapped symmetric memory to “slow” memory. So, I have not thought about such a hint. That said, it is a valid usage scenario. So, let's discuss this during the reading and see what rest of the committee thinks about adding it.

[manjugv]

It seems most code (and I believe all of the examples in the specification) use global or static variables for atomics and signals rather than shmem_malloc'ed memory. Should it be recommended to use shmem_malloc_with hints over static/global?

In general, managing global/static memory for global access via OpenSHMEM interfaces presents more constraints and thus fewer optimization opportunities. However, I’m still hesitant to provide a blanket recommendation because the performance depends on the system and implementation.

We could add some examples where we can show atomics allocated via shmem_malloc interfaces, if it is useful.

[manjugv]

Often the lowest latency memory pools have the highest bandwidth (with some exceptions). Are we relying on users to know whether their application needs lower latency or higher bandwidth and to understand which pools the flags implicitly allocate from?

That is correct.

[shamisp]

Low-latency relatively to what ? Low latency for local access ? remote access ? both ? only one of those ? Why I would ever what to allocate memory with low-latency and (related to next flag) slow bandwidth ?

[manjugv]

Low-latency relatively to what ? Low latency for local access ? remote access ? both ?

This is symmetric heap’s memory. So, it is for the memory accessed via shmem routines (which I would guess will be dominated by remote access).

If some architectures and use cases require both local access and remote access, it is easy to expand the hints. If the memory is needed only for local access, I guess, the programs should not allocate the memory from the symmetric heap.

[manjugv]

Why I would ever what to allocate memory with low-latency and (related to next flag) slow bandwidth ?

Already discussed here. Please refer to this discussion (#259 (comment))

[shamisp]

The memory is access locally using load and store semantics without shmem. This is how you initialize the memory with some data in a first place. It is also accessed remotely using load and store semantics without going through SHMEM API. My point the we cannot claim that something is low latency (the same true for high BW and most of other hints) since it really depends on who is accessing the data and this runtime information. For example, for Nic it can be some local sram region on the die and for the socket it L1/L2/L3. If you want to allocate memory on the nic for low latency access, just call it NIC memory, GPU memory, L1 memory ,etc. The low latency and high bw definitions are misleading.

[shamisp]

why we introducing tuning for something that we are deprecating ?

[manjugv]

This is always a challenge with tickets (that are developed in parallel) that has impact on each other.

Already discussed here. Please refer to this discussion (#259 (comment))

[shamisp]

why we introducing tuning for something that we are deprecating ?

[manjugv]

Already discussed here. Please refer to this discussion (#259 (comment))

[shamisp]

What happens with ATOMIC hint was not passed but atomic operation was used ?

[manjugv]

I don't explicitly cover the bad behavior semantics. But, based on the use case experience we have currently, I'm inclined to say that It should not impact correctness. However, the programs might not be able to get the best performance characteristics for that environment.

Do you prefer that we should explicitly say this?

[shamisp]

My initial assumption is that if this is hint, implementation can completely ignore it. But we still have issue with this semantics. For HCA optimal location of the memory for Atomics will be on the HCA. For core it will be optimal to have in L1/L2 which is suboptimal for ATOMICs coming from the network and vice-versa. Essentially such hint may actually do more harm to applications. My suggestion is to use explicit names - HCA1, HCA2, GPU, L2, LLC, etc. This will provide very clear description to user what actually happens.

[shamisp]

Since in this case you optimize for remote PEs only, I think it should be named SHMEM_HINT_REMOTE_ATOMICS to be clear that you don't optimize for local PEs. Without explicit specification local/remote the hint is not very useful.

[shamisp]

What happens when the flag was not used, but signal operation was used ?

[manjugv]

Hmm .. Which flag are you referring to, can you please elaborate?

[shamisp]

Above SHMEM_HINT_SIGNAL

[shamisp]

Also we should to state that neither alignment requirements or memory properties, such as cache line size are not get impacted by the hint. We also shell state the memory semantics for local assess (load and store) are not impacted by the hint.

[manjugv]

AFAIU, for the shmem_malloc routine the implementation is free to allocate the memory, which is either cache aligned or not. One of the constraints is that it should be word-aligned. Similarly, the memory access model (which is yet be defined or clarified here #229) will provide certain access guarantees to the memory allocated by shmem_malloc. In both cases, the proposal intends to follow the semantics of shmem_malloc.

[shamisp]

The memory gets eventually mapped into the core. The core architecture defined multiple way how it can be mapped. Each on of the mapping has own semantics and constrains. For example semantics between normal cacheable (write back) and non-cacheable (WC) is very different. Since user has direct access through the pointer, code that worked on one machine will break on another with exception or even data corruption.

[shamisp]

In order to support something like this, you have to remove shmem_ptr and prohibit any direct asses to the memory through load and store semantics. Next you would have to introduce shmem_memcpy function to copy-in-out shmem_malloced region.

[nspark]

Editorial suggestions

[manjugv]

Special ballot changes:

1. Change the name of the hint from SHMEM_HINT_ATOMICS to SHMEM_HINT_ATOMICS_REMOTE (755a4ed)
2. Change option SHMEM_HINT_NONE to “0”, when no hint is desired. (e7d011c)
3. Trim the hints list - remove hint for PSYNC, PWORK, Low latency, and high bandwidth (bf4de2e)

Editorial changes:
Fixing the typo by changing "hint" to “hints”, when relevant.

[jdinan]

Comments from Intel review of PR:

• The behavior for unrecognized bits in the hints argument should be more clearly specified -- unrecognized bits are ignored, behavior is undefined, routine returns NULL, machine catches fire, etc.
• There is concern that the routine could be hard to use in situations where users require that hints are recognized. That is, users can’t find out if hints were used successfully. One suggestion was to add a long *hints_out argument that indicates which hints were utilized (allow NULL for this argument if the user doesn't care).
• Can the routine return NULL if e.g. a memory allocation could not be performed? From reviewing the text in shmem_malloc, the conditions in which NULL is returned are not clearly specified.
• Should we add a hints bit SHMEM_HINT_USE_OR_NULL that requests this routine to return NULL if the hints could not be utilized?
• What is behavior of shmem_realloc on a pointer return by shmem_malloc_with_hints? Are the hints preserved? This should be specified (there is precedent with realloc on aligned allocations).
• Should we have calloc and realloc with hints operations?

[jdinan]

@nspark Could you please update (or dismiss) your review so that the PR is no longer flagged as "changes requested"? Thanks!

[shamisp]

Minor: I would consider to move the text under SHMEM_HINT_ATOMICS_REMOTE since this is critical information. BTW, probably the same true for remove SHMEM_HINT_SIGNAL vs local SHMEM_HINT_SIGNAL ? @naveen-rn

[naveen-rn]

If I understand the definition of remote vs local wrt this change, I'm not sure whether there is need to differentiate the local side to the signal. All signals from the source PE are passed-by-value to the remote signal address buffer. A buffer created with SHMEM_HINT_SIGNAL will mostly be updated by some remote PE.

void shmem_put_signal(shmem_ctx_t ctx, TYPE *dest, const TYPE *source, size_t nelems,
uint64_t *sig_addr, uint64_t signal, int sig_op, int pe);

[manjugv]

Comments from Intel review of PR:

• The behavior for unrecognized bits in the hints argument should be more clearly specified -- unrecognized bits are ignored, behavior is undefined, routine returns NULL, machine catches fire, etc.
• There is concern that the routine could be hard to use in situations where users require that hints are recognized. That is, users can’t find out if hints were used successfully. One suggestion was to add a long *hints_out argument that indicates which hints were utilized (allow NULL for this argument if the user doesn't care).
• Can the routine return NULL if e.g. a memory allocation could not be performed? From reviewing the text in shmem_malloc, the conditions in which NULL is returned are not clearly specified.
• Should we add a hints bit SHMEM_HINT_USE_OR_NULL that requests this routine to return NULL if the hints could not be utilized?
• What is behavior of shmem_realloc on a pointer return by shmem_malloc_with_hints? Are the hints preserved? This should be specified (there is precedent with realloc on aligned allocations).
• Should we have calloc and realloc with hints operations?

1. Only valid options to provide are the defined constants and I capture that in the text. It is similar to OpenSHMEM context options. I’m I missing something? Let us discuss on the call.

2. This usage I read semantically as constraints. I’m not sure if the Users should rely on any behavior like that. There are performance hints, really. The implementation provides no guarantee but does the best effort.
   Also, let's use the interface and gather experience, if Users think there is a need for such information. We can design an API for it. For example, a local query interface that provides information about memory’s attributes such as access model, size, alignment, and memory kind would be more useful, IMO.

3. IIRC, we discussed this at F2F and we decided to do nothing on this .i.e., no hints need to be inherited. The library already has information about how the User intends to use the memory. If the library implementation can optimize it, it will.

4. IMHO, it is not same alignment because alignment is more of a constraint than a hint.

5. Yes. As far as fixing the shmem_malloc - I think we will be over-specifying if we specify the conditions when the memory allocation fails. It is probably outside the scope of this ticket anyways.

6. For me it is not clear why would we need it? Is there a specific use case that would drive this? Also, I don’t want to add to this ticket. If I add, then I have to justify the need :)

I did not see them until this morning and I wanted to answer them before the vote :). So, the answers are a bit brief but hopefully, it makes the point. Also, if need be, we can discuss it during the vote.

[jdinan]

Suggest that we rename SHMEM_HINT_ATOMICS_REMOTE to SHMEM_MALLOC_ATOMICS_REMOTE to maintain the convention that we namespace constants according to which family of API routines they are used with. We follow this convention with SHMEM_CTX_* and SHMEM_TEAM_NUM_CONTEXTS constants.

[jdinan]

@manjugv Please let me know when the PR is ready to merge. We may need to use section committee review to approve edits prior to the merge.

[manjugv]

@jdinan The PR is ready to merge.

The two changes (1 and 2) we discussed today, I will bring it as separate PRs. So, it is easy to process. Both (1) and (2) we can do it as a chapter committee change. If the committee prefers to vote on (2) that is fine too.

(1) Change SHMEM_HINT_SIGNAL to SHMEM_HINT_SIGNAL_REMOTE
(2) Text about shmem realloc behavior with hints

[jdinan]

@manjugv Thanks -- will proceed with the merge.

Please bring into the discussion of (1) renaming these constants SHMEM_MALLOC_* to match the constant namespacing convention that we have established with contexts and teams constants.

I reviewed the existing text for (2) and I think I misremembered having text indicating how realloc deals with alignment. We may need to deal with the alignment part as a separate ticket. Something to keep in mind as you draft the text for how realloc deals with memory allocated by shmem_malloc_with_hints.