[WIP] Uncomment the use of MPI_Win_lock/MPI_Win_unlock

[janciesko]

Restores the use of MPI_Win_lock/MPI_Win_unlock.
This currently fails the unit test as:

[1,0]<stdout>:[ RUN      ] TEST_CATEGORY.test_deepcopy
[weaver11:90579] *** An error occurred in MPI_Win_flush
[weaver11:90579] *** reported by process [3964796929,1]
[weaver11:90579] *** on win rdma window 3
[weaver11:90579] *** MPI_ERR_RMA_SYNC: error executing rma sync
[weaver11:90579] *** MPI_ERRORS_ARE_FATAL (processes in this win will now abort,
[weaver11:90579] ***    and potentially your MPI job)

Reproducer:

cmake .. -DCMAKE_CXX_COMPILER=mpicxx  -DKokkos_ENABLE_MPISPACE=On -Kokkos_ENABLE_TESTS=On
mpirun --tag-output --np 2 --npernode 2  unit_tests/KokkosRemote_TestAll

[janciesko]

@jeffhammond - Jeff, is this the correct usage pattern for the sequence of lock/unlock and flush? The code us multi-threaded. I am seeing the error as described above.

[jeffhammond]

This is definitely wrong:

    MPI_Win_lock(MPI_LOCK_SHARED, pe, 0, win);                                 \
    MPI_Put(&val, 1, mpi_type, pe,                                             \
            sizeof(SharedAllocationHeader) + offset * _typesize, 1, mpi_type,  \
            win);                                                              \
    MPI_Win_unlock(pe, win);                                                   \
    MPI_Win_flush(pe, win);                                                    \

unlock does a flush, and if you have a flash outside of a lock epoch, that's wrong too.

The correct pattern for RMA is to do lock_all immediately after allocate, and unlock_all immediately prior to free, and use only flush or flush_local to complete RMA operations.

[jeffhammond]

cf9eb3b should be correct, and if an MPI implementation errors with that, I think the implementation is wrong.

[jeffhammond]

The problem is due to threads. See https://www.mail-archive.com/[email protected]/msg30676.html. Using request-based synchronization is the correct change here, and shouldn't be too hard.

[jeffhammond]

You need to figure out a way to make this thread-safe:

void MPISpace::fence() {
  for (int i = 0; i < mpi_windows.size(); i++) {
    if (mpi_windows[i] != MPI_WIN_NULL) {
      MPI_Win_flush_all(mpi_windows[i]);
    } else {
      break;
    }
  }
  MPI_Barrier(MPI_COMM_WORLD);
}

I will send you a pull-request for flush->wait but it isn't exactly equivalent, because you had put+flush, which does remote completion, whereas rput+wait only does remote completion. There is no request-based remote completion (I proposed it but it didn't go anywhere) so you'll have to make sure that all KRS code uses KRS::fence to do remote completion of rput ops.

[devreal]

@janciesko I cannot reproduce this issue with OMPI 5.0.x. Which version of OMPI are you using?

[janciesko]

@janciesko I cannot reproduce this issue with OMPI 5.0.x. Which version of OMPI are you using?

ompi 4.0.5

[janciesko]

Updated the PR that removes the wrong use. @jeffhammond, I have mistakenly reintroduced the wrong code after your previous PR. Sorry about the confusion. The code as in this PR now works.

Does the issue (mpiwg-rma/rma-issues#23) still apply? In this case here, we're still calling RMA ops and flush concurrently.

[janciesko]

Retest this please

[janciesko]

Superseded by #53