Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Init/Finalize Clarification #37

Merged
merged 10 commits into from
Aug 1, 2017

Conversation

jdinan
Copy link
Collaborator

@jdinan jdinan commented Jan 23, 2017

Clarify that shmem_init must be matched with a call to shmem_finalize.

James Dinan added 2 commits January 23, 2017 10:49
Remove passage from API notes that contradicts the following semantic
from the API description of shmem_init: "At the end of the OpenSHMEM
program which it initialized, the call to shmem_init must be matched
with a call to shmem_finalize."

Signed-off-by: James Dinan <james.dinan@intel.com>
Signed-off-by: James Dinan <james.dinan@intel.com>
@jdinan jdinan self-assigned this Feb 6, 2017
@jdinan
Copy link
Collaborator Author

jdinan commented Feb 22, 2017

Remove the following from finalize notes:

There is an implicit shmem_finalize at the end of main, so that having an
explicit call to shmem_finalize is optional. However, an explicit shmem_finalize may be required as an
entry point for wrappers used by profiling or other tools that need to perform their own finalization.

@jdinan
Copy link
Collaborator Author

jdinan commented Feb 22, 2017

Update mention of shmem_finalize in Annex A

@@ -440,6 +440,10 @@ \section{Version 1.5}

\begin{itemize}
%
\item Clarified that \FUNC{shmem\_init} must be matched with a call to
\FUNC{shmem\_finalize}.
\\See Section \ref{subsec:shmem_init}.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Also ref shmem_finalize

James Dinan added 3 commits March 8, 2017 10:48
Signed-off-by: James Dinan <james.dinan@intel.com>
Signed-off-by: James Dinan <james.dinan@intel.com>
Signed-off-by: James Dinan <james.dinan@intel.com>
@naveen-rn
Copy link
Contributor

I'm bit confused with this change...Just to be sure, in this change we are not removing implicit finalize from the spec?

@jdinan
Copy link
Collaborator Author

jdinan commented Mar 20, 2017

This change removes implicit finalize in cases where the library was initialized with a call to shmem_init. We don't document this adequately (probably should be in deprecated interfaces), but implicit finalize can still be used with start_pes.

@naveen-rn
Copy link
Contributor

At present, we have information about "implicit finalize" at two places - at shmem_init and shmem_finalize explanations. With this change, we seem to remove this information at both these places. At least, to me it looks like we have removed implicit finalize completely from the spec.
Can you please verify?

@jdinan
Copy link
Collaborator Author

jdinan commented Mar 20, 2017

Do you have an objection to the semantic that I mentioned in my previous comment, or are you requesting that we update the deprecated interfaces section to include implicit finalize as part of this proposal? Or something else?

I don't mind updating the deprecated interfaces documentation, including the start_pes section before we ratify this proposal.

@naveen-rn
Copy link
Contributor

naveen-rn commented Mar 20, 2017

We are removing implicit finalize usage along with shmem_init in this PR. I don't want this semantics change without fixing the backward incompatibility issues.

This change removes implicit finalize in cases where the library was initialized with a call to shmem_init

At present, we require every shmem_init to be matched with shmem_finalize. But, we don't specifically say whether the finalize comes from explicit or implicit call. But this PR tends to make the explicit shmem_finalize usage a required model.

I like the intention behind this PR and this clarification is also important. So let us add new routines: shmem_finalized and shmem_initialized like MPI_initialized and MPI_finalized along with this change and completely remove implicit finalize from the specification.

@naveen-rn
Copy link
Contributor

FWIU, as per the current specification the following is the desired and allowed usage model.

  1. Use shmem_init along with explicit shmem_finalize and there is no implicit finalize at the end
  2. Deprecated, but allow using start_pes() along with implicit finalize.

As you said, the current spec is inadequate with the following unanswered question:

  1. Is using shmem_init() with implicit finalize a valid usecase? (May not be a desired model but is it valid?)
  2. Is implicit finalize supposed to work only with start_pes() and a shmem_init() should always be used with shmem_finalize()?

This PR tries to clarify the above question, by removing details about implicit finalize from all places w.r.t to shmem_init. To enhance this clarification, as you suggested we should update the deprecated interfaces documentation, by including the start_pes section and also explicitly telling implicit finalize is valid only with start_pes.

@naveen-rn
Copy link
Contributor

naveen-rn commented Mar 22, 2017

I take back my previous request, I don't have any use for implicit finalize now ;)
Assuming no one comes with the request for implicit finalize, we could proceed adding the reference-count semantics (either first-call or nested-call) to further increase the usability for long-term.
As we are dealing with almost similar changes and based on usage requirement priority,

  1. We could either add this new semantics as part of this PR or
  2. We could open two different tickets.

@jdinan
Copy link
Collaborator Author

jdinan commented Mar 23, 2017

Reference counting requires the library to support re-initialization, because of applications that may use more than one SHMEM library:

FOO_init(); /* e.g. I/O Library */
FOO_finalize();
BAR_init(); /* e.g. Compute Library */
BAR_finalize();

I suggest that we split these up -- use this ticket to clarify the current semantics as you described above and start a second ticket for reference counting. Would you be ok with that approach?

@jdinan
Copy link
Collaborator Author

jdinan commented Apr 10, 2017

@naveen-rn Could you please review the updated draft?
PDF version: main_spec.pdf

@naveen-rn
Copy link
Contributor

Looks fine to me.

resources without terminating the program. Calling any \openshmem routine after
\FUNC{shmem\_finalize} leads to undefined behavior.
program concludes its use of the \openshmem library when all \acp{PE} call
\FUNC{shmem\_finalize} or when any \ac{PE} calls \FUNC{shmem\_global\_exit}.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From @shamisp - Do we want to preserve the bit about completing all pending communication and releasing all resources in finalize?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Signed-off-by: James Dinan <james.dinan@intel.com>
@jdinan
Copy link
Collaborator Author

jdinan commented May 10, 2017

Special ballot changes: 53843d4

program finishes execution by returning from the main routine or when any PE
calls \FUNC{shmem\_global\_exit}. When returning from main, \openshmem must
program concludes its use of the \openshmem library when all \acp{PE} call
\FUNC{shmem\_finalize} or when any \ac{PE} calls \FUNC{shmem\_global\_exit}.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Split into two sentences: "In addition, the program can be aborted when any PE calls shmem_global_exit."

calls \FUNC{shmem\_global\_exit}. When returning from main, \openshmem must
program concludes its use of the \openshmem library when all \acp{PE} call
\FUNC{shmem\_finalize} or when any \ac{PE} calls \FUNC{shmem\_global\_exit}.
During finalization, \openshmem must
Copy link
Collaborator

@BryantLam BryantLam May 25, 2017

Choose a reason for hiding this comment

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

Suggested

  • "finalization" -> "shmem_finalize"
  • "openshmem" -> "the openshmem library"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed in c1dfa16, please take a look!

Signed-off-by: James Dinan <james.dinan@intel.com>
@jdinan
Copy link
Collaborator Author

jdinan commented May 31, 2017

For next time, special ballot readings: 53843d4..c1dfa16

@jdinan jdinan merged commit 5fa584e into openshmem-org:osh_spec_next Aug 1, 2017
@jdinan jdinan deleted the proposal/init-finalize branch September 6, 2017 18:40
nspark pushed a commit to nspark/specification that referenced this pull request Sep 28, 2018
davidozog pushed a commit to davidozog/openshmem-specification that referenced this pull request Feb 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants