Skip to content

Conversation

@minsii
Copy link
Collaborator

@minsii minsii commented Oct 24, 2019

This PR rewrites Annex D - Interoperability with other Programming Models. It clarifies MPI Interoperability and defines an OpenSHMEM extended API for querying interoperability features. The interoperability with other communication libraries and languages will be addressed in future work.

Related discussion can be found at ticket-243 and pr-1

minsii and others added 27 commits October 24, 2019 10:11
Adjust the text to address two issues:
1. It is recommendation to users but not requirement because such
constraints are valid only when the implementation provides both models.

2. The additional rule for THREAD_SERIALIZED is misleading.
@minsii
Copy link
Collaborator Author

minsii commented Oct 24, 2019

Diff from September F2F meeting: 843d8bb..d560514

Summary of changes:

  • 1.2 Dynamic Process Creation: delete "MPMD" from the section title
  • 1.3 Thread Safety:
    • For the additional thread safety rules, mention them as suggestions to ensure portable hybrid program because they are required only for implementations that provide both OpenSHMEM and MPI.
    • Improve the description of additional rule for THREAD_SERIALIZED safety.
  • 1.6 Communication Progress:
    • More specific guidance to implementors: use software progress thread if the implementation supports asynchronous communication for both MPI and OpenSHMEM.
    • Add example in 2.1 SHMEM_QUERY_INTEROPERABILITY to showcase how the progress support can help program to avoid deadlock.
  • Others:
    • Example cleanup (header file, unneeded function)
    • Grammar fixes

Performance impact when implementation supports progress for MPI:

  • It should only impact on implementations that supports both MPI and OpenSHMEM
  • For implementations that already uses a software thread for OpenSHMEM progress, the performance impact should be very small.
  • For implementations that provide full hardware progress for OpenSHMEM but has to create a software thread to support MPI progress, performance may be degraded. However, the implementation can optimize it. For example, it can reduce the frequency of thread polling to minimize thread contention&oversubscription overhead. Or it can use other approaches for MPI progress such as Casper: https://github.com/pmodels/casper.
  • For implementations that has to involve extra heavy overhead to support MPI progress, it can choose to disable the feature.

Nevertheless, I feel that the optimizations of asynchronous progress is irrelevant to this proposal. Thus, I do not discuss them in detail in the proposal.

@minsii
Copy link
Collaborator Author

minsii commented Oct 24, 2019

@jdinan @nspark I just moved the PR to again the main spec. Could you please check if all your comments are addressed?

@minsii
Copy link
Collaborator Author

minsii commented Oct 24, 2019

@manjugv For the performance concern about private context, I do not think adding support for MPI progress (e.g., using a software thread) can degrade the performance of SHMEM operations that use a private context. The additional thread should only touch the "context" used for MPI and does not access the resource isolated for any private context. What do you think?

@minsii
Copy link
Collaborator Author

minsii commented Oct 29, 2019

The PDF version for official reading at Nov 13, 2019 specification meeting:
openshmem-spec-interoperability-20191029.pdf

@jdinan jdinan dismissed their stale review November 13, 2019 20:35

Feedback was addressed -- thanks!

@jdinan
Copy link
Collaborator

jdinan commented Nov 13, 2019

Would it be helpful for the example to also show how to create an MPI communicator that has the same rank and OpenSHMEM PE numbering? I think the easiest way is something like:

MPI_Comm_split(MPI_COMM_WORLD, 0, shmem_my_pe(), &shmem_comm);

@minsii
Copy link
Collaborator Author

minsii commented Nov 13, 2019

Would it be helpful for the example to also show how to create an MPI communicator that has the same rank and OpenSHMEM PE numbering? I think the easiest way is something like:

MPI_Comm_split(MPI_COMM_WORLD, 0, shmem_my_pe(), &shmem_comm);

Agreed. Then the user program can use shmem_comm instead of MPI_COMM_WORLD in the program and safely assume that the rank and PE are equal. I will add the example.

@minsii
Copy link
Collaborator Author

minsii commented Dec 6, 2019

The last push contains two changes:

  1. Add reference to SHMEM progress definition section (4.1)
  2. Add example that manages rank-PE mapping by using MPI_Comm_split.

@manjugv @jdinan Can you please review?

@minsii
Copy link
Collaborator Author

minsii commented Dec 6, 2019

Special ballot changes: ec2ca5fe...816b271a

@RaymondMichael
Copy link
Collaborator

Instead of:

that contains all
processes in \VAR{MPI_COMM_WORLD} and uses the same \ac{MPI} rank and
\openshmem \ac{PE} numbering.

I think the following might read a little easier:

that contains all
processes in \VAR{MPI_COMM_WORLD} and each process has the same \ac{MPI} rank number as its \openshmem \ac{PE} number.

@minsii
Copy link
Collaborator Author

minsii commented Dec 9, 2019

@RaymondMichael Thanks! Fixed.

@minsii
Copy link
Collaborator Author

minsii commented Jan 10, 2020

@jdinan I just noticed that I forget to remove the green color highlights in this PR. Sorry for this mistake. Can I create a different PR to add the fix?

\color{ForestGreen}
\input{content/interoperability}
\color{black}

jdinan added a commit to jdinan/specification that referenced this pull request Jan 10, 2020
Signed-off-by: James Dinan <james.dinan@gmail.com>
@jdinan
Copy link
Collaborator

jdinan commented Jan 10, 2020

Good catch -- DocEdit posted in #327

jdinan added a commit that referenced this pull request Jan 10, 2020
BryantLam added a commit to BryantLam/openshmem-specification that referenced this pull request Apr 3, 2020
BryantLam added a commit to BryantLam/openshmem-specification that referenced this pull request Apr 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants