Tuned bcast data corruption

[nysal]

There are multiple issues with bcast algorithms when the tasks involved specify different counts and datatypes for a given bcast operation.

One low hanging fix is :

--- a/ompi/mca/coll/tuned/coll_tuned_decision_fixed.c
+++ b/ompi/mca/coll/tuned/coll_tuned_decision_fixed.c
@@ -252,7 +252,7 @@ int ompi_coll_tuned_bcast_intra_dec_fixed(void *buff, int count,

     /* Handle messages of small and intermediate size, and
        single-element broadcasts */
-    if ((message_size < small_message_size) || (count <= 1)) {
+    if (message_size < small_message_size) {
         /* Binomial without segmentation */
         segsize = 0;
         return  ompi_coll_base_bcast_intra_binomial(buff, count, datatype,

I'm seeing issues with both the pipelined and split binary tree algorithms. This is probably a known issue with pipelined algorithms in tuned module. We should probably start discussing the right approach to solve this.

Here's a reproducer:

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

#define MAXLEN 10000000

int 
main(int argc, char **argv)
{
   int root,*out,i,j,k;
   int myself,tasks;
   MPI_Datatype type;

   MPI_Init(&argc,&argv);
   MPI_Comm_rank(MPI_COMM_WORLD,&myself);
   MPI_Comm_size(MPI_COMM_WORLD,&tasks);
   MPI_Type_contiguous(10, MPI_INT, &type);
   MPI_Type_commit(&type);
   /* Add this section because some os/architectures can't allocate
      this much memory on the stack */

   out = malloc(sizeof(int) * MAXLEN);
   if (out == NULL) {
     printf( "Doh!  Rank %d was not able to allocate enough memory.  MPI test aborted!\n", myself);
     exit(1);
   }

   root = tasks-1;
   for (j=1000000;j<=MAXLEN;j*=10)  {
      if (myself == root)
         for (i=0;i<j;i++)  
       out[i] = i;
      fprintf(stderr,"bcast %d integers\n",j);
      if(myself == root) {
          MPI_Bcast(out,j,MPI_INT,root,MPI_COMM_WORLD);
      } else {
          MPI_Bcast(out,j/10,type,root,MPI_COMM_WORLD);
      }

      for (k=0;k<j;k++) {
         if (out[k] != k) {  
       printf("bad answer (%d) at index %d of %d (should be %d)\n",out[k],k,j,k); 
           exit(1);
     }
      }
   }
   MPI_Barrier(MPI_COMM_WORLD);
   printf("Rank %d exiting successfully\n", myself);
   MPI_Type_free(&type);
   MPI_Finalize();

   /* Clean up, 'cause it's the Right Thing to do */

   free(out);

   return 0;
}

[ggouaillardet]

I confirm this is a known issue.
iirc, the workaround is to force the segment size to zero.
there has been some discussions on how to fix that, one of them involves flatten datatypes.
generally speaking, this requires an internal copy, which does impact performances.

[bosilca]

Indeed, the problem is that despite the fact that all processes must provide the same type signature, they can provide different counts, and the collective algorithm selection will be unable to cut the datatype at the expected boundary. Thus flattening the datatype, will generate the packed type that match the user provided type on all processes, a type that can be easily split in multiple fragments.

Another possible approach would be to allow the collective to call into the PML at a lower level, where they can provide the convertor and the length to be sent.

[ggouaillardet]

i like the second option better

consider the following academic case

• one hand sends n MPI_BYTE
• the other hand sends one ddt, which is m MPI_BYTE followed by padding and then (n-m) MPI_BYTE
• m is a prime number

the only way I can think of sending data into several pieces is to copy the data into a buffer (flatten datatype) first.
this is very unefficient ...

so let's enhance the PML framework to basically

• send up to the first n bytes of a message
• resume and send up to n bytes of a previous send

makes sense ?

[nysal]

I like the second suggestion better as well. So this will require an addition to the pml send interface. Lets discuss how that might look like. So an interface where we pass a convertor to the PML will be sufficient ?

[bosilca]

@ggouaillardet I don't think we can make this work the way you describe. My suggestion was that instead of the usual (pointer, datatype, count) we provide a well formed convertor and a length to the isend/irecv calls. This means that we maintain the same semantics as today (including matching per message), but we gain more flexibility on what par of the user data become a message by itself.

@ggouaillardet the academic case is trivial. The 2 datatypes have exactly the same type signature, so one flattened the 2 datatypes should be identical. In the worst case the flattened datatype will lack a high level count, but because the signature is the same, the current collective algorithms will work.

[ggouaillardet]

@bosilca ok for the pml extension you suggested

back to the academic case, this is something i never understood ... but i d like to understand at some point in time ...

let's create the ddt with
MPI_Type_create_struct(2, {3,1}, {0,4}, MPI_BYTE, &ddt);

and let's have one task call

MPI_Bcast(buf, 4, MPI_BYTE, ...);

and an other one call

MPI_Bcast(buf, 1, ddt, ...);

and we want to split the message into two (two bytes) chunks.

that is trivial if 4 * MPI_BYTE are used
now, with ddt, the flatten datatype is also 4 * MPI_BYTE
that is trivial if

MPI_Bcast(buf, 1, ddt, ...);

ends up doing

MPI_Sendrecv(buf, 1, ddt, ..., buf2, 4, MPI_BYTE, ...);
MPI_Bcast(buf2, 4, MPI_BYTE, ...);

put that requires extra memory and a local send/recv, which does impact performance (and sometimes pretty badly)

is there a way (and if yes, which one) to use flatten datatypes without requiring extra memory and a local send/recv ?

[ggouaillardet]

I made #1789 as a proof of concept
Only ob1 implements the extended pml interface for now.
The test case works now except with the openib btl
(I still need to debug some paths in ob1)
Heterogeneous clusters are currently broken.
I pass a size_t* parameter, but i could also pass a size_t, since non blocking subroutine do not update this value (yet)

Any thoughts ?

[jjhursey]

@bosilca You mentioned (in person) that there is an MCA parameter to route around this issue. What was that parameter?

[bosilca]

@jjhursey I said we should provide one ;) Right now you can use the tuned collective dynamic configuration to change the decision function to prevent any pipelined function from being called. We might want to create a simpler way, by taking advantage of the PVARs.

[jjhursey]

For anyone fixing this issue make sure to also fix it in libnbc - per Issue #2256. The resolution of that issue implements a work around - which should be removed once a full fix is available.

[jsquyres]

Per discussion on the 2017-02-21 call, removed the blocker label and moved to v3.0.0.

[hppritcha]

Doesn't look like this is going to make it into 3.0.0. Should we move to future?

[bosilca]

👍

[jsquyres]

@nysal @bosilca @ggouaillardet @jjhursey Did this get fixed after v3.0.x?

Regardless, unless the fix is trivial, it's unlikely to get fixed in v3.0.x or v3.1.x. Should this be moved to v4.0.x or closed?

[bosilca]

The underlying cause of this issue cannot get fixed in an efficient way in the current OMPI infrastructure. @ggouaillardet created a proof of concept PR but due to time constraints it never got completed, which means we are stuck with a suboptimal solution: disabling all tuned decisions for collective where the user provides different counts on the different nodes (implemented by @jjhursey for libnbc).

[jsquyres]

Ah, that's #1789? Looks like that was closed with no time to work on it.

Ok, let's leave this issue open, but I'll remove the milestone targets.