Re: [Minios-devel] [UNIKRAFT PATCH v2 8/8] lib/uksched: Minor refinements

[Florian Schmidt <Florian.Schmidt@xxxxxxxxx>]

Hi Costin,

my suggestion for this patch would be to actually split it. All those 
things are more or less independent, and while each patch would then be 
very small, I think it's better to have conceptually different code 
changes in separate commits. That way you could also add some short 
description about the why's of the patches. For example, why the 
schedulers should set the scheduler reference on the thread instead of 
thread adding logic, or why uk_sched_start should be public.
Also, I'm not sure I understand the reent part. The REENT_PTR gets 
initialized, but then never used? Is this strictly for the _r functions 
of newlib? And what happens if you don't add this? I have to say I'm not 
a huge fan of having code that deals with external libraries in the main 
unikraft code base, but I'm open to arguments.
Cheers,
Florian

On 1/11/19 12:22 AM, Costin Lupu wrote:
> - Schedulers implementations should set scheduler reference on threads
> - uk_sched_start is public
> - initialize reentrant field on threads if newlib enabled
> - remove redundant config dependencies on ukschedcoop lib
>
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> ---
>   lib/uksched/include/uk/sched.h  | 14 ++++++--------
>   lib/uksched/include/uk/thread.h |  4 ++--
>   lib/uksched/thread.c            |  6 ++++--
>   lib/ukschedcoop/Config.uk       |  2 --
>   lib/ukschedcoop/schedcoop.c     |  1 +
>   5 files changed, 13 insertions(+), 14 deletions(-)
>
> diff --git a/lib/uksched/include/uk/sched.h b/lib/uksched/include/uk/sched.h
> index 5800c07..9e04bfa 100644
> --- a/lib/uksched/include/uk/sched.h
> +++ b/lib/uksched/include/uk/sched.h
> @@ -112,7 +112,6 @@ static inline int uk_sched_thread_add(struct uk_sched *s,
>   {
>         UK_ASSERT(s);
>         UK_ASSERT(t);
> -       t->sched = s;
>         return s->thread_add(s, t, attr);
>   }
>   
> @@ -122,7 +121,6 @@ static inline void uk_sched_thread_remove(struct uk_sched *s,
>         UK_ASSERT(s);
>         UK_ASSERT(t);
>         s->thread_remove(s, t);
> -       t->sched = NULL;
>   }
>   
>   static inline int uk_sched_thread_set_prio(struct uk_sched *s,
> @@ -184,12 +182,6 @@ static inline struct uk_thread *uk_sched_get_idle(struct 
> uk_sched *s)
>         return &s->idle;
>   }
>   
> -/*
> - * Public scheduler functions
> - */
> -
> -void uk_sched_start(struct uk_sched *sched) __noreturn;
> -
>   #define uk_sched_init(s, yield_func, \
>                 thread_add_func, thread_remove_func, \
>                 thread_set_prio_func, thread_get_prio_func, \
> @@ -205,6 +197,12 @@ void uk_sched_start(struct uk_sched *sched) __noreturn;
>                 uk_sched_register((s)); \
>         } while (0)
>   
> +/*
> + * Public scheduler functions
> + */
> +
> +void uk_sched_start(struct uk_sched *sched) __noreturn;
> +
>   
>   /*
>    * Internal thread scheduling functions
> diff --git a/lib/uksched/include/uk/thread.h b/lib/uksched/include/uk/thread.h
> index 4c4e037..7f3c2fd 100644
> --- a/lib/uksched/include/uk/thread.h
> +++ b/lib/uksched/include/uk/thread.h
> @@ -29,7 +29,7 @@
>   #define __UK_THREAD_H__
>   
>   #include <stdint.h>
> -#ifdef CONFIG_HAVE_LIBC
> +#ifdef CONFIG_LIBNEWLIBC
>   #include <sys/reent.h>
>   #endif
>   #include <uk/arch/lcpu.h>
> @@ -56,7 +56,7 @@ struct uk_thread {
>         struct uk_waitq waiting_threads;
>         struct uk_sched *sched;
>         void *sched_info;
> -#ifdef CONFIG_HAVE_LIBC
> +#ifdef CONFIG_LIBNEWLIBC
>         struct _reent reent;
>   #endif
>   };
> diff --git a/lib/uksched/thread.c b/lib/uksched/thread.c
> index bf1e0d3..d9d340c 100644
> --- a/lib/uksched/thread.c
> +++ b/lib/uksched/thread.c
> @@ -29,6 +29,7 @@
>    * Thread definitions
>    * Ported from Mini-OS
>    */
> +#include <string.h>
>   #include <stdlib.h>
>   #include <errno.h>
>   #include <uk/plat/config.h>
> @@ -89,10 +90,11 @@ int uk_thread_init(struct uk_thread *thread,
>         thread->flags = 0;
>         thread->wakeup_time = 0LL;
>         uk_waitq_init(&thread->waiting_threads);
> +       thread->sched = NULL;
>         thread->sched_info = NULL;
>   
> -#ifdef CONFIG_HAVE_LIBC
> -       //TODO _REENT_INIT_PTR(&thread->reent);
> +#ifdef CONFIG_LIBNEWLIBC
> +       _REENT_INIT_PTR(&thread->reent);
>   #endif
>   
>   	uk_pr_info("Thread \"%s\": pointer: %p, stack: %p\n",
> diff --git a/lib/ukschedcoop/Config.uk b/lib/ukschedcoop/Config.uk
> index b4277a1..8a50725 100644
> --- a/lib/ukschedcoop/Config.uk
> +++ b/lib/ukschedcoop/Config.uk
> @@ -1,6 +1,4 @@
>   config LIBUKSCHEDCOOP
>         bool "ukschedcoop: Cooperative Round-Robin scheduler"
>         default n
> -       select LIBNOLIBC if !HAVE_LIBC
> -       select LIBUKDEBUG
>         select LIBUKSCHED
> diff --git a/lib/ukschedcoop/schedcoop.c b/lib/ukschedcoop/schedcoop.c
> index 3536ea6..3d42626 100644
> --- a/lib/ukschedcoop/schedcoop.c
> +++ b/lib/ukschedcoop/schedcoop.c
> @@ -148,6 +148,7 @@ static int schedcoop_thread_add(struct uk_sched *s, struct 
> uk_thread *t,
>         }
>   
>   	set_runnable(t);
> +       t->sched = s;
>   
>   	flags = ukplat_lcpu_save_irqf();
>         UK_TAILQ_INSERT_TAIL(&prv->thread_list, t, thread_list);
--
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