Re: [Minios-devel] [UNIKRAFT PATCH v2 5/8] lib/uksched: Add support for waiting threads

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Actually, now that I look at the next patch...

On 1/11/19 12:22 AM, Costin Lupu wrote:

> diff --git a/lib/ukschedcoop/schedcoop.c b/lib/ukschedcoop/schedcoop.c
> index e565240..f7ab92d 100644
> --- a/lib/ukschedcoop/schedcoop.c
> +++ b/lib/ukschedcoop/schedcoop.c
> @@ -125,6 +125,12 @@ static void schedcoop_schedule(struct uk_sched *s)
>                 uk_sched_thread_switch(s, prev, next);
>   
>   	UK_TAILQ_FOREACH_SAFE(thread, &prv->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);
> @@ -167,6 +173,7 @@ static void schedcoop_thread_remove(struct uk_sched *s, 
> struct uk_thread *t)
>   
>   	/* Put onto exited list */
>         UK_TAILQ_INSERT_HEAD(&prv->exited_threads, t, thread_list);
> +       uk_thread_exit(t);
>   
>   	ukplat_lcpu_restore_irqf(flags);
Doesn't calling uk_thread_exit(t) only after putting it on the 
exited_threads list a race condition? Because if now the main scheduling 
loop (as in schedcoop_schedule() is called again after inserting, but 
before thread_exit() is called, the could lead to an assertion failure 
when schedcoop_schedule() calls uk_sched_thread_destroy() on that 
thread, but uk_thread_exit() hasn't finished yet.
I understand that this probably is a crazy corner case, and I'm not sure 
I can even ever occur without SMP support, but, on the other hand: is 
there any harm in switching the order? And actually, wouldn't it make 
sense to put the list insertion into uk_thread_exit()? It's an important 
part of thread handling, so it could just be done in there, right? 
Although I realize that you only do this in the next patch, so... maybe 
only do that in there? Seems regardless of the order, one thing from the 
earlier patch always better waits until the latter patch.
Cheers,
Florian

--
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