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

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Florian,

Please see my comments inline.

On 1/2/19 4:45 PM, Florian Schmidt wrote:
> 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?
> 

That's 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.
> 

You're right. Now if I think it over again, it would also make sense to
swap these 2 commits ("Add support for waiting..." before "Revisit..."),
in addition to your suggestions.

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

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