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 5:41 PM, Simon Kuenzer wrote:
> Hey Costin,
> 
> On 20.05.19 11:42, Costin Lupu wrote:
>> 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.
> 
> uksched is already a higher-level library needing this feature. So, I
> think in this sense this is correct. Also, not all programs or higher
> -level libs will make use of TLS; so it makes sense to split this
> functionality, right?
> 

I thought a higher-level library is an external library. Now that I
think about it, it's the first time I hear about a 'higher-level'
library and I don't even know where or when its definition was established.

However, the thread related functionality that's in the platform code
and it's actually arch dependent is also used by a 'higher-level'
library (e.g. the thread switching code which is used/called by uksched
and ukschedcoop). Event this TLS series has both changes for arch/ and
plat/ and both are used by the 'higher-level' libs.

>>
>> 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.
> 
> I agree. This structuring has always been difficult and is still fuzzy.
> The origin of this problem is that we had to learn how the different
> systems work and how we could do the splitting. In my opinion we should
> revisit how to reorganize the architecture and especially platform code.
> I hope that we are now/soon able come up with a better scheme. For
> instance, we had already in mind to use an approach that considers
> functional aspect instead: Instead of having a folder for each platform,
> we would differentiate different virtual CPU types. Each platform
> Makefile would then pick and compile a combination together... But yes,
> I also see the need to sit down and discuss this. But this also means
> that I would worry less for these patches.
> 

If a refactoring is needed, then it will be simpler if we continue using
the conventions we used so far until then and put the code in one single
place instead of putting code in different places.

>>
>> 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
>>
> 
> Thanks,
> 
> Simon

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