From e59357ba4f93c5bca3121f386f00e1cea7429a0d Mon Sep 17 00:00:00 2001 From: Martin Quinson Date: Thu, 23 Feb 2023 22:21:47 +0100 Subject: [PATCH] Sanitize the handling of timeouts in CommImpl, ExecImpl and IoImpl There is no need for a timeout detector in ExecImpl and IoImpl, as wait_for injects a Synchro to that extend on need. Use a uniq_ptr on the CommImpl timeout detectors, so that the memory is managed automatically. As it used to be for ExecImpl. This kills the last occurrence of surf in the code. For real this time. --- src/kernel/activity/ActivityImpl.cpp | 5 +++-- src/kernel/activity/CommImpl.cpp | 20 ++------------------ src/kernel/activity/CommImpl.hpp | 8 +++++--- src/kernel/activity/ExecImpl.cpp | 14 -------------- src/kernel/activity/ExecImpl.hpp | 3 --- src/kernel/activity/IoImpl.cpp | 16 ---------------- src/kernel/activity/IoImpl.hpp | 2 -- 7 files changed, 10 insertions(+), 58 deletions(-) diff --git a/src/kernel/activity/ActivityImpl.cpp b/src/kernel/activity/ActivityImpl.cpp index 5d7c92222a..f425dc8eff 100644 --- a/src/kernel/activity/ActivityImpl.cpp +++ b/src/kernel/activity/ActivityImpl.cpp @@ -115,9 +115,9 @@ void ActivityImpl::wait_for(actor::ActorImpl* issuer, double timeout) sleep_action->set_activity(comm); if (issuer == comm->src_actor_) - comm->src_timeout_ = sleep_action; + comm->src_timeout_.reset(sleep_action); else - comm->dst_timeout_ = sleep_action; + comm->dst_timeout_.reset(sleep_action); } else { SynchroImplPtr synchro(new SynchroImpl([this, issuer]() { this->unregister_simcall(&issuer->simcall_); @@ -236,4 +236,5 @@ void intrusive_ptr_release(ActivityImpl* activity) delete activity; } } + } // namespace simgrid::kernel::activity diff --git a/src/kernel/activity/CommImpl.cpp b/src/kernel/activity/CommImpl.cpp index ee24f501ea..0d81366c34 100644 --- a/src/kernel/activity/CommImpl.cpp +++ b/src/kernel/activity/CommImpl.cpp @@ -96,7 +96,7 @@ CommImpl::~CommImpl() { XBT_DEBUG("Really free communication %p in state %s (detached = %d)", this, get_state_str(), detached_); - cleanup_surf(); + clean_action(); if (detached_ && get_state() != State::DONE) { /* the communication has failed and was detached: @@ -397,22 +397,6 @@ void CommImpl::cancel() } } -/** @brief This is part of the cleanup process, probably an internal command */ -void CommImpl::cleanup_surf() -{ - clean_action(); - - if (src_timeout_) { - src_timeout_->unref(); - src_timeout_ = nullptr; - } - - if (dst_timeout_) { - dst_timeout_->unref(); - dst_timeout_ = nullptr; - } -} - void CommImpl::post() { on_completion(*this); @@ -438,7 +422,7 @@ void CommImpl::post() src_actor_.get(), dst_actor_.get(), detached_); /* destroy the model actions associated with the communication activity */ - cleanup_surf(); + clean_action(); /* Answer all simcalls associated with the synchro */ finish(); diff --git a/src/kernel/activity/CommImpl.hpp b/src/kernel/activity/CommImpl.hpp index c2758ab718..7540c4a3aa 100644 --- a/src/kernel/activity/CommImpl.hpp +++ b/src/kernel/activity/CommImpl.hpp @@ -14,9 +14,10 @@ namespace simgrid::kernel::activity { enum class CommImplType { SEND, RECEIVE }; +using timeout_action_type = std::unique_ptr>; + class XBT_PUBLIC CommImpl : public ActivityImpl_T { ~CommImpl() override; - void cleanup_surf(); static std::function copy_data_callback_; @@ -80,8 +81,9 @@ expectations of the other side, too. See */ std::function copy_data_fun; /* Model actions */ - resource::Action* src_timeout_ = nullptr; /* represents the timeout set by the sender */ - resource::Action* dst_timeout_ = nullptr; /* represents the timeout set by the receiver */ + timeout_action_type src_timeout_{nullptr, [](resource::Action* a) { a->unref(); }}; /* timeout set by the sender */ + timeout_action_type dst_timeout_{nullptr, [](resource::Action* a) { a->unref(); }}; /* timeout set by the receiver */ + actor::ActorImplPtr src_actor_ = nullptr; actor::ActorImplPtr dst_actor_ = nullptr; diff --git a/src/kernel/activity/ExecImpl.cpp b/src/kernel/activity/ExecImpl.cpp index 88432f686d..d7368f613c 100644 --- a/src/kernel/activity/ExecImpl.cpp +++ b/src/kernel/activity/ExecImpl.cpp @@ -41,15 +41,6 @@ ExecImpl& ExecImpl::set_hosts(const std::vector& hosts) return *this; } -ExecImpl& ExecImpl::set_timeout(double timeout) -{ - if (timeout >= 0 && not MC_is_active() && not MC_record_replay_is_active()) { - timeout_detector_.reset(get_host()->get_cpu()->sleep(timeout)); - timeout_detector_->set_activity(this); - } - return *this; -} - ExecImpl& ExecImpl::set_flops_amount(double flops_amount) { flops_amounts_.assign(1, flops_amount); @@ -153,16 +144,11 @@ void ExecImpl::post() } else if (model_action_->get_state() == resource::Action::State::FAILED) { /* If all the hosts are running the synchro didn't fail, then the synchro was canceled */ set_state(State::CANCELED); - } else if (timeout_detector_ && timeout_detector_->get_state() == resource::Action::State::FINISHED && - model_action_->get_remains() > 0.0) { - model_action_->set_state(resource::Action::State::FAILED); - set_state(State::TIMEOUT); } else { set_state(State::DONE); } clean_action(); - timeout_detector_.reset(); if (get_actor() != nullptr) { get_actor()->activities_.erase(this); } diff --git a/src/kernel/activity/ExecImpl.hpp b/src/kernel/activity/ExecImpl.hpp index d75bf41ae8..d15280cee6 100644 --- a/src/kernel/activity/ExecImpl.hpp +++ b/src/kernel/activity/ExecImpl.hpp @@ -14,8 +14,6 @@ namespace simgrid::kernel::activity { class XBT_PUBLIC ExecImpl : public ActivityImpl_T { - std::unique_ptr> timeout_detector_{ - nullptr, [](resource::Action* a) { a->unref(); }}; double sharing_penalty_ = 1.0; double bound_ = 0.0; std::vector flops_amounts_; @@ -26,7 +24,6 @@ class XBT_PUBLIC ExecImpl : public ActivityImpl_T { public: ExecImpl(); - ExecImpl& set_timeout(double timeout) override; ExecImpl& set_bound(double bound); ExecImpl& set_sharing_penalty(double sharing_penalty); ExecImpl& update_sharing_penalty(double sharing_penalty); diff --git a/src/kernel/activity/IoImpl.cpp b/src/kernel/activity/IoImpl.cpp index d270c4fbb7..cd57ecea8a 100644 --- a/src/kernel/activity/IoImpl.cpp +++ b/src/kernel/activity/IoImpl.cpp @@ -38,14 +38,6 @@ IoImpl& IoImpl::update_sharing_penalty(double sharing_penalty) return *this; } -IoImpl& IoImpl::set_timeout(double timeout) -{ - const s4u::Host* host = get_disk()->get_host(); - timeout_detector_ = host->get_cpu()->sleep(timeout); - timeout_detector_->set_activity(this); - return *this; -} - IoImpl& IoImpl::set_type(s4u::Io::OpType type) { type_ = type; @@ -116,19 +108,11 @@ void IoImpl::post() set_state(State::FAILED); else set_state(State::CANCELED); - } else if (timeout_detector_ && timeout_detector_->get_state() == resource::Action::State::FINISHED && - model_action_->get_remains() > 0.0) { - model_action_->set_state(resource::Action::State::FAILED); - set_state(State::TIMEOUT); } else { set_state(State::DONE); } clean_action(); - if (timeout_detector_) { - timeout_detector_->unref(); - timeout_detector_ = nullptr; - } /* Answer all simcalls associated with the synchro */ finish(); diff --git a/src/kernel/activity/IoImpl.hpp b/src/kernel/activity/IoImpl.hpp index 0e3dd9fa28..22e46a31d3 100644 --- a/src/kernel/activity/IoImpl.hpp +++ b/src/kernel/activity/IoImpl.hpp @@ -20,13 +20,11 @@ class XBT_PUBLIC IoImpl : public ActivityImpl_T { sg_size_t size_ = 0; s4u::Io::OpType type_ = s4u::Io::OpType::READ; sg_size_t performed_ioops_ = 0; - resource::Action* timeout_detector_ = nullptr; public: IoImpl(); IoImpl& set_sharing_penalty(double sharing_penalty); - IoImpl& set_timeout(double timeout) override; IoImpl& set_size(sg_size_t size); IoImpl& set_type(s4u::Io::OpType type); IoImpl& set_disk(resource::DiskImpl* disk); -- 2.20.1