non-blocking collectives: retain MPI_op and MPI_Datatype(s) #2154

ggouaillardet · 2016-10-04T02:49:48Z

No description provided.

ggouaillardet · 2016-10-04T07:12:00Z

@bosilca can you please review this ?

when a non blocking collective is invoked, user operators and datatypes (e.g. MPI_Op and MPI_Datatype) are OBJ_RETAIN'ed, and then OBJ_RELEASE'd when the request completes.

this solves issues when user op/datatype are free'd by the app before a non-blocking collective completes. i previously made a prototype but at the coll/libnbc level. these patches do it at the ompi level.

there cannot be any issues with intrinsic operators nor predefined datatypes, so i did not bother retain/release them. my understanding is there is a plan to stop doing that for non blocking point-to-point (e.g. MPI_Isend, MPI_Irecv), so i think that is fine.

i implemented that by "inserting" a completion callback (req_complete_cb) in a non blocking request.
ideally, i guess that should be done in the req_free callback (or maybe in both req_complete_cb and req_free ?) to properly handle cancelled requests.
a pointer is required to release op/datatype(s), and currently, only req_complete_cb_data is available, so it was easier to fix req_complete_cb

@jsquyres @hjelmn FYI

jsquyres

Please add a Signed-off-by line to this PR's commit.

added Signed-off-by

jjhursey · 2016-10-27T03:11:50Z

bot:ibm:retest

jjhursey · 2016-11-18T17:25:44Z

@ggouaillardet Is this PR is supposed to fix Issue #2151?

ggouaillardet · 2016-11-18T17:52:38Z

@jjhursey yes
@bosilca this is the PR we briefly discussed yesterday
Note i might rework it a bit after #2393 is merged

bosilca · 2016-11-22T04:18:28Z

@ggouaillardet as we briefly discussed during SC, I think the best approach will be to stop doing any refcount inside the library and instead move them all up into the MPI layer. This will allow us to release/retain all the non-predefined types and ops only once per operation.

hppritcha · 2016-12-07T23:36:00Z

moving to 2.0.3

ggouaillardet · 2016-12-08T00:08:47Z

@bosilca this PR can be seen as a first step.

it handles ddt/ops for non blocking collectives at the MPI level.
next step is to move ddt handling for pt2pt to the MPI level

meanwhile, do you have any comments on this PR ?

bosilca · 2016-12-08T22:39:26Z

This patch looks extremely big, a lot of code for a little benefit. I wonder why can't we simply add the datatype and op release code next to the calls to ompi_request_complete ?

Aren't we supposed to retain the datatype (ompi_coll_base_retain_datatypes) and op (ompi_coll_base_retain_op) before the call to the non-blocking functions themselves ?

ibm-ompi · 2017-04-11T12:19:48Z

The IBM CI (PGI Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/3a4218f168345072c128648ce107aeef

ibm-ompi · 2017-09-01T08:01:48Z

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/2d5c1a28af517e6eef3a24c202f7cce9

ibm-ompi · 2017-09-01T08:03:32Z

The IBM CI (XL Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/c7c9ec868e4ece21fece1f8e503c878c

ibm-ompi · 2017-09-01T08:04:45Z

The IBM CI (PGI Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/3b47b61d703cc97ac46275531114cc15

ibm-ompi · 2017-09-01T08:20:07Z

The IBM CI (GNU Compiler) build failed! Please review the log, linked below.

Gist: https://gist.github.com/e0d9e4795c9d80a35b550f2ed3ae84ea

ggouaillardet · 2019-04-09T07:03:29Z

:bot:aws:retest

ggouaillardet · 2019-04-09T08:30:10Z

@bosilca this is quite an old PR we discussed way before it was issued.
Currently, onesided operations retain/release the datatype in the pml component.
I initially wrote some code (PR ?) that retain/release the datatypes and operator in the coll component (e.g. coll/libnbc) but you argued it would be better do this for all components at the ompi level, so I ended up with this PR, that will work for components other than coll/libnbc but that is a bit heavy (e.g. we end up chaining two callbacks when completing a request). We might be able to avoid this chaining by moving some data into the ompi_request_t struct (and I wanted to avoid this in the first place)

jjhursey · 2019-05-07T16:06:52Z

Someone asked me about the state of this PR today. Just checking in to see where things are standing? This seems to fix Issue #2151

ggouaillardet · 2019-05-08T00:45:22Z

I am waiting for directions from @bosilca since he objected this PR.

ggouaillardet · 2019-05-08T00:46:06Z

@jjhursey note I did not test this PR vs persistent non blocking collectives.

jsquyres · 2019-07-03T14:17:31Z

@bosilca Thoughts on this PR?

bosilca · 2019-07-03T17:17:08Z

Here are few questions and suggestions:

Create a specialized class of collective requests that contains the retain_op_data in the request (save a memory allocation/deallocation)
In ompi_coll_base_retain_datatypes_w do you really need to make a copy of the datatype arrays ? I could not find anything in the MPI standard that define the life-expectancy of the array argyments for the non-blocking calls. But it would make sense if they were expected to exists for the entire duration of the collective. Anyway, the answer to this question might change the implementation of ompi_coll_base_retain_datatypes_w.
I can see why we want to have distinct functions for all possible combinations of ops and datatype. However, in these functions we only use the opal_object_t capabilities of the different objects, so we could reduce the number of callbacks to 2 (2 opal_objects and 1 array of opal_objects).

ggouaillardet · 2019-07-04T02:08:36Z

Thanks @bosilca, I will update the PR based on your feedback.

The MPI standard is explicit about the life expectancy of the array arguments for the non blocking calls, and this makes things even easier as you suggested.
From MPI 3.1 chapter 5.12 page 197

Once initiated, all associated send buffers and buffers associated with input arguments (such
as arrays of counts, displacements, or datatypes in the vector versions of the collectives)
should not be modified, and all associated receive buffers should not be accessed, until the
collective operation completes.

bosilca · 2019-07-04T04:13:32Z

Thanks @ggouaillardet, I've been looking for that sentence for hours.

ggouaillardet · 2019-07-04T06:25:20Z

I pushed a new commit to address the first point

ggouaillardet · 2019-07-08T08:28:17Z

@bosilca I addressed all your points. Can you please give this PR a final review ?
@jjhursey FYI

bosilca

Overall looking good. I had few minor suggestions for improvement, mostly to make the code more readable. If you make them, feel free to merge directly.

ompi/mca/coll/base/coll_base_util.c

bosilca · 2019-07-08T14:09:48Z

ompi/mca/coll/base/coll_base_util.c

+
+static void release_vecs_callback(ompi_coll_base_nbc_request_t *request) {
+    ompi_communicator_t *comm = request->super.req_mpi_object.comm;
+    int count = OMPI_COMM_IS_INTER(comm)?ompi_comm_remote_size(comm):ompi_comm_size(comm);


How does this work for persistent neighbor collectives ?

I guess you meant non blocking neighbor alltoallw (regardless this is a persistent operation or not), right ? in this case, this is a good catch I fixed in the latest commit.

ompi/mca/coll/libnbc/nbc.c

ompi/mca/coll/retain.diff

gpaulsen · 2019-07-10T15:19:57Z

Two questions:

Is this at all dependent on A big refresh of the datatype engine #6695 ?
How difficult would it be to port this to v4.0.x?

ggouaillardet · 2019-07-11T01:18:48Z

@gpaulsen

this PR and A big refresh of the datatype engine #6695 are independent
I do not expect any difficulty backporting this to the v4.0.x branch, but I'll see when I get there.

ggouaillardet · 2019-07-11T04:25:45Z

@bosilca I made the requested changes. Unless some changes are requested, I will squash the commits and merge this PR tomorrow.

ggouaillardet · 2019-07-11T04:34:29Z

@gpaulsen fwiw, this PR applies nicely on the v4.0.x branch once 65660e5 has been backported.

MPI standard states a user MPI_Op and/or user MPI_Datatype can be free'd after a call to a non blocking collective and before the non-blocking collective completes. Retain user (only) MPI_Op and MPI_Datatype when the non blocking call is invoked, and set a request callback so they are free'd when the MPI_Request completes. Thanks Thomas Ponweiser for reporting this Fixes open-mpi#2151 Fixes open-mpi#1304 Signed-off-by: Gilles Gouaillardet <gilles@rist.or.jp>

gpaulsen · 2020-11-04T23:01:58Z

@ggouaillardet @hppritcha, This functionality (along with 65660e5) was never backported to either the v4.0.x, or v4.1.x release streams.

My opinion is that this is quite a bit of change for this late in the release streams, and that we should probably wait for v5.0.0 for this functionality. What are your thoughts?

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from 5b1cc1d to a525940 Compare October 4, 2016 04:16

ggouaillardet mentioned this pull request Oct 5, 2016

v2.x: libnbc: retain datatype and schedule them for release open-mpi/ompi-release#1274

Closed

jsquyres previously requested changes Oct 25, 2016

View reviewed changes

ggouaillardet mentioned this pull request Oct 27, 2016

libnbc: retain datatype and schedule them for release #1305

Closed

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from a525940 to c9b7ac4 Compare October 27, 2016 02:33

jjhursey added the bug label Nov 18, 2016

jjhursey added this to the v2.0.2 milestone Nov 18, 2016

jjhursey assigned bosilca Nov 18, 2016

jsquyres added the pushed-back label Dec 5, 2016

hppritcha modified the milestones: v2.0.3, v2.0.2 Dec 7, 2016

jjhursey mentioned this pull request Jan 10, 2017

'nonblocking3' BVT test fails #2151

Closed

hppritcha modified the milestone: v2.0.3 Jun 1, 2017

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from c9b7ac4 to e776fab Compare September 1, 2017 07:51

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from e776fab to 4fe431b Compare September 1, 2017 08:08

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from 4fe431b to f5fcd84 Compare September 1, 2017 08:20

ggouaillardet mentioned this pull request Apr 9, 2019

fortran/use-mpi-f08: add support for Fortran 2018 ISO_Fortran_binding.h #6569

Open

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from f5fcd84 to 8f6a839 Compare April 9, 2019 06:17

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from 8f6a839 to 646391b Compare April 9, 2019 07:12

bwbarrett added the Target: main label Jun 4, 2019

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from 646391b to e4dcf83 Compare July 4, 2019 06:24

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from 541a868 to a578e45 Compare July 8, 2019 04:38

bosilca approved these changes Jul 8, 2019

View reviewed changes

bosilca approved these changes Jul 11, 2019

View reviewed changes

ggouaillardet force-pushed the topic/retain_op_and_datatypes branch from f11203a to 0fe756d Compare July 12, 2019 00:16

ggouaillardet removed the pushed-back label Jul 12, 2019

ggouaillardet merged commit 020a591 into open-mpi:master Jul 13, 2019

ggouaillardet mentioned this pull request Jul 13, 2019

Fortran: MPI_Alltoallw/MPI_Ialltoallw using MPI_IN_PLACE do not ignore sendtypes #5459

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

non-blocking collectives: retain MPI_op and MPI_Datatype(s) #2154

non-blocking collectives: retain MPI_op and MPI_Datatype(s) #2154

ggouaillardet commented Oct 4, 2016

ggouaillardet commented Oct 4, 2016

jsquyres left a comment

jjhursey commented Oct 27, 2016

jjhursey commented Nov 18, 2016

ggouaillardet commented Nov 18, 2016 •

edited

Loading

bosilca commented Nov 22, 2016

hppritcha commented Dec 7, 2016

ggouaillardet commented Dec 8, 2016

bosilca commented Dec 8, 2016

ibm-ompi commented Apr 11, 2017

ibm-ompi commented Sep 1, 2017

ibm-ompi commented Sep 1, 2017

ibm-ompi commented Sep 1, 2017

ibm-ompi commented Sep 1, 2017

ggouaillardet commented Apr 9, 2019

ggouaillardet commented Apr 9, 2019

jjhursey commented May 7, 2019

ggouaillardet commented May 8, 2019

ggouaillardet commented May 8, 2019

jsquyres commented Jul 3, 2019

bosilca commented Jul 3, 2019

ggouaillardet commented Jul 4, 2019

bosilca commented Jul 4, 2019

ggouaillardet commented Jul 4, 2019

ggouaillardet commented Jul 8, 2019

bosilca left a comment

bosilca Jul 8, 2019

ggouaillardet Jul 11, 2019

gpaulsen commented Jul 10, 2019

ggouaillardet commented Jul 11, 2019

ggouaillardet commented Jul 11, 2019

ggouaillardet commented Jul 11, 2019

gpaulsen commented Nov 4, 2020

non-blocking collectives: retain MPI_op and MPI_Datatype(s) #2154

non-blocking collectives: retain MPI_op and MPI_Datatype(s) #2154

Conversation

ggouaillardet commented Oct 4, 2016

ggouaillardet commented Oct 4, 2016

jsquyres left a comment

Choose a reason for hiding this comment

jjhursey commented Oct 27, 2016

jjhursey commented Nov 18, 2016

ggouaillardet commented Nov 18, 2016 • edited Loading

bosilca commented Nov 22, 2016

hppritcha commented Dec 7, 2016

ggouaillardet commented Dec 8, 2016

bosilca commented Dec 8, 2016

ibm-ompi commented Apr 11, 2017

ibm-ompi commented Sep 1, 2017

ibm-ompi commented Sep 1, 2017

ibm-ompi commented Sep 1, 2017

ibm-ompi commented Sep 1, 2017

ggouaillardet commented Apr 9, 2019

ggouaillardet commented Apr 9, 2019

jjhursey commented May 7, 2019

ggouaillardet commented May 8, 2019

ggouaillardet commented May 8, 2019

jsquyres commented Jul 3, 2019

bosilca commented Jul 3, 2019

ggouaillardet commented Jul 4, 2019

bosilca commented Jul 4, 2019

ggouaillardet commented Jul 4, 2019

ggouaillardet commented Jul 8, 2019

bosilca left a comment

Choose a reason for hiding this comment

bosilca Jul 8, 2019

Choose a reason for hiding this comment

ggouaillardet Jul 11, 2019

Choose a reason for hiding this comment

gpaulsen commented Jul 10, 2019

ggouaillardet commented Jul 11, 2019

ggouaillardet commented Jul 11, 2019

ggouaillardet commented Jul 11, 2019

gpaulsen commented Nov 4, 2020

ggouaillardet commented Nov 18, 2016 •

edited

Loading