|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Minios-devel] [UNIKRAFT PATCH 2/4] lib/uksched: create and delete thread-local storage area
Hi Florian,
Please see my comments inline.
On 4/15/19 1:03 PM, Florian Schmidt wrote:
> Most of the actual TLS setup is architecture-specific, so sched.c and
> thread.c call functions from a header file provided in arch/.
>
> Signed-off-by: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> ---
> arch/x86/x86_64/include/uk/asm/thread.h | 64 +++++++++++++++++++++++++
> include/uk/arch/thread.h | 40 ++++++++++++++++
> lib/uksched/include/uk/sched.h | 3 ++
> lib/uksched/include/uk/thread.h | 3 +-
> lib/uksched/sched.c | 54 ++++++++++++++-------
> lib/uksched/thread.c | 8 ++--
> 6 files changed, 152 insertions(+), 20 deletions(-)
> create mode 100644 arch/x86/x86_64/include/uk/asm/thread.h
> create mode 100644 include/uk/arch/thread.h
>
> diff --git a/arch/x86/x86_64/include/uk/asm/thread.h
> b/arch/x86/x86_64/include/uk/asm/thread.h
> new file mode 100644
> index 00000000..6e95848d
> --- /dev/null
> +++ b/arch/x86/x86_64/include/uk/asm/thread.h
I had a discussion with Simon (CCed) a while ago, when I was working on
the changes for preemptive scheduling, about where we should put the
arch dependent code of threads. We agreed back then to keep it in the
plat/ code because arch/ contains only code that is available to user
applications as well. This is not the case for the TLS changes since
they are private to Unikraft, so please move this to
plat/common/include/x86/ . Similarly, .c files for thread arch dependent
code should be in plat/common/x86/.
Moreover, I would rename it to tls.h and use it also in the next patch
instead of thread_switch.h (see the discussion on the next patch).
> @@ -0,0 +1,64 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> + *
> + * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +
> +#ifndef __UKARCH_THREAD_H__
> +#error Do not include this header directly
> +#endif
> +
> +extern char _tls_start[], _etdata[], _tls_end[];
> +
> +static inline unsigned long tls_area_size(void)
> +{
> + /* x86_64 ABI requires that fs:%0 contains the address of itself, to
> + * allow certain optimizations. Hence, the overall size of the size of
> + * the TLS area, plus 8 bytes.
> + */
> + return _tls_end - _tls_start + 8;
> +}
> +
> +static inline unsigned int tls_area_align(void)
> +{
> + return 8;
> +}
> +
> +static inline void tls_copy(void *tls_area)
> +{
> + unsigned int tls_len = _tls_end - _tls_start;
> + unsigned int tls_data_len = _etdata - _tls_start;
> + unsigned int tls_bss_len = _tls_end - _etdata;
> +
> + memcpy(tls_area, _tls_start, tls_data_len);
> + memset(tls_area+tls_data_len, 0, tls_bss_len);
> + *((uintptr_t *)(tls_area+tls_len)) = (uintptr_t)(tls_area+tls_len);
I know this passes the checkpatch, but please use the Unikraft coding
style for the spacing of these 2 lines of code in order to avoid
propagating such examples.
> +}
> diff --git a/include/uk/arch/thread.h b/include/uk/arch/thread.h
> new file mode 100644
> index 00000000..76fd3ad5
> --- /dev/null
> +++ b/include/uk/arch/thread.h
Same comments as for the location of
arch/x86/x86_64/include/uk/asm/thread.h .
> @@ -0,0 +1,40 @@
> +/* SPDX-License-Identifier: BSD-3-Clause */
> +/*
> + * Authors: Florian Schmidt <florian.schmidt@xxxxxxxxx>
> + *
> + * Copyright (c) 2019, NEC Europe Ltd., NEC Corporation. All rights reserved.
> + *
> + * Redistribution and use in source and binary forms, with or without
> + * modification, are permitted provided that the following conditions
> + * are met:
> + *
> + * 1. Redistributions of source code must retain the above copyright
> + * notice, this list of conditions and the following disclaimer.
> + * 2. Redistributions in binary form must reproduce the above copyright
> + * notice, this list of conditions and the following disclaimer in the
> + * documentation and/or other materials provided with the distribution.
> + * 3. Neither the name of the copyright holder nor the names of its
> + * contributors may be used to endorse or promote products derived from
> + * this software without specific prior written permission.
> + *
> + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS
> IS"
> + * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
> + * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
> + * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE
> + * LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
> + * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
> + * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
> + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
> + * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
> + * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
> + * POSSIBILITY OF SUCH DAMAGE.
> + *
> + * THIS HEADER MAY NOT BE EXTRACTED OR MODIFIED IN ANY WAY.
> + */
> +
> +#ifndef __UKARCH_THREAD_H__
> +#define __UKARCH_THREAD_H__
> +
> +#include <uk/asm/thread.h>
> +
> +#endif /* __UKARCH_THREAD_H__ */
> diff --git a/lib/uksched/include/uk/sched.h b/lib/uksched/include/uk/sched.h
> index cc5fbe93..3e21d8d8 100644
> --- a/lib/uksched/include/uk/sched.h
> +++ b/lib/uksched/include/uk/sched.h
> @@ -50,6 +50,9 @@ struct uk_sched;
>
> struct uk_sched *uk_sched_default_init(struct uk_alloc *a);
>
> +extern char _tls_start[], _etdata[], _tls_end[];
> +#define have_tls_area (_tls_end - _tls_start)
Given that this is actually a predicate and you don't use uppercase in
the name of the macro, renaming it to have_tls_area() might be more
suggestive.
> +
> extern struct uk_sched *uk_sched_head;
> int uk_sched_register(struct uk_sched *s);
> struct uk_sched *uk_sched_get_default(void);
> diff --git a/lib/uksched/include/uk/thread.h b/lib/uksched/include/uk/thread.h
> index 71e39225..8cb3d6b6 100644
> --- a/lib/uksched/include/uk/thread.h
> +++ b/lib/uksched/include/uk/thread.h
> @@ -50,6 +50,7 @@ struct uk_sched;
> struct uk_thread {
> const char *name;
> void *stack;
> + void *tls;
> void *ctx;
> UK_TAILQ_ENTRY(struct uk_thread) thread_list;
> uint32_t flags;
> @@ -106,7 +107,7 @@ struct uk_thread *uk_thread_current(void)
>
> int uk_thread_init(struct uk_thread *thread,
> struct ukplat_ctx_callbacks *cbs, struct uk_alloc *allocator,
> - const char *name, void *stack,
> + const char *name, void *stack, void *tls,
> void (*function)(void *), void *arg);
> void uk_thread_fini(struct uk_thread *thread,
> struct uk_alloc *allocator);
> diff --git a/lib/uksched/sched.c b/lib/uksched/sched.c
> index e0416736..c46db730 100644
> --- a/lib/uksched/sched.c
> +++ b/lib/uksched/sched.c
> @@ -33,10 +33,12 @@
> */
>
> #include <stdlib.h>
> +#include <string.h>
> #include <uk/plat/config.h>
> #include <uk/plat/thread.h>
> #include <uk/alloc.h>
> #include <uk/sched.h>
> +#include <uk/arch/thread.h>
> #if CONFIG_LIBUKSCHEDCOOP
> #include <uk/schedcoop.h>
> #endif
> @@ -148,27 +150,42 @@ static void *create_stack(struct uk_alloc *allocator)
> return stack;
> }
>
> +static void *uk_thread_tls_create(struct uk_alloc *allocator)
> +{
> + void *tls;
> +
> + if (uk_posix_memalign(allocator, &tls,
> + tls_area_align(), tls_area_size()))
I believe it would be nice to be able to check the returned value on
errors, so please use a variable for that.
> + return NULL;
> + tls_copy(tls);
> + return tls;
> +}
> +
> void uk_sched_idle_init(struct uk_sched *sched,
> void *stack, void (*function)(void *))
> {
> struct uk_thread *idle;
> - int rc;
Please don't remove the rc variable. See the comments below.
> + void *tls = NULL;
>
> UK_ASSERT(sched != NULL);
>
> - if (stack == NULL)
> - stack = create_stack(sched->allocator);
> - UK_ASSERT(stack != NULL);
> + if (!stack && !(stack = create_stack(sched->allocator)))
> + goto out_crash;
Please don't squash these 2 lines since this change makes debugging more
difficult. With the original code one can put a breakpoint on the line
with the call for create_stack() and check the returned value. I suggest
you would use the same approach for the following lines of code as well.
> + if (have_tls_area && !(tls = uk_thread_tls_create(sched->allocator)))
> + goto out_crash;
>
> idle = &sched->idle;
>
> - rc = uk_thread_init(idle,
> + if (uk_thread_init(idle,
Please keep this line as it was because this change make debugging more
difficult. We use variables to save the returned values of functions in
order to inspect the returned value when debugging. It doesn't hurt the
performance at all, so keeping this approach is on the plus side.
> &sched->plat_ctx_cbs, sched->allocator,
> - "Idle", stack, function, NULL);
> - if (rc)
> - UK_CRASH("Error initializing idle thread.");
> + "Idle", stack, tls, function, NULL))
> + goto out_crash;
>
> idle->sched = sched;
> + return;
> +
> +out_crash:
> + UK_CRASH("Error initializing the idle thread.");
> }
>
> struct uk_thread *uk_sched_thread_create(struct uk_sched *sched,
> @@ -177,7 +194,7 @@ struct uk_thread *uk_sched_thread_create(struct uk_sched
> *sched,
> {
> struct uk_thread *thread = NULL;
> void *stack = NULL;
> - int rc;
Again, please don't remove the rc variable.
> + void *tls = NULL;
>
> thread = uk_malloc(sched->allocator, sizeof(struct uk_thread));
> if (thread == NULL) {
> @@ -188,18 +205,17 @@ struct uk_thread *uk_sched_thread_create(struct
> uk_sched *sched,
> /* We can't use lazy allocation here
> * since the trap handler runs on the stack
> */
> - stack = create_stack(sched->allocator);
> - if (stack == NULL)
> + if (!(stack = create_stack(sched->allocator)))
> + goto err;
Again, please keep this as it was. Same discussion as above.
> + if (have_tls_area && !(tls = uk_thread_tls_create(sched->allocator)))
> goto err;
>
> - rc = uk_thread_init(thread,
> + if (uk_thread_init(thread,
Same here.
> &sched->plat_ctx_cbs, sched->allocator,
> - name, stack, function, arg);
> - if (rc)
> + name, stack, tls, function, arg))
> goto err;
>
> - rc = uk_sched_thread_add(sched, thread, attr);
> - if (rc)
> + if (uk_sched_thread_add(sched, thread, attr))
Same here.
> goto err_add;
>
> return thread;
> @@ -207,6 +223,8 @@ struct uk_thread *uk_sched_thread_create(struct uk_sched
> *sched,
> err_add:
> uk_thread_fini(thread, sched->allocator);
> err:
> + if (tls)
> + uk_free(sched->allocator, tls);
> if (stack)
> uk_free(sched->allocator, stack);
> if (thread)
> @@ -219,11 +237,15 @@ void uk_sched_thread_destroy(struct uk_sched *sched,
> struct uk_thread *thread)
> {
> UK_ASSERT(sched != NULL);
> UK_ASSERT(thread != NULL);
> + UK_ASSERT(thread->stack != NULL);
> + UK_ASSERT(!have_tls_area || thread->tls != NULL);
When I first saw this usage of have_tls_area I thought it's a (global)
variable. But it's not, so that's why I recommend changing it to
have_tls_area().
> UK_ASSERT(is_exited(thread));
>
> UK_TAILQ_REMOVE(&sched->exited_threads, thread, thread_list);
> uk_thread_fini(thread, sched->allocator);
> uk_pfree(sched->allocator, thread->stack, STACK_SIZE_PAGE_ORDER);
> + if (thread->tls)
> + uk_free(sched->allocator, thread->tls);
> uk_free(sched->allocator, thread);
> }
>
> diff --git a/lib/uksched/thread.c b/lib/uksched/thread.c
> index 7400baee..a97fad9e 100644
> --- a/lib/uksched/thread.c
> +++ b/lib/uksched/thread.c
> @@ -88,13 +88,14 @@ struct _reent *__getreent(void)
>
> int uk_thread_init(struct uk_thread *thread,
> struct ukplat_ctx_callbacks *cbs, struct uk_alloc *allocator,
> - const char *name, void *stack,
> + const char *name, void *stack, void *tls,
> void (*function)(void *), void *arg)
> {
> unsigned long sp;
>
> UK_ASSERT(thread != NULL);
> UK_ASSERT(stack != NULL);
> + UK_ASSERT(!have_tls_area || tls != NULL);
>
> /* Save pointer to the thread on the stack to get current thread */
> *((unsigned long *) stack) = (unsigned long) thread;
> @@ -108,6 +109,7 @@ int uk_thread_init(struct uk_thread *thread,
>
> thread->name = name;
> thread->stack = stack;
> + thread->tls = tls;
>
> /* Not runnable, not exited, not sleeping */
> thread->flags = 0;
> @@ -121,8 +123,8 @@ int uk_thread_init(struct uk_thread *thread,
> reent_init(&thread->reent);
> #endif
>
> - uk_pr_info("Thread \"%s\": pointer: %p, stack: %p\n",
> - name, thread, thread->stack);
> + uk_pr_warn("Thread \"%s\": pointer: %p, stack: %p, tls: %p\n",
> + name, thread, thread->stack, thread->tls);
Since we're changing this, please change it to uk_pr_debug() instead
because:
1) It is a debug message, not a warning.
2) It's exiting counterpart message is printed only for debug.
3) This would flood the output for scenarios when lots of threads are
used (e.g. the unit tests of pthread-embedded).
>
> return 0;
> }
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |