Skip to content
This repository has been archived by the owner on Sep 30, 2022. It is now read-only.

coll/libnbc: do not handle MPI_IN_PLACE in neighborhood collectives #1004

Conversation

ggouaillardet
Copy link
Contributor

This is a one off and a quick fix, and it unlikely works with complex types
(e.g. negative lower bound).
libnbc considers MPI_Ineighbor_alltoallw is in place when send and recv buffers are identical,
and even if they do not overlap because of distinct displacements.

Thanks Jun Kudo for the bug report.

@ggouaillardet ggouaillardet force-pushed the topic/v1.10/nbc_ineighbor_alltoallw_in_place branch from b9e738c to f277bea Compare March 8, 2016 01:55
@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1403/ for details.

@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1404/ for details.

@hppritcha hppritcha added this to the v1.10.3 milestone Mar 8, 2016
@rhc54
Copy link

rhc54 commented Mar 8, 2016

@ggouaillardet can you please request a reviewer?

@rhc54 rhc54 added the bug label Mar 8, 2016
@ggouaillardet
Copy link
Contributor Author

:bot:assign: @bosilca

see my comments at http://www.open-mpi.org/community/lists/users/2016/03/28656.php

@bosilca
Copy link
Member

bosilca commented Mar 9, 2016

@ggouaillardet I don't think this patch is correct. The MPI_INEIGHBOR_ALLTOALLW function (defined in MPI 3.1 page 328) identifies rdispl as the

integer array (of length indegree). Entry i specifies the displacement in bytes (relative to recvbuf) at which to place the incoming data from neighbor i (array of integers)

Thus, your patch that does compute the receive buffer for each MPI_Recv using the recvtypes and recvcounts for the particular peer does not respect the MPI standard. I think the correct receive buffer for each planned receive should be NBC_Sched_recv((char*)recvbuf+rdisps[i]...

@ggouaillardet
Copy link
Contributor Author

@bosilca
thanks for the review
i fixed the inplace case only, and kept the change as minimal as possible
since we are inplace, data is sent from sendbuf and received into a temporary buffer, and at the end it is copied into recvbuf
if i use rdisps to compute offset into the temporary buffer, that means i might have to allocate a bigger buffer and that can be simply avoided.

as i wrote in the ML, i had several interrogations

  • is it valid to call MPI_Ineighbor_alltoallw with sendbuf`` ==recvbuf``` ?
  • NBC_IN_PLACE is very primitive and not robust
    if sendbuf == recvbuf but sdpisps`` andrdispsare built so data to be sent and data to be received do not overlap (see user provided test case) theninplacecould/should be set to false, assuming this is valid per the mpi standard ifsendbuf != recvbufbutsdispsandrdispsare built so data to be sent and data to be received do overlap, theninplace``` should be true, assuming this is a valid use case per the mpi standard

bottom line, if the user test case is valid from the MPI standard, then i think this commit is helping.
master does handle things differently, so i initially did not plan to invest much efforts on this.

@bosilca
Copy link
Member

bosilca commented Mar 9, 2016

@ggouaillardet I am sorry I missed both the fact that you were in the INPLACE branch and your email on the mailing list. Let me try to quickly remedy to this.

  1. If the data overlap between any tuple of (sendbuf + sdispl[i], stype[i], scount[i]) and (recvbuf + rdispl[i], rtype[i], rcount[i]) the call would be incorrect.
  2. If the user does craft the displacements in a way to avoid any overlap, then technically we are not in an inplace code.

Thus, the inplace code should be entirely stripped out. I guess @hjelmn reached the same conclusion when he removed the inplace support from master for all the neighborhood collectives (d42e096) (and here I am not talking about the MPI_IN_PLACE which has no meaning for the neighborhood collective).

@ggouaillardet
Copy link
Contributor Author

@bosilca i quickly checked the standard and i reached the same conclusion

  • MPI_IN_PLACE has no meaning for neighborhood collectives
  • it is the user responsability to ensure there is no overlapping between send and receive buffers
    (ompi does not check that, we could do that and return MPI_ERR_BUFFER)
    so i will remove the inplace from the neighborhood collectives from now

@ggouaillardet ggouaillardet force-pushed the topic/v1.10/nbc_ineighbor_alltoallw_in_place branch from f277bea to f3ddb03 Compare March 9, 2016 06:17
@ggouaillardet ggouaillardet changed the title coll/libnbc: fix MPI_Ineighbor_alltoallw when in place. coll/libnbc: do not handle MPI_IN_PLACE in neighborhood collectives Mar 9, 2016
MPI_IN_PLACE is not a valid send buffer for neighborhood collectives, so just ignore it here.

This commit is a small subset of open-mpi/ompi@d42e096

Thanks Jun Kudo for the report.
@ggouaillardet ggouaillardet force-pushed the topic/v1.10/nbc_ineighbor_alltoallw_in_place branch from f3ddb03 to bbfa865 Compare March 9, 2016 06:19
@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1416/ for details.

@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1417/ for details.

@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1418/ for details.

@ggouaillardet ggouaillardet force-pushed the topic/v1.10/nbc_ineighbor_alltoallw_in_place branch from c952464 to ea2fcb9 Compare March 10, 2016 00:11
@mellanox-github
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ompi-release-pr/1422/ for details.

@jsquyres
Copy link
Member

@bosilca Can you have a look at this?

@bosilca
Copy link
Member

bosilca commented Mar 29, 2016

The patch removes all support for MPI_INPLACE from neighborhood collective, which put us in compliance with the MPI standard. 👍

@jsquyres
Copy link
Member

@hppritcha Good to go.

@rhc54
Copy link

rhc54 commented Apr 3, 2016

well, I approve it too 😄

@rhc54 rhc54 merged commit f3bad94 into open-mpi:v1.10 Apr 3, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants