From 13cfb5c4733854671750256e92a6aca384d25626 Mon Sep 17 00:00:00 2001 From: Martin Quinson Date: Thu, 8 Jun 2017 07:08:24 +0200 Subject: [PATCH] eradicate all remaining manual refcounting on ActivityImpl --- src/kernel/activity/ActivityImpl.cpp | 36 ++++++++++------------------ src/kernel/activity/ActivityImpl.hpp | 7 +----- src/kernel/activity/CommImpl.cpp | 1 - src/kernel/activity/ExecImpl.cpp | 2 ++ src/msg/msg_gos.cpp | 2 +- src/msg/msg_private.h | 3 --- src/simix/ActorImpl.cpp | 15 +++--------- src/simix/smx_host.cpp | 15 ++++++------ src/simix/smx_io.cpp | 1 - src/simix/smx_synchro.cpp | 4 ---- 10 files changed, 27 insertions(+), 59 deletions(-) diff --git a/src/kernel/activity/ActivityImpl.cpp b/src/kernel/activity/ActivityImpl.cpp index ea655de7eb..6a9b6e83df 100644 --- a/src/kernel/activity/ActivityImpl.cpp +++ b/src/kernel/activity/ActivityImpl.cpp @@ -5,7 +5,7 @@ #include "src/kernel/activity/ActivityImpl.hpp" -XBT_LOG_EXTERNAL_CATEGORY(simix_network); +XBT_LOG_EXTERNAL_CATEGORY(simix_process); namespace simgrid { namespace kernel { @@ -14,35 +14,25 @@ namespace activity { ActivityImpl::ActivityImpl() = default; ActivityImpl::~ActivityImpl() = default; -void ActivityImpl::ref() -{ - xbt_assert(refcount_ >= 0); - refcount_++; - XBT_CDEBUG(simix_network, "%p->refcount++ ~> %d", this, (int)refcount_); - if (XBT_LOG_ISENABLED(simix_network, xbt_log_priority_trace)) - xbt_backtrace_display_current(); -} - -void ActivityImpl::unref() -{ - XBT_CDEBUG(simix_network, "%p->refcount-- ~> %d", this, ((int)refcount_) - 1); - xbt_assert(refcount_ >= 0); - refcount_--; - if (XBT_LOG_ISENABLED(simix_network, xbt_log_priority_trace)) - xbt_backtrace_display_current(); - if (refcount_ <= 0) - delete this; -} - // boost::intrusive_ptr support: void intrusive_ptr_add_ref(simgrid::kernel::activity::ActivityImpl* activity) { - activity->ref(); + xbt_assert(activity->refcount_ >= 0); + activity->refcount_++; + XBT_CDEBUG(simix_process, "%p->refcount++ ~> %d", activity, (int)activity->refcount_); + if (XBT_LOG_ISENABLED(simix_process, xbt_log_priority_trace)) + xbt_backtrace_display_current(); } void intrusive_ptr_release(simgrid::kernel::activity::ActivityImpl* activity) { - activity->unref(); + XBT_CDEBUG(simix_process, "%p->refcount-- ~> %d", activity, ((int)activity->refcount_) - 1); + xbt_assert(activity->refcount_ >= 0); + activity->refcount_--; + if (XBT_LOG_ISENABLED(simix_process, xbt_log_priority_trace)) + xbt_backtrace_display_current(); + if (activity->refcount_ <= 0) + delete activity; } } } diff --git a/src/kernel/activity/ActivityImpl.hpp b/src/kernel/activity/ActivityImpl.hpp index be030363e8..2a50de8059 100644 --- a/src/kernel/activity/ActivityImpl.hpp +++ b/src/kernel/activity/ActivityImpl.hpp @@ -31,12 +31,7 @@ namespace activity { virtual void resume()=0; virtual void post() =0; // What to do when a simcall terminates - /** @brief Increases the refcount */ - void ref(); - /** @brief Reduces the refcount */ - void unref(); - - // boost::intrusive_ptr support: + // boost::intrusive_ptr support: friend void intrusive_ptr_add_ref(ActivityImpl * activity); friend void intrusive_ptr_release(ActivityImpl * activity); diff --git a/src/kernel/activity/CommImpl.cpp b/src/kernel/activity/CommImpl.cpp index 1b61834929..33d31e7d03 100644 --- a/src/kernel/activity/CommImpl.cpp +++ b/src/kernel/activity/CommImpl.cpp @@ -61,7 +61,6 @@ void simgrid::kernel::activity::CommImpl::cancel() if (state == SIMIX_WAITING) { mbox->remove(this); state = SIMIX_CANCELED; - this->unref(); } else if (not MC_is_active() /* when running the MC there are no surf actions */ && not MC_record_replay_is_active() && (state == SIMIX_READY || state == SIMIX_RUNNING)) { diff --git a/src/kernel/activity/ExecImpl.cpp b/src/kernel/activity/ExecImpl.cpp index 56c6436df8..aee76bc532 100644 --- a/src/kernel/activity/ExecImpl.cpp +++ b/src/kernel/activity/ExecImpl.cpp @@ -16,6 +16,7 @@ simgrid::kernel::activity::ExecImpl::ExecImpl(const char* name, sg_host_t host) if (name) this->name = name; this->state = SIMIX_RUNNING; + XBT_DEBUG("Create exec %p", this); } simgrid::kernel::activity::ExecImpl::~ExecImpl() @@ -24,6 +25,7 @@ simgrid::kernel::activity::ExecImpl::~ExecImpl() surf_exec->unref(); if (timeoutDetector) timeoutDetector->unref(); + XBT_DEBUG("Destroy exec %p", this); } void simgrid::kernel::activity::ExecImpl::suspend() { diff --git a/src/msg/msg_gos.cpp b/src/msg/msg_gos.cpp index f3880e3de9..4ec569470c 100644 --- a/src/msg/msg_gos.cpp +++ b/src/msg/msg_gos.cpp @@ -71,7 +71,7 @@ msg_error_t MSG_parallel_task_execute_with_timeout(msg_task_t task, double timeo boost::static_pointer_cast(simcall_execution_parallel_start( task->name, simdata->host_nb, simdata->host_list, simdata->flops_parallel_amount, simdata->bytes_parallel_amount, 1.0, -1.0, timeout)); - XBT_DEBUG("Parallel execution action created: %p", simdata->compute); + XBT_DEBUG("Parallel execution action created: %p", simdata->compute.get()); } else { simdata->compute = boost::static_pointer_cast( simcall_execution_start(task->name, simdata->flops_amount, simdata->priority, simdata->bound)); diff --git a/src/msg/msg_private.h b/src/msg/msg_private.h index 00164dbd14..2a45111d2d 100644 --- a/src/msg/msg_private.h +++ b/src/msg/msg_private.h @@ -30,9 +30,6 @@ public: typedef struct simdata_task { ~simdata_task() { - if (this->compute) - this->compute->unref(); - /* parallel tasks only */ xbt_free(this->host_list); } diff --git a/src/simix/ActorImpl.cpp b/src/simix/ActorImpl.cpp index ea43de9a68..0d207a81d9 100644 --- a/src/simix/ActorImpl.cpp +++ b/src/simix/ActorImpl.cpp @@ -106,11 +106,6 @@ void SIMIX_process_cleanup(smx_actor_t process) comm, comm->detached, (int)comm->state, comm->src_proc, comm->dst_proc); comm->src_proc = nullptr; - /* I'm not supposed to destroy a detached comm from the sender side, */ - if (comm->detached) - XBT_DEBUG("Don't destroy it since it's a detached comm and I'm the sender"); - else - comm->unref(); } else if (comm->dst_proc == process) { XBT_DEBUG("Found an unfinished recv comm %p, state %d, src = %p, dst = %p", comm, (int)comm->state, comm->src_proc, comm->dst_proc); @@ -423,7 +418,7 @@ void SIMIX_process_kill(smx_actor_t process, smx_actor_t issuer) { process->exception = nullptr; /* destroy the blocking synchro if any */ - if (process->waiting_synchro) { + if (process->waiting_synchro != nullptr) { simgrid::kernel::activity::ExecImplPtr exec = boost::dynamic_pointer_cast(process->waiting_synchro); @@ -437,7 +432,6 @@ void SIMIX_process_kill(smx_actor_t process, smx_actor_t issuer) { boost::dynamic_pointer_cast(process->waiting_synchro); if (exec != nullptr) { - exec->unref(); } else if (comm != nullptr) { process->comms.remove(process->waiting_synchro); @@ -446,13 +440,11 @@ void SIMIX_process_kill(smx_actor_t process, smx_actor_t issuer) { auto i = boost::range::find(process->waiting_synchro->simcalls, &process->simcall); if (i != process->waiting_synchro->simcalls.end()) process->waiting_synchro->simcalls.remove(&process->simcall); - comm->unref(); } else if (sleep != nullptr) { SIMIX_process_sleep_destroy(process->waiting_synchro); } else if (raw != nullptr) { SIMIX_synchro_stop_waiting(process, &process->simcall); - process->waiting_synchro->unref(); } else if (io != nullptr) { SIMIX_io_destroy(process->waiting_synchro); @@ -723,7 +715,6 @@ static int SIMIX_process_join_finish(smx_process_exit_status_t status, void* syn sleep->surf_sleep->unref(); sleep->surf_sleep = nullptr; } - sleep->unref(); // intrusive_ptr_release(process); // FIXME: We are leaking here. See comment in SIMIX_process_join() return 0; } @@ -731,7 +722,7 @@ static int SIMIX_process_join_finish(smx_process_exit_status_t status, void* syn smx_activity_t SIMIX_process_join(smx_actor_t issuer, smx_actor_t process, double timeout) { smx_activity_t res = SIMIX_process_sleep(issuer, timeout); - (&*res)->ref(); + intrusive_ptr_add_ref(res.get()); /* We are leaking the process here, but if we don't take the ref, we get a "use after free". * The correct solution would be to derivate the type SynchroSleep into a SynchroProcessJoin, * but the code is not clean enough for now for this. @@ -773,7 +764,7 @@ smx_activity_t SIMIX_process_sleep(smx_actor_t process, double duration) void SIMIX_process_sleep_destroy(smx_activity_t synchro) { - XBT_DEBUG("Destroy synchro %p", synchro); + XBT_DEBUG("Destroy sleep synchro %p", synchro.get()); simgrid::kernel::activity::SleepImplPtr sleep = boost::dynamic_pointer_cast(synchro); diff --git a/src/simix/smx_host.cpp b/src/simix/smx_host.cpp index 6fd7c620e0..5605745391 100644 --- a/src/simix/smx_host.cpp +++ b/src/simix/smx_host.cpp @@ -161,13 +161,14 @@ smx_activity_t SIMIX_execution_start(smx_actor_t issuer, const char *name, doubl double bound){ /* alloc structures and initialize */ - simgrid::kernel::activity::ExecImpl* exec = new simgrid::kernel::activity::ExecImpl(name, issuer->host); + simgrid::kernel::activity::ExecImplPtr exec = + simgrid::kernel::activity::ExecImplPtr(new simgrid::kernel::activity::ExecImpl(name, issuer->host)); /* set surf's action */ if (not MC_is_active() && not MC_record_replay_is_active()) { exec->surf_exec = issuer->host->pimpl_cpu->execution_start(flops_amount); - exec->surf_exec->setData(exec); + exec->surf_exec->setData(exec.get()); exec->surf_exec->setPriority(priority); if (bound > 0) @@ -184,7 +185,8 @@ smx_activity_t SIMIX_execution_parallel_start(const char* name, int host_nb, sg_ { /* alloc structures and initialize */ - simgrid::kernel::activity::ExecImpl* exec = new simgrid::kernel::activity::ExecImpl(name, nullptr); + simgrid::kernel::activity::ExecImplPtr exec = + simgrid::kernel::activity::ExecImplPtr(new simgrid::kernel::activity::ExecImpl(name, nullptr)); /* set surf's synchro */ sg_host_t *host_list_cpy = xbt_new0(sg_host_t, host_nb); @@ -201,10 +203,10 @@ smx_activity_t SIMIX_execution_parallel_start(const char* name, int host_nb, sg_ /* set surf's synchro */ if (not MC_is_active() && not MC_record_replay_is_active()) { exec->surf_exec = surf_host_model->executeParallelTask(host_nb, host_list_cpy, flops_amount, bytes_amount, rate); - exec->surf_exec->setData(exec); + exec->surf_exec->setData(exec.get()); if (timeout > 0) { exec->timeoutDetector = host_list[0]->pimpl_cpu->sleep(timeout); - exec->timeoutDetector->setData(exec); + exec->timeoutDetector->setData(exec.get()); } } XBT_DEBUG("Create parallel execute synchro %p", exec); @@ -298,9 +300,6 @@ void SIMIX_execution_finish(simgrid::kernel::activity::ExecImplPtr exec) simcall_execution_wait__set__result(simcall, exec->state); SIMIX_simcall_answer(simcall); } - - /* We no longer need it */ - exec->unref(); } void SIMIX_set_category(smx_activity_t synchro, const char *category) diff --git a/src/simix/smx_io.cpp b/src/simix/smx_io.cpp index 4371b8da76..d28660d0b7 100644 --- a/src/simix/smx_io.cpp +++ b/src/simix/smx_io.cpp @@ -182,7 +182,6 @@ void SIMIX_io_destroy(smx_activity_t synchro) XBT_DEBUG("Destroy synchro %p", synchro); if (io->surf_io) io->surf_io->unref(); - io->unref(); } void SIMIX_io_finish(smx_activity_t synchro) diff --git a/src/simix/smx_synchro.cpp b/src/simix/smx_synchro.cpp index 8ea26ceefe..1b26ac4e9f 100644 --- a/src/simix/smx_synchro.cpp +++ b/src/simix/smx_synchro.cpp @@ -90,7 +90,6 @@ void SIMIX_synchro_finish(smx_activity_t synchro) SIMIX_synchro_stop_waiting(simcall->issuer, simcall); simcall->issuer->waiting_synchro = nullptr; - synchro->unref(); SIMIX_simcall_answer(simcall); XBT_OUT(); } @@ -176,7 +175,6 @@ void MutexImpl::unlock(smx_actor_t issuer) if (xbt_swag_size(this->sleeping) > 0) { /*process to wake up */ smx_actor_t p = (smx_actor_t) xbt_swag_extract(this->sleeping); - p->waiting_synchro->unref(); p->waiting_synchro = nullptr; this->owner = p; SIMIX_simcall_answer(&p->simcall); @@ -319,7 +317,6 @@ void SIMIX_cond_signal(smx_cond_t cond) if ((proc = (smx_actor_t) xbt_swag_extract(cond->sleeping))) { /* Destroy waiter's synchronization */ - proc->waiting_synchro->unref(); proc->waiting_synchro = nullptr; /* Now transform the cond wait simcall into a mutex lock one */ @@ -432,7 +429,6 @@ void SIMIX_sem_release(smx_sem_t sem) XBT_DEBUG("Sem release semaphore %p", sem); if ((proc = (smx_actor_t) xbt_swag_extract(sem->sleeping))) { - proc->waiting_synchro->unref(); proc->waiting_synchro = nullptr; SIMIX_simcall_answer(&proc->simcall); } else { -- 2.20.1