Ompi: RMA accumulate is busted in 4.0

[jeffhammond]

ARMCI-MPI is experiencing incorrect results with shared-memory accumulate with Open-MPI 4.0. Version 3.x is fine.

Background information

What version of Open MPI are you using? (e.g., v1.10.3, v2.1.0, git branch name and hash, etc.)

4.0.0

Describe how Open MPI was installed (e.g., from a source/distribution tarball, from a git clone, from an operating system distribution package, etc.)

Homebrew or built from source.

Please describe the system on which you are running

• Operating system/version: Mac or Linux
• Computer hardware: Intel CPUs.
• Network type: Shared-memory communication.

---

Details of the problem

https://travis-ci.org/pmodels/armci-mpi/jobs/477111650 is but one example.

Local testing

MacOS Mojave

Open-MPI from Homebrew:

jrhammon-mac02:prk-repo jrhammon$ mpicc -show
clang -I/usr/local/Cellar/open-mpi/4.0.0/include -L/usr/local/opt/libevent/lib -L/usr/local/Cellar/open-mpi/4.0.0/lib -lmpi
jrhammon-mac02:prk-repo jrhammon$ brew info open-mpi
open-mpi: stable 4.0.0 (bottled), HEAD
High performance message passing library
https://www.open-mpi.org/
Conflicts with:
  mpich (because both install MPI compiler wrappers)
/usr/local/Cellar/open-mpi/4.0.0 (753 files, 10.7MB) *
  Poured from bottle on 2019-01-14 at 11:27:52
From: https://github.com/Homebrew/homebrew-core/blob/master/Formula/open-mpi.rb
==> Dependencies
Required: gcc ✔, libevent ✔
==> Options
--HEAD
    Install HEAD version
==> Analytics
install: 8,380 (30 days), 32,457 (90 days), 95,919 (365 days)
install_on_request: 2,965 (30 days), 11,847 (90 days), 37,813 (365 days)
build_error: 0 (30 days)

Not using datatypes works fine:

ARMCI_STRIDED_METHOD=IOV ARMCI_IOV_METHOD=BATCHED ARMCI_USE_WIN_ALLOCATE=1 mpirun -oversubscribe -n 4 ./tests/contrib/armci-test
ARMCI_STRIDED_METHOD=IOV ARMCI_IOV_METHOD=BATCHED ARMCI_USE_WIN_ALLOCATE=0 mpirun -oversubscribe -n 4 ./tests/contrib/armci-test

Using datatypes fails differently depending on whether windows are allocated or created:

ARMCI_STRIDED_METHOD=DIRECT ARMCI_IOV_METHOD=DIRECT ARMCI_USE_WIN_ALLOCATE=1 mpirun -oversubscribe -n 4 ./tests/contrib/armci-test

fails here:

Testing non-blocking vector gets and puts

    Now veryfying the vector put data for correctness
    Puts OK


1:while verifying data of a op from proc=0 giov index=0 ptr_arr_index=0 
 :element index=0 elem was supposed to be 3.890000 but is 0.000000
2:while verifying data of a op from proc=0 giov index=0 ptr_arr_index=0 
 :element index=0 elem was supposed to be 4.890000 but is 0.000000
3:while verifying data of a op from proc=0 giov index=0 ptr_arr_index=0 
 :element index=0 elem was supposed to be 5.890000 but is 0.000000  Now veryfying the vector get data for correctness
0:while verifying data of a op from proc=1 giov index=0 ptr_arr_index=0 
 :element index=0 elem was supposed to be 3.890000 but is 1010105040100.000000[3] ARMCI Error: vector non-blocking failed
[0] ARMCI Error: vector non-blocking failed
[1] ARMCI Error: vector non-blocking failed
[2] ARMCI Error: vector non-blocking failed
--------------------------------------------------------------------------
MPI_ABORT was invoked on rank 2 in communicator MPI COMMUNICATOR 3 DUP FROM 0
with errorcode 0.

ARMCI_STRIDED_METHOD=DIRECT ARMCI_IOV_METHOD=DIRECT ARMCI_USE_WIN_ALLOCATE=0 mpirun -oversubscribe -n 4 ./tests/contrib/armci-test 
[0] ARMCI Warning: MPI Datatypes are broken in RMA in older versions of Open-MPI!
ARMCI test program (4 processes)
[0] ARMCI Warning: MPI Datatypes are broken in RMA in older versions of Open-MPI!
[0] ARMCI Warning: MPI Datatypes are broken in RMA in older versions of Open-MPI!

Testing strided gets and puts
(Only std output for process 0 is printed)

[0] ARMCI Warning: MPI Datatypes are broken in RMA in older versions of Open-MPI!
--------array[5]--------
local[2:4] -> remote[2:4] -> local[0:2] 
ERROR: a [2] (proc=2):2.000000 b [0] 0.000000

A = 2.000000 B = 0.000000
ERROR: a [2] (proc=1):2.000000 b [0] 0.000000

A = 2.000000 B = 0.000000
ERROR: a [2] (proc=0):2.000000[jrhammon-mac02:19084] *** Process received signal ***
[jrhammon-mac02:19084] Signal: Abort trap: 6 (6)
[jrhammon-mac02:19084] Signal code:  (0)
[jrhammon-mac02:19084] [ 0] 0   libsystem_platform.dylib            0x00007fff63f33b3d _sigtramp + 29
[jrhammon-mac02:19084] [ 1] 0   libsystem_c.dylib                   0x00007fff63d95000 __dso_handle + 0
[jrhammon-mac02:19084] [ 2] 0   libsystem_c.dylib                   0x00007fff63df11c9 abort + 127
[jrhammon-mac02:19084] [ 3] 0   libsystem_c.dylib                   0x00007fff63df133c _UTF2_init + 0
[jrhammon-mac02:19084] [ 4] 0   libsystem_c.dylib                   0x00007fff63e15c8e __chk_fail_overlap + 0
[jrhammon-mac02:19084] [ 5] 0   libsystem_c.dylib                   0x00007fff63e15c5e __chk_fail + 0
[jrhammon-mac02:19084] [ 6] 0   libsystem_c.dylib                   0x00007fff63e1615b __memcpy_chk + 0
[jrhammon-mac02:19084] [ 7] 0   armci-test                          0x0000000106d59f68 compare_patches + 1656
[jrhammon-mac02:19084] [ 8] 0   armci-test                          0x0000000106d5aa52 test_dim + 1682
[jrhammon-mac02:19084] [ 9] 0   armci-test                          0x0000000106d61fa8 main + 344
[jrhammon-mac02:19084] [10] 0   libdyld.dylib                       0x00007fff63d48ed9 start + 1
[jrhammon-mac02:19084] *** End of error message ***
--------------------------------------------------------------------------
Primary job  terminated normally, but 1 process returned
a non-zero exit code. Per user-direction, the job has been aborted.
--------------------------------------------------------------------------
--------------------------------------------------------------------------
MPI_ABORT was invoked on rank 2 in communicator MPI COMMUNICATOR 3 DUP FROM 0
with errorcode 0.

NOTE: invoking MPI_ABORT causes Open MPI to kill all MPI processes.
You may or may not see output from other processes, depending on
exactly when Open MPI kills them.
--------------------------------------------------------------------------
[1] ARMCI Error: Bailing out
[2] ARMCI Error: Bailing out
--------------------------------------------------------------------------
mpirun noticed that process rank 0 with PID 0 on node jrhammon-mac02 exited on signal 6 (Abort trap: 6).
--------------------------------------------------------------------------
[jrhammon-mac02.ra.intel.com:19083] 1 more process has sent help message help-mpi-api.txt / mpi-abort
[jrhammon-mac02.ra.intel.com:19083] Set MCA parameter "orte_base_help_aggregate" to 0 to see all help / error messages

[jeffhammond]

@hjelmn This is a regression from Open-MPI 3.x. I was very happy to see that all of the ARMCI-MPI tests were passing with Open-MPI until 4.0 appeared.

Open-MPI 3.1.2 was all-green e.g. here:

• https://travis-ci.org/jeffhammond/old-armci-mpi/jobs/443766403
• https://travis-ci.org/jeffhammond/old-armci-mpi/jobs/443766405

[hjelmn]

May or not be a regression per-se. Try running with OMPI_MCA_osc=sm,pt2pt . That will isolate whether something broke in the datatypes or if this was possibly an existing bug in osc/rdma (which now works with shared memory). I will probably have a chance to take a look tomorrow.

[hjelmn]

I will see about getting ARMCI added to CI and/or MTT. No idea when this happened as the osc support in v3.1.x and v4.0.x isn't much different (except for better win_create + shared memory support). There were some changes to the vector datatypes though...

[ggouaillardet]

mpirun --mca osc ^rdma fixes both cases on a single node (e.g. btl/vader) with Open MPI v4.0.x

The first case works fine with the same option on master, but the second case crashes in btl/vader.
The crash could be unrelated to this though.

[ggouaillardet]

@hjelmn it seems that with osc/rdma, MPI_Get_accumulate() only works when result_datatype is a predefined datatype.

The simple program below produces incorrect results when osc/rdma is used (and works fine otherwise)

#include <stdio.h>

#include <mpi.h>

int main (int argc, char *argv[]) {
    MPI_Datatype datatype;
    MPI_Win win;
    MPI_Info info;
    int rank;

    MPI_Init(&argc, &argv);
    int loc = -1;
    int * shared;

    MPI_Comm_rank(MPI_COMM_WORLD, &rank);
    int peer = rank^1;

    MPI_Info_create(&info);
    MPI_Info_set(info, "alloc_shm", "true");
    MPI_Alloc_mem(sizeof(int), info, &shared);
    MPI_Win_create(shared, sizeof(int), 1, MPI_INFO_NULL, MPI_COMM_WORLD, &win);
    MPI_Win_lock_all(MPI_MODE_NOCHECK, win);

    *shared = rank;

    MPI_Type_dup(MPI_INT, &datatype);
    MPI_Type_commit(&datatype);

    MPI_Get_accumulate(NULL, 0, MPI_INT,
                       &loc, 1, datatype,
                       peer, 0, 1, MPI_INT, MPI_NO_OP, win);

    MPI_Win_unlock_all(win);
    MPI_Win_free(&win);
    MPI_Free_mem(shared);

    if (loc != peer) fprintf (stderr, "rank %d: expected %d but got %d\n", rank, peer, loc);

    MPI_Finalize();
    return 0;
}

[hjelmn]

Interesting. Thanks for tracking that down. MTT should have caught that. Will fix today.

[jsquyres]

@gpaulsen @hppritcha Is this a blocker for v4.0.1?

[gpaulsen]

Yes, I'd say this should be a v4.0.1 blocker.

[jeffhammond]

How do I set OMPI_MCA_osc=sm,pt2pt inside the program (without using the evil setenv)? I will add that to ARMCI-MPI for 4.0.0.

[hjelmn]

@jeffhammond Shouldn't be necessary. The simple reproducer @ggouaillardet provided should help my get to the bottom of this. Should have the fix later today. Adding the v3.1.x tag as well since this may affect osc/rdma there.

[jeffhammond]

@hjelmn I would agree with you if this was the first time ARMCI-MPI revealed bugs in Open-MPI 😉

[hjelmn]

The variable can be set via environment, command line (mpirun --mca osc sm,pt2pt), or via MPI_T before MPI_Init.

[hjelmn]

... or a configuration file.

[hjelmn]

Found the issue. It's a bug in MPI_OP_NO_OP support. Testing a fix now.

[hjelmn]

Long story short: at some point (probably long ago) the source and target looping logic got reversed. This lead to the loop not executing when source_count == 0. I fixed the loop logic and it works with the reproducer @ggouaillardet posted.

[hjelmn]

@ggouaillardet Do you want to put the reproducer in ompi-tests or should I?

[ggouaillardet]

@hjelmn I will add the test to the ibm suite

@jeffhammond what is wrong with setenv() ?

I can confirm the fix backported to v4.0.x does fix the reproducer (ARMCI_NbGetV() like),
but there are some more issues (starting with ARMCI_NbPutV() __and__ ARMCI_USE_WIN_ALLOCATE=0). I will write (and add to the ibm test suite) some more reproducers.

[jeffhammond]

@jeffhammond what is wrong with setenv() ?

https://blogs.gnome.org/mcatanzaro/2018/02/28/on-setenv-and-explosions/

Unlikely to affect codes that use ARMCI but I try not to assume bad things are going to happen in software.

Bigger issue is I can’t set anything before MPI init so I’m probably out of luck in any case.

[ggouaillardet]

I see, thanks for the link.
As pointed by Nathan, using MPI_T is likely your best option.
It might be feasible to add an other info key to disable osc/rdma on a per window basis in MPI_Win_create() and friends, but I cannot commit this is the path that will be chosen.

[jeffhammond]

Per window with info key would be nice in general but I don’t need it for this bug.

[hjelmn]

@jeffhammond That is a reasonable request. I can add an info key: ompi_osc_component

[ggouaillardet]

@hjelmn it seems there is an other bug when result_datatype is not contiguous.

The test program below (to be ran with 2 MPI tasks) produces incorrect results when osc/rdma is used
(and correct results otherwise)

#include <stdio.h>
#include <string.h>

#include <mpi.h>

int main (int argc, char *argv[]) {
    MPI_Datatype datatype;
    MPI_Win win;
    MPI_Info info;
    int rank;

    MPI_Init(&argc, &argv);
    int res[3];
    int * shar;

    MPI_Comm_rank(MPI_COMM_WORLD, &rank);
    int peer = rank^1;

    MPI_Info_create(&info);
    MPI_Info_set(info, "alloc_shm", "true");
    MPI_Alloc_mem(3*sizeof(int), info, &shar);
    MPI_Win_create(shar, 2*sizeof(int), 4, MPI_INFO_NULL, MPI_COMM_WORLD, &win);

    k = peer;
    shar[0] = k++;
    shar[1] = k++;

    res[0] = -1;
    res[1] = -1;
    res[2] = -1;

    MPI_Type_vector(2, 1, 2, MPI_INT, &datatype);
    MPI_Type_commit (&datatype);

    MPI_Win_lock_all(MPI_MODE_NOCHECK, win);

    MPI_Get_accumulate(NULL, 0, MPI_INT,
                       res, 1, datatype, peer,
                       0, 2, MPI_INT, MPI_NO_OP, win);
    MPI_Win_flush(peer, win);

    MPI_Win_unlock_all(win);
    MPI_Win_free(&win);

    MPI_Free_mem(shar);

    k = rank;
    if (res[0] != k) fprintf(stderr, "res[0] got %d expected %d\n", res[0], k);
    k++;
    if (res[1] != -1) fprintf(stderr, "res[1] got %d expected %d\n", res[1], -1);
    if (res[2] != k) fprintf(stderr, "res[2] got %d expected %d\n", res[1], k);

    MPI_Finalize();
    return 0;
}

[ggouaillardet]

@hjelmn it was indeed a simple typo, and I pushed a second commit into your PR in order to address it.

the test suite now crashes at

Testing Vector Interface using triangular patches of a 2-D array

I will re-iterate after the weekend.

[gpaulsen]

@hppritcha agrees this is a blocker for v4.0.1

[ggouaillardet]

@hjelmn here is an other reproducer

#include <stdio.h>

#include <mpi.h>

int main (int argc, char *argv[]) {
    MPI_Datatype datatype;
    MPI_Win win;
    MPI_Info info;
    int i, rank;

    MPI_Init(&argc, &argv);
    int loc;
    int * shar;

    MPI_Comm_rank(MPI_COMM_WORLD, &rank);
    int peer = rank^1;

    MPI_Info_create(&info);
    MPI_Info_set(info, "alloc_shm", "true");
    MPI_Win_allocate(sizeof(int), 1, info, MPI_COMM_WORLD, (void *)&shar, &win);
    loc = -1;
    *shar = rank+i;
    MPI_Barrier(MPI_COMM_WORLD);
    MPI_Win_lock_all(MPI_MODE_NOCHECK, win);

    int bl[1] = {1};
    MPI_Aint disps[1];
    MPI_Get_address(&loc, disps);
    MPI_Type_create_hindexed(1, bl, disps, MPI_INT, &datatype);
    MPI_Type_commit(&datatype);

    MPI_Get_accumulate(NULL, 0, MPI_INT,
                       MPI_BOTTOM, 1, datatype,
                       peer, 0, 1, MPI_INT, MPI_NO_OP, win);

    MPI_Win_unlock_all(win);
    MPI_Win_free(&win);

    if (loc != peer+i) fprintf (stderr, "rank %d: expected %d but got %d\n", rank, peer+i, loc);

    MPI_Finalize();
    return 0;
}

in ompi_osc_rdma_gacc_local() we test NULL != result_buffer, and the test is incorrect here since result_buffer is MPI_BOTTOM and we expect some action.

if replace the test with if (true), then the reproducer works, but it breaks other parts of the code I have no time to investigate today.

[ggouaillardet]

@hjelmn testing result_datatype instead does the trick, and I updated the PR.

I backported this PR into my v4.0.x branch, and now armci-test passes with ARMCI_STRIDED_METHOD=DIRECT ARMCI_IOV_METHOD=DIRECT ARMCI_USE_WIN_ALLOCATE=1

ARMCI_STRIDED_METHOD=DIRECT ARMCI_IOV_METHOD=DIRECT ARMCI_USE_WIN_ALLOCATE=0 still has a (last) issue with test_vector()

[ggouaillardet]

and here is a new reproducer related to the last issue.
At this stage, I think there is yet an other bug that is not evidenced by this reproducer.

#include <stdio.h>
#include <string.h>

#include <mpi.h>

int main (int argc, char *argv[]) {
    MPI_Datatype origin_datatype, target_datatype;
    MPI_Win win;
    MPI_Info info;
    int k, rank;

    MPI_Init(&argc, &argv);
    int loc[2];
    int * shar;

    MPI_Comm_rank(MPI_COMM_WORLD, &rank);
    int peer = rank^1;

    MPI_Info_create(&info);
    MPI_Info_set(info, "alloc_shm", "true");
    MPI_Alloc_mem(2*sizeof(int), info, &shar);

    k = peer;
    shar[0] = -1;
    shar[1] = -1;

    loc[0] = rank;
    loc[1] = rank+1;

    MPI_Win_create(shar, 2*sizeof(int), 4, MPI_INFO_NULL, MPI_COMM_WORLD, &win);

    MPI_Type_contiguous(2, MPI_INT, &target_datatype);
    MPI_Type_commit (&target_datatype);

    int bl[1] = {2};
    MPI_Aint disps[1];
    MPI_Get_address(loc, disps);
    MPI_Type_create_hindexed(1, bl, disps, MPI_INT, &origin_datatype);
    MPI_Type_commit(&origin_datatype);

    MPI_Win_lock_all(MPI_MODE_NOCHECK, win);

    MPI_Accumulate(MPI_BOTTOM, 1, origin_datatype,
                       peer, 0, 1, target_datatype, MPI_REPLACE, win);
    MPI_Win_flush(peer, win);

    MPI_Win_unlock_all(win);
    MPI_Win_free(&win);


    k = peer;
    if (shar[0] != k) fprintf(stderr, "shar[0] got %d expected %d\n", shar[0], k);
    k++;
    if (shar[1] != k) fprintf(stderr, "shar[1] got %d expected %d\n", shar[1], k);

    MPI_Free_mem(shar);
    MPI_Finalize();
    return 0;
}

[hjelmn]

Thanks @ggouaillardet. Will take a look today. Once that is fixed I will squash everything down and PR to v4.0.x, v3.1.x, and v3.0.x.

[hjelmn]

For the record. MPI_BOTTOM: ugh.

[ggouaillardet]

here is an other reproducer that evidences the same issue than the previous one, but without MPI_BOTTOM

#include <stdio.h>
#include <string.h>

#include <mpi.h>

int main (int argc, char *argv[]) {
    MPI_Datatype origin_datatype, target_datatype;
    MPI_Win win;
    MPI_Info info;
    int k, rank;

    MPI_Init(&argc, &argv);
    int loc[2];
    int * shar;

    MPI_Comm_rank(MPI_COMM_WORLD, &rank);
    int peer = rank^1;

    MPI_Info_create(&info);
    MPI_Info_set(info, "alloc_shm", "true");
    MPI_Alloc_mem(sizeof(int), info, &shar);

    k = peer;
    shar[0] = -1;

    loc[0] = -2;
    loc[1] = rank;

    MPI_Win_create(shar, sizeof(int), 4, MPI_INFO_NULL, MPI_COMM_WORLD, &win);

    int bl[1] = {1};
    MPI_Aint disps[1] = {sizeof(int)};
    MPI_Type_create_hindexed(1, bl, disps, MPI_INT, &origin_datatype);
    MPI_Type_commit(&origin_datatype);

    MPI_Win_lock_all(MPI_MODE_NOCHECK, win);

    MPI_Accumulate(loc, 1, origin_datatype,
                   peer, 0, 1, MPI_INT, MPI_REPLACE, win);
    MPI_Win_flush(peer, win);

    MPI_Win_unlock_all(win);
    MPI_Win_free(&win);


    if (shar[0] != peer) fprintf(stderr, "shar[0] got %d expected %d\n", shar[0], peer);

    MPI_Free_mem(shar);
    MPI_Finalize();
    return 0;
}

FWIW, here is a patch that fixes the reproducer, but does not fix the full armci-test
(even if it fixes the crash, the data is incorrect)

diff --git a/ompi/mca/osc/rdma/osc_rdma_accumulate.c b/ompi/mca/osc/rdma/osc_rdma_accumulate.c
index 4206f18216..76f6a3ce7f 100644
--- a/ompi/mca/osc/rdma/osc_rdma_accumulate.c
+++ b/ompi/mca/osc/rdma/osc_rdma_accumulate.c
@@ -538,9 +538,9 @@ static int ompi_osc_rdma_fetch_and_op_cas (ompi_osc_rdma_sync_t *sync, const voi
         new_value = old_value;

         if (&ompi_mpi_op_replace.op == op) {
-            memcpy ((void *)((intptr_t) &new_value + offset), origin_addr, extent);
+            memcpy ((void *)((intptr_t) &new_value + offset), origin_addr + dt->super.true_lb, extent);
         } else if (&ompi_mpi_op_no_op.op != op) {
-            ompi_op_reduce (op, (void *) origin_addr, (void*)((intptr_t) &new_value + offset), 1, dt);
+            ompi_op_reduce (op, (void *) origin_addr + dt->super.true_lb, (void*)((intptr_t) &new_value + offset), 1, dt);
         }

         ret = ompi_osc_rdma_btl_cswap (module, peer->data_endpoint, address, target_handle,

[ggouaillardet]

The new reproducer below illustrates yet an other (and hopefully final) issue.

It works fine without osc/rdma and fails otherwise.

#include <stdio.h>
#include <string.h>

#include <mpi.h>

#define N 312

int main (int argc, char *argv[]) {
    MPI_Datatype origin_datatype, target_datatype;
    MPI_Win win;
    MPI_Info info;
    int k, rank;

    MPI_Init(&argc, &argv);
    int loc[2];
    int * shar;

    MPI_Comm_rank(MPI_COMM_WORLD, &rank);
    int peer = rank^1;

    MPI_Info_create(&info);
    MPI_Info_set(info, "alloc_shm", "true");
    MPI_Alloc_mem(N*sizeof(int), info, &shar);

    k = peer;
    memset(shar, -1, N*sizeof(int));

    loc[0] = rank;
    loc[1] = rank+1;

    MPI_Win_create(shar, N*sizeof(int), 4, MPI_INFO_NULL, MPI_COMM_WORLD, &win);

    MPI_Type_contiguous(2, MPI_INT, &origin_datatype);
    MPI_Type_commit(&origin_datatype);

    int bl[1] = {2};
    MPI_Aint disps[1] = {(N-2)*sizeof(int)};
    MPI_Type_create_hindexed(1, bl, disps, MPI_INT, &target_datatype);
    MPI_Type_commit(&target_datatype);

    MPI_Win_lock_all(MPI_MODE_NOCHECK, win);

    MPI_Accumulate(loc, 1, origin_datatype,
                   peer, 0, 1, target_datatype, MPI_REPLACE, win);
    MPI_Win_flush(peer, win);

    MPI_Win_unlock_all(win);
    MPI_Win_free(&win);


    if (shar[N-2] != peer) fprintf(stderr, "shar[%d] got %d expected %d\n", N-2, shar[N-2], peer);
    if (shar[N-1] != peer+1) fprintf(stderr, "shar[%d] got %d expected %d\n", N-1, shar[N-1], peer);

    MPI_Free_mem(shar);
    MPI_Finalize();
    return 0;
}

[ggouaillardet]

and here is a patch that fixes the very last issue

diff --git a/ompi/mca/osc/rdma/osc_rdma_accumulate.c b/ompi/mca/osc/rdma/osc_rdma_accumulate.c
index 4206f18216..c94c739e15 100644
--- a/ompi/mca/osc/rdma/osc_rdma_accumulate.c
+++ b/ompi/mca/osc/rdma/osc_rdma_accumulate.c
@@ -805,7 +805,7 @@ int ompi_osc_rdma_rget_accumulate_internal (ompi_osc_rdma_sync_t *sync, const vo
     ompi_osc_rdma_module_t *module = sync->module;
     mca_btl_base_registration_handle_t *target_handle;
     uint64_t target_address;
-    ptrdiff_t lb, origin_extent, target_span;
+    ptrdiff_t lb, target_lb, origin_extent, target_span;
     bool lock_acquired = false;
     int ret;

@@ -818,11 +818,11 @@ int ompi_osc_rdma_rget_accumulate_internal (ompi_osc_rdma_sync_t *sync, const vo
         return OMPI_SUCCESS;
     }

-    target_span = opal_datatype_span(&target_datatype->super, target_count, &lb);
+    target_span = opal_datatype_span(&target_datatype->super, target_count, &target_lb);

     // a buffer defined by (buf, count, dt)
     // will have data starting at buf+offset and ending len bytes later:
-    ret = osc_rdma_get_remote_segment (module, peer, target_disp, target_span+lb, &target_address, &target_handle);
+    ret = osc_rdma_get_remote_segment (module, peer, target_disp, target_span+target_lb, &target_address, &target_handle);
     if (OPAL_UNLIKELY(OMPI_SUCCESS != ret)) {
         return ret;
     }
@@ -846,10 +846,10 @@ int ompi_osc_rdma_rget_accumulate_internal (ompi_osc_rdma_sync_t *sync, const vo
     if (origin_extent <= 8 && 1 == origin_count && !ompi_osc_rdma_peer_local_base (peer)) {
         if (module->acc_use_amo && ompi_datatype_is_predefined (origin_datatype)) {
             if (NULL == result_addr) {
-                ret = ompi_osc_rdma_acc_single_atomic (sync, origin_addr, origin_datatype, origin_extent, peer, target_address,
+                ret = ompi_osc_rdma_acc_single_atomic (sync, origin_addr, origin_datatype, origin_extent, peer, target_address+target_lb,
                                                        target_handle, op, request, lock_acquired);
             } else {
-                ret = ompi_osc_rdma_fetch_and_op_atomic (sync, origin_addr, result_addr, origin_datatype, origin_extent, peer, target_address,
+                ret = ompi_osc_rdma_fetch_and_op_atomic (sync, origin_addr, result_addr, origin_datatype, origin_extent, peer, target_address+target_lb,
                                                          target_handle, op, request, lock_acquired);
             }

@@ -858,7 +858,7 @@ int ompi_osc_rdma_rget_accumulate_internal (ompi_osc_rdma_sync_t *sync, const vo
             }
         }

-        ret = ompi_osc_rdma_fetch_and_op_cas (sync, origin_addr, result_addr, origin_datatype, origin_extent, peer, target_address,
+        ret = ompi_osc_rdma_fetch_and_op_cas (sync, origin_addr, result_addr, origin_datatype, origin_extent, peer, target_address+target_lb,
                                               target_handle, op, request, lock_acquired);
         if (OMPI_SUCCESS == ret) {
             return OMPI_SUCCESS;

for the records, here is a fix I described earlier.

I back ported the branch to v4.0.x and applied the last 3 patches inlined in the comments, and the armci test suite now works for me :-)

diff --git a/ompi/mca/osc/rdma/osc_rdma_accumulate.c b/ompi/mca/osc/rdma/osc_rdma_accumulate.c
index 4206f18216..c94c739e15 100644
--- a/ompi/mca/osc/rdma/osc_rdma_accumulate.c
+++ b/ompi/mca/osc/rdma/osc_rdma_accumulate.c
@@ -64,7 +64,7 @@ static int ompi_osc_rdma_gacc_local (const void *source_buffer, int source_count
     do {
         OSC_RDMA_VERBOSE(MCA_BASE_VERBOSE_TRACE, "performing accumulate with local region(s)");

-        if (NULL != result_buffer) {
+        if (NULL != result_datatype) {
             /* get accumulate */

             ret = ompi_datatype_sndrcv ((void *) (intptr_t) target_address, target_count, target_datatype,

[hjelmn]

@ggouaillardet Thanks! Be sure to commit all the reproducers into MTT. We didn't have enough coverage of accumulate + derived datatypes. I will add all these changes to my PR.

[ggouaillardet]

yep, will do.

meanwhile, the ARMCI_AccS_latency fails and here is a reproducer

#include <stdio.h>
#include <string.h>

#include <mpi.h>

int main (int argc, char *argv[]) {
    MPI_Datatype origin_datatype, target_datatype;
    MPI_Win win;
    MPI_Info info;
    int k, rank;

    MPI_Init(&argc, &argv);
    double loc;
    double * shar;

    MPI_Comm_rank(MPI_COMM_WORLD, &rank);
    int peer = rank^1;

    MPI_Info_create(&info);
    MPI_Info_set(info, "alloc_shm", "true");
    MPI_Alloc_mem(sizeof(double), info, &shar);

    shar[0] = rank+1;

    loc = rank+1;

    MPI_Win_create(shar, sizeof(double), sizeof(double), MPI_INFO_NULL, MPI_COMM_WORLD, &win);

    MPI_Type_contiguous(1, MPI_DOUBLE, &origin_datatype);
    MPI_Type_commit(&origin_datatype);

    MPI_Type_contiguous(1, MPI_DOUBLE, &target_datatype);
    MPI_Type_commit(&target_datatype);

    MPI_Win_lock_all(MPI_MODE_NOCHECK, win);

    MPI_Accumulate(&loc, 1, origin_datatype,
                   peer, 0, 1, target_datatype, MPI_SUM, win);
    MPI_Win_flush(peer, win);

    MPI_Win_unlock_all(win);
    MPI_Win_free(&win);


    if (shar[0] != (peer+rank+2)) fprintf(stderr, "shar[0] got %d expected %d\n", shar[0], (peer+rank+2));

    MPI_Free_mem(shar);
    MPI_Finalize();
    return 0;
}

the reproducer works with osc/pt2pt and fails with osc/rdma.

That being said, MPI_SUM is used with non predefined datatypes, so is this a legal program (w.r.t. the MPI standard) in the first place ?

Fun fact, if we replace MPI_Type_contiguous(1, ...) with MPI_Type_dup(...) as in the new reproducer below, osc/rdma works but osc/pt2pt crashes.

#include <stdio.h>
#include <string.h>

#include <mpi.h>

int main (int argc, char *argv[]) {
    MPI_Datatype origin_datatype, target_datatype;
    MPI_Win win;
    MPI_Info info;
    int k, rank;

    MPI_Init(&argc, &argv);
    double loc;
    double * shar;

    MPI_Comm_rank(MPI_COMM_WORLD, &rank);
    int peer = rank^1;

    MPI_Info_create(&info);
    MPI_Info_set(info, "alloc_shm", "true");
    MPI_Alloc_mem(sizeof(double), info, &shar);

    shar[0] = rank+1;

    loc = rank+1;

    MPI_Win_create(shar, sizeof(double), sizeof(double), MPI_INFO_NULL, MPI_COMM_WORLD, &win);

    MPI_Type_dup(MPI_DOUBLE, &origin_datatype);
    MPI_Type_commit(&origin_datatype);

    MPI_Type_dup(MPI_DOUBLE, &target_datatype);
    MPI_Type_commit(&target_datatype);

    MPI_Win_lock_all(MPI_MODE_NOCHECK, win);

    MPI_Accumulate(&loc, 1, origin_datatype,
                   peer, 0, 1, target_datatype, MPI_SUM, win);
    MPI_Win_flush(peer, win);

    MPI_Win_unlock_all(win);
    MPI_Win_free(&win);


    if (shar[0] != (peer+rank+2)) fprintf(stderr, "shar[0] got %d expected %d\n", shar[0], (peer+rank+2));

    MPI_Free_mem(shar);
    MPI_Finalize();
    return 0;
}

[hjelmn]

At first glance I think both programs should work and are semantically equivalent. Interesting... Might be worth dumping the datatypes and seeing what is different. The dup is probably crashing due to a bug in the datatype serialization/deserialization code.

[jeffhammond]

That being said, MPI_SUM is used with non predefined datatypes, so is this a legal program (w.r.t. the MPI standard) in the first place ?

Yes it’s legal. Accumulate allows this because op acts on underlying basis datatype element-wise.

[ggouaillardet]

please disregard my previous question/comment. I found the answer in the code

                   /* ACCUMULATE, unlike REDUCE, can use with derived
                       datatypes with predefinied operations, with some
                       restrictions outlined in MPI-3:11.3.4.  The derived
                       datatype must be composed entierly from one predefined
                       datatype (so you can do all the construction you want,
                       but at the bottom, you can only use one datatype, say,
                       MPI_INT).  If the datatype at the target isn't
                       predefined, then make sure it's composed of only one
                       datatype, and check that datatype against
                       ompi_op_is_valid(). */

[ggouaillardet]

thanks, I think I just fixed everything, see https://travis-ci.com/ggouaillardet/armci-mpi/builds/98763177

The osc/pt2pt issue is from ompi_datatype_set_args(), I will issue a separate PR for that

here is the latest patch

diff --git a/ompi/datatype/ompi_datatype_args.c b/ompi/datatype/ompi_datatype_args.c
index 737d3e5..68be984 100644
--- a/ompi/datatype/ompi_datatype_args.c
+++ b/ompi/datatype/ompi_datatype_args.c
@@ -13,8 +13,8 @@
  * Copyright (c) 2009      Oak Ridge National Labs.  All rights reserved.
  * Copyright (c) 2013-2017 Los Alamos National Security, LLC.  All rights
  *                         reserved.
- * Copyright (c) 2015-2017 Research Organization for Information Science
- *                         and Technology (RIST). All rights reserved.
+ * Copyright (c) 2015-2019 Research Organization for Information Science
+ *                         and Technology (RIST).  All rights reserved.
  * Copyright (c) 2017      IBM Corporation. All rights reserved.
  * $COPYRIGHT$
  *
@@ -237,6 +237,8 @@ int32_t ompi_datatype_set_args( ompi_datatype_t* pData,
              */
             OBJ_RETAIN( d[pos] );
             pArgs->total_pack_size += ((ompi_datatype_args_t*)d[pos]->args)->total_pack_size;
+        } else {
+            pArgs->total_pack_size += sizeof(int);
         }
         pArgs->total_pack_size += sizeof(int);  /* each data has an ID */
     }
diff --git a/ompi/op/op.h b/ompi/op/op.h
index aa52688..7e1fe33 100644
--- a/ompi/op/op.h
+++ b/ompi/op/op.h
@@ -15,6 +15,8 @@
  * Copyright (c) 2009      Sun Microsystems, Inc.  All rights reserved.
  * Copyright (c) 2015      Los Alamos National Security, LLC. All rights
  *                         reserved.
+ * Copyright (c) 2019      Research Organization for Information Science
+ *                         and Technology (RIST).  All rights reserved.
  * $COPYRIGHT$
  *
  * Additional copyrights may follow
@@ -569,9 +571,16 @@ static inline void ompi_op_reduce(ompi_op_t * op, void *source,

     /* For intrinsics, we also pass the corresponding op module */
     if (0 != (op->o_flags & OMPI_OP_FLAGS_INTRINSIC)) {
-        op->o_func.intrinsic.fns[ompi_op_ddt_map[dtype->id]](source, target,
-                                                             &count, &dtype,
-                                                             op->o_func.intrinsic.modules[ompi_op_ddt_map[dtype->id]]);
+        int dtype_id;
+        if (!ompi_datatype_is_predefined(dtype)) {
+            ompi_datatype_t *dt = ompi_datatype_get_single_predefined_type_from_args(dtype);
+            dtype_id = ompi_op_ddt_map[dt->id];
+        } else {
+            dtype_id = ompi_op_ddt_map[dtype->id];
+        }
+        op->o_func.intrinsic.fns[dtype_id](source, target,
+                                           &count, &dtype,
+                                           op->o_func.intrinsic.modules[dtype_id]);
         return;
     }

FWIW, a cumulative patch that can be applied vs the 4.0.0 tarball is at https://gist.github.com/ggouaillardet/fad4d29e32331b6f3fdfcdede36c097c

[hjelmn]

@ggouaillardet Ok, will pull all the osc/rdma changes into my PR and squash everything together. Should be merged by the end of the day. Then will backport to relevant branches. @jeffhammond Thanks for bringing this to our attention. Will get ARMCI into MTT soon to hopefully avoid any future issues.

[hjelmn]

@ggouaillardet Do you want to open a separate PR for the op changes?

[hjelmn]

@ggouaillardet I think I got all the fixes into #6301. Please take a look.

[ggouaillardet]

@hjelmn I will issue a separate PR for the op changes, double check all the fixes went in your PR and push all my reproducers into the ibm test suite.

[ggouaillardet]

since osc/rdma uses opal_convertor_raw() #6295 should also be included in 4.0.1

[ggouaillardet]

travis tests look good with v4.0.x and (backports of) #6295 #6301 #6305 and #6312

[ggouaillardet]

@hjelmn is osc/rdma supposed to work with pml/ucx ?

ompi_osc_rdma_peer_btl_endpoint() returns NULL and I wonder if osc/rdma assumes pml/ob1.
In that case, should we disable it earlier (currently, MPI_Win_allocate() fails and aborts the program).

[hjelmn]

There is another patch on master that fixes btl/vader when pml/ucx is in use. Not sure if it had been merged into v4.0.x yet.

[hjelmn]

Hah, tested everything on a Cray and got one failure:

FAIL: tests/mpi/test_win_create

@jeffhammond I don't know if that is a reasonable test. Your test is exhausting registration resources and (as we are allowed to) we return an error (or in this case abort):

 + Creating window 915
[nid00098:48694] finished creating osc/rdma window with id 917
[nid00098:48694] selected btl: ugni
[nid00098:48695] selected btl: ugni
[nid00098:48694] creating osc/rdma window of flavor 1 with id 918
[nid00098:48695] creating osc/rdma window of flavor 1 with id 918
[nid00098:48695] selected btl: ugni
[nid00098:48694] selected btl: ugni
[nid00098:48695] allocating shared internal state
[nid00098:48694] allocating shared internal state
[nid00098:48694] registering segment with btl. range: 0x2aaab7c77008 - 0x2aaab7c77328 (800 bytes)
[nid00098:48694] failed to register pointer with selected BTL. base: 0x2aaab7c77008, size: 800. file: ../../../../../ompi/mca/osc/rdma/osc_rdma_component.c, line: 655
[nid00098:48694] failed to allocate internal state
[nid00098:48695] failed to allocate internal state
[nid00098:48694] rdma component destroying window with id 918
[nid00098:48695] rdma component destroying window with id 918
[nid00098:48694] *** An error occurred in MPI_Win_create
[nid00098:48694] *** reported by process [1228996609,0]
[nid00098:48694] *** on communicator MPI_COMM_WORLD
[nid00098:48694] *** MPI_ERR_WIN: invalid window
[nid00098:48694] *** MPI_ERRORS_ARE_FATAL (processes in this communicator will now abort,
[nid00098:48694] ***    and potentially your MPI job)
[nid00098:48657] 1 more process has sent help message help-mpi-errors.txt / mpi_errors_are_fatal
[nid00098:48657] Set MCA parameter "orte_base_help_aggregate" to 0 to see all help / error messages

1000 windows is very aggressive :)

[jeffhammond]

@jdinan added this one because the GA test suite includes a test that creates 1000 global arrays, which broke a version of MVAPICH2 many years ago. I can't say for sure that NWChem will ever create 1000 global arrays but it creates a nontrivial number and any MPI implementation that can't create at least a hundred global arrays is probably DOA for NWChem.

There are multiple reasons why per-window registration isn't a good idea, although it is certainly the simplest one. However, once I implement https://github.com/pmodels/armci-mpi/issues/16, the amortization will happen inside of ARMCI-MPI so one can ignore test failures related to lots of windows.

[hjelmn]

Per-window state registration is the only reasonable approach. I can somewhat reduce the number of registrations but unless you expect the implementation to waste memory because your could create a large number of windows you will use O(# of windows) registrations. On Cray the available number of registrations is rather small (2k) but a couple hundred windows is ok.

[jeffhammond]

@hjelmn I would allocate a 2M (single-page) slab for use by small windows...

[jeffhammond]

@hjelmn https://github.com/pmodels/armci-mpi/pull/22 is for you

[hppritcha]

fixes for this issue have been committed to 4.0.x hopefully

[jsquyres]

This has been fixed on all relevant branches. Closing.