Message-ID: <BY5PR11MB38931405FE73FF989B773B77CC810@BY5PR11MB3893.namprd11.prod.outlook.com>
Date:   Fri, 12 Jun 2020 06:20:48 +0000
From:   "Tan, Ley Foon" <ley.foon.tan@...el.com>
To:     Christian Brauner <christian.brauner@...ntu.com>,
        Arnd Bergmann <arnd@...db.de>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "Thomas Gleixner" <tglx@...utronix.de>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: RE: [PATCH] nios2: enable HAVE_COPY_THREAD_TLS, switch to
 kernel_clone_args



> -----Original Message-----
> From: Christian Brauner <christian.brauner@...ntu.com>
> Sent: Sunday, May 24, 2020 6:06 PM
> To: Tan, Ley Foon <ley.foon.tan@...el.com>; Arnd Bergmann
> <arnd@...db.de>
> Cc: linux-kernel@...r.kernel.org; Thomas Gleixner <tglx@...utronix.de>;
> Sebastian Andrzej Siewior <bigeasy@...utronix.de>; Christian Brauner
> <christian.brauner@...ntu.com>
> Subject: [PATCH] nios2: enable HAVE_COPY_THREAD_TLS, switch to
> kernel_clone_args
> 
> This is part of a larger series that aims at getting rid of the
> copy_thread()/copy_thread_tls() split that makes the process creation
> codepaths in the kernel more convoluted and error-prone than they need to
> be.
> I'm converting all the remaining arches that haven't yet switched and am
> collecting individual acks. Once I have them, I'll send the whole series
> removing the copy_thread()/copy_thread_tls() split, the
> HAVE_COPY_THREAD_TLS define and the legacy do_fork() helper. The only
> kernel-wide process creation entry point for anything not going directly
> through the syscall path will then be based on struct kernel_clone_args.
> No more danger of weird process creation abi quirks between architectures
> hopefully, and easier to maintain overall.
> It also unblocks implementing clone3() on architectures not support
> copy_thread_tls(). Any architecture that wants to implement clone3() will
> need to select HAVE_COPY_THREAD_TLS and thus need to implement
> copy_thread_tls(). So both goals are connected but independently beneficial.
> 
> HAVE_COPY_THREAD_TLS means that a given architecture supports
> CLONE_SETTLS and not setting it should usually mean that the architectures
> doesn't implement it but that's not how things are. In fact all architectures
> support CLONE_TLS it's just that they don't follow the calling convention that
> HAVE_COPY_THREAD_TLS implies. That means all architectures can be
> switched over to select HAVE_COPY_THREAD_TLS. Once that is done we can
> remove that macro (yay, less code), the unnecessary do_fork() export in
> kernel/fork.c, and also rename copy_thread_tls() back to copy_thread(). At
> this point
> copy_thread() becomes the main architecture specific part of process
> creation but it will be the same layout and calling convention for all
> architectures. (Once that is done we can probably cleanup each
> copy_thread() function even more but that's for the future.)
> 
> Since nios2 does support CLONE_SETTLS there's no reason to not select
> HAVE_COPY_THREAD_TLS. This brings us one step closer to getting rid of the
> copy_thread()/copy_thread_tls() split we still have and ultimately the
> HAVE_COPY_THREAD_TLS define in general. A lot of architectures have
> already converted and nios2 is one of the few hat haven't yet. This also
> unblocks implementing the clone3() syscall on nios2. Once that is done we
> can get of another ARCH_WANTS_* macro.
> 
> Once Any architecture that supports HAVE_COPY_THREAD_TLS cannot call
> the
> do_fork() helper anymore. This is fine and intended since it should be
> removed in favor of the new, cleaner _do_fork() calling convention based on
> struct kernel_clone_args. In fact, most architectures have already switched.
> With this patch, nios2 joins the other arches which can't use the fork(),
> vfork(), clone(), clone3() syscalls directly and who follow the new process
> creation calling convention that is based on struct kernel_clone_args which
> we introduced a while back. This means less custom assembly in the
> architectures entry path to set up the registers before calling into the process
> creation helper and it is easier to to support new features without having to
> adapt calling conventions. It also unifies all process creation paths between
> fork(), vfork(), clone(), and clone3(). (We can't fix the ABI nightmare that
> legacy
> clone() is but we can prevent stuff like this happening in the future.)
> 
> For some more context, please see:
> commit 606e9ad20094f6d500166881d301f31a51bc8aa7
> Merge: ac61145a725a 457677c70c76
> Author: Linus Torvalds <torvalds@...ux-foundation.org>
> Date:   Sat Jan 11 15:33:48 2020 -0800
> 
>     Merge tag 'clone3-tls-v5.5-rc6' of
> git://git.kernel.org/pub/scm/linux/kernel/git/brauner/linux
> 
>     Pull thread fixes from Christian Brauner:
>      "This contains a series of patches to fix CLONE_SETTLS when used with
>       clone3().
> 
>       The clone3() syscall passes the tls argument through struct clone_args
>       instead of a register. This means, all architectures that do not
>       implement copy_thread_tls() but still support CLONE_SETTLS via
>       copy_thread() expecting the tls to be located in a register argument
>       based on clone() are currently unfortunately broken. Their tls value
>       will be garbage.
> 
>       The patch series fixes this on all architectures that currently define
>       __ARCH_WANT_SYS_CLONE3. It also adds a compile-time check to
> ensure
>       that any architecture that enables clone3() in the future is forced to
>       also implement copy_thread_tls().
> 
>       My ultimate goal is to get rid of the copy_thread()/copy_thread_tls()
>       split and just have copy_thread_tls() at some point in the not too
>       distant future (Maybe even renaming copy_thread_tls() back to simply
>       copy_thread() once the old function is ripped from all arches). This
>       is dependent now on all arches supporting clone3().
> 
>       While all relevant arches do that now there are still four missing:
>       ia64, m68k, sh and sparc. They have the system call reserved, but not
>       implemented. Once they all implement clone3() we can get rid of
>       ARCH_WANT_SYS_CLONE3 and HAVE_COPY_THREAD_TLS.
> 
> Note that in the meantime, m68k has already switched to the new calling
> convention. And I've got sparc patches acked by Dave and ia64 is already
> done too. You can find a link to a booting qemu nios2 system with all the
> changes here at [1].
> 
> [1]: https://asciinema.org/a/333353
> Cc: linux-kernel@...r.kernel.org
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Cc: Ley Foon Tan <ley.foon.tan@...el.com>
> Signed-off-by: Christian Brauner <christian.brauner@...ntu.com>
Acked-by: Ley Foon Tan <ley.foon.tan@...el.com>

Assume this patch will merge with your series.

Regards
Ley Foon

Powered by blists - more mailing lists