Skip to content

Conversation

@aa-g
Copy link
Contributor

@aa-g aa-g commented Jan 29, 2021

Proposed Changes

Replace references to the global MPI communicator MPI_COMM_WORLD with SU2_MPI::GetComm() where appropriate.

This prevents deadlocking in cases where SU2 is given an MPI communicator other than MPI_COMM_WORLD, for example when creating an instance of CDriver through the Python interface.

Related Work

Issue #1179

PR Checklist

  • I am submitting my contribution to the develop branch.
  • My contribution generates no new compiler warnings (try with the '-Wall -Wextra -Wno-unused-parameter -Wno-empty-body' compiler flags, or simply --warnlevel=2 when using meson).
  • My contribution is commented and consistent with SU2 style.
  • I have added a test case that demonstrates my contribution, if necessary.
  • I have updated appropriate documentation (Tutorials, Docs Page, config_template.cpp) , if necessary.

Copy link
Member

@pcarruscag pcarruscag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for opening the PR!

Delete line 470 of mpi_structure.hpp please.

@aa-g
Copy link
Contributor Author

aa-g commented Jan 29, 2021

Thank you for opening the PR!

Delete line 470 of mpi_structure.hpp please.

No worries. It took a while to get it going due to some issues on my side catching up with 7.1.0 (in particular, compiling against MPICH).

Regarding the deletion of the dummy MPI_COMM_WORLD from mpi_structure.hpp: the value is still referenced in several places, namely line 32 of mpi_structure.cpp, and in each of the SU2_CFD.cpp, SU2_DEF.cpp, SU2_DOT.cpp, SU2_GEO.cpp, SU2_SOL.cpp, where MPI_COMM_WORLD would be the default communicator.

@pcarruscag
Copy link
Member

Add a method to the MPI wrappers to get the global communicator or something equivalent, the only way to make this kind of change future-compatible is to break compilation.

@pcarruscag
Copy link
Member

I merged #1178 earlier and fixed the old and new conflicts, some MPI_COMM_WORLD's will have been re-introduced in CFVMFlowSolverBase.hpp and .inl because in my previous 2 PR's I moved a lot of code.

@aa-g
Copy link
Contributor Author

aa-g commented Feb 1, 2021

I've removed the dummy MPI_COMM_WORLD altogether now. To keep the changes minimal, I've just changed the way that CBaseMPIWrapper::currentComm is initialised in mpi_structure.cpp, rather than introducing a new method (which would only be used once, at this location).

Now, the CBaseMPIWrapper::currentComm is initialised to MPI_COMM_WORLD if compiling with MPI, otherwise it is set to the dummy value 0, consistent with the previous behaviour.

Comment on lines +35 to +40
/* Set the default MPI Communicator */
#ifdef HAVE_MPI
CBaseMPIWrapper::Comm CBaseMPIWrapper::currentComm = MPI_COMM_WORLD;
#else
CBaseMPIWrapper::Comm CBaseMPIWrapper::currentComm = 0; // dummy value
#endif
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this solution.
The rank and size are updated by the SetComm function so everything should work ok when creating a driver from scratch.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Glad to hear that. Is anything still missing ? Not sure how to go about the failing check for code complexity ...

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not for me, I'll ask around the developers meeting tomorrow if anyone else has comments, codefactor glitches out sometimes no need to do anything.

By the way would you like to be added to su2code? You can then have branches here instead of in your fork, makes it simpler to manage future PR's you might open.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I suppose it would be more convenient that way. Thanks !

@pcarruscag pcarruscag merged commit ea77689 into su2code:develop Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants