Looks good. Thanks. I'll commit. On Mon, Jan 13, 2020 at 12:44 AM Waldemar Kozaczuk <[email protected]> wrote:
> 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 <[email protected]> > --- > core/elf.cc | 27 +++++++++++++++++++-------- > core/sched.cc | 8 +------- > include/osv/sched.hh | 20 +++++--------------- > 3 files changed, 25 insertions(+), 30 deletions(-) > > diff --git a/core/elf.cc b/core/elf.cc > index a25fb3c1..86ccc5d9 100644 > --- 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; > Oh, I forgot we can't "return" from inside the lambda :-( You could have also used "break" here instead of "next_parent_id = 0". But never mind :-) + } 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 > index fde98432..06f849d1 100644 > --- 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 > index 0c2aa143..0fb44f77 100644 > --- 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()) { > -- > 2.20.1 > > -- > 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 [email protected]. > To view this discussion on the web visit > https://groups.google.com/d/msgid/osv-dev/20200112224434.13489-1-jwkozaczuk%40gmail.com > . > -- 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 [email protected]. To view this discussion on the web visit https://groups.google.com/d/msgid/osv-dev/CANEVyjuOwSrrrjq0-ig1hESg-j5efezZd6tAwRcaqwzbjVn1qQ%40mail.gmail.com.
