Re: [Minios-devel] [UNIKRAFT PATCH v2 5/5] lib/ukschedcoop: Fix race condition when trying to wake up current thread

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Costin,

I'll be honest: I'm not a big fan of this solution. After our offline 
discussion, I feel like going the way of making a current_thread global 
variable instead of carrying the information around on the stack solves 
the same problem with less complexity: it basically obviates the need 
for patches 2 and 3 of the series, and if setting current_thread at the 
right point during the thread switchingt, there's no need to introduce 
this new "queueable" flag, which I'm a bit worried we're adding now and 
might never get rid of again because nobody might get around to revisit 
this.
However, in light of the time constraints that we have currently, I'm 
willing to upstream this, because it doesn't help if this patch series 
gets stuck in limbo, either.
Reviewed-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>

On 5/29/19 5:56 AM, Costin Lupu wrote:
> The previous patch introduced a race condition: if the current thread
> blocks and yields, and an interrupt is triggered whose handle tries to
> wake up the current thread then the thread will remain blocked until the
> next interrupt (potentially forever) because the current thread cannot
> be added to the ready threads list.
>
> The solution is to set a flag on the current thread as soon as the next
> thread is scheduled. The interrupt will queue the current thread if and
> only if this flag is set.
>
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> ---
>   lib/uksched/include/uk/thread.h | 5 +++++
>   lib/ukschedcoop/schedcoop.c     | 7 ++++++-
>   2 files changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/lib/uksched/include/uk/thread.h b/lib/uksched/include/uk/thread.h
> index 71e39225..10fd4a33 100644
> --- a/lib/uksched/include/uk/thread.h
> +++ b/lib/uksched/include/uk/thread.h
> @@ -96,6 +96,7 @@ struct uk_thread *uk_thread_current(void)
>   
>   #define RUNNABLE_FLAG   0x00000001
>   #define EXITED_FLAG     0x00000002
> +#define QUEUEABLE_FLAG  0x00000004
>   
>   #define is_runnable(_thread)    ((_thread)->flags &   RUNNABLE_FLAG)
>   #define set_runnable(_thread)   ((_thread)->flags |=  RUNNABLE_FLAG)
> @@ -104,6 +105,10 @@ struct uk_thread *uk_thread_current(void)
>   #define is_exited(_thread)      ((_thread)->flags &   EXITED_FLAG)
>   #define set_exited(_thread)     ((_thread)->flags |=  EXITED_FLAG)
>   
> +#define is_queueable(_thread)    ((_thread)->flags &   QUEUEABLE_FLAG)
> +#define set_queueable(_thread)   ((_thread)->flags |=  QUEUEABLE_FLAG)
> +#define clear_queueable(_thread) ((_thread)->flags &= ~QUEUEABLE_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/ukschedcoop/schedcoop.c b/lib/ukschedcoop/schedcoop.c
> index 28667bf1..5e7dbe89 100644
> --- a/lib/ukschedcoop/schedcoop.c
> +++ b/lib/ukschedcoop/schedcoop.c
> @@ -101,6 +101,9 @@ static void schedcoop_schedule(struct uk_sched *s)
>                         if (is_runnable(prev))
>                                 UK_TAILQ_INSERT_TAIL(&prv->thread_list, prev,
>                                                 thread_list);
> +                       else
> +                               set_queueable(prev);
> +                       clear_queueable(next);
>                         ukplat_stack_set_current_thread(next);
>                         break;
>                 } else if (is_runnable(prev)) {
> @@ -196,8 +199,10 @@ static void schedcoop_thread_woken(struct uk_sched *s, 
> struct uk_thread *t)
>   
>   	if (t->wakeup_time > 0)
>                 UK_TAILQ_REMOVE(&prv->sleeping_threads, t, thread_list);
> -       if (t != uk_thread_current())
> +       if (t != uk_thread_current() || is_queueable(t)) {
>                 UK_TAILQ_INSERT_TAIL(&prv->thread_list, t, thread_list);
> +               clear_queueable(t);
> +       }
>   }
>   
>   static void idle_thread_fn(void *unused __unused)
--
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