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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

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?
> 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.
> 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