From: Waldemar Kozaczuk <jwkozac...@gmail.com>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

thread: make walking parent-child thread relationship safer

This patch replaces _parent_link/thread_list with a simple parent_thread_id
field to make walking parent-child list safer in elf::visible() method.

Signed-off-by: Waldemar Kozaczuk <jwkozac...@gmail.com>
Message-Id: <20200112224434.13489-1-jwkozac...@gmail.com>

---
diff --git a/core/elf.cc b/core/elf.cc
--- a/core/elf.cc
+++ b/core/elf.cc
@@ -151,17 +151,29 @@ bool object::visible(void) const
         return true;
     }

+    auto current_thread = sched::thread::current();
     if (level == VisibilityLevel::ThreadOnly) {
-        return thread == sched::thread::current();
+        return thread == current_thread;
     }

-    // Is current thread the same as "thread" or is it a child ?`
- for (auto t = sched::thread::current(); t != nullptr; t = t->parent()) {
-        if (t == thread) {
-            return true;
-        }
+    // Is current thread the same as "thread" or is it a child of it?
+    if (thread == current_thread) {
+        return true;
     }
-    return false;
+
+    bool visible = false;
+    auto next_parent_id = current_thread->parent_id();
+    while (next_parent_id) {
+ sched::with_thread_by_id(next_parent_id, [&next_parent_id, &visible, thread] (sched::thread *t) {
+            if (t == thread) {
+                visible = true;
+                next_parent_id = 0;
+            } else {
+                next_parent_id = t ? t->parent_id() : 0;
+            }
+        });
+    }
+    return visible;
 }

 void object::set_visibility(elf::VisibilityLevel visibilityLevel)
@@ -171,7 +183,6 @@ void object::set_visibility(elf::VisibilityLevel visibilityLevel)
     _visibility_level.store(visibilityLevel, std::memory_order_release);
 }

-
 template <>
 void* object::lookup(const char* symbol)
 {
diff --git a/core/sched.cc b/core/sched.cc
--- a/core/sched.cc
+++ b/core/sched.cc
@@ -1002,11 +1002,7 @@ thread::thread(std::function<void ()> func, attr attr, bool main, bool app)
                 sizeof(_attr._name) - 1);
     }

- // Note that we never delete _parent_link. This is intentional as it lets us track - // parent-child relationship safely even when given thread is destroyed.
-    _parent_link = new thread_list();
-    _parent_link->_self.store(this);
- _parent_link->_parent = (s_current && !main) ? s_current->_parent_link : nullptr;
+    _parent_id = s_current ? s_current->id() : 0;
 }

 static std::list<std::function<void ()>> exit_notifiers
@@ -1044,8 +1040,6 @@ thread::~thread()
 {
     cancel_this_thread_alarm();

-    _parent_link->_self.store(nullptr, std::memory_order_release);
-
     if (!_attr._detached) {
         join();
     }
diff --git a/include/osv/sched.hh b/include/osv/sched.hh
--- a/include/osv/sched.hh
+++ b/include/osv/sched.hh
@@ -364,13 +364,6 @@ public:
             return *this;
         }
     };
-    // Simple single-linked list that allows to keep track of
-    // "parent-child" relationship between threads. The "parent" is
-    // the thread that constructs given "child" thread
-    struct thread_list {
-        std::atomic<thread*> _self;
-        thread_list *_parent;
-    };

 private:
     // Unlike the detached user visible attribute, those are internal to
@@ -640,7 +633,10 @@ public:
     {
         return static_cast<T*>(do_remote_thread_local_var(var));
     }
-    thread *parent();
+    unsigned int parent_id() const
+    {
+        return _parent_id;
+    }
 private:
     virtual void timer_fired() override;
     struct detached_state;
@@ -777,7 +773,7 @@ private:
     inline void cputime_estimator_get(
             osv::clock::uptime::time_point &running_since,
             osv::clock::uptime::duration &total_cpu_time);
-    thread_list *_parent_link;
+    unsigned int _parent_id;
 };

 class thread_handle {
@@ -1304,12 +1300,6 @@ inline thread_handle thread::handle()
     return thread_handle(*this);
 }

-inline thread* thread::parent()
-{
-    auto _parent = _parent_link->_parent;
- return _parent ? _parent->_self.load(std::memory_order_acquire) : nullptr;
-}
-
 inline void* thread::get_tls(ulong module)
 {
     if (module >= _tls.size()) {

--
You received this message because you are subscribed to the Google Groups "OSv 
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email 
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit 
https://groups.google.com/d/msgid/osv-dev/000000000000d7b5a7059bf93a3f%40google.com.

Reply via email to