Thread Safety Proposal (RM Ticket #218) #43

manjugv · 2017-02-07T15:20:23Z

This proposal specifies the interaction between OpenSHMEM interfaces and the user threads.
Further, also introduces interfaces to initialize and query thread-safe OpenSHMEM library.

BryantLam · 2017-02-10T17:15:57Z

Since we are planning on deprecating the Fortran API, we should probably refrain from adding to it.

manjugv · 2017-02-10T19:01:35Z

@BryantLam Makes sense. I'm ok with removing Fortran API.

nspark

Miscellaneous editorial notes:

Please use the \FUNC{} command when referencing a function by name.
Please use consistent capitalization of function names; e.g., SHMEM_BARRIER_ALL() vs shmem_barrier_all(). (Generally, I prefer lowercase C-style names to uppercase Fortran-style names, and I recognize this needs consistent treatment throughout the whole spec.)

nspark · 2017-02-13T19:08:18Z

content/shmem_init_thread.tex

+    \FUNC{shmem\_init\_thread}  initializes the \openshmem{} library similar to
+    \FUNC{shmem\_init}, and in addition perform the initialization required for
+    thread-safe invocation of \openshmem{} functions. The argument
+    \textit{requested} is used to specify the desired level of thread support.


s/textit/VAR/

nspark · 2017-02-13T19:09:23Z

content/shmem_init_thread.tex

+    \FUNC{shmem\_init}, and in addition perform the initialization required for
+    thread-safe invocation of \openshmem{} functions. The argument
+    \textit{requested} is used to specify the desired level of thread support.
+    The function returns the support provided by the library. There are four 


Are there four or three? You only call out SHMEM_THREAD_{SINGLE, FUNNELED, MULTIPLE} under the API arguments, but also include SERIALIZED below.

Suggest: "...returns the support level provided..."

nspark · 2017-02-13T19:16:48Z

content/shmem_init_thread.tex

+
+The function may be used to initialize \openshmem{}, and to initialize the
+\openshmem{} with thread safety, instead of SHMEM\_INIT. The SHMEM\_INIT\_THREAD
+may not be called multiple times in an \openshmem{} program.


I think it should also be noted here and in shmem_init() that it is only permissible to call one of the routines once. For example, we explicitly say calling shmem_init() and shmem_init_thread() more than once is erroneous, but what about the sequence of shmem_init(); shmem_init_thread(...);? This should be undefined and need to be called out as such.

@nspark The sequence shmem_init(); shmem_init_thread() should be valid.

That seems odd. What's the rationale behind this? Is the following pattern also acceptable?

shmem_init(); shmem_put(...); /* ...and other shmem calls */ shmem_init_thread(); shmem_put(...); /* ...and more shmem calls */

If so, this makes it seem like a program has a one-time chance to change its thread-support level from the default implied by shmem_init() to whatever is specified and supported by shmem_init_thread(). It also seems messy that one could shmem_init_thread() after some arbitrary number of OpenSHMEM calls.

Edit Update: How about calling shmem_init_thread() before calling shmem_init()? Is that allowed?

That seems odd. What's the rationale behind this? Is the following pattern also acceptable?

shmem_init();
shmem_put(...); /* ...and other shmem calls /
shmem_init_thread();
shmem_put(...); / ...and more shmem calls */

Yes. The implementations can avoid all threading overheads until the shmem_init_thread is called. So, in your example, the first shmem_put might be implemented more efficiently than the second.

Edit Update: How about calling shmem_init_thread() before calling shmem_init()? Is that allowed?

There is no need for calling shmem_init(), since shmem_init_thread() initializes the library as well as the environment for threads.

There is no need for calling shmem_init(), since shmem_init_thread() initializes the library as well as the environment for threads.

Yes, but is it permitted or prohibited (I'd rather it not be ambiguous)? If it is permitted, does calling shmem_init() after shmem_init_thread() change the threading model (e.g., back to the default)?

Though the shmem_init(); shmem_init_thread() sequence usage model seems to provide some performance efficiency at present, in the Context time frame this would be too difficult to handle. The default property of all the contexts depends on the threading model value from shmem_init_thread(). Allowing contexts usage in @nspark example like;

shmem_init(); shmem_ctx_create(0, ctx1); /* consider 0 as the default option */ shmem_ctx_create(0, ctx2); shmem_init_thread();

would require the implementation to change the property of all the contexts and rearrange the network mapping after shmem_init_thread(). This would be hard for the implementation to handle.
@jdinan please clarify, if my understanding on the Context approach is correct.

There is no need for calling shmem_init(), since shmem_init_thread() initializes the library as well as the environment for threads.
Yes, but is it permitted or prohibited (I'd rather it not be ambiguous)? If it is permitted, does calling shmem_init() after shmem_init_thread() change the threading model (e.g., back to the default)?

Ok. It should be prohibited.

Though the shmem_init(); shmem_init_thread() sequence usage model seems to provide some performance efficiency at present, in the Context time frame this would be too difficult to handle. The default property of all the contexts depends on the threading model value from shmem_init_thread(). Allowing contexts usage in @nspark example like;

shmem_init();
shmem_ctx_create(0, ctx1); /* consider 0 as the default option */
shmem_ctx_create(0, ctx2);
shmem_init_thread();
would require the implementation to change the property of all the contexts and rearrange the network mapping after shmem_init_thread(). This would be hard for the implementation to handle.
@jdinan please clarify, if my understanding on the Context approach is correct.

@naveen-rn As long as, the context can be created in SHARED or PRIVATE mode, and can be used with/without threads, it should be enough to support this. I’m not sure, I understand the new challenge.

@naveen-rn As long as, the context can be created in SHARED or PRIVATE mode, and can be used with/without threads, it should be enough to support this. I’m not sure, I understand the new challenge.

As per my understanding from the current standing Context proposal, Contexts can be created either by explicitly mentioning the property like PRIVATE/SERIALIZED, or they can be created with a default property. The default property depends on the value of the threading model set during initialization. In this example;

#define DEFAULT 0 /* If 0 is used on the context property - the default property is set */ shmem_init(); shmem_ctx_create(DEFAULT, ctx1); /* ctx1 property PRIVATE because of shmem_init */ shmem_ctx_create(SHARED, ctx2); shmem_init_thread(SHMEM_THREAD_MULTIPLE); shmem_ctx_create(DEFAULT, ctx3); /* ctx3 property SHARED because of shmem_init_thread */

The default property of the Contexts is inconsistent across different regions in the application

When the default property changes, should we modify the existing property of the already created Contexts(probably not - too expensive) to make it consistent?

And, then there is SHMEM_CTX_DEFAULT - this definitely needs modification of its existing property before and after shmem_init_thread(). I think this particular change is reasonable.

Yes. The implementations can avoid all threading overheads until the shmem_init_thread is called. So, in your example, the first shmem_put might be implemented more efficiently than the second.

If removing threading overheads is the main reason behind allowing users to use the shmem_init(); shmem_init_thread() sequence, why don't they just use a PRIVATE context to achieve that?

PS: After reading my own reply, I feel this seems to be more like a Context related discussion. If you feel this usage sequence is very important, we could handle this in the Context proposal.

We would have to say that context properties are defined by the current library state for this to be sane.

My feeing on the init/init_thread discussion is that if users truly need it, we can add allowing runtime changes to the threading model in the future without impacting backward compatibility. I don't think we have a compelling reason to take on this design complexity right now.

nspark · 2017-02-13T19:19:54Z

content/shmem_init_thread.tex

+The \openshmem{} program may be multithreaded, and any 
+thread may invoke the \openshmem{} interfaces.
+
+\item SHMEM\_THREAD\_UNSUPPORTED: The \openshmem{} specification 


What does this actually mean? How is this different from SHMEM_THREAD_SINGLE? This seems intended as a catch-all value, but doesn't seem different from SINGLE, which I think could confuse users.

This mode is intended for supporting backward compatibility - supports any variation of thread model that exists in the current implementations. In the case of SHMEM_THREAD_SINGLE, it is illegal to have more than one thread in the OpenSHMEM program, even if only one thread invokes the OpenSHMEM interfaces. This, however, could be allowed in the UNSUPPORTED mode.

I agree that it's different from SHMEM_THREAD_SINGLE, but it's also not informative, which I think is my fundamental issue. If a developer is calling shmem_init_thread() or shmem_query_thread(), then they're already using an API beyond 1.3. A return value of SHMEM_THREAD_UNSUPPORTED does not tell the developer anything meaningful about whether any threads can be used and, if so, whether those threads can make OpenSHMEM API calls.

An OpenSHMEM implementation that wants to make no substantial effort for thread support should at a minimum identify which of the four models (likely, SINGLE or FUNNELED) is the default level of support.

+1 for Nick's comment. I believe the model any existing openshmem implementations provide must fit into one of the listed thread levels. Or else, we might be missing a thread level that is important. And I think we are not missing one.

@nspark Do you agree that we should support backward comptability, i.e., no thread support ?
If so, I don’t think just having SINGLE or FUNNELED captures that. The implementation can choose to make just SHMEM_INIT and functions that can be called outside the INIT/FINALIZE thread safe, and rest of it not thread safe, which is an useful implementation. But this is not supported by any of one of the four modes.

May be UNSUPPORTED is not a precise name (we can choose to call UNDEFINED/DEFAULT as suggested by Jim).

I think that @spotluri is saying that the SERIALIZED model could support the case you identified. There might be implementation implications, such as memory fences around some calls, that aren't needed for the case you identified, but it would still work within the model.

Given the limited state of OpenSHMEM threading extensions, is this case you identified one we need to seriously consider supporting? I don't think it is.

I think that @spotluri is saying that the SERIALIZED model could support the case you identified. There might be implementation implications, such as memory fences around some calls, that aren't needed for the case you identified, but it would still work within the model.

It is closely related to FUNNELED rather than SERIALIZED. If we relax the main thread constraint in FUNNELED, it will support the mode that I mentioned above. However, there are overheads for that.

Given the limited state of OpenSHMEM threading extensions, is this case you identified one we need to seriously consider supporting? I don't think it is.

I agree, we should not support it. In the #43 (comment)) reply to @spotluri , the point I was making was that the four modes are not an exhaustive list.

Summary of discussion and options (please correct me , if I missed any options @nspark @spotluri @jdinan )

We need to define that there is a default mode when we initialize with shmem_init, and then we have to define the default mode. The three options for the default mode are:

The default mode has to be one of the four modes (SINGLE,FUNNELED,SERIALIZED,MULTIPLE).

The default mode has to be one of the four modes or a “no thread support mode” where only a subset of shmem interfaces are thread safe.

The default mode is the extra mode - (UNSUPPORTED/UNDEFINED/IMPLEMENTATION_DEFINED)

@manjugv thanks for clarifying. I get the difference now. I agree that the new mode is not common and need not be a supported.

I think we should go with option 1 above, with default being THREAD_SINGLE. My understanding of default is that the implementation is free to provide anything >= default. when shmem_init is called.

I agree with @nspark that we should have a list of calls are thread-safe irrespective of the thread mode. At least shmem_query_thread, no?

I agree with @nspark that we should have a list of calls are thread-safe irrespective of the thread mode. At least shmem_query_thread, no?

@spotluri If we decide to go with option 1, I would say think there is no need this.

nspark · 2017-02-13T19:21:06Z

content/shmem_init_thread.tex

+\end{itemize}
+
+The function may be used to initialize \openshmem{}, and to initialize the
+\openshmem{} with thread safety, instead of SHMEM\_INIT. The SHMEM\_INIT\_THREAD


Suggest: "\openshmem{} library with thread safety..."

nspark · 2017-02-13T19:38:54Z

content/threads_intro.tex

+
+\item
+In a thread-compliant implementation, an \openshmem{} \ac{PE} is multithreaded. 
+Each thread can issue \openshmem{} calls; however the threads


I'm having trouble reconciling the two statements:

In a thread-compliant implementation, an OpenSHMEM is multithreaded.

Each thread can issue OpenSHMEM calls; ...

Is the latter point part of the definition of a "thread-compliant implementation"? If so, then an implementation that supports SHMEM_THREAD_FUNNELED is not "thread compliant".

I also think this mixes up two points. That a PE is multithreaded is an application characteristic, but I think the point you're trying to make about thread compliance is an implementation characteristic.

Ah .. the final changes did not make it.

@nspark What do you think about this to address your concern ? "A multithreaded OpenSHMEM PE using thread-compliant implementation can issue OpenSHMEM calls from any of its threads. However, the threads are not separately addressable."

That still seems to conflict with, say, the FUNNELED model. A program using the FUNNELED model is an example of a multi-threaded OpenSHMEM PE, but only one (the main) thread can issue OpenSHMEM calls.

What is your intent in establishing the term "thread-compliant implementation"? Assuming that an updated version of this PR is accepted for 1.4, aren't all implementations required to provide, for example, shmem_init_thread()?

I think a "thread-supporting implementation" might be better defined as one that supports a threading model higher (need a better word here) than SHMEM_THREAD_SINGLE. That said, as a user, I don't want any implementation to take the low-bar out and say "we support threads!" but only provide FUNNELED. I'm not sure establishing such a term is necessary here.

nspark · 2017-02-13T19:42:24Z

content/threads_intro.tex

+be executed by all processes in the same order also applies in a threaded
+environment. 
+
+\item In a thread-compliant implementation, 


This statement's implication seems more direct: A thread-compliant implementation is only one that supports SHMEM_THREAD_MULTIPLE. Is that the intent here?

@nspark A thread-compliant implementation should support all modes. This statement applies to only MULTIPLE. Remember, at this paragraph, we haven't introduced the modes yet.

Well, that's an ambiguous forward reference that's defining a term I'm not sure we need. I've already asked in this comment about the intent in establishing the term "thread-compliant implementation", so I won't ask again here.

nspark · 2017-02-13T19:46:29Z

content/threads_intro.tex

+must be treated as collective operations.
+
+\item
+\openshmem{} thread calling SHMEM\_INIT() is designated as the main thread.


Suggest: "\openshmem{} thread calling \FUNC{SHMEM_INIT()} or \FUNC{SHMEM_THREAD_INIT()} ..."

@nspark How do you want to define the behavior, when shmem_init and shmem_init_thread are called by the different threads ? I'm not sure, we want to introduce that behavior

As I commented here, I don't see why we would allow calls to both shmem_init() and shmem_init_thread() in the same program, but not allow two or more calls of either function individually. I think it introduces more issues than it helps resolve (and I'm not sure what issue(s) it resolves). If you cannot call both functions, then the problem is solved.

nspark · 2017-02-13T19:47:58Z

content/threads_intro.tex

+
+\item
+\openshmem{} thread calling SHMEM\_INIT() is designated as the main thread.
+Multiple threads may not call SHMEM\_INIT(). Similarly, \openshmem{} finalize


Suggest: "\FUNC{SHMEM_INIT()}"

Suggest: "\openshmem{} finalize" ==>"\FUNC{shmem_finalize}"

nspark · 2017-02-13T19:48:38Z

content/threads_intro.tex

+In a thread-compliant implementation, if multiple threads call the collective
+calls, it is the programmer's responsibility to ensure the correct ordering of
+collective calls.  The symmetric heap management functions, which are defined to call
+SHMEM\_BARRIER\_ALL()(\ref{sec:mem_routines}) before they return 


Suggest: "\FUNC{SHMEM_BARRIER_ALL()}"

spotluri · 2017-02-14T20:00:23Z

content/shmem_init_thread.tex

+\apinotes{
+The \openshmem{} programming model does not recognize individual threads.  Any
+\openshmem{} operation initiated by a thread is considered an action of the
+process as a whole. Thread-safety should not be activated unless needed.


should this be PE instead of process

spotluri · 2017-02-14T20:06:51Z

content/shmem_finalize.tex

@@ -24,7 +24,7 @@
    releases resources used by the \openshmem library. This collective
    operation requires all \acp{PE} to participate in the call. There is an
    implicit global barrier in \FUNC{shmem\_finalize} so that pending
-    communications are completed, and no resources can be released until all
+    communications \newtext{by all threads of the \ac{PE}} are completed, and no resources can be released until all


I fear there might have been a lot of conversation on this which I missed.

I think "\newtext{by all threads of the \ac{PE}}" can be safely removed. We already say else where that actions initiated by a thread as if they are initiated by the PE.

@spotluri Yes, we did discuss quite a bit :). As I mentioned on the WG call, this is to emphasize that the User should complete all communications on all threads.

this is to emphasize that the User should complete all communications on all threads.

@manjugv Except that the text specifies an "implicit global barrier", which does just the opposite; it emphasizes to the user that the library will take care of completing communication.

Seems like adding this creates more confusion. :)

spotluri · 2017-02-14T20:26:06Z

content/threads_intro.tex

+routine such as \FUNC{SHMEM\_MALLOC()} 
+allocates memory that is accesible by all threads of the \ac{PE}. 
+The requirement that the same symmetric heap operations must
+be executed by all processes in the same order also applies in a threaded


processes -> PEs

spotluri · 2017-02-14T20:28:26Z

content/threads_intro.tex

+some order, even if their execution is interleaved.
+
+\item Blocking \openshmem{} calls will block the calling thread only, allowing another
+thread to execute, if available. The calling thread will be blocked until the


another thread -> other threads?

spotluri · 2017-02-14T20:29:14Z

content/threads_intro.tex

+runnable, within a finite time. A blocked thread will not prevent progress of
+other runnable threads on the same \ac{PE}, and will not prevent them from
+executing other \openshmem{} calls. Also, a blocked thread will not prevent the
+progress of other \openshmem{} calls on other \acp{PE}. 


other openshmem calls on other PEs -> openshmem calls on other PEs?

nspark · 2017-02-22T20:44:56Z

content/shmem_init_thread.tex

+}
+
+\apireturnvalues{    
+	NONE


Suggest: "NONE" --> "None"

spotluri · 2017-02-22T20:55:45Z

content/shmem_wait.tex

@@ -45,14 +45,16 @@

 \apidescription{ 
    \FUNC{shmem\_wait} and \FUNC{shmem\_wait\_until} wait for \VAR{ivar} to be
-    changed by a remote write or an atomic operation issued by a different \ac{PE}.
+    changed by a remote write or an atomic operation issued by a


this can be changed by a local write.

nspark

Mostly minor typo/editorial changes

nspark · 2017-02-22T20:52:43Z

content/shmem_init_thread.tex

 \begin{apiarguments}
 \apiargument{IN}{requested}  {The thread level support requested by the user.
-The correct values are SHMEM\_THREAD\_SINGLE, SHMEM\_THREAD\_FUNNELED, or SHMEM\_THREAD\_MULTIPLE}
+The correct values are SHMEM\_THREAD\_SINGLE, SHMEM\_THREAD\_FUNNELED,


Suggest: Use \CONST{} here

nspark · 2017-02-22T20:53:10Z

content/threads_intro.tex

 allocates memory that is accesible by all threads of the \ac{PE}. 
 The requirement that the same symmetric heap operations must
-be executed by all processes in the same order also applies in a threaded
+be executed by all \acp{PE} in the same order also applies in a threaded


Is \acp{PE} a typo?

nspark · 2017-02-22T20:59:34Z

content/threads_intro.tex

 event on which it is waiting occurs. Once the blocked communication is enabled
 and can proceed, then the call will complete and the thread will be marked
 runnable, within a finite time. A blocked thread will not prevent progress of
 other runnable threads on the same \ac{PE}, and will not prevent them from
 executing other \openshmem{} calls. Also, a blocked thread will not prevent the
-progress of other \openshmem{} calls on other \acp{PE}. 
+progress of \openshmem{} calls on other \acp{PE}. 


Again, \acp{PE}; typo?

nspark · 2017-02-22T20:59:57Z

content/threads_intro.tex

@@ -65,8 +65,8 @@

 \item[]
 The completion semantics are not impacted by the multiple threads. 
-For example, the shmem\_barrier\_all() is completed when all \acp{PE} enter and
-exit the shmem\_barrier\_all() call, even though only one thread in the \ac{PE} is
+For example, the \FUNC{shmem\_barrier\_all} is completed when all \acp{PE} enter and


Again, \acp{PE}; typo?

Again, \acp{PE}; typo?

No. This \acp{PE} will expand to PEs (for second instance and onwards)

@manjugv Feel free to delete these comments, they were my mistake.

…ton before finalize

manjugv · 2017-08-26T11:12:43Z

@jdinan Back to you for review.

manjugv · 2017-08-26T11:13:45Z

@BryantLam Let me know, If I haven't addressed your comments.

jdinan · 2017-08-26T12:19:02Z

content/shmem_barrier_all.tex

@@ -1,6 +1,5 @@
 \apisummary{
-    Registers the arrival of a \ac{PE} at a barrier and suspends \ac{PE} execution
-    until all other \acp{PE} arrive at the barrier and all local and remote memory
+    Registers the arrival of a \ac{PE} at a barrier and blocks the \ac{PE} until all other \acp{PE} arrive at the barrier and all local and remote memory


jdinan · 2017-08-26T12:23:22Z

content/shmem_wait.tex

-\apiargument{OUT}{ivar}{A remotely accessible integer variable that is being updated
-    by another \ac{PE}.  When using \CorCpp, the type of \VAR{ivar} should
-    match that implied in the SYNOPSIS section.} 
+\apiargument{OUT}{ivar}{A remotely accessible integer variable. If you are using \CorCpp, the type of \VAR{ivar} should


jdinan · 2017-08-26T12:23:34Z

content/shmem_quiet.tex

    \FUNC{shmem\_quiet} does not have an effect on the ordering between memory 
-    accesses issued by the target PE.
-    \FUNC{shmem\_wait}, \FUNC{shmem\_wait\_until}, \FUNC{shmem\_test}, \FUNC{shmem\_barrier},
+    accesses issued by the target PE. \FUNC{shmem\_wait}, \FUNC{shmem\_wait\_until}, \FUNC{shmem\_test}, \FUNC{shmem\_barrier},


jdinan · 2017-08-26T12:27:55Z

content/synchronization_model.tex

 used when computation on the local \ac{PE} cannot proceed without the value that
-the remote \ac{PE} is to update.} \tabularnewline
+the remote \ac{PE} is to update. \tabularnewline


Why were the braces removed?

jdinan · 2017-08-26T12:29:29Z

content/threads_intro.tex

+prevent them from executing other \openshmem calls when the thread level permits. 
+Also, a blocked thread will not prevent the progress of \openshmem calls on other \acp{PE}. 
+
+\item In \CONST{SHMEM\_THREAD\_MULTIPLE} thread level, all \openshmem calls are thread-safe, 


In +the+ ...

jdinan · 2017-08-26T12:29:44Z

content/threads_intro.tex

+i.e., two concurrently running threads may make \openshmem calls and the outcome 
+will be as if the calls executed in some order, even if their execution is interleaved.
+
+\item In \CONST{SHMEM\_THREAD\_SERIALIZED} and \CONST{SHMEM\_THREAD\_MULTIPLE} thread levels, 


In +the+ ...

jdinan · 2017-08-26T12:30:23Z

main_spec.tex

-\acp{API} allow one to dynamically allocate, deallocate, reallocate and align
+\label{sec:mem_routines}
+\openshmem provides a set of \ac{API}s for managing the symmetric heap. The
+\ac{API}s allow one to dynamically allocate, deallocate, reallocate and align


Replace with \acp{API}. Looks like a merge mistake.

Signed-off-by: James Dinan <james.dinan@intel.com>

jdinan · 2017-08-28T01:41:31Z

content/backmatter.tex

 \\See Section \ref{subsec:shmem_wait}
-
+%


Love it or hate it, this is how we format the changelog entries... :meh:

jdinan · 2017-08-28T01:42:37Z

@manjugv I pushed a few patches to resolve a couple remaining DocEdits and clean up whitespace.

This is ready to go. Any additional edits should be done by the section committee.

Requesting +1 from @BryantLam, @manjugv, @abouteiller to merge.

manjugv · 2017-08-28T10:46:14Z

@jdinan This comment (#43 (comment)) has to be handled. Otherwise, there are no outstanding changes. @jdinan @abouteiller Please comment on this.

jdinan · 2017-08-28T12:57:41Z

@manjugv With regard to #43 (comment), I'd like to merge what was read and move editing to the section committee. I think we'll get more benefit out of merging the proposal and getting it in front of the section committees than continuing to work out of this PR.

manjugv · 2017-08-28T14:13:17Z

@jdinan Sure, I don't mind handling that way. 👍 to merge

abouteiller · 2017-08-28T14:40:22Z

content/shmem_barrier_all.tex

-    have called \FUNC{shmem\_barrier\_all}.  This routine must be used with
+    once.  This routine blocks the \ac{PE} until all \acp{PE} have called
+    \FUNC{shmem\_barrier\_all}. In a multithreaded \openshmem
+    program, only the calling thread is blocked. This routine must be used with
    \acp{PE} started by \FUNC{shmem\_init}.


add "or shmem_init_thread"
or substitute "\acp{PE} that initialized the \openshmem library"
or remove altogether? Isn't that precision redundant with the stated need for a OpenSHMEM program to call shmem_init explicitly since 1.2?

Beside this, 👍

The sentence was not introduced in this ticket. But, this is a good fix and collectives chapter committee can handle this. Can you please make a note there ?

The sentence in question will be deleted by the contexts proposal. :)

I created #119 for this edit. How about those +1's?

manjugv · 2017-08-29T14:25:12Z

👍

abouteiller · 2017-08-29T14:26:57Z

👍

jdinan · 2017-08-29T22:07:59Z

@BryantLam Waiting for you to resolve your review.

BryantLam

@jdinan, @manjugv
These comments should be handled in the section committees or as the document editor. Merge approved.

BryantLam · 2017-08-30T15:07:40Z

main_spec.tex

@@ -103,10 +101,16 @@ \subsubsection{\textbf{SHPCLMOVE}}\label{subsec:shpclmove}
 \subsubsection{\textbf{SHPDEALLOC}}\label{subsec:shpdealloc}
 \input{content/shpdealloc.tex}

+\subsection{Thread Support}


Why is this section ordered as 8.3 after Memory Management and not after Library Setup? I would expect Thread Support to be section 8.2 (section-adjacent to shmem_init).

BryantLam · 2017-08-30T15:08:25Z

main_spec.tex

@@ -103,10 +101,16 @@ \subsubsection{\textbf{SHPCLMOVE}}\label{subsec:shpclmove}
 \subsubsection{\textbf{SHPDEALLOC}}\label{subsec:shpdealloc}
 \input{content/shpdealloc.tex}

+\subsection{Thread Support}
+\input{content/threads_intro.tex} 
+\label{subsec:thread_support}


Labels used wrong here. These labels should be after the section heading (subsection, subsubsection) and not after \input. Same applies for the two new API functions below. It doesn't affect the actual bookmarks in the PDF document, but better safe than sorry if whatever-is-in-input's-tex-document ever changes.

BryantLam · 2017-08-30T15:10:50Z

content/threads_intro.tex

+the \openshmem implementation.
+
+\begin{itemize}
+\item {\bf \CONST{SHMEM\_THREAD\_SINGLE}} The \openshmem program may not be multithreaded.


Styling: Needs more separation between item and its description.

Also, \bf should be \textbf unless we want \bf to stomp all over whatever the actual styling of \CONST is. It appears to be okay in this use. This suggestion is a general issue with the document though.

Captured in #121

Signed-off-by: James Dinan <james.dinan@intel.com>

jdinan · 2017-08-30T17:51:20Z

Ok, one last set of changes. I pushed two commits to resolve the labels issue and move the threading section to be the new section 8.2 (i.e. so that the section with shmem_init_thread follows the section with shmem_init). Other edits will be handled by the management section committee.

@manjugv, @abouteiller, and @BryantLam please +1 again. Last time, I swear.

jdinan · 2017-08-30T18:04:22Z

I think I would actually like to (in section committee) merge the new routines into Section 8.1, so that shmem_init_thread appears immediately after shmem_init. If we did that, I would move the "Thread Support" section (which defines semantics) to "Execution Model" or to a separate subsection before the API section.

manjugv · 2017-08-30T18:58:08Z

I think I would actually like to (in section committee) merge the new routines into Section 8.1, so that shmem_init_thread appears immediately after shmem_init. If we did that, I would move the "Thread Support" section (which defines semantics) to "Execution Model" or to a separate subsection before the API section.

I prefer the current flow for this version of the specification. :) The text is structured as an introduction to the subsection. But if it is desirable to have a more introduction to the thread model in 8.1, we can do it as a different ticket (not chapter committee).

jdinan · 2017-08-30T20:57:59Z

@manjugv, I logged #122 for this. We can discuss with the broader committee at Thursday's meeting when we walk the list of open issues.

@abouteiller Waiting on your +1.

jdinan · 2017-08-30T21:18:41Z

Merged!!! 🎉 👏 😀

manjugv added the PendingReading label Feb 7, 2017

manjugv self-assigned this Feb 7, 2017

nspark requested changes Feb 13, 2017

View reviewed changes

spotluri reviewed Feb 14, 2017

View reviewed changes

nspark reviewed Feb 22, 2017

View reviewed changes

content/shmem_init_thread.tex Outdated

}

\apireturnvalues{

NONE

Copy link

Contributor

nspark Feb 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest: "NONE" --> "None"

spotluri reviewed Feb 22, 2017

View reviewed changes

nspark requested changes Feb 22, 2017

View reviewed changes

jdinan removed the PendingReading label Mar 8, 2017

manjugv added the Work-In-Progress label Mar 18, 2017

manjugv added 16 commits April 3, 2017 10:36

Adding Threads Intro

406c034

Adding Init Thread Interface

98afefb

Adding Query Thread Interface

5c87edb

Adding thread subsection

9a59bc0

cleaning up intro text

165fdac

Clean-up threads intro

ecf1760

Cleanups: shmem_init_thread; edits - thread intro

9e96f22

More edits to the text

f11baab

Clarifying with an example the meaning of completion semantics

4e0fba6

Editing the listing of shmem_malloc/alloc routines

daff54d

Fixing typo

abbdce2

Adding Text - Explicitly calls out threads should complete communicai…

6e72c29

…ton before finalize

Add serialized and unsupported thread modes

1d79d65

Text edit - cleanup

cc8a9a2

Removing Fortran interfaces

38aac8a

Add macros to function name; use lower case names

a194c42

jdinan requested changes Aug 26, 2017

View reviewed changes

Minor edits: Jim's comments

2d8c789

openshmem-org deleted a comment from manjugv Aug 26, 2017

James Dinan added 3 commits August 27, 2017 21:23

Whitespace fixes (only whitespace edits)

3bbe016

Signed-off-by: James Dinan <james.dinan@intel.com>

Fix error merging change from PR openshmem-org#71

73294b7

Signed-off-by: James Dinan <james.dinan@intel.com>

None -> None. for consistency

bb7ae09

Signed-off-by: James Dinan <james.dinan@intel.com>

jdinan approved these changes Aug 28, 2017

View reviewed changes

jdinan reviewed Aug 28, 2017

View reviewed changes

abouteiller suggested changes Aug 28, 2017

View reviewed changes

jdinan mentioned this pull request Aug 29, 2017

PEs started by shmem_init #119

Closed

BryantLam approved these changes Aug 30, 2017

View reviewed changes

James Dinan added 2 commits August 30, 2017 10:43

Fix labels for threads section

b310e00

Signed-off-by: James Dinan <james.dinan@intel.com>

Move threading section to follow library setup, etc.

f9814df

Signed-off-by: James Dinan <james.dinan@intel.com>

jdinan merged commit e597d32 into openshmem-org:osh_spec_next Aug 30, 2017

nspark mentioned this pull request Oct 5, 2017

Thread support interfaces not listed in "Programming Model Overview" #163

Closed

Thread Safety Proposal (RM Ticket #218) #43

Thread Safety Proposal (RM Ticket #218) #43

Conversation

manjugv commented Feb 7, 2017 • edited

BryantLam commented Feb 10, 2017

manjugv commented Feb 10, 2017

nspark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nspark Feb 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naveen-rn Feb 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manjugv Feb 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manjugv Feb 17, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nspark left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

manjugv commented Aug 26, 2017

manjugv commented Aug 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdinan commented Aug 28, 2017

manjugv commented Aug 28, 2017 • edited

jdinan commented Aug 28, 2017

manjugv commented Feb 7, 2017 •

edited

nspark Feb 17, 2017 •

edited

naveen-rn Feb 17, 2017 •

edited

manjugv Feb 14, 2017 •

edited

manjugv Feb 17, 2017 •

edited

manjugv commented Aug 28, 2017 •

edited