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

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Felipe,

On 9/25/18 1:16 PM, Felipe Huici wrote:
> Hi,
> 
> 
>> Similar to waiting processes, we introduce support for waiting
>> threads. This is the conventional way of getting notified by
>> threads termination events and freeing the threads resources upon
>> exit.
> 
> s/by threads termination/by thread termination
> s/freeing the threads/freeing thread
> 
>> A thread can be waited by other threads. If no such behaviour is
>> desired then one should set the detached attribute for the thread.
>> This implies that the thread resources will be automatically freed
>> when the thread exits.
> 
> s/waited by/waited on by
> 
>> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
>> ---
>> lib/uksched/exportsyms.uk       |  3 +++
>> lib/uksched/include/uk/thread.h |  6 +++++
>> lib/uksched/thread.c            | 55
>> +++++++++++++++++++++++++++++++++++++++++
>> lib/ukschedcoop/schedcoop.c     |  7 ++++++
>> 4 files changed, 71 insertions(+)
>>
>> diff --git a/lib/uksched/exportsyms.uk b/lib/uksched/exportsyms.uk
>> index 951cd20..22c8913 100644
>> --- a/lib/uksched/exportsyms.uk
>> +++ b/lib/uksched/exportsyms.uk
>> @@ -14,6 +14,9 @@ uk_thread_fini
>> uk_thread_block_timeout
>> uk_thread_block
>> uk_thread_wake
>> +uk_thread_exit
>> +uk_thread_wait
>> +uk_thread_detach
>> uk_thread_set_prio
>> uk_thread_get_prio
>> uk_thread_set_timeslice
>> diff --git a/lib/uksched/include/uk/thread.h
>> b/lib/uksched/include/uk/thread.h
>> index 7a1b630..3267ab2 100644
>> --- a/lib/uksched/include/uk/thread.h
>> +++ b/lib/uksched/include/uk/thread.h
>> @@ -36,6 +36,7 @@
>> #include <uk/arch/time.h>
>> #include <uk/plat/thread.h>
>> #include <uk/thread_attr.h>
>> +#include <uk/wait_types.h>
>> #include <uk/list.h>
>> #include <uk/essentials.h>
>>
>> @@ -52,6 +53,7 @@ struct uk_thread {
>>      UK_TAILQ_ENTRY(struct uk_thread) thread_list;
>>      uint32_t flags;
>>      __snsec wakeup_time;
>> +    struct uk_waitq waiting_threads;
>>      struct uk_sched *sched;
>>      void *sched_info;
>> #ifdef CONFIG_HAVE_LIBC
>> @@ -68,6 +70,10 @@ UK_TAILQ_HEAD(uk_thread_list, struct uk_thread);
>>      uk_thread_create_attr(name, NULL, function, data)
>> #define uk_thread_destroy(thread) \
>>      uk_sched_thread_destroy(thread->sched, thread)
>> +void uk_thread_exit(struct uk_thread *thread);
>> +
>> +int uk_thread_wait(struct uk_thread *thread);
>> +int uk_thread_detach(struct uk_thread *thread);
>>
>> int uk_thread_set_prio(struct uk_thread *thread, prio_t prio);
>> int uk_thread_get_prio(const struct uk_thread *thread, prio_t *prio);
>> diff --git a/lib/uksched/thread.c b/lib/uksched/thread.c
>> index 3e69cfe..a06e338 100644
>> --- a/lib/uksched/thread.c
>> +++ b/lib/uksched/thread.c
>> @@ -36,6 +36,7 @@
>> #include <uk/thread.h>
>> #include <uk/_thread_info.h>
>> #include <uk/sched.h>
>> +#include <uk/wait.h>
>> #include <uk/print.h>
>> #include <uk/assert.h>
>>
>> @@ -87,6 +88,7 @@ int uk_thread_init(struct uk_thread *thread,
>>      /* Not runnable, not exited, not sleeping */
>>      thread->flags = 0;
>>      thread->wakeup_time = 0LL;
>> +    uk_waitq_init(&thread->waiting_threads);
>>      thread->sched_info = NULL;
>>
>> #ifdef CONFIG_HAVE_LIBC
>> @@ -132,6 +134,59 @@ void uk_thread_wake(struct uk_thread *thread)
>>      set_runnable(thread);
>> }
>>
>> +void uk_thread_exit(struct uk_thread *thread)
>> +{
>> +    struct thread_info_base *tib;
>> +
>> +    UK_ASSERT(thread);
>> +    UK_ASSERT(thread->sched);
>> +
>> +    set_exited(thread);
>> +
>> +    tib = thread->sched_info;
>> +
>> +    if (!tib->is_detached)
>> +            uk_waitq_wake_up(&thread->waiting_threads);
>> +
>> +    uk_printd(DLVL_EXTRA, "Thread \"%s\" exited.\n", thread->name);
>> +}
>> +
>> +int uk_thread_wait(struct uk_thread *thread)
>> +{
>> +    struct thread_info_base *tib;
>> +
>> +    UK_ASSERT(thread);
>> +    UK_ASSERT(thread->sched);
>> +
>> +    tib = thread->sched_info;
>> +
>> +    /* TODO critical region */
> 
> Is this coming in v2 of these series? :) Seems like a fairly important
> TODO.
> 

Unfortunately this won't be in v2. We need more analysis for identifying
all the critical regions that have to do with threads and thus it would
make sense to fix it in a different patch series. For now, it shouldn't
be a problem given that we don't have SMP or preemption support.

>> +
>> +    if (tib->is_detached)
>> +            return -1;
>> +
>> +    uk_waitq_wait_event(&thread->waiting_threads, is_exited(thread));
>> +
>> +    tib->is_detached = 1;
>> +
>> +    uk_sched_thread_destroy(thread->sched, thread);
>> +
>> +    return 0;
>> +}
>> +
>> +int uk_thread_detach(struct uk_thread *thread)
>> +{
>> +    struct thread_info_base *tib;
>> +
>> +    UK_ASSERT(thread);
>> +    UK_ASSERT(thread->sched);
>> +
>> +    tib = thread->sched_info;
>> +    tib->is_detached = 1;
>> +
>> +    return 0;
>> +}
> 
> As in previous patches, please use macros.
> 

As mentioned before, we can skip this for the current series.

>> +
>> int uk_thread_set_prio(struct uk_thread *thread, prio_t prio)
>> {
>>      if (!thread)
>> diff --git a/lib/ukschedcoop/schedcoop.c b/lib/ukschedcoop/schedcoop.c
>> index a2bf2ad..0a735bd 100644
>> --- a/lib/ukschedcoop/schedcoop.c
>> +++ b/lib/ukschedcoop/schedcoop.c
>> @@ -124,6 +124,12 @@ static void schedcoop_schedule(struct uk_sched *s)
>>              uk_sched_thread_switch(s, prev, next);
>>
>>      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_thread_destroy(thread);
>>      }
>> @@ -163,6 +169,7 @@ static void schedcoop_thread_remove(struct uk_sched
>> *s, struct uk_thread *t)
>>
>>      /* Put onto exited list */
>>      UK_TAILQ_INSERT_HEAD(&s->exited_threads, t, thread_list);
>> +    uk_thread_exit(t);
>>
>>      ukplat_lcpu_restore_irqf(flags);
>>
>> -- 
>> 2.11.0
>>
> 
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel
> 

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