Tests for Teuchos::Comm unexpected behavior on waterman #3356
Conversation
mhoemmen
added
pkg: Teuchos
There was a problem hiding this comment.
I've convinced myself that Teuchos is doing the right thing, and that these are conforming MPI and C++ programs. See comments for a possible cause of the issue. I'm wondering if this could be a compiler bug related to order of destructor invocation at end of scope.
One work-around could be to reimplement Teuchos::DefaultComm<int>::getComm like this:
Teuchos::RCP<const Teuchos::Comm<OrdinalType>>
DefaultComm<OrdinalType>::getComm ()
{
static MPI_Comm theComm = MPI_COMM_WORLD;
return Teuchos::rcp (new Teuchos::MpiComm<OrdinalType> (&theComm), false);
} See C++ Standard, stmt.dcl (para 4 in the current draft N4762) and basic.start.term (para 1). The destructor of theComm doesn't get invoked until std::exit, so it's safe to refer to it in main().
| MPI_Comm commdup; | ||
|
|
||
| if (dupComm) | ||
| MPI_Comm_dup(comm, &commdup); |
There was a problem hiding this comment.
This test never calls MPI_Comm_free on commdup. This will likely leak memory and thus not be clean for Valgrind etc.
It's an interesting question whether this is actually a conforming MPI program. Section 8.7 of MPI 3.1 says that "[b]efore an MPI process invokes MPI_FINALIZE, the process must perform all MPI calls needed to complete its involvement in MPI communications..." (emphasis added). The next paragraph says, "The call to MPI_FINALIZE does not free objects created by MPI calls; these objects are freed using MPI_XXX_FREE calls." However, the Standard does not say anything like "failure to call MPI_COMM_FREE on a communicator created using one of the communicator constructors makes the program nonportable / have undefined behavior." (The Standard does say this of other things; see e.g., MPI_FILE_OPEN, Section 13.2.1.) The examples in the Standard do say things like "free communicators appropriately" (see e.g., Section 6.6.3, Example 1). However, examples are not normative (see Section 2.10). My guess is that the program is conforming and portable, but will leak memory as written.
| { | ||
| Teuchos::GlobalMPISession mpiSession(&narg,&arg); | ||
|
|
||
| Teuchos::RCP<const Teuchos::Comm<int> > |
There was a problem hiding this comment.
Teuchos::DefaultComm<int>::getComm() does something more than just return a wrapped MpiComm -- it schedules the resulting object for deletion at MPI_Finalize. See the implementation here. This means that any use of the result of this function after MPI_Finalize has been called is user error.
This code example is correct in standard C++, because destructor order is supposed to be the reverse of constructor order. (GlobalMPISession::~GlobalMPISession() calls MPI_Finalize.) However, it could be that some compiler bug on this platform somehow has allowed these destructors to be called out of order. That should be OK, since this code does not use comm before end of scope.
| Teuchos::ArrayView<const int> list(ids, np/2+1); | ||
| for (int i = 0; i < niter; i++) { | ||
| Teuchos::RCP<const Teuchos::Comm<int> > a | ||
| = comm->createSubcommunicator(list); |
There was a problem hiding this comment.
Both createSubcommunicator() and duplicate() create an RCP with a custom deleter. The deleter checks whether MPI_Finalize has been called before attempting to call MPI_Comm_free on the RCP's internal MPI_Comm. See the implementation here, and the implementation of safeCommFree here. The FIXME in the latter code should only apply if the result of createSubcommunicator() or duplicate() outlives the MPI_Finalize call, but see my note below about the MPI Standard not explicitly forbidding failure to call MPI_Comm_free.
| bool dupComm = (arg[1][0] == 'Y' ? true : false); | ||
|
|
||
| if (dupComm) | ||
| Teuchos::RCP<const Teuchos::Comm<int> > commdup = comm->duplicate(); |
There was a problem hiding this comment.
See above note. The duplicate() method behaves like createSubcommunicator().
|
Correction to my above comment: the work-around I presented above would violate the current invariant that The reason for the complicated current implementation, is that historically we have had trouble allowing RCPs to outlive The situation is worse, however. |
|
I added MPI_Comm_free to the MPI-only test. |
|
@kddevin I was perhaps a bit confused. I'll summarize -- please correct me if I'm wrong:
Is this right? If so, is |
|
The subcommunicator test was faulty; I updated it and it no longer hangs on waterman. Progress! |
|
There was a comment today that mpi on waterman is bad. We were told to back off to version 2.1.1 on that machine. @nmhamster can you comment? |
|
@mhoemmen your understanding is correct. Teuchos::MpiComm{size=4,rank=2,rawMpiComm=0x7fff98d608d0} Given @rppawlo 's comment, I will commit what I have and wait until @bartlettroscoe amd @fryeguy52 decide about the MPI version there. Once a new ATDM environment is available, we can resume this testing. |
|
@kddevin Thanks for investigating! I can't actually review this PR (since I made it), but if you want to merge the tests, please add a review. |
mhoemmen
removed
the
AT: WIP
mhoemmen
changed the title
|
Given the above discussion, I've removed the WIP label so this can go through PR testing. |
|
@rppawlo I think what is known to be bad is passing GPU memory pointers to the OpenMPI version 3 install on Waterman. This means we need to turn off any code that takes advantage of CUDA-aware MPI if using that install. Any bugs unrelated to CUDA-aware MPI would be news to me, and should definitely be explored. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: mhoemmen |
There was a problem hiding this comment.
For the most part, this PR adds new tests to Teuchos. These tests should pass without problem in the PR testing and most environments. Test TeuchosComm_waterman_teuchoscomm_with_comm_duplicate will segfault with the current ATDM configuration on waterman. This behavior is expected and is the purpose of this test. Merging this PR will enable the tests to be run easily on waterman.
|
@ibaned - The way Si put it this morning, it sounded much worse. He told us to change all trilinos testing to use openmpi 2.1.1 and don't use the version 3 stack at all. |
Yea, I talked with Si in more detail this afternoon and OpenMPI 3x is just really broken on Power9 systems. |
|
FYI: After PR #3363 is merged, we should rebase this branch on top of that and then run these tests on 'waterman' to see if they pass. |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.9.3 # 1434 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_gcc_4.8.4 # 1128 (click to expand)
Console Output (last 100 lines) : Trilinos_pullrequest_intel_17.0.1 # 677 (click to expand)
|
|
@ibaned @rppawlo @bartlettroscoe - thanks for update the issue. The way we are looking at this is that OpenMPI 3.X series will be the basis for IBM's Spectrum MPI offering moving forward. They take this code base and then add a suite of tuning and optimization for POWER. They also support their product on the machine and conduct lots of testing. So we are trying to align our POWER systems with OpenMPI 3.X to decrease porting gaps. But .. there is a problem with the Mellanox driver and OpenMPI 3.X on the POWER platform which has not been fixed yet. I would prefer to talk offline about that issue and how we are tracking it. On POWER8 (White/Ride), we do not have this issue and so OpenMPI 3.X is fine for use there. On POWER9 (Waterman), we have identified the problem and would recommend switching to OpenMPI 2.1.2 for now until we can rework the problem. Hope that helps. |
|
@nmhamster Thanks for the clarification! |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: mhoemmen |
I put normal values into the command-line arguments
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ kddevin ]! |
|
Status Flag 'Pull Request AutoTester' - AutoMerge IS ENABLED, but the Label AT: AUTOMERGE is not set. Either set Label AT: AUTOMERGE or manually merge the PR... |
bartlettroscoe
force-pushed
the
fix3331
branch
from
07a9e13 to
f2a68ca
Compare
|
Okay, I checked out the branch and it returned: So I guess that means that the OpenMPI 2.1.2 env does not have any problems since these TeuchosComm tests pass, right? I then did a forced push. Now this is a nice clean topic branch with one intersection with 'develop' (until it gets merged). The auto PR tester will need to test this again, but I will put the |
bartlettroscoe
added
the
AT: AUTOMERGE
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection Is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
Using Repos:
Pull Request Author: mhoemmen |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_pullrequest_gcc_4.9.3
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_gcc_4.8.4
Jenkins Parameters
Build InformationTest Name: Trilinos_pullrequest_intel_17.0.1
Jenkins Parameters
|
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ bartlettroscoe ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
|
Merge on Pull Request# 3356: IS A SUCCESS - Pull Request successfully merged |
trilinos-autotester
removed
the
AT: AUTOMERGE
|
Thank you, @bartlettroscoe |
@trilinos/teuchos @trilinos/zoltan2
I made this PR from @kddevin 's "fix3331" branch and did not otherwise modify the branch. Please see this comment on #3355 for details. This is a WIP; even if it passes PR testing, please do not merge it until the test failures on waterman (a GPU platform) have been fixed.