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

Skip empty particle tiles in operator++ to avoid race condition in constructor. #3951

Merged
merged 7 commits into from
Jun 2, 2024
Merged

Skip empty particle tiles in operator++ to avoid race condition in constructor. #3951

merged 7 commits into from
Jun 2, 2024

Conversation

atmyers
Copy link
Member

@atmyers atmyers commented May 22, 2024 •
edited
Loading

The race condition was found by @lucafedeli88 using an automated tool.

Summary

Additional background

Checklist

The proposed changes:

  • fix a bug or incorrect behavior in AMReX
  • add new capabilities to AMReX
  • changes answers in the test suite to more than roundoff level
  • are likely to significantly affect the results of downstream AMReX users
  • include documentation in the code and/or rst files, if appropriate

@atmyers atmyers marked this pull request as ready for review May 22, 2024 18:30
@lucafedeli88 lucafedeli88 mentioned this pull request May 23, 2024
Closed
@lucafedeli88
Copy link
Contributor

@atmyers , thanks a lot for working on this!
For the moment I've left just one comment. I am currently trying to test your PR with the thread sanitizer to see if it fixes all the issues.

@WeiqunZhang
Copy link
Member

I think we can simplify ParIter by getting rid of m_pariter_index, m_valid_index and m_particle_tiles and adding ParticleTilePtr m_particle_current_tile = nullptr;. As for opeator++, we don't need separate versions from OMP. It can be something like

    void operator++ ()
    {
        m_particle_current_tile = nullptr;
        auto const& particles = m_pc.GetParticles(level);
        do
        {
            MFIter::operator++();
            auto key = std::make_pair(index(), LocalTileIndex());
            auto f = particles.find(key);
            if (f != particles.end() && f->second.numParticles() > 0) {
                m_particle_current_tile = &(f->second);
            }
        }
        while (isValid() && m_particle_current_tile == nullptr);
    }

(Code not tested.)

@WeiqunZhang
Copy link
Member

The code snippet I posted has a bug. Cannot call index() if it's not valid. So maybe it's easier to just use while (true) loop and break if it's either invalid or a non-empty tile is found.

@lucafedeli88
Copy link
Contributor

The thread sanitizer test seems to still see an issue:
https://github.com/ECP-WarpX/WarpX/actions/runs/9210693739/job/25338291461
I compiled WarpX using this branch of AMReX: AMReX repository: https://github.com/atmyers/amrex.git (0cf6d9b) and I still get something like:

WARNING: ThreadSanitizer: data race (pid=14679)
  Read of size 8 at 0x7b0c0001d428 by thread T3:
    #0 std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::size() const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:1034:24 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562283) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #1 std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::_M_get_insert_hint_unique_pos(std::_Rb_tree_const_iterator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::pair<int, int> const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:2220:8 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562283)
    #2 std::_Rb_tree_iterator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>> std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::_M_emplace_hint_unique<std::piecewise_construct_t const&, std::tuple<std::pair<int, int>&&>, std::tuple<>>(std::_Rb_tree_const_iterator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::piecewise_construct_t const&, std::tuple<std::pair<int, int>&&>&&, std::tuple<>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:2463:15 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x56277a) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #3 std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::operator[](std::pair<int, int>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_map.h:532:15 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c24c) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #4 amrex::ParticleContainer_impl<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator, amrex::DefaultAssignor>::DefineAndReturnParticleTile(int, int, int) /__w/WarpX/WarpX/build/_deps/fetchedamrex-src/Src/Particle/AMReX_ParticleContainer.H:1147:9 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c24c)
    #5 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) (.omp_outlined_debug__) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:399:61 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5540d2) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #6 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) (.omp_outlined) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:392:1 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5540d2)
    #7 __kmp_invoke_microtask <null> (libomp.so.5+0xdbb82) (BuildId: bc9f8970ab3c208335f64564550b062c0d9eb665)
    #8 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:392:26 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x553d75) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)

  Previous write of size 8 at 0x7b0c0001d428 by main thread:
    #0 std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::_M_insert_node(std::_Rb_tree_node_base*, std::_Rb_tree_node_base*, std::_Rb_tree_node<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>*) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:2387:7 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562822) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #1 std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::_Auto_node::_M_insert(std::pair<std::_Rb_tree_node_base*, std::_Rb_tree_node_base*>) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:1657:21 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562822)
    #2 std::_Rb_tree_iterator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>> std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::_M_emplace_hint_unique<std::piecewise_construct_t const&, std::tu==================
WARNING: ThreadSanitizer: data race (pid=14678)
  Read of size 4 at 0x7b5400007fa0 by thread T3:
    #0 bool std::operator<<int, int>(std::pair<int, int> const&, std::pair<int, int> const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_pair.h:836:18 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c1d0) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #1 std::less<std::pair<int, int>>::operator()(std::pair<int, int> const&, std::pair<int, int> const&) const /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_function.h:408:20 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c1d0)
    #2 std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::_M_lower_bound(std::_Rb_tree_node<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>*, std::_Rb_tree_node_base*, std::pair<int, int> const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:1952:7 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c1d0)
    #3 std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::lower_bound(std::pair<int, int> const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:1271:16 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c1d0)
    #4 std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::lower_bound(std::pair<int, int> const&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_map.h:1309:21 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c1d0)
    #5 std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::operator[](std::pair<int, int>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_map.h:529:17 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c1d0)
    #6 amrex::ParticleContainer_impl<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator, amrex::DefaultAssignor>::DefineAndReturnParticleTile(int, int, int) /__w/WarpX/WarpX/build/_deps/fetchedamrex-src/Src/Particle/AMReX_ParticleContainer.H:1147:9 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c1d0)
    #7 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) (.omp_outlined_debug__) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:399:61 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5540d2) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #8 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) (.omp_outlined) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:392:1 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5540d2)
    #9 __kmp_invoke_microtask <null> (libomp.so.5+0xdbb82) (BuildId: bc9f8970ab3c208335f64564550b062c0d9eb6ple<std::pair<int, int>&&>, std::tuple<>>(std::_Rb_tree_const_iterator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::piecewise_construct_t const&, std::tuple<std::pair<int, int>&&>&&, std::tuple<>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:2465:15 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562822)
    #3 std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::operator[](std::pair<int, int>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_map.h:532:15 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c24c) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #4 amrex::ParticleContainer_impl<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator, amrex::DefaultAssignor>::DefineAndReturnParticleTile(int, int, int) /__w/WarpX/WarpX/build/_deps/fetchedamrex-src/Src/Particle/AMReX_ParticleContainer.H:1147:9 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x55c24c)
    #5 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) (.omp_outlined_debug__) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:399:61 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5540d2) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #6 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) (.omp_outlined) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:392:1 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5540d2)
    #7 __kmp_invoke_microtask <null> (libomp.so.5+0xdbb82) (BuildId: bc9f8970ab3c208335f64564550b062c0d9eb665)
    #8 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:392:26 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x553d75) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #9 WarpX::HandleParticlesAtBoundaries(int, float, int) /__w/WarpX/WarpX/Source/Evolve/WarpXEvolve.cpp:492:33 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x1849ba) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #10 WarpX::Evolve(int) /__w/WarpX/WarpX/Source/Evolve/WarpXEvolve.cpp:228:9 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x1807ff) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #11 main /__w/WarpX/WarpX/Source/main.cpp:40:15 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x139114) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #12 main /__w/WarpX/WarpX/Source/main.cpp:26:5 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x13909d) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)

  Location is heap block of size 48 at 0x7b0c0001d400 allocated by main thread:
    #0 operator new(unsigned long) <null> (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x13811b) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #1 std::__new_allocator<std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>>::allocate(unsigned long, void const*) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/new_allocator.h:147:27 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5573aa) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #2 std::allocator_traits<std::allocator<std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>>>::allocate(std::allocator<std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>>&, unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/alloc_traits.h:482:20 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5573aa)
    #3 std::_Vector_base<std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>, std::allocator<std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>>>::_M_allocate(unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_vector.h:378:20 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5573aa)
    #4 std::vector<std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>, std::allocator<std::map<std::pair<int, int>, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>>>::reserve(unsigned long) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/vector.tcc:79:22 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5573aa)
    #5 amrex::ParticleContainer_impl<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator, amrex::DefaultAssignor>::reserveData() /__w/WarpX/WarpX/build/_deps/fetchedamrex-src/Src/Particle/AMReX_ParticleContainerI.H:299:17 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5659b7) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #6 amrex::ParticleContainer_impl<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator, amrex::DefaultAssignor>::ParticleContainer_impl(amrex::ParGDBBase*) /__w/WarpX/WarpX/build/_deps/fetchedamrex-src/Src/Particle/AMReX_ParticleContainer.H:221:33 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x5659b7)
    #7 amrex::ParticleContainer_impl<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator, amrex::DefaultAssignor> amrex::ParticleContainer_impl<amrex::SoAParticle<6, 0>, 6, 0, amrex::DefaultAllocator, amrex::DefaultAssignor>::make_alike<amrex::PinnedArenaAllocator>() const /__w/WarpX/WarpX/build/_deps/fetchedamrex-src/Src/Particle/AMReX_ParticleContainer.H:1334:37 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x563a2d) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #8 NamedComponentParticleContainer<amrex::PinnedArenaAllocator> NamedComponentParticleContainer<amrex::DefaultAllocator>::make_alike<amrex::PinnedArenaAllocator>() const /__w/WarpX/WarpX/Source/Particles/NamedComponentParticleContainer.H:143:87 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x563a2d)
    #9 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:380:36 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x553942) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #10 WarpX::HandleParticlesAtBoundaries(int, float, int) /__w/WarpX/WarpX/Source/Evolve/WarpXEvolve.cpp:492:33 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x1849ba) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #11 WarpX::Evolve(int) /__w/WarpX/WarpX/Source/Evolve/WarpXEvolve.cpp:228:9 (warpx.2d.MPI.OMP.SP65)
    #10 ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries(MultiParticleContainer&) /__w/WarpX/WarpX/Source/Particles/ParticleBoundaryBuffer.cpp:392:26 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x553d75) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)

  Previous write of size 8 at 0x7b5400007fa0 by main thread:
    #0 std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>::pair<std::pair<int, int>&&, 0ul>(std::tuple<std::pair<int, int>&&>&, std::tuple<>&, std::_Index_tuple<0ul>, std::_Index_tuple<>) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/tuple:2253:9 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562750) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #1 std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>::pair<std::pair<int, int>&&>(std::piecewise_construct_t, std::tuple<std::pair<int, int>&&>, std::tuple<>) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/tuple:2241:9 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562750)
    #2 void std::__new_allocator<std::_Rb_tree_node<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::construct<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::piecewise_construct_t const&, std::tuple<std::pair<int, int>&&>, std::tuple<>>(std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>*, std::piecewise_construct_t const&, std::tuple<std::pair<int, int>&&>&&, std::tuple<>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/new_allocator.h:187:23 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562750)
    #3 void std::allocator_traits<std::allocator<std::_Rb_tree_node<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>>::construct<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::piecewise_construct_t const&, std::tuple<std::pair<int, int>&&>, std::tuple<>>(std::allocator<std::_Rb_tree_node<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>&, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>*, std::piecewise_construct_t const&, std::tuple<std::pair<int, int>&&>&&, std::tuple<>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/alloc_traits.h:537:8 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562750)
    #4 void std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::_M_construct_node<std::piecewise_construct_t const&, std::tuple<std::pair<int, int>&&>, std::tuple<>>(std::_Rb_tree_node<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>*, std::piecewise_construct_t const&, std::tuple<std::pair<int, int>&&>&&, std::tuple<>&&) /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:597:8 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x562750)
    #5 std::_Rb_tree_node<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>* std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaA.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x1807ff) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #12 main /__w/WarpX/WarpX/Source/main.cpp:40:15 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x139114) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #13 main /__w/WarpX/WarpX/Source/main.cpp:26:5 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x13909d) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)

  Thread T3 (tid=14686, running) created by main thread at:
    #0 pthread_create <null> (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0xaeebf) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #1 <null> <null> (libomp.so.5+0xb3eb3) (BuildId: bc9f8970ab3c208335f64564550b062c0d9eb665)
    #2 amrex::Initialize(int&, char**&, bool, ompi_communicator_t*, std::function<void ()> const&, std::ostream&, std::ostream&, void (*)(char const*)) /__w/WarpX/WarpX/build/_deps/fetchedamrex-src/Src/Base/AMReX.cpp:620:5 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x8263de) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #3 amrex::Initialize(int&, char**&, bool, ompi_communicator_t*, std::function<void ()> const&, std::ostream&, std::ostream&, void (*)(char const*)) /__w/WarpX/WarpX/build/_deps/fetchedamrex-src/Src/Base/AMReX.cpp:465:5 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x825aad) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #4 warpx::initialization::amrex_init(int&, char**&, bool) /__w/WarpX/WarpX/Source/Initialization/WarpXAMReXInit.cpp:70:16 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x28a24e) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)
    #5 main /__w/WarpX/WarpX/Source/main.cpp:26:5 (warpx.2d.MPI.OMP.SP.PSP.OPMD.PSATD.QED.GENQEDTABLES+0x13909d) (BuildId: f470761fd4dda67d3dbc566a37c719fc89b705e2)

SUMMARY: ThreadSanitizer: data race /usr/bin/../lib/gcc/x86_64-linux-gnu/13/../../../../include/c++/13/bits/stl_tree.h:1034:24 in std::_Rb_tree<std::pair<int, int>, std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>, std::_Select1st<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>, std::less<std::pair<int, int>>, std::allocator<std::pair<std::pair<int, int> const, amrex::ParticleTile<amrex::SoAParticle<6, 0>, 6, 0, amrex::PinnedArenaAllocator>>>>::size() const

@WeiqunZhang
Copy link
Member

I think this is a different issue from the issue in ParIter.

@WeiqunZhang
Copy link
Member

Re: operator++, this might work

    void operator++ ()
    {
        m_particle_current_tile = nullptr;
        auto& particles = m_pc->GetParticles(m_level);
        while (true)
        {
            MFIter::operator++();
            if (isValid()) {
                auto key = std::make_pair(index(), LocalTileIndex());
                auto f = particles.find(key);
                if (f != particles.end() && f->second.numParticles() > 0) {
                    m_particle_current_tile = &(f->second);
                    break;
                }
            } else {
                break;
            }
        }
    }

@WeiqunZhang
Copy link
Member

Re: the new issue reported by @lucafedeli88, I think the change needs to be made in WarpX like below. The root issue is DefineAndReturnParticleTile is not thread safe.

diff --git a/Source/Particles/ParticleBoundaryBuffer.cpp b/Source/Particles/ParticleBoundaryBuffer.cpp
index 345a0bfa5..9fbc332d4 100644
--- a/Source/Particles/ParticleBoundaryBuffer.cpp
+++ b/Source/Particles/ParticleBoundaryBuffer.cpp
@@ -387,17 +387,22 @@ void ParticleBoundaryBuffer::gatherParticlesFromDomainBoundaries (MultiParticleC
                 auto& species_buffer = buffer[i];
                 for (int lev = 0; lev < pc.numLevels(); ++lev)
                 {
+                    for (PIter pti(pc, lev); pti.isValid(); ++pti) {
+                        species_buffer.DefineAndReturnParticleTile(
+                            lev, pti.index(), pti.LocalTileIndex());
+                    }
+
                     const auto& plevel = pc.GetParticles(lev);
+
 #ifdef AMREX_USE_OMP
 #pragma omp parallel if (amrex::Gpu::notInLaunchRegion())
 #endif
                     for(PIter pti(pc, lev); pti.isValid(); ++pti)
                     {
                         auto index = std::make_pair(pti.index(), pti.LocalTileIndex());
-                        if(plevel.find(index) == plevel.end()) { continue; }
 
-                        auto& ptile_buffer = species_buffer.DefineAndReturnParticleTile(
-                                                        lev, pti.index(), pti.LocalTileIndex());
+                        auto& ptile_buffer = species_buffer.ParticlesAt
+                            (lev, pti.index(), pti.LocalTileIndex());
                         const auto& ptile = plevel.at(index);
                         auto np = ptile.numParticles();
                         if (np == 0) { continue; }

@WeiqunZhang
Copy link
Member

@atmyers My suggestion on operator++ has a flaw. So it will not work. However, the flaw also exist in this PR. Suppose there are four tiles and a single box. Only one tile has particles. We have 4 threads. In the development branch, only one thread (say thread 0) will enter the loop body and the other three threads will fail isValid as expected. But in this PR, because operator++ is run after at least going through one loop iteration, three threads will execute loop body with no particles. I think we need to modify the isValid test too.

@WeiqunZhang
Copy link
Member

Using this amrex branch https://github.com/WeiqunZhang/amrex/tree/fix_part_race_condition and this WarpX branch https://github.com/WeiqunZhang/WarpX/tree/test_fix_part_race_condition, the thread sanitizer no longer fails with race condition errors. It does fail eventually with MLMG failure below

2024-05-24T20:16:21.1393577Z STEP 2 starts ...
2024-05-24T20:16:21.2574426Z MLMG: # of AMR levels: 1
2024-05-24T20:16:21.2575045Z       # of MG levels on the coarsest AMR level: 7
2024-05-24T20:16:21.2593382Z MLMG: Initial rhs               = 796188480
2024-05-24T20:16:21.2594020Z MLMG: Initial residual (resid0) = 437492352
2024-05-24T20:16:21.2941102Z MLMG: Iteration   1 Fine resid/bnorm = 0.001869145315
2024-05-24T20:16:21.3261483Z MLMG: Iteration   2 Fine resid/bnorm = 9.022988706e-06
2024-05-24T20:16:21.3589663Z MLMG: Iteration   3 Fine resid/bnorm = 1.40670204e-06
2024-05-24T20:16:21.3922571Z MLMG: Iteration   4 Fine resid/bnorm = 1.848808438e-06
2024-05-24T20:16:21.4252564Z MLMG: Iteration   5 Fine resid/bnorm = 2.009574473e-06
2024-05-24T20:16:21.4576628Z MLMG: Iteration   6 Fine resid/bnorm = 1.446893634e-06
2024-05-24T20:16:21.4913793Z MLMG: Iteration   7 Fine resid/bnorm = 1.567468075e-06
2024-05-24T20:16:21.5249652Z MLMG: Iteration   8 Fine resid/bnorm = 1.446893634e-06
2024-05-24T20:16:21.5587989Z MLMG: Iteration   9 Fine resid/bnorm = 1.637803166e-06
2024-05-24T20:16:21.5927009Z MLMG: Iteration  10 Fine resid/bnorm = 1.40670204e-06
2024-05-24T20:16:21.6263505Z MLMG: Iteration  11 Fine resid/bnorm = 1.728233997e-06
2024-05-24T20:16:21.6588959Z MLMG: Iteration  12 Fine resid/bnorm = 1.326319079e-06
2024-05-24T20:16:21.6919761Z MLMG: Iteration  13 Fine resid/bnorm = 1.40670204e-06
2024-05-24T20:16:21.7255194Z MLMG: Iteration  14 Fine resid/bnorm = 1.326319079e-06
...
2024-05-24T20:16:27.7558465Z MLMG: Iteration 195 Fine resid/bnorm = 1.36651056e-06
2024-05-24T20:16:27.7896107Z MLMG: Iteration 196 Fine resid/bnorm = 1.36651056e-06
2024-05-24T20:16:27.8216529Z MLMG: Iteration 197 Fine resid/bnorm = 1.286127599e-06
2024-05-24T20:16:27.8545770Z MLMG: Iteration 198 Fine resid/bnorm = 1.173089117e-06
2024-05-24T20:16:27.8874770Z MLMG: Iteration 199 Fine resid/bnorm = 1.537324465e-06
2024-05-24T20:16:27.9221741Z amrex::Abort::1::MLMG failed. !!!
2024-05-24T20:16:27.9222623Z MLMG: Iteration 200 Fine resid/bnorm = 1.36651056e-06
2024-05-24T20:16:27.9223841Z SIGABRT
2024-05-24T20:16:27.9225693Z MLMG: Failed to converge after 200 iterations. resid, resid/bnorm = 1088, 1.36651056e-06
2024-05-24T20:16:27.9237822Z amrex::Abort::0::MLMG failed. !!!

But this is probably a different issue. Maybe the tolerance needs to be loosened.

@WeiqunZhang
Copy link
Member

@atmyers My suggestion on operator++ has a flaw. So it will not work. However, the flaw also exist in this PR. Suppose there are four tiles and a single box. Only one tile has particles. We have 4 threads. In the development branch, only one thread (say thread 0) will enter the loop body and the other three threads will fail isValid as expected. But in this PR, because operator++ is run after at least going through one loop iteration, three threads will execute loop body with no particles. I think we need to modify the isValid test too.

No need to overload isValid. What we need is to call MFIter::opearator++ in the ctor to make sure it advances to the first non-empty particle tile. See my branch. Note that MFIter::operator++ is thread safe.

@WeiqunZhang
Copy link
Member

Let's not merge this until after the next release.

@WeiqunZhang WeiqunZhang merged commit f138787 into AMReX-Codes:development Jun 2, 2024
68 of 69 checks passed
@lucafedeli88 lucafedeli88 mentioned this pull request Jun 2, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lucafedeli88 lucafedeli88 lucafedeli88 left review comments

@WeiqunZhang WeiqunZhang WeiqunZhang approved these changes

Assignees
No one assigned
Labels
do not merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@atmyers @lucafedeli88 @WeiqunZhang