mpi_ialltoall apparently erroneous complaint about invalid argument

[emmanuelthome]

This complains with openmpi-1.10.rc2 (well, not specific to that version. Same goes for 1.8.8).
The blocking function accepts the arguments, though.
Same goes for ialltoallv and ialltoallw

Here is the transcript, and next the source code.

Best regards,

E.

catrel-30 ~ $ ~/Packages/openmpi-1.10.1rc2/bin/mpicc -std=c99 test2.c && ~/Packages/openmpi-1.10.1rc2/bin/mpirun -n 2 ./a.out
[catrel-30:51966] *** An error occurred in MPI_Ialltoall
[catrel-30:51966] *** reported by process [4104650753,0]
[catrel-30:51966] *** on communicator MPI_COMM_WORLD
[catrel-30:51966] *** MPI_ERR_ARG: invalid argument of some other kind
[catrel-30:51966] *** MPI_ERRORS_ARE_FATAL (processes in this communicator will now abort,
[catrel-30:51966] ***    and potentially your MPI job)
[catrel-30.loria.fr:51964] 1 more process has sent help message help-mpi-errors.txt / mpi_errors_are_fatal
[catrel-30.loria.fr:51964] Set MCA parameter "orte_base_help_aggregate" to 0 to see all help / error messages

#include <mpi.h>
#include <stdio.h>
#include <stdlib.h>

int main(int argc, char * argv[])
{
    int size;
    int rank;

    MPI_Init(&argc, &argv);
    MPI_Comm_size(MPI_COMM_WORLD, &size);
    MPI_Comm_rank(MPI_COMM_WORLD, &rank);

    int * recvbuf = malloc(size * sizeof(int));
    for(int r = 0 ; r < size ; r++) {
        recvbuf[r] = rank*size+r;
    }   
    MPI_Request req;
    MPI_Ialltoall(MPI_IN_PLACE, 1, MPI_INT, recvbuf, 1, MPI_INT, MPI_COMM_WORLD, &req);
    MPI_Wait(&req, MPI_STATUS_IGNORE);

    for(int r = 0 ; r < size ; r++) {
        if (recvbuf[r] != r*size+rank)
            abort();
    }       
    free(recvbuf);

    MPI_Finalize();
}   

[jsquyres]

@bosilca It looks like Ialltoall(v|w) do not allow sendbuf to be IN_PLACE.

Is the Right solution simply to remove the (sendbuf == MPI_IN_PLACE) checks in the MPI_PARAM_CHECK sections in mpi/c/alltoall.c (and friends)?

[emmanuelthome]

I tried the following patch

--- /tmp/openmpi-1.10.1rc2/./ompi/mpi/c/ialltoall.c     2015-07-16 17:10:59.000000000 +0200
+++ ./ompi/mpi/c/ialltoall.c    2015-10-15 11:56:18.101378202 +0200
@@ -71,7 +71,7 @@
         if (ompi_comm_invalid(comm)) {
             return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_COMM,
                                           FUNC_NAME);
-        } else if (MPI_IN_PLACE == sendbuf || MPI_IN_PLACE == recvbuf) {
+        } else if (MPI_IN_PLACE == recvbuf) {
             return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_ARG,
                                           FUNC_NAME);
         } else {
--- /tmp/openmpi-1.10.1rc2/./ompi/mpi/c/ialltoallv.c    2015-07-16 17:10:59.000000000 +0200
+++ ./ompi/mpi/c/ialltoallv.c   2015-10-15 11:56:43.949950943 +0200
@@ -91,9 +91,8 @@
                                           FUNC_NAME);
         }

-        if ((NULL == sendcounts) || (NULL == sdispls) ||
-            (NULL == recvcounts) || (NULL == rdispls) ||
-            MPI_IN_PLACE == sendbuf || MPI_IN_PLACE == recvbuf) {
+        if ((NULL == recvcounts) || (NULL == rdispls) ||
+            MPI_IN_PLACE == recvbuf) {
             return OMPI_ERRHANDLER_INVOKE(comm, MPI_ERR_ARG, FUNC_NAME);
         }

--- /tmp/openmpi-1.10.1rc2/./ompi/mpi/c/ialltoallw.c    2015-07-16 17:10:59.000000000 +0200
+++ ./ompi/mpi/c/ialltoallw.c   2015-10-15 11:56:57.834258577 +0200
@@ -87,9 +87,8 @@
                                           FUNC_NAME);
         }

-        if ((NULL == sendcounts) || (NULL == sdispls) || (NULL == sendtypes) ||
-            (NULL == recvcounts) || (NULL == rdispls) || (NULL == recvtypes) ||
-            MPI_IN_PLACE == sendbuf || MPI_IN_PLACE == recvbuf) {
+        if ((NULL == recvcounts) || (NULL == rdispls) || (NULL == recvtypes) ||
+            MPI_IN_PLACE == recvbuf) {
             return OMPI_ERRHANDLER_INVOKE(comm, MPI_ERR_ARG, FUNC_NAME);
         }

and ran my test case in a loop like this.

~/Packages/openmpi-1.10.1rc2tweaked/bin/mpicc test2.c && while ~/Packages/openmpi-1.10.1rc2tweaked/bin/mpirun -n 4 ./a.out ; do echo -n . ; done

I end up with the check failing about once every ten trials. Race condition somewhere ?

[jsquyres]

Wow, that's totally weird -- there should be no race condition here: it's a trivial check at the beginning of the function that's not based on any other MPI process.

Are you getting the same ERR_ARG failure (in the one-in-ten failures)?

FWIW, I'm unable to replicate this behavior. With your patch, I get no failures:

$ while mpirun -n 4 ./a.out ; do echo -n . ; done
............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................etc.

[bosilca]

The patch proposed above only impact the MPI interface, basically allowing
the non-blocking alltoall* call to propagate to the selected non-blocking
module. However, there is no guarantee that the underlying collective
module is able to correctly handle the IN_PLACE argument.

I have a slightly different patch. Once applied if I use the libnbc, I can
only confirm Jeff's findings: your test application runs.

George.

PS: This patch is only a proof of concept. We should check that similar
changes are not required in other non-blocking collectives, as well as
removing the redundant send datatype check when MPI_IN_PLACE is used from
all collectives.

On Thu, Oct 15, 2015 at 11:38 AM, Jeff Squyres notifications@github.com
wrote:

Wow, that's totally weird -- there should be no race condition here: it's
a trivial check at the beginning of the function that's not based on any
other MPI process.

Are you getting the same ERR_ARG failure (in the one-in-ten failures)?

FWIW, I'm unable to replicate this behavior. With your patch, I get no
failures:

$ while mpirun -n 4 ./a.out ; do echo -n . ; done
............................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................................etc.

—
Reply to this email directly or view it on GitHub
#1023 (comment).

[emmanuelthome]

To Jeff: the failure I get is that my data check fails. More precisely, if I let my abort() be a bit more verbose like this:

        if (recvbuf[r] != r * size + rank) {
            printf("\n");
            fflush(stdout);
            fprintf(stderr, "rank %d recvbuf[%d]=%d, expected %d\n",
                    rank, r, recvbuf[r], r * size + rank);
            abort();
        }

Then every so often I see failures like this:

cassoulet ...og/Valgrind/mpi $ ~/Local/Packages/openmpi-1.10.1rc2tweaked/bin/mpicc test2.c && while ~/Local/Packages/openmpi-1.10.1rc2tweaked/bin/mpirun -n 10 ./a.out ; do echo -n . ; done
................................

rank 2 recvbuf[8]=28, expected 82
[cassoulet:08419] *** Process received signal ***
[cassoulet:08419] Signal: Aborted (6)
[cassoulet:08419] Signal code:  (-6)
rank 5 recvbuf[8]=58, expected 85
[cassoulet:08424] *** Process received signal ***
[cassoulet:08424] Signal: Aborted (6)
[cassoulet:08424] Signal code:  (-6)

(where I've stripped the rest, backtraces, and also reports at other ranks).

To George: yes of course, I understand that the argument checks may be here for a reason. What puzzles me a bit is the fact the the blocking collective works fine in this case. But I haven't looked the innards at all.

[ggouaillardet]

@emmanuelthome how many nodes are involved in your test ?
are you using libnbc module for ialltoall (that would be the default on most systems) or an other module
(ml, portals4, hcoll for example) ?

[ggouaillardet]

@emmanuelthome I just found libnbc does not correctly implement ialltoall with MPI_IN_PLACE

Libnbc basically calls both Isend and Irecv with the same buffer, so there is a race condition.
In your example, task 8 receives from 2 before it sends to 2, that is why you do not get the correct results.

As suggested by @bosilca, if no module correctly implements ialltoall with MPI_IN_PLACE, then it is fine not to update the MPI part. But if some module(s) do that correctly, then each module should check MPI_IN_PLACE and fail on a case by case basis.

In the mean time, the simplest way to fix this issue is to use a temp buffer, copy the user buffer, and send the tmp buffer (and have it freed it on completion).
I will give it a try today

[ggouaillardet]

@jsquyres i had no issue reproducing the issue on one node and with --mca btl tcp,self

@emmanuelthome this inline patch fixes the issue, but i only tested libnbc module

diff --git a/ompi/mca/coll/libnbc/nbc_ialltoall.c b/ompi/mca/coll/libnbc/nbc_ialltoall.c
index e9a73f5..de7fa6b 100644
--- a/ompi/mca/coll/libnbc/nbc_ialltoall.c
+++ b/ompi/mca/coll/libnbc/nbc_ialltoall.c
@@ -85,10 +85,22 @@ int ompi_coll_libnbc_ialltoall(const void* sendbuf, int sendcount, MPI_Datatype
     return res;
   }

+  if (inplace) {
+    MPI_Aint true_lb, true_extent;
+    void * buf;
+    MPI_Type_get_true_extent(sendtype, &true_lb, &true_extent);
+    buf = malloc(true_extent + sndext*(p-1));
+    if (OPAL_UNLIKELY(NULL == sendbuf)) {
+      return OMPI_ERR_OUT_OF_RESOURCE;
+    }
+    ompi_datatype_sndrcv(recvbuf, p*recvcount, recvtype, buf, p*recvcount, recvtype);
+    sendbuf = buf;
+  }
+
   /* algorithm selection */
   a2asize = sndsize * sendcount * p;
   /* this number is optimized for TCP on odin.cs.indiana.edu */
-  if((p <= 8) && ((a2asize < 1<<17) || (sndsize*sendcount < 1<<12))) {
+  if(inplace || ((p <= 8) && ((a2asize < 1<<17) || (sndsize*sendcount < 1<<12)))) {
     /* just send as fast as we can if we have less than 8 peers, if the
      * total communicated size is smaller than 1<<17 *and* if we don't
      * have eager messages (msgsize < 1<<13) */
@@ -113,6 +125,9 @@ int ompi_coll_libnbc_ialltoall(const void* sendbuf, int sendcount, MPI_Datatype
   if (OPAL_UNLIKELY(OMPI_SUCCESS != res)) {
     return res;
   }
+  if (inplace) {
+    handle->tmpbuf = (void *)sendbuf;
+  }

   /* allocate temp buffer if we need one */
   if (alg == NBC_A2A_DISS) {
diff --git a/ompi/mpi/c/ialltoall.c b/ompi/mpi/c/ialltoall.c
index af560da..cca9e7a 100644
--- a/ompi/mpi/c/ialltoall.c
+++ b/ompi/mpi/c/ialltoall.c
@@ -70,7 +70,7 @@ int MPI_Ialltoall(const void *sendbuf, int sendcount, MPI_Datatype sendtype,
         if (ompi_comm_invalid(comm)) {
             return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_COMM,
                                           FUNC_NAME);
-        } else if (MPI_IN_PLACE == sendbuf || MPI_IN_PLACE == recvbuf) {
+        } else if (MPI_IN_PLACE == recvbuf) {
             return OMPI_ERRHANDLER_INVOKE(MPI_COMM_WORLD, MPI_ERR_ARG,
                                           FUNC_NAME);
         } else {

[emmanuelthome]

Gilles, your patch fixes my issue. My test is with several (e.g. 10) processes oversubscribed on the same node. Regarding which coll layer I'm using, the failures I witnessed before your patch were visible with "-mca coll tuned,libnbc,basic" or "-mca coll libnbc,basic".

Thanks a lot.

[ggouaillardet]

Thanks,

I will review other non blocking collectives and other modules.

Btw, the coll/tuned module does not implement any non blocking collective, that is why the behavior is similar regardless tuned is used or not

[emmanuelthome]

@ggouaillardet You have probably noticed already, but for recvcount>1, your malloc() for the temp buffer misses a count multiplier. I think it should rather be

    buf = malloc((true_extent + sndext*(p-1))*recvcount);

[ggouaillardet]

Thanks for noticing the mistake.
There are p*recvcount elements in the buffer, so I think we should allocate

buf = malloc (true_extent + sndext*(p*recvcount-1))

Makes sense ?