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

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Apologies, this mail got only sent to Costin directly instead of to the 
list. My comment was:

This patch looks fine. My remark about the ASSERT in v1 doesn't apply if 
you invert the order, so:
Reviewed-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>

On 1/10/19 4:22 PM, Costin Lupu wrote:
> We use a list for terminated threads on all schedulers because it
> keeps references to those threads until wait is called for them.
>
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> ---
>   lib/uksched/include/uk/sched.h |  1 +
>   lib/uksched/sched.c            |  7 ++++---
>   lib/ukschedcoop/schedcoop.c    | 11 +++--------
>   3 files changed, 8 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/sched.c b/lib/uksched/sched.c
> index 6e1d8ee..ed55a64 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));
> +
> +       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_pr_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 f7ab92d..5616ee1 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,18 +123,15 @@ 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) {
> +       UK_TAILQ_FOREACH_SAFE(thread, &s->exited_threads, thread_list, tmp) {
>                 struct thread_info_base *tib = thread->sched_info;
>   
>   		if (!tib->is_detached)
>                         /* someone will eventually wait for it */
>                         continue;
>   
> -		if (thread != prev) {
> -                       UK_TAILQ_REMOVE(&prv->exited_threads,
> -                                       thread, thread_list);
> +               if (thread != prev)
>                         uk_thread_destroy(thread);
> -               }
>         }
>   }
>   
> @@ -172,7 +168,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);
>         uk_thread_exit(t);
>   
>   	ukplat_lcpu_restore_irqf(flags);
> @@ -218,7 +214,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