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.

Sign up for GitHub

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

Jump to bottom

VR does not run in MPI+hydro mode #54

Closed
MatthieuSchaller opened this issue Dec 10, 2020 · 6 comments
Closed

VR does not run in MPI+hydro mode #54

MatthieuSchaller opened this issue Dec 10, 2020 · 6 comments

Comments

@MatthieuSchaller
Copy link

MatthieuSchaller commented Dec 10, 2020 •
edited

Describe the bug

Running VR with MPI switched on, OMP switched off and hydro switched on breaks on EAGLE boxes.

To Reproduce

  1. Compile the latest master (cb4336d) with VR_USE_HYDRO=ON, VR_MPI=ON and VR_OPENMP=OFF.
  2. Run mpirun -np 16 stf -C vrconfig_3dfof_subhalos_SO_hydro.cfg -i eagle_0036 -o halos_mpi_0036 -I 2

This is a standard XL snapshot with our standard config file.
The code segfaults after printing

[0000] [ 346.430] [ info] search.cxx:352 Finished linking across MPI domains in 1.258 [min]

somewhere in a MPI call trying to clear a vector of size 10^16 (!!).

The input can be found here if necessary: /snap7/scratch/dp004/jlvc76/SWIFT/EoS_tests/swiftsim/examples/EAGLE_ICs/EAGLE_25/eagle_0036.hdf5 and /snap7/scratch/dp004/jlvc76/SWIFT/EoS_tests/swiftsim/examples/EAGLE_ICs/EAGLE_25/vrconfig_3dfof_subhalos_SO_hydro.cfg.

The same setup works either:

  • On 1 MPI rank
  • Without VR_USE_HYDRO

Note that the snapshot is made of one single file if that is relevant.
@rtobar
Copy link

rtobar commented Dec 11, 2020

@MatthieuSchaller thanks for the report. I don't think I'll be able to look into this until early next week though. In the meanwhile, could you please share the data, or push for my cosma application to proceed? I tried to reproduce locally with some of the inputs I had received in the past but they seem to lack the fields needed by the hydro-enabled code.

@MatthieuSchaller
Copy link
Author

If it helps, there is smaller test case here:

  • /snap7/scratch/dp004/jlvc76/SWIFT/EoS_tests/swiftsim/examples/EAGLE_low_z/EAGLE_6/eagle_0000.hdf5
  • /snap7/scratch/dp004/jlvc76/SWIFT/EoS_tests/swiftsim/examples/EAGLE_low_z/EAGLE_6/vrconfig_3dfof_subhalos_SO_hydro.cfg

This one crashes about 20s after start so might be easier.

Config is: cmake ../ -DVR_USE_HYDRO=ON -DCMAKE_BUILD_TYPE=Debug
Run command line is: stf -C vrconfig_3dfof_subhalos_SO_hydro.cfg -i eagle_0000 -o halos_0000 -I 2

  • Problem happens with gcc or ICC,
  • Running with -DVR_OPENMP=OFF also crashes in the same way,
  • Running without VR_MPI_REDUCE crashes in a different way. There the crash happens when reading in stuff.

@rtobar
Copy link

rtobar commented Dec 16, 2020

Thanks @MatthieuSchaller for the extra information. I managed to reproduce this locally with the data you pointed above, I hope I can post an update soon with some information

rtobar added a commit that referenced this issue Dec 16, 2020
@rtobar
Reserve correct amount of memory
082ff68 
The MPI sendrecv operations happening a few lines below require
proprecvbuff to be the same size of the corresponding propsendbuff; that
is, numrecv * numextrafields. This became clear when running the program
under valgrind, and after inspecting it under a debugger.

This commit fixes the immediate problem. A separate one will be added to
add assertions on the sizes of the send buffers to add clarity to the
code.

This addresses #54.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar
Copy link

rtobar commented Dec 16, 2020

Running under valgrind revealed an invalid write:

==14074==    by 0x2D2CC6: MPISendReceiveFOFHydroInfoBetweenThreads(Options&, fofid_in*, std::vector<long long, std::allocator<long long> >&, std::vector<float, std::allocator<float> >&, int, int, ompi_communicator_t*&) (mpiroutines.cxx:2697)

The in gdb the reason became clear: the proprecvbuff vector needed to be properly sized before receiving data. Its size should be the same that propsendbuff, which in turn should be the size of indicessend times numextrafields.

On a new issue-54 branch I fixed the resizing of proprecvbuff to accommodate for all incoming data, and also added an assertion to make sure the sizes of the input vectors are related to each other as expected. After these changes I can run the code to completion. @MatthieuSchaller please confirm that this is working for you too.

I also tried out running without VR_MPI_REDUCE and also got a crash. That looks like a separate issue though, so I'll create a separate ticket to keep track of it separately.

@rtobar rtobar mentioned this issue Dec 16, 2020
Open
@MatthieuSchaller
Copy link
Author

I confirm this works both with and without OpenMP (as expected since unrelated). Thanks for tracking this down!

(I am never brave enough to fire up valgrind inside an mpirun call...)

@rtobar
Copy link

rtobar commented Dec 16, 2020

Great! I merged this into the master branch now, thanks for testing!

@rtobar rtobar closed this as completed Dec 16, 2020
@rtobar rtobar mentioned this issue Jun 15, 2021
Closed
rtobar added a commit that referenced this issue Jun 15, 2021
@rtobar
Fix and centralise indices/props exchange logic
7ed88c7 
The different routines exchanging extra information (gas, start, BH and
dark matter) during particle MPI exchange pass around two buffers,
indices and property data, whose sizes are related: for N indices there
are N * M properties. However most of the routines were flawed because
they allocated a property reception array of N elements, leading to
memory corruption.

Additionally, this problem affected all these routines (except one)
because the code performing these MPI operations was duplicated in all
of them.

This commit fixes these two problems in one go. Firstly, it adds a new
function to perform the data exchange that is then called by the four
original routines. Secondly, the new centralised version of the data
exchange code correctly sizes the input buffers to avoid memory
corruption.

This commit addresses the issue reported in #87. A similar situation had
been reported in #54, but at the time I hadn't realised this was a
problem affecting several similarly-looking functions, and therefore the
fix at the time (082ff68) was constrained only to the routine where the
problem was originally reported, leaving the rest with the problem still
to be fixed.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
rtobar added a commit that referenced this issue Jun 15, 2021
@rtobar
Fix and centralise indices/props exchange logic
8fe6623 
The different routines exchanging extra information (gas, start, BH and
dark matter) during particle MPI exchange pass around two buffers,
indices and property data, whose sizes are related: for N indices there
are N * M properties. However most of the routines were flawed because
they allocated a property reception array of N elements, leading to
memory corruption.

Additionally, this problem affected all these routines (except one)
because the code performing these MPI operations was duplicated in all
of them.

This commit fixes these two problems in one go. Firstly, it adds a new
function to perform the data exchange that is then called by the four
original routines. Secondly, the new centralised version of the data
exchange code correctly sizes the input buffers to avoid memory
corruption.

This commit addresses the issue reported in #87. A similar situation had
been reported in #54, but at the time I hadn't realised this was a
problem affecting several similarly-looking functions, and therefore the
fix at the time (082ff68) was constrained only to the routine where the
problem was originally reported, leaving the rest with the problem still
to be fixed.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
rtobar added a commit that referenced this issue Jun 15, 2021
@rtobar
Fix and centralise indices/props exchange logic
1fa1e1e 
The different routines exchanging extra information (gas, start, BH and
dark matter) during particle MPI exchange pass around two buffers,
indices and property data, whose sizes are related: for N indices there
are N * M properties. However most of the routines were flawed because
they allocated a property reception array of N elements, leading to
memory corruption.

Additionally, this problem affected all these routines (except one)
because the code performing these MPI operations was duplicated in all
of them.

This commit fixes these two problems in one go. Firstly, it adds a new
function to perform the data exchange that is then called by the four
original routines. Secondly, the new centralised version of the data
exchange code correctly sizes the input buffers to avoid memory
corruption.

This commit addresses the issue reported in #87. A similar situation had
been reported in #54, but at the time I hadn't realised this was a
problem affecting several similarly-looking functions, and therefore the
fix at the time (082ff68) was constrained only to the routine where the
problem was originally reported, leaving the rest with the problem still
to be fixed.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
@rtobar rtobar mentioned this issue Jun 15, 2021
Open
rtobar added a commit that referenced this issue Jun 18, 2021
@rtobar
Fix and centralise indices/props exchange logic
3b58872 
The different routines exchanging extra information (gas, start, BH and
dark matter) during particle MPI exchange pass around two buffers,
indices and property data, whose sizes are related: for N indices there
are N * M properties. However most of the routines were flawed because
they allocated a property reception array of N elements, leading to
memory corruption.

Additionally, this problem affected all these routines (except one)
because the code performing these MPI operations was duplicated in all
of them.

This commit fixes these two problems in one go. Firstly, it adds a new
function to perform the data exchange that is then called by the four
original routines. Secondly, the new centralised version of the data
exchange code correctly sizes the input buffers to avoid memory
corruption.

This commit addresses the issue reported in #87. A similar situation had
been reported in #54, but at the time I hadn't realised this was a
problem affecting several similarly-looking functions, and therefore the
fix at the time (082ff68) was constrained only to the routine where the
problem was originally reported, leaving the rest with the problem still
to be fixed.

Signed-off-by: Rodrigo Tobar <rtobar@icrar.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@rtobar @MatthieuSchaller