[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Minios-devel] [UNIKRAFT PATCH 5/8] lib/uksched: Revisit thread exiting logic



Hi Costin,

On 9/18/18 5:27 PM, Costin Lupu wrote:
We use a list for terminated threads on all schedulers because it
keeps references to those threads until wait will be called for
them.

Just to make sure: your point is that since every scheduler will have to keep track of terminated threads, we might as well put it into the main uk_sched information instead of the private one, right?


Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
---
  lib/uksched/include/uk/sched.h  |  1 +
  lib/uksched/include/uk/thread.h |  4 ++++
  lib/uksched/sched.c             |  7 ++++---
  lib/ukschedcoop/schedcoop.c     | 11 +++--------
  4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/lib/uksched/include/uk/sched.h b/lib/uksched/include/uk/sched.h
index 443dbf3..c3e2866 100644
--- a/lib/uksched/include/uk/sched.h
+++ b/lib/uksched/include/uk/sched.h
@@ -87,6 +87,7 @@ struct uk_sched {
/* internal */
        struct uk_thread idle;
+       struct uk_thread_list exited_threads;
        struct ukplat_ctx_callbacks plat_ctx_cbs;
        struct uk_alloc *allocator;
        struct uk_sched *next;
diff --git a/lib/uksched/include/uk/thread.h b/lib/uksched/include/uk/thread.h
index d28c458..7a1b630 100644
--- a/lib/uksched/include/uk/thread.h
+++ b/lib/uksched/include/uk/thread.h
@@ -87,11 +87,15 @@ struct uk_thread *uk_thread_current(void)
  }
#define RUNNABLE_FLAG 0x00000001
+#define EXITED_FLAG     0x00000002
#define is_runnable(_thread) ((_thread)->flags & RUNNABLE_FLAG)
  #define set_runnable(_thread)   ((_thread)->flags |=  RUNNABLE_FLAG)
  #define clear_runnable(_thread) ((_thread)->flags &= ~RUNNABLE_FLAG)
+#define is_exited(_thread) ((_thread)->flags & EXITED_FLAG)
+#define set_exited(_thread)     ((_thread)->flags |=  EXITED_FLAG)
+
  int uk_thread_init(struct uk_thread *thread,
                struct ukplat_ctx_callbacks *cbs, struct uk_alloc *allocator,
                const char *name, void *stack,
diff --git a/lib/uksched/sched.c b/lib/uksched/sched.c
index 968723c..c5b7c9f 100644
--- a/lib/uksched/sched.c
+++ b/lib/uksched/sched.c
@@ -122,6 +122,7 @@ struct uk_sched *uk_sched_create(struct uk_alloc *a, size_t 
prv_size)
        }
sched->allocator = a;
+       UK_TAILQ_INIT(&sched->exited_threads);
        sched->prv = (void *) sched + sizeof(struct uk_sched);
return sched;
@@ -217,6 +218,9 @@ void uk_sched_thread_destroy(struct uk_sched *sched, struct 
uk_thread *thread)
  {
        UK_ASSERT(sched != NULL);
        UK_ASSERT(thread != NULL);
+       UK_ASSERT(is_exited(thread));

I don't like that this patch introduces an assertion on is_exited, but never calls set_exited. This means that at this revision, any call to uk_sched_thread_destroy will immediately crash with an assertion error. That might be really nasty if we ever end up tracking a bug during bisecting and end up with this revision.

Instead, I suggest you introduce this UK_ASSERT when you introduce more of the scheduling logic in the next patch ("Add support for waiting threads"), because in general, it makes a lot of sense as an assertion.

+
+       UK_TAILQ_REMOVE(&sched->exited_threads, thread, thread_list);
uk_free(sched->allocator, thread->sched_info);
        uk_thread_fini(thread, sched->allocator);
@@ -238,9 +242,6 @@ void uk_sched_thread_exit(void)
        struct uk_thread *thread;
thread = uk_thread_current();
-
-       uk_printd(DLVL_INFO, "Thread \"%s\" exited.\n", thread->name);
-
        UK_ASSERT(thread->sched);
        uk_sched_thread_remove(thread->sched, thread);
        UK_CRASH("Error stopping thread.");
diff --git a/lib/ukschedcoop/schedcoop.c b/lib/ukschedcoop/schedcoop.c
index f616330..a2bf2ad 100644
--- a/lib/ukschedcoop/schedcoop.c
+++ b/lib/ukschedcoop/schedcoop.c
@@ -37,7 +37,6 @@
struct schedcoop_private {
        struct uk_thread_list thread_list;
-       struct uk_thread_list exited_threads;
        int threads_started;
  };
@@ -124,12 +123,9 @@ static void schedcoop_schedule(struct uk_sched *s)
        if (prev != next)
                uk_sched_thread_switch(s, prev, next);
- UK_TAILQ_FOREACH_SAFE(thread, &prv->exited_threads, thread_list, tmp) {
-               if (thread != prev) {
-                       UK_TAILQ_REMOVE(&prv->exited_threads,
-                                       thread, thread_list);
+       UK_TAILQ_FOREACH_SAFE(thread, &s->exited_threads, thread_list, tmp) {
+               if (thread != prev)
                        uk_thread_destroy(thread);
-               }
        }
  }
@@ -166,7 +162,7 @@ static void schedcoop_thread_remove(struct uk_sched *s, struct uk_thread *t)
        clear_runnable(t);
/* Put onto exited list */
-       UK_TAILQ_INSERT_HEAD(&prv->exited_threads, t, thread_list);
+       UK_TAILQ_INSERT_HEAD(&s->exited_threads, t, thread_list);
ukplat_lcpu_restore_irqf(flags); @@ -211,7 +207,6 @@ struct uk_sched *uk_schedcoop_init(struct uk_alloc *a)
        ukplat_ctx_callbacks_init(&sched->plat_ctx_cbs, ukplat_ctx_sw);
prv = sched->prv;
-       UK_TAILQ_INIT(&prv->exited_threads);
        UK_TAILQ_INIT(&prv->thread_list);
        prv->threads_started = 0;

--
Dr. Florian Schmidt
フローリアン・シュミット
Research Scientist,
Systems and Machine Learning Group
NEC Laboratories Europe
Kurfürsten-Anlage 36, D-69115 Heidelberg
Tel.     +49 (0)6221 4342-265
Fax:     +49 (0)6221 4342-155
e-mail:  florian.schmidt@xxxxxxxxx
============================================================
Registered at Amtsgericht Mannheim, Germany, HRB728558

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.