Re: [Minios-devel] [UNIKRAFT PATCH 2/4] lib/uksched: create and delete thread-local storage area

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Simon,

On 5/20/19 12:26 PM, Simon Kuenzer wrote:
> On 16.04.19 15:21, 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/.
>>
>> 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).
>>
> 
> Actually, after reading this patch, I agree to put this TLS code to
> arch. The main criteria for arch code is that it only depends on a
> particular architecture but is completely independent to any platform
> (linuxu, xen, kvm). It is right that programs and higher-level libraries
> can access this defintions, but this is also intended: Since this is a
> compiler thing I expect that every architecture provides this kind of
> code, so that uksched or any libc is able to do TLS operations
> independent of archictecture and platform.
> 
> However, I would not call the header thread, because it implements just
> a subset. I would call it something along `tls.h` and also namespace the
> provided functions, symbols, and macros: e.g., ukarch_tls_* and
> UKARCH_TLS_*. Please double-check this in all headers.

I don't think this is intended to be accessed by higher level libraries.
It's just logic internal to threads management, higher level libs
shouldn't care about it, nor access it.

Besides that, this would also add another exception, wouldn't it?
Because there is no precedent case which can be classified in the same
category. Following this new criteria you intend to introduce, we have
thread related code which would suit the same criteria but it was
previously put under platform code.

The conclusion is clear: right now, the way we split code into arch and
platform is wrong. We don't have clear principles regarding arch code
and platform code, that's why we have this kind of discussions over and
over again.

To clarify my position, personally, I have always opted for putting arch
code into the arch directory and I believe this starts to get
unavoidable, as we can already see. But I also prefer consistency.

Cheers,
Costin

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel