From 6a046487fbc7153826e8babb75ed1c9c37641c0b Mon Sep 17 00:00:00 2001 From: Arnaud Giersch Date: Thu, 3 Jun 2021 11:28:56 +0200 Subject: [PATCH] Make smpi_switch_data_segment check if a switch is needed, and return true when it occurs. Kill global SMPI_switch_data_segment. --- include/smpi/smpi.h | 1 - src/kernel/actor/ActorImpl.cpp | 8 ++--- src/kernel/actor/ActorImpl.hpp | 2 -- src/simix/smx_global.cpp | 2 -- src/smpi/include/private.hpp | 4 ++- src/smpi/internals/smpi_actor.cpp | 4 +-- src/smpi/internals/smpi_bench.cpp | 4 +-- src/smpi/internals/smpi_config.cpp | 50 +++++++++++++++--------------- src/smpi/internals/smpi_global.cpp | 17 +++------- src/smpi/internals/smpi_memory.cpp | 23 ++++++++++---- src/smpi/mpi/smpi_comm.cpp | 26 ++++++---------- src/smpi/mpi/smpi_datatype.cpp | 5 ++- src/smpi/mpi/smpi_op.cpp | 7 ++--- src/smpi/mpi/smpi_request.cpp | 13 ++------ 14 files changed, 73 insertions(+), 93 deletions(-) diff --git a/include/smpi/smpi.h b/include/smpi/smpi.h index f9cba1c87b..69d1169c0b 100644 --- a/include/smpi/smpi.h +++ b/include/smpi/smpi.h @@ -1141,7 +1141,6 @@ XBT_PUBLIC MPI_Comm smpi_process_comm_self(); XBT_PUBLIC MPI_Info smpi_process_info_env(); XBT_PUBLIC void* smpi_process_get_user_data(); XBT_PUBLIC void smpi_process_set_user_data(void*); -XBT_PUBLIC void smpi_init_options(); XBT_PUBLIC void smpi_execute_flops(double flops); XBT_PUBLIC void smpi_execute_flops_benched(double flops); diff --git a/src/kernel/actor/ActorImpl.cpp b/src/kernel/actor/ActorImpl.cpp index 78f5be79ab..c70843fee1 100644 --- a/src/kernel/actor/ActorImpl.cpp +++ b/src/kernel/actor/ActorImpl.cpp @@ -3,9 +3,9 @@ /* This program is free software; you can redistribute it and/or modify it * under the terms of the license (GNU LGPL) which comes with this package. */ +#include "simgrid/s4u/Actor.hpp" #include "mc/mc.h" #include "simgrid/Exception.hpp" -#include "simgrid/s4u/Actor.hpp" #include "simgrid/s4u/Exec.hpp" #include "src/kernel/EngineImpl.hpp" #include "src/kernel/activity/CommImpl.hpp" @@ -16,6 +16,7 @@ #include "src/mc/mc_replay.hpp" #include "src/mc/remote/AppSide.hpp" #include "src/simix/smx_private.hpp" +#include "src/smpi/include/private.hpp" #include "src/surf/HostImpl.hpp" #include "src/surf/cpu_interface.hpp" @@ -307,9 +308,8 @@ void ActorImpl::yield() } } - if (SMPI_switch_data_segment && not finished_) { - SMPI_switch_data_segment(get_iface()); - } + if (not finished_) + smpi_switch_data_segment(get_iface()); } /** This actor will be terminated automatically when the last non-daemon actor finishes */ diff --git a/src/kernel/actor/ActorImpl.hpp b/src/kernel/actor/ActorImpl.hpp index 289e3aedd8..b8fc8ac3b8 100644 --- a/src/kernel/actor/ActorImpl.hpp +++ b/src/kernel/actor/ActorImpl.hpp @@ -205,6 +205,4 @@ XBT_PUBLIC unsigned long* get_maxpid_addr(); // In MC mode, the application send } // namespace kernel } // namespace simgrid -extern void (*SMPI_switch_data_segment)(simgrid::s4u::ActorPtr actor); - #endif diff --git a/src/simix/smx_global.cpp b/src/simix/smx_global.cpp index 4bc14b56e4..21a2e63f72 100644 --- a/src/simix/smx_global.cpp +++ b/src/simix/smx_global.cpp @@ -29,8 +29,6 @@ XBT_LOG_NEW_DEFAULT_SUBCATEGORY(simix_kernel, simix, "Logging specific to SIMIX std::unique_ptr simix_global; -void (*SMPI_switch_data_segment)(simgrid::s4u::ActorPtr) = nullptr; - namespace simgrid { namespace simix { config::Flag cfg_verbose_exit{"debug/verbose-exit", "Display the actor status at exit", true}; diff --git a/src/smpi/include/private.hpp b/src/smpi/include/private.hpp index 4e56553aec..8c5eb133fc 100644 --- a/src/smpi/include/private.hpp +++ b/src/smpi/include/private.hpp @@ -115,10 +115,12 @@ XBT_PRIVATE double smpi_cfg_auto_shared_malloc_thresh(); XBT_PRIVATE bool smpi_cfg_display_alloc(); // utilities +XBT_PUBLIC void smpi_init_options(bool called_by_smpimain = false); + extern XBT_PRIVATE char* smpi_data_exe_start; // start of the data+bss segment of the executable extern XBT_PRIVATE size_t smpi_data_exe_size; // size of the data+bss segment of the executable -XBT_PRIVATE void smpi_switch_data_segment(simgrid::s4u::ActorPtr actor); +XBT_PRIVATE bool smpi_switch_data_segment(simgrid::s4u::ActorPtr actor, const void* addr = nullptr); XBT_PRIVATE void smpi_prepare_global_memory_segment(); XBT_PRIVATE void smpi_backup_global_memory_segment(); diff --git a/src/smpi/internals/smpi_actor.cpp b/src/smpi/internals/smpi_actor.cpp index bdb72ce3d9..616c8a0c90 100644 --- a/src/smpi/internals/smpi_actor.cpp +++ b/src/smpi/internals/smpi_actor.cpp @@ -217,7 +217,7 @@ void ActorExt::init() "Aborting, please check compilation process and use smpirun."); simgrid::s4u::Actor* self = simgrid::s4u::Actor::self(); - // cheinrich: I'm not sure what the impact of the SMPI_switch_data_segment on this call is. I moved + // cheinrich: I'm not sure what the impact of the smpi_switch_data_segment on this call is. I moved // this up here so that I can set the privatized region before the switch. ActorExt* ext = smpi_process(); // if we are in MPI_Init and argc handling has already been done. @@ -228,7 +228,7 @@ void ActorExt::init() /* Now using the segment index of this process */ ext->set_privatized_region(smpi_init_global_memory_segment_process()); /* Done at the process's creation */ - SMPI_switch_data_segment(self); + smpi_switch_data_segment(self); } ext->instance_id_ = self->get_property("instance_id"); diff --git a/src/smpi/internals/smpi_bench.cpp b/src/smpi/internals/smpi_bench.cpp index a213c7f68b..86b297f661 100644 --- a/src/smpi/internals/smpi_bench.cpp +++ b/src/smpi/internals/smpi_bench.cpp @@ -78,9 +78,7 @@ void smpi_execute_flops_benched(double flops) { void smpi_bench_begin() { - if (smpi_cfg_privatization() == SmpiPrivStrategies::MMAP) { - smpi_switch_data_segment(simgrid::s4u::Actor::self()); - } + smpi_switch_data_segment(simgrid::s4u::Actor::self()); if (MC_is_active() || MC_record_replay_is_active()) return; diff --git a/src/smpi/internals/smpi_config.cpp b/src/smpi/internals/smpi_config.cpp index 7d640a1d72..9a4fe2efbe 100644 --- a/src/smpi/internals/smpi_config.cpp +++ b/src/smpi/internals/smpi_config.cpp @@ -187,7 +187,8 @@ double smpi_cfg_auto_shared_malloc_thresh(){ return _smpi_cfg_auto_shared_malloc_thresh; } -void smpi_init_options(){ +void smpi_init_options(bool called_by_smpimain) +{ // return if already called if(_smpi_options_initialized) return; @@ -212,30 +213,29 @@ void smpi_init_options(){ if (default_privatization == nullptr) default_privatization = "no"; - - simgrid::config::declare_flag( "smpi/privatization", - "How we should privatize global variable at runtime (no, yes, mmap, dlopen).", - default_privatization, [](const std::string& smpi_privatize_option){ - if (smpi_privatize_option == "no" || smpi_privatize_option == "0") - _smpi_cfg_privatization = SmpiPrivStrategies::NONE; - else if (smpi_privatize_option == "yes" || smpi_privatize_option == "1") - _smpi_cfg_privatization = SmpiPrivStrategies::DEFAULT; - else if (smpi_privatize_option == "mmap") - _smpi_cfg_privatization = SmpiPrivStrategies::MMAP; - else if (smpi_privatize_option == "dlopen") - _smpi_cfg_privatization = SmpiPrivStrategies::DLOPEN; - else - xbt_die("Invalid value for smpi/privatization: '%s'", smpi_privatize_option.c_str()); - - if (not SMPI_switch_data_segment) { - XBT_DEBUG("Running without smpi_main(); disable smpi/privatization."); - _smpi_cfg_privatization = SmpiPrivStrategies::NONE; - } - if (not HAVE_WORKING_MMAP && _smpi_cfg_privatization == SmpiPrivStrategies::MMAP) { - XBT_INFO("mmap privatization is broken on this platform, switching to dlopen privatization instead."); - _smpi_cfg_privatization = SmpiPrivStrategies::DLOPEN; - } - }); + simgrid::config::declare_flag( + "smpi/privatization", "How we should privatize global variable at runtime (no, yes, mmap, dlopen).", + default_privatization, [called_by_smpimain](const std::string& smpi_privatize_option) { + if (smpi_privatize_option == "no" || smpi_privatize_option == "0") + _smpi_cfg_privatization = SmpiPrivStrategies::NONE; + else if (smpi_privatize_option == "yes" || smpi_privatize_option == "1") + _smpi_cfg_privatization = SmpiPrivStrategies::DEFAULT; + else if (smpi_privatize_option == "mmap") + _smpi_cfg_privatization = SmpiPrivStrategies::MMAP; + else if (smpi_privatize_option == "dlopen") + _smpi_cfg_privatization = SmpiPrivStrategies::DLOPEN; + else + xbt_die("Invalid value for smpi/privatization: '%s'", smpi_privatize_option.c_str()); + + if (not called_by_smpimain) { + XBT_DEBUG("Running without smpi_main(); disable smpi/privatization."); + _smpi_cfg_privatization = SmpiPrivStrategies::NONE; + } + if (not HAVE_WORKING_MMAP && _smpi_cfg_privatization == SmpiPrivStrategies::MMAP) { + XBT_INFO("mmap privatization is broken on this platform, switching to dlopen privatization instead."); + _smpi_cfg_privatization = SmpiPrivStrategies::DLOPEN; + } + }); simgrid::config::declare_flag("smpi/privatize-libs", "Add libraries (; separated) to privatize (libgfortran for example)." diff --git a/src/smpi/internals/smpi_global.cpp b/src/smpi/internals/smpi_global.cpp index ba06a81e18..7fbb861053 100644 --- a/src/smpi/internals/smpi_global.cpp +++ b/src/smpi/internals/smpi_global.cpp @@ -184,21 +184,15 @@ void smpi_comm_copy_buffer_callback(simgrid::kernel::activity::CommImpl* comm, v auto private_blocks = merge_private_blocks(src_private_blocks, dst_private_blocks); check_blocks(private_blocks, buff_size); void* tmpbuff=buff; - if ((smpi_cfg_privatization() == SmpiPrivStrategies::MMAP) && - (static_cast(buff) >= smpi_data_exe_start) && - (static_cast(buff) < smpi_data_exe_start + smpi_data_exe_size)) { + if (smpi_switch_data_segment(comm->src_actor_->get_iface(), buff)) { XBT_DEBUG("Privatization : We are copying from a zone inside global memory... Saving data to temp buffer !"); - smpi_switch_data_segment(comm->src_actor_->get_iface()); tmpbuff = xbt_malloc(buff_size); memcpy_private(tmpbuff, buff, private_blocks); } - if ((smpi_cfg_privatization() == SmpiPrivStrategies::MMAP) && - ((char*)comm->dst_buff_ >= smpi_data_exe_start) && - ((char*)comm->dst_buff_ < smpi_data_exe_start + smpi_data_exe_size)) { + if (smpi_switch_data_segment(comm->dst_actor_->get_iface(), comm->dst_buff_)) XBT_DEBUG("Privatization : We are copying to a zone inside global memory - Switch data segment"); - smpi_switch_data_segment(comm->dst_actor_->get_iface()); - } + XBT_DEBUG("Copying %zu bytes from %p to %p", buff_size, tmpbuff, comm->dst_buff_); memcpy_private(comm->dst_buff_, tmpbuff, private_blocks); @@ -524,9 +518,8 @@ int smpi_main(const char* executable, int argc, char* argv[]) * configuration tools */ return 0; } - - SMPI_switch_data_segment = &smpi_switch_data_segment; - smpi_init_options(); + + smpi_init_options(true); simgrid::instr::init(); SIMIX_global_init(&argc, argv); diff --git a/src/smpi/internals/smpi_memory.cpp b/src/smpi/internals/smpi_memory.cpp index 991377ab89..40bd1e4a6d 100644 --- a/src/smpi/internals/smpi_memory.cpp +++ b/src/smpi/internals/smpi_memory.cpp @@ -175,16 +175,25 @@ void* smpi_temp_shm_mmap(int fd, size_t size) /** Map a given SMPI privatization segment (make an SMPI process active) * * When doing a state restoration, the state of the restored variables might not be consistent with the state of the - * virtual memory. In this case, we to change the data segment. + * virtual memory. In this case, we have to change the data segment. + * + * If 'addr' is not null, only switch if it's an address from the data segment. + * + * Returns 'true' if the segment has to be switched (mmap privatization and 'addr' in data segment). */ -void smpi_switch_data_segment(simgrid::s4u::ActorPtr actor) +bool smpi_switch_data_segment(simgrid::s4u::ActorPtr actor, const void* addr) { + if (smpi_cfg_privatization() != SmpiPrivStrategies::MMAP || smpi_data_exe_size == 0) + return false; // no need to switch + + if (addr != nullptr && + not(static_cast(addr) >= smpi_data_exe_start && + static_cast(addr) < smpi_data_exe_start + smpi_data_exe_size)) + return false; // no need to switch, addr is not concerned + static aid_t smpi_loaded_page = -1; if (smpi_loaded_page == actor->get_pid()) // no need to switch, we've already loaded the one we want - return; - - if (smpi_data_exe_size == 0) // no need to switch - return; + return true; // return 'true' anyway #if HAVE_PRIVATIZATION // FIXME, cross-process support (mmap across process when necessary) @@ -196,6 +205,8 @@ void smpi_switch_data_segment(simgrid::s4u::ActorPtr actor) "Couldn't map the new region (errno %d): %s", errno, strerror(errno)); smpi_loaded_page = actor->get_pid(); #endif + + return true; } /** diff --git a/src/smpi/mpi/smpi_comm.cpp b/src/smpi/mpi/smpi_comm.cpp index 2c32d66a46..115b798de6 100644 --- a/src/smpi/mpi/smpi_comm.cpp +++ b/src/smpi/mpi/smpi_comm.cpp @@ -71,10 +71,9 @@ void Comm::destroy(Comm* comm) } int Comm::dup(MPI_Comm* newcomm){ - if (smpi_cfg_privatization() == SmpiPrivStrategies::MMAP) { - // we need to switch as the called function may silently touch global variables - smpi_switch_data_segment(s4u::Actor::self()); - } + // we need to switch as the called function may silently touch global variables + smpi_switch_data_segment(s4u::Actor::self()); + auto* cp = new Group(this->group()); (*newcomm) = new Comm(cp, this->topo()); @@ -411,10 +410,9 @@ void Comm::init_smp(){ smpi_process()->set_replaying(false); } - if (smpi_cfg_privatization() == SmpiPrivStrategies::MMAP) { - // we need to switch as the called function may silently touch global variables - smpi_switch_data_segment(s4u::Actor::self()); - } + // we need to switch as the called function may silently touch global variables + smpi_switch_data_segment(s4u::Actor::self()); + // identify neighbors in comm MPI_Comm comm_intra = find_intra_comm(&leader); @@ -425,11 +423,6 @@ void Comm::init_smp(){ allgather__ring(&leader, 1, MPI_INT , leaders_map, 1, MPI_INT, this); - if (smpi_cfg_privatization() == SmpiPrivStrategies::MMAP) { - // we need to switch as the called function may silently touch global variables - smpi_switch_data_segment(s4u::Actor::self()); - } - if(leaders_map_==nullptr){ leaders_map_= leaders_map; }else{ @@ -499,10 +492,9 @@ void Comm::init_smp(){ } bcast__scatter_LR_allgather(&is_uniform_, 1, MPI_INT, 0, comm_intra); - if (smpi_cfg_privatization() == SmpiPrivStrategies::MMAP) { - // we need to switch as the called function may silently touch global variables - smpi_switch_data_segment(s4u::Actor::self()); - } + // we need to switch as the called function may silently touch global variables + smpi_switch_data_segment(s4u::Actor::self()); + // Are the ranks blocked ? = allocated contiguously on the SMP nodes int is_blocked=1; int prev = this->group()->rank(comm_intra->group()->actor(0)); diff --git a/src/smpi/mpi/smpi_datatype.cpp b/src/smpi/mpi/smpi_datatype.cpp index 3f5b211697..ecdef35705 100644 --- a/src/smpi/mpi/smpi_datatype.cpp +++ b/src/smpi/mpi/smpi_datatype.cpp @@ -341,9 +341,8 @@ int Datatype::copy(const void* sendbuf, int sendcount, MPI_Datatype sendtype, vo { // FIXME Handle the case of a partial shared malloc. - if (smpi_cfg_privatization() == SmpiPrivStrategies::MMAP) { - smpi_switch_data_segment(simgrid::s4u::Actor::self()); - } + smpi_switch_data_segment(simgrid::s4u::Actor::self()); + /* First check if we really have something to do */ size_t offset = 0; std::vector> private_blocks; diff --git a/src/smpi/mpi/smpi_op.cpp b/src/smpi/mpi/smpi_op.cpp index afc3264188..a0501c12ab 100644 --- a/src/smpi/mpi/smpi_op.cpp +++ b/src/smpi/mpi/smpi_op.cpp @@ -266,11 +266,8 @@ namespace smpi{ void Op::apply(const void* invec, void* inoutvec, const int* len, MPI_Datatype datatype) const { - if (smpi_cfg_privatization() == SmpiPrivStrategies::MMAP) { - // we need to switch as the called function may silently touch global variables - XBT_DEBUG("Applying operation, switch to the right data frame "); - smpi_switch_data_segment(simgrid::s4u::Actor::self()); - } + // we need to switch as the called function may silently touch global variables + smpi_switch_data_segment(simgrid::s4u::Actor::self()); if (not smpi_process()->replaying() && *len > 0) { XBT_DEBUG("Applying operation of length %d from %p and from/to %p", *len, invec, inoutvec); diff --git a/src/smpi/mpi/smpi_request.cpp b/src/smpi/mpi/smpi_request.cpp index fe8e35b36c..2526f7c4a8 100644 --- a/src/smpi/mpi/smpi_request.cpp +++ b/src/smpi/mpi/smpi_request.cpp @@ -506,12 +506,9 @@ void Request::start() if (not(type_->flags() & DT_FLAG_DERIVED)) { oldbuf = buf_; if (not process->replaying() && oldbuf != nullptr && size_ != 0) { - if ((smpi_cfg_privatization() != SmpiPrivStrategies::NONE) && - (static_cast(buf_) >= smpi_data_exe_start) && - (static_cast(buf_) < smpi_data_exe_start + smpi_data_exe_size)) { + if (smpi_switch_data_segment(simgrid::s4u::Actor::by_pid(src_), buf_)) XBT_DEBUG("Privatization : We are sending from a zone inside global memory. Switch data segment "); - smpi_switch_data_segment(simgrid::s4u::Actor::by_pid(src_)); - } + //we need this temporary buffer even for bsend, as it will be released in the copy callback and we don't have a way to differentiate it //so actually ... don't use manually attached buffer space. buf = xbt_malloc(size_); @@ -939,12 +936,8 @@ void Request::finish_wait(MPI_Request* request, MPI_Status * status) // FIXME Handle the case of a partial shared malloc. if (((req->flags_ & MPI_REQ_ACCUMULATE) != 0) || (datatype->flags() & DT_FLAG_DERIVED)) { // && (not smpi_is_shared(req->old_buf_))){ - if (not smpi_process()->replaying() && smpi_cfg_privatization() != SmpiPrivStrategies::NONE && - static_cast(req->old_buf_) >= smpi_data_exe_start && - static_cast(req->old_buf_) < smpi_data_exe_start + smpi_data_exe_size) { + if (not smpi_process()->replaying() && smpi_switch_data_segment(simgrid::s4u::Actor::self(), req->old_buf_)) XBT_VERB("Privatization : We are unserializing to a zone in global memory Switch data segment "); - smpi_switch_data_segment(simgrid::s4u::Actor::self()); - } if(datatype->flags() & DT_FLAG_DERIVED){ // This part handles the problem of non-contiguous memory the unserialization at the reception -- 2.20.1