From b5eb9daac42a57ffddff9d693bc39ba984a91df5 Mon Sep 17 00:00:00 2001 From: Martin Quinson Date: Fri, 13 Sep 2019 11:20:47 +0200 Subject: [PATCH] try to reduce the memory consistency requirements in the actor refcounting some people actually seem to like those C++'s hardcore brain teasers --- src/kernel/actor/ActorImpl.hpp | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/src/kernel/actor/ActorImpl.hpp b/src/kernel/actor/ActorImpl.hpp index d0c75f2c60..ee082de7a6 100644 --- a/src/kernel/actor/ActorImpl.hpp +++ b/src/kernel/actor/ActorImpl.hpp @@ -79,11 +79,14 @@ public: int get_refcount() { return refcount_; } friend void intrusive_ptr_add_ref(ActorImpl* actor) { - // std::memory_order_relaxed ought to be enough here instead of std::memory_order_seq_cst - // But then, we have a threading issue when an actor commits a suicide: - // it seems that in this case, the worker thread kills the last occurrence of the actor - // while usually, the maestro does so. FIXME: we should change how actors suicide - actor->refcount_.fetch_add(1, std::memory_order_seq_cst); + // This whole memory consistency semantic drives me nuts. + // std::memory_order_relaxed proves to not be enough: There is a threading issue when actors commit suicide. + // My guess is that the maestro context wants to propagate changes to the actor's fields after the + // actor context frees that memory area or something. But I'm not 100% certain of what's going on. + // std::memory_order_seq_cst works but that's rather demanding. + // AFAIK, std::memory_order_acq_rel works on all tested platforms, so let's stick to it. + // Reducing the requirements to _relaxed would require to fix our suicide procedure, which is a messy piece of code. + actor->refcount_.fetch_add(1, std::memory_order_acq_rel); } friend void intrusive_ptr_release(ActorImpl* actor) { -- 2.20.1