[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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.