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

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Florian,

Please see my comments inline.

On 1/23/19 3:48 PM, Florian Schmidt wrote:
> 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.
> 

Will move this before the insertion in v3.

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

uk_thread_exit() is a thread 'method', while the exited_threads list
belongs to the scheduler. Since the thread doesn't have any kind of
ownership of the exited_threads list, it's better to not move the
insertion in uk_thread_list().

Costin

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