|
[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,
I'll comment only the minor issues in this mail. Please see inline.
On 5/9/19 4:25 PM, Florian Schmidt wrote:
> 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.
>
Yes, please.
>>
>>> +}
>>> 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.
>
I want the breakpoint for only when the function is called (i.e. stack
is not NULL). Even if you would use a conditional breakpoint it would
still complicate things. Besides that, given that is nothing wrong with
the original code, changing it just to look different doesn't sound
right to me.
>>
>>> + 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;
>>> }
>>>
>
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |