ch4/coll: Fix reduce composition alpha with non-zero root

[raffenet]

Pull Request Description

We need to handle the case where a non-zero root uses
MPI_IN_PLACE. Otherwise we could try reading from a bad address and
crash. Fixes #6540.

NOTE: For single node reduce operation with non-zero root, this
composition incurs an unnecessary extra copy.

Author Checklist

• Provide Description
  Particularly focus on why, not what. Reference background, issues, test failures, xfail entries, etc.
• Commits Follow Good Practice
  Commits are self-contained and do not do two things at once.
  Commit message is of the form: module: short description
  Commit message explains what's in the commit.
• Passes All Tests
  Whitespace checker. Warnings test. Additional tests via comments.
• Contribution Agreement
  For non-Argonne authors, check contribution agreement.
  If necessary, request an explicit comment from your companies PR approval manager.

[raffenet]

test:mpich/ch4/most
test:mpich/ch3/most

[raffenet]

This patch is wrong. The issue is the root rank is using MPI_IN_PLACE but an internal collective is treating the root as a regular sender rank and trying to copy from a bad address. Still working on a fix.

[raffenet]

test:mpich/ch4/most
test:mpich/ch3/most

[hzhou]

I was trying to enhance the test to cover this bug, with following patch:

diff --git a/test/mpi/coll/reduce.c b/test/mpi/coll/reduce.c
index 5c61a85150..97fcf104de 100644
--- a/test/mpi/coll/reduce.c
+++ b/test/mpi/coll/reduce.c
@@ -81,9 +81,25 @@ static int test_reduce(mtest_mem_type_e oddmem, mtest_mem_type_e evenmem)
                 MTestCopyContent(recvbuf_h, recvbuf, count * sizeof(int), memtype);

                 MPI_Reduce(sendbuf, recvbuf, count, MPI_INT, MPI_SUM, root, comm);
-                MTestCopyContent(recvbuf, recvbuf_h, count * sizeof(int), memtype);

                 if (rank == root) {
+                    MTestCopyContent(recvbuf, recvbuf_h, count * sizeof(int), memtype);
+                    check_buf(rank, size, count, &errs, recvbuf_h);
+                }
+
+                /* test again using MPI_IN_PLACE */
+                if (rank == root) {
+                    set_send_buf(count, recvbuf_h);
+                    MTestCopyContent(recvbuf_h, recvbuf, count * sizeof(int), memtype);
+                    MPI_Reduce(MPI_IN_PLACE, recvbuf, count, MPI_INT, MPI_SUM, root, comm);
+                } else {
+                    set_send_buf(count, sendbuf_h);
+                    MTestCopyContent(sendbuf_h, sendbuf, count * sizeof(int), memtype);
+                    MPI_Reduce(sendbuf, NULL, count, MPI_INT, MPI_SUM, root, comm);
+                }
+
+                if (rank == root) {
+                    MTestCopyContent(recvbuf, recvbuf_h, count * sizeof(int), memtype);
                     check_buf(rank, size, count, &errs, recvbuf_h);
                 }
             }

But strangely, the enhanced test passes even without your fix. It appears that the posix release gather algorithm works even with the wrong usage of MPI_IN_PLACE (on non-root). I have confirmed it segfaults if we set MPIR_CVAR_REDUCE_POSIX_INTRA_ALGORITHM=mpir. I would like to understand more clearly how the selection works, and fix the release gather algorithm (i.e. it should error out on the wrong usage of MPI_IN_PLACE).

[raffenet]

test:mpich/ch4/most

[raffenet]

But strangely, the enhanced test passes even without your fix. It appears that the posix release gather algorithm works even with the wrong usage of MPI_IN_PLACE (on non-root). I have confirmed it segfaults if we set MPIR_CVAR_REDUCE_POSIX_INTRA_ALGORITHM=mpir. I would like to understand more clearly how the selection works, and fix the release gather algorithm (i.e. it should error out on the wrong usage of MPI_IN_PLACE).

I was wondering why red3.c or red4.c didn't trigger the bug, but yeah, the selection is complicated. We could always add the reporter's reproducer with permission. The latest push is working with the example finally.

[hzhou]

I was wondering why red3.c or red4.c didn't trigger the bug

Both red3.c and red4.c are for non-commutative op. We need the coverage for commutative (built-in) op.

[raffenet]

@hzhou did you figure out a consistent test config for this one?

[hzhou]

I haven't got time yet. I'll approve the PR and add the test as future PRs.

[hzhou]

LGTM

TODO: add test as future PRs

[hzhou]

I found why the release_gather code was immune to the bug. It is due to

mpich/src/mpid/ch4/shm/posix/release_gather/nb_reduce_release_gather.h

Lines 397 to 399 in 8451885

	if (send_buf == MPI_IN_PLACE) {
	send_buf = recv_buf;
	}

. Essentially it was doing the same recvbuf replacement inside the algorithm.

In general, are we allowed to have sendbuf and recvbuf overlap? Apparently all our reduce algorithms works with this replacement, but I wonder whether this is a potential hazard.