[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 Costin, Hi Simon,
On 4/16/19 3:21 PM, Costin Lupu wrote:
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/.
I'll be honest, in my personal opinion, the organization of code that is
platform *and* architecture specific, and code that is architecture,
*but not* platform specific, is a huge mess at the moment. For example,
your reasoning that arch/ should only contain code "available to the
application" is new to me, I can't remember hearing it before. It's
certainly one way to structure it, though my first reaction is to
disagree with this concept. :-) I'd rather have both in arch/, and put
the "public" functions into one hierarchy, and the "non-public"
functions in another. If that's even necessary... if an application
developer wants to manually save and restore their registers, or change
the stack pointer, or whatever... we give everyone the freedom to shoot
themselves in the foot with the type of gun they choose.
About architecture-dependent code, I already had many discussion with
Simon when I did the extended register patch series, and it seems we're
always ending up where we started. Last time, I just metaphorically
threw my hands up in the air at some point and put it wherever, part of
which is why there's x86-specific code in sw_ctx.c (where it patently
shouldn't be, considering imminent arm support.) My point on your
comment is thus: I don't care any more where all this code goes. :-) I
foresee a massive refactoring in the future. Until then, I put the code
where it makes most sense to me, and am mostly dispassionate about where
it ends up in the end.
Bottom lime: as long as you two (you and Simon) come up with a
consistent suggestion where I should put this code, I will go with it
without any further complains.
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).
While it might not seem like it at first glance, this naming is actually
part of the discussions above. The idea was to eventually move other
architecture-specific thread switching code (register saving and
restoring) to that file, too, because that code is solely dependent on
the architecture and not on the platform. Depending on the outcome of
the above discussion, I'm fine with a name change though.
@@ -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.
Do you mean the spaces around the '+'? I can do that, sure.
+}
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.
Ok, sure, if you think that helps, I'll do it.
+
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.
I'm not sure I get your argument. What prevents you from still putting a
break point on the line and inspecting the return code after the
changes? Instead of a 'p rc', you do a 'fini' in create_stack, or a 'p
$rax' after returning from it.
+ 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().
OK, I'll change it.
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).
Whoops. Yeah, this is a holdover from my testing where I just wanted it
to spew out a message every time without having to switch down to "print
everything". Gonna fix that.
return 0;
}
--
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
|
