Re: [Minios-devel] [UNIKRAFT PATCH v4 08/12] lib/uksched: Set scheduler reference only if adding the thread was successful

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Costin,

thanks for the quick iteration to v4. I like this much better, I just 
have one remark:
uk_sched_thread_create should imho check the (now introduced) return 
value of uk_thread_sched_add(), and do error handling (probably a 'goto 
err' in that function to clean up and return NULL).
Also, one more thing that I wouldn't have remarked if it had been the 
only thing, but now that I'm suggesting changes already:
On 3/22/19 3:16 PM, Costin Lupu wrote:
> Scheduler may reject adding threads if certain conditions are not met.
> Therefore we set the scheduler reference only if adding the thread was
> successful.
>
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> ---
>   lib/uksched/include/uk/sched.h | 17 ++++++++++++-----
>   lib/uksched/thread.c           |  1 +
>   lib/ukschedcoop/schedcoop.c    |  4 +++-
>   3 files changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/lib/uksched/include/uk/sched.h b/lib/uksched/include/uk/sched.h
> index 9e218688..3eb1f4ec 100644
> --- a/lib/uksched/include/uk/sched.h
> +++ b/lib/uksched/include/uk/sched.h
> @@ -59,7 +59,7 @@ int uk_sched_set_default(struct uk_sched *s);
>   typedef void  (*uk_sched_yield_func_t)
>                 (struct uk_sched *s);
>   
> -typedef void  (*uk_sched_thread_add_func_t)
> +typedef int   (*uk_sched_thread_add_func_t)
>                 (struct uk_sched *s, struct uk_thread *t,
>                         const uk_thread_attr_t *attr);
>   typedef void  (*uk_sched_thread_remove_func_t)
> @@ -107,24 +107,31 @@ static inline void uk_sched_yield(void)
>         s->yield(s);
>   }
>   
> -static inline void uk_sched_thread_add(struct uk_sched *s,
> +static inline int uk_sched_thread_add(struct uk_sched *s,
>                 struct uk_thread *t, const uk_thread_attr_t *attr)
>   {
> +       int rc;
> +
>         UK_ASSERT(s);
>         UK_ASSERT(t);
>         if (attr)
>                 t->detached = attr->detached;
> -       t->sched = s;
> -       s->thread_add(s, t, attr);
> +       rc = s->thread_add(s, t, attr);
> +       if (rc == 0)
> +               t->sched = s;
> +       return rc;
>   }
>   
> -static inline void uk_sched_thread_remove(struct uk_sched *s,
> +static inline int uk_sched_thread_remove(struct uk_sched *s,
>                 struct uk_thread *t)
>   {
>         UK_ASSERT(s);
>         UK_ASSERT(t);
> +       if (t->sched != s)
> +               return -EINVAL;
I like this extra sanity check. However, considering this is an internal 
function that is not exported, any such error would mean something's 
seriously wrong with uksched itself, and it's not just a user error. It 
simply "should never happen"(tm). So I would suggest to make this check 
a UK_ASSERT instead, so it'll be checked on debug builds, but optimized 
out on non-debug builds.
>         s->thread_remove(s, t);
>         t->sched = NULL;
> +       return 0;
>   }
>   
>   static inline int uk_sched_thread_set_prio(struct uk_sched *s,
> diff --git a/lib/uksched/thread.c b/lib/uksched/thread.c
> index ff0fb39f..4d931a74 100644
> --- a/lib/uksched/thread.c
> +++ b/lib/uksched/thread.c
> @@ -89,6 +89,7 @@ int uk_thread_init(struct uk_thread *thread,
>         thread->wakeup_time = 0LL;
>         thread->detached = false;
>         uk_waitq_init(&thread->waiting_threads);
> +       thread->sched = NULL;
>   
>   #ifdef CONFIG_HAVE_LIBC
>         //TODO _REENT_INIT_PTR(&thread->reent);
> diff --git a/lib/ukschedcoop/schedcoop.c b/lib/ukschedcoop/schedcoop.c
> index a3f265c6..559df006 100644
> --- a/lib/ukschedcoop/schedcoop.c
> +++ b/lib/ukschedcoop/schedcoop.c
> @@ -132,7 +132,7 @@ static void schedcoop_schedule(struct uk_sched *s)
>         }
>   }
>   
> -static void schedcoop_thread_add(struct uk_sched *s, struct uk_thread *t,
> +static int schedcoop_thread_add(struct uk_sched *s, struct uk_thread *t,
>         const uk_thread_attr_t *attr __unused)
>   {
>         unsigned long flags;
> @@ -143,6 +143,8 @@ static void schedcoop_thread_add(struct uk_sched *s, struct 
> uk_thread *t,
>         flags = ukplat_lcpu_save_irqf();
>         UK_TAILQ_INSERT_TAIL(&prv->thread_list, t, thread_list);
>         ukplat_lcpu_restore_irqf(flags);
> +
> +       return 0;
>   }
>   
>   static void schedcoop_thread_remove(struct uk_sched *s, struct uk_thread *t)
--
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