persistent collectives #4618

ggouaillardet · 2017-12-13T05:07:28Z

No description provided.

ggouaillardet · 2017-12-13T08:27:04Z

@kawashima-fj @dholmes-epcc-ed-ac-uk @bosilca
this PR integrates the persistent collective framework of #2758 , and the persistent collectives are implemented by coll/libnbc i minimally extended for that purpose based on ideas from #4515

i still need to finalize a few things

MPI_Startall()
some optimizations are no more valid with persistent collectives (for example, Allgather on one proc is a simple copy and return ompi_request_empty, this is fine for non blocking, but bogus on persistent)

kawashima-fj · 2018-02-22T02:50:31Z

@ggouaillardet @dholmes-epcc-ed-ac-uk @bosilca I've added commits to implement MPI_STARTALL and fix the optimization issue in this PR branch.

The version of COLL interface is not updated yet. I have questions.

We need to update all mca_coll_base_module_2_2_0_t to mca_coll_base_module_2_3_0_t since coll_*_init members are added to it. Correct? These fields are NULLed in coll_base_module_construct so that other COLL components work correctly without persistent collective functions.
We don't need to update MCA_COLL_BASE_VERSION_2_0_0 to MCA_COLL_BASE_VERSION_2_3_0 (or anything) since it represents a component version, not module version. Correct?

After the version update, there are two remaining works.

MPI_ interfaces. Currently only C bindings with MPIX_ prefix are available in this PR. I believe the persistent collective proposal in Adding Persistent Collective Communication mpi-forum/mpi-issues#25 will pass soon. After that, I'll move to MPI_ prefix and add Fortran bindings.
Test. I've tested basic functionalities but they are not enough. I'll continue the test.

@dholmes-epcc-ed-ac-uk This PR is based on your work and your idea. Thank you very much. I think this PR is almost ready to be merged into Open MPI. So your #4515 can be replaced with this PR. Do you have any comments?

dholmes-epcc-ed-ac-uk · 2018-02-22T14:29:06Z

ompi/mca/coll/libnbc/nbc.c

  if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
    return res;
  }
+  handle->super.req_state = OMPI_REQUEST_ACTIVE;


Possible inconsistency? The state should be set to ACTIVE before starting the first round, so that no work can be done for this request while it is INACTIVE (or PENDING).

The state transition sequence here should be:
INACTIVE -> ACTIVE -> COMPLETE -> INACTIVE

MPI_Start goes from INACTIVE to ACTIVE then

finishing the last round of the schedule goes from ACTIVE to COMPLETE then

MPI_Wait goes from COMPLETE to INACTIVE

As there are two separate boolean variables we should have:
INACTIVE && COMPLETE -> ACTIVE && !COMPLETE -> ACTIVE && COMPLETE -> INACTIVE && COMPLETE

Incidentally, why are there 3 separate boolean state variables (8 possible states) to represent a total of 3 valid states?

handle->super.req_complete = { REQUEST_PENDING || REQUEST_COMPLETED }; handle->super.req_state = { OMPI_REQUEST_ACTIVE || OMPI_REQUEST_INACTIVE }; handle->nbc_complete = { false || true }; // possible synonym for handle->super.req_complete

@dholmes-epcc-ed-ac-uk Thanks. Regarding ACTIVE, I'll update the PR next week. Regarding nbc_complete, the commit message 4866c82 explains the necessity. I need the status information before req_state or req_complete is updated because NBC_Progress is called recursively. I know it is not a smart solution.

dholmes-epcc-ed-ac-uk · 2018-04-11T10:22:52Z

ompi/mca/coll/libnbc/nbc_ialltoall.c

+int ompi_coll_libnbc_alltoall_init (const void* sendbuf, int sendcount, MPI_Datatype sendtype, void* recvbuf, int recvcount,
+                                    MPI_Datatype recvtype, struct ompi_communicator_t *comm, MPI_Info info, ompi_request_t ** request,
+                                    struct mca_coll_base_module_2_2_0_t *module) {
+    int res = nbc_ialltoall(sendbuf, sendcount, sendtype, recvbuf, recvcount, recvtype,


This delegation is, conceptually, the wrong way around. The initialisation functions for persistent operations do less work than the nonblocking functions. Thus, nbc_ialltoall could delegate to alltoall_init (and then immediately call nbc_start) but this way around can/will cause problems.

In this particular example (I have not checked elsewhere), the nbc_ialltoall function actually performs the memcpy or MPI_Pack that moves the user data, i.e. it (correctly for nonblocking) does both initialisation and start.

@dholmes-epcc-ed-ac-uk Thanks. I replaced NBC_Copy with NBC_Sched_copy in 373af2f but memcpy and PMPI_Pack were left in nbc_ialltoall.c unintentionally. I'll update the PR.

If the intention for this function is to be the common initialisation routine for persistent and nonblocking all-to-all, then it would be better to name it nbc_alltoall_init (rather than nbc_ialltoall). Future programmers will be less easily confused by that name, and less likely to (re-)introduce data-movement code into the initialisation-only step. Also, the nonblocking code path will then read correctly (initialise via nbc_alltoall_init then start via NBC_Start).

Have a point. I'll fix it.

kawashima-fj · 2018-05-22T06:26:48Z

All my tests have completed and bugs are fixed now.

My planned works before OMPI v4.0:

Write man pages
- Put the source files as ompi/mpiext/pcollreq/c/MPIX_*.3in
- Write that current implementation is based on the draft standard and it may change
Update README
- Write that persistent collective operations are available with MPIX_ prefix but the interface/behavior may be changed in the future

Works after standardization in the MPI Forum:

Change the MPIX_ prefix to MPI_ prefix
Move codes to ompi/mpi from ompi/mpiext
Add Fortran bindings (don't need to wait standardization)

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

…request() - set schedule in NBC_Schedule_request instead of NBC_Start() - update NBC_Start() prototype Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

prepare the upcoming persistent collectives by pre-factoring some code Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp> fixup 808c3c62cd9475edd91ecde9d2d53b12e28b2c04

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

Now libnbc COLL supports persistent collectives and all `*_init` functions of the COLL interface are available. So let's enable the check of availability of those functions on a communicator creation. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

kawashima-fj · 2018-06-11T05:42:26Z

I've completed all works. I'll merge this next week. If you are interested in this feature, please review.

Some notes:

man pages are available for all MPIX_*_init functions. MPIX_Barrier_init.3 has contents for all functions and all other man pages redirect you to MPI_Barrier_init.3.
README contains a short explanation of this MPI extension.
Fortran bindings are not available now.
The COLL module interface version is updated to 2.3.0.

Until the MPI Forum decides to add the persistent collective communication request feature to the MPI Standard, these functions are supported through MPI extensions with the `MPIX_` prefix. Only C bindings are supported currently. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

and errors otherwise. Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

Because a persistent reuqest does not free its `schedule` object when the communication completes, the `NBC_Progress` function cannot determine the completion using `schedule`. Without this change, a hang occurs when the `NBC_Progress` function is called recursively through the `NBC_Start_round` function. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

`NBC_Copy` shoud not be called in `MPI_*_INIT`. `NBC_Sched_copy` should be called instead. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

Persistent operation for `NBC_A2A_DISS` is not supported currently. Though the algorithm is not selected at all currently, I put an assertion not to select it by mistake. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

The `nbc_i*` functions don't start communication, but create a request. `nbc_*_init` are appropriate names for them. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

Members for persistent operations are added to the module structure in a prior commit. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

ggouaillardet added enhancement Target: main labels Dec 13, 2017

ggouaillardet force-pushed the topic/pcoll branch 4 times, most recently from f793596 to a125778 Compare December 13, 2017 07:35

kawashima-fj mentioned this pull request Dec 13, 2017

Persistent collective communication request #2758

Closed

ggouaillardet added the ⚠️ WIP-DNM! label Dec 13, 2017

ggouaillardet force-pushed the topic/pcoll branch from a125778 to ce88cad Compare December 15, 2017 04:52

dholmes-epcc-ed-ac-uk approved these changes Feb 22, 2018

View reviewed changes

dholmes-epcc-ed-ac-uk reviewed Apr 11, 2018

View reviewed changes

kawashima-fj force-pushed the topic/pcoll branch 2 times, most recently from 14e3c1f to 5a80703 Compare May 22, 2018 05:49

open-mpi deleted a comment from ibm-ompi May 22, 2018

ggouaillardet and others added 6 commits June 11, 2018 09:53

coll/libnbc: revamp ibcast and use NBC_Schedule_request()

360a76f

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

coll/libnbc: misc revamp - merge NBC_Init_handle() into NBC_Schedule_…

fe0bb6c

…request() - set schedule in NBC_Schedule_request instead of NBC_Start() - update NBC_Start() prototype Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

coll/libnbc: code refactoring

c753e9b

prepare the upcoming persistent collectives by pre-factoring some code Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp> fixup 808c3c62cd9475edd91ecde9d2d53b12e28b2c04

coll: Add persistent collective communication request feature

a9fdea5

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

coll/libnbc: add persistent collectives implementation

a9609b6

Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

kawashima-fj force-pushed the topic/pcoll branch from 5a80703 to ec2d96a Compare June 11, 2018 05:38

kawashima-fj removed the ⚠️ WIP-DNM! label Jun 11, 2018

kawashima-fj added this to the v4.0.0 milestone Jun 11, 2018

kawashima-fj and others added 3 commits June 11, 2018 17:22

mpi: check MPI_Start[all] is invoked on persistent requests

9a63dac

and errors otherwise. Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

ompi/request: Add ompi_request_persistent_noop_create

e72f510

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

kawashima-fj added 8 commits June 11, 2018 17:22

coll/libnbc: Correct persistent request handling

8e5690b

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

coll/libnbc: Adapt local copy for persistent request

ed0144b

`NBC_Copy` shoud not be called in `MPI_*_INIT`. `NBC_Sched_copy` should be called instead. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

coll/libnbc: Implement MPI_STARTALL

0b8b0f8

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

coll/libnbc: Add assertion for NBC_A2A_DISS

5c21903

Persistent operation for `NBC_A2A_DISS` is not supported currently. Though the algorithm is not selected at all currently, I put an assertion not to select it by mistake. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

coll/libnbc: Rename internal functions

e12a505

The `nbc_i*` functions don't start communication, but create a request. `nbc_*_init` are appropriate names for them. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

coll: Update COLL module interface version to 2.3.0

a38e9e0

Members for persistent operations are added to the module structure in a prior commit. Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

README: Add explanation of persistent collectives

3dce039

Signed-off-by: KAWASHIMA Takahiro <t-kawashima@jp.fujitsu.com>

kawashima-fj force-pushed the topic/pcoll branch from ec2d96a to 3dce039 Compare June 11, 2018 08:23

kawashima-fj merged commit a8da78e into open-mpi:master Jun 26, 2018

kawashima-fj mentioned this pull request Jun 27, 2018

Libpnbc - persistent collectives for Open MPI #4515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

persistent collectives #4618

persistent collectives #4618

Uh oh!

ggouaillardet commented Dec 13, 2017

Uh oh!

ggouaillardet commented Dec 13, 2017

Uh oh!

kawashima-fj commented Feb 22, 2018

Uh oh!

dholmes-epcc-ed-ac-uk Feb 22, 2018

Uh oh!

kawashima-fj Feb 23, 2018

Uh oh!

dholmes-epcc-ed-ac-uk Apr 11, 2018

Uh oh!

kawashima-fj Apr 12, 2018

Uh oh!

dholmes-epcc-ed-ac-uk Apr 12, 2018

Uh oh!

kawashima-fj Apr 12, 2018

Uh oh!

kawashima-fj commented May 22, 2018

Uh oh!

kawashima-fj commented Jun 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

persistent collectives #4618

persistent collectives #4618

Uh oh!

Conversation

ggouaillardet commented Dec 13, 2017

Uh oh!

ggouaillardet commented Dec 13, 2017

Uh oh!

kawashima-fj commented Feb 22, 2018

Uh oh!

dholmes-epcc-ed-ac-uk Feb 22, 2018

Choose a reason for hiding this comment

Uh oh!

kawashima-fj Feb 23, 2018

Choose a reason for hiding this comment

Uh oh!

dholmes-epcc-ed-ac-uk Apr 11, 2018

Choose a reason for hiding this comment

Uh oh!

kawashima-fj Apr 12, 2018

Choose a reason for hiding this comment

Uh oh!

dholmes-epcc-ed-ac-uk Apr 12, 2018

Choose a reason for hiding this comment

Uh oh!

kawashima-fj Apr 12, 2018

Choose a reason for hiding this comment

Uh oh!

kawashima-fj commented May 22, 2018

Uh oh!

kawashima-fj commented Jun 11, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants