Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Set default window type to non-shared static #639

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

devreal
Copy link
Member

@devreal devreal commented Mar 25, 2019

This PR changes the build scripts and CMakeList to configure DART to use regular windows instead of dynamic + shared memory windows in dart_team_memalloc_aligned.

Background:
The use of dynamic windows causes all sorts of troubles:

  1. It prevents the use of RDMA capabilities on any decent hardware.
  2. Open MPI chokes if MPI_THREAD_MULTIPLE is enabled and no RDMA-capable network is found (thanks to @bertwesarg for bringing this back up) because the shared-memory component does not support dynamic windows (how could it?) and the pt2pt component lacks support for MPI_THREAD_MULTIPLE (not sure this will ever be fixed). Hence, DASH won't even work on a single node.

The MPI implementation will likely short-cut through shared memory for on-node communication so the benefit of doing it ourselves really seems marginal. On the other hand, changing the default avoids some bad surprises.

@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #639 into development will decrease coverage by 0.02%.
The diff coverage is n/a.

@@               Coverage Diff               @@
##           development     #639      +/-   ##
===============================================
- Coverage        84.94%   84.92%   -0.03%     
===============================================
  Files              335      335              
  Lines            24821    24821              
  Branches         11236    11709     +473     
===============================================
- Hits             21085    21080       -5     
  Misses            3733     3733              
- Partials             3        8       +5
Impacted Files Coverage Δ
dash/include/dash/Team.h 87.36% <0%> (-5.27%) ⬇️

Copy link
Member

@bertwesarg bertwesarg left a comment

Choose a reason for hiding this comment

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

Could mention somewhere, why shared and dynamic windows are off by default. Not that someone thinks, better enable it.

@bertwesarg
Copy link
Member

I would not consider the build*.sh scripts a good place to document CMake variables. why not inside CMakeLists.txt into the options()?

Btw, does other combinations instead of off&off or on&on make sense too?

Copy link

@rkowalewski rkowalewski left a comment

Choose a reason for hiding this comment

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

LGTM

@devreal
Copy link
Member Author

devreal commented Mar 27, 2019

I would not consider the build*.sh scripts a good place to document CMake variables. why not inside CMakeLists.txt into the options()?

We can move it to the options in the CMake file. So far these descriptions are kept quite short though and the description of these variables would have to be split into two.

Btw, does other combinations instead of off&off or on&on make sense too?

There is one case where dynamic windows without shared windows make sense. That is related to #615: dynamic windows do not register the memory and thus don't fault the pages, leading to better NUMA behavior. There are also some MPI implementations that don't really do well with shared memory window IIRC. So as long as we don't have the NUMA issue under control I'd leave the two options separate but we should consider merging them at some point.

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.

3 participants