[PATCH RFC] kthread: Unify kernel_thread() and user_mode_thread()

[PATCH RFC] kthread: Unify kernel_thread() and user_mode_thread()

[Huacai Chen]

Commit 343f4c49f2438d8 ("kthread: Don't allocate kthread_struct for init
and umh") introduces a new function user_mode_thread() for init and umh.
But the name is a bit confusing because init and umh are indeed kernel
threads at creation time, the real difference is "they will become user
processes". So let's unify the kernel_thread() and user_mode_thread() to
kernel_thread() again, and add a new 'user' parameter for init and umh.

Signed-off-by: Huacai Chen <chenhuacai@loongson.cn>
---
 include/linux/sched/task.h |  3 +--
 init/main.c                |  4 ++--
 kernel/fork.c              | 20 ++------------------
 kernel/kthread.c           |  2 +-
 kernel/umh.c               |  6 +++---
 5 files changed, 9 insertions(+), 26 deletions(-)

diff --git a/include/linux/sched/task.h b/include/linux/sched/task.h
index 537cbf9a2ade..1fc09768257c 100644
--- a/include/linux/sched/task.h
+++ b/include/linux/sched/task.h
@@ -98,8 +98,7 @@ struct task_struct *copy_process(struct pid *pid, int trace, int node,
 struct task_struct *create_io_thread(int (*fn)(void *), void *arg, int node);
 struct task_struct *fork_idle(int);
 extern pid_t kernel_thread(int (*fn)(void *), void *arg, const char *name,
-			    unsigned long flags);
-extern pid_t user_mode_thread(int (*fn)(void *), void *arg, unsigned long flags);
+			    unsigned long flags, int user);
 extern long kernel_wait4(pid_t, int __user *, int, struct rusage *);
 int kernel_wait(pid_t pid, int *stat);
 
diff --git a/init/main.c b/init/main.c
index af50044deed5..487d93da5eea 100644
--- a/init/main.c
+++ b/init/main.c
@@ -697,7 +697,7 @@ noinline void __ref __noreturn rest_init(void)
 	 * the init task will end up wanting to create kthreads, which, if
 	 * we schedule it before we create kthreadd, will OOPS.
 	 */
-	pid = user_mode_thread(kernel_init, NULL, CLONE_FS);
+	pid = kernel_thread(kernel_init, NULL, NULL, CLONE_FS, 1);
 	/*
 	 * Pin init on the boot CPU. Task migration is not properly working
 	 * until sched_init_smp() has been run. It will set the allowed
@@ -710,7 +710,7 @@ noinline void __ref __noreturn rest_init(void)
 	rcu_read_unlock();
 
 	numa_default_policy();
-	pid = kernel_thread(kthreadd, NULL, NULL, CLONE_FS | CLONE_FILES);
+	pid = kernel_thread(kthreadd, NULL, NULL, CLONE_FS | CLONE_FILES, 0);
 	rcu_read_lock();
 	kthreadd_task = find_task_by_pid_ns(pid, &init_pid_ns);
 	rcu_read_unlock();
diff --git a/kernel/fork.c b/kernel/fork.c
index ed4e01daccaa..eeaf50944a0b 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -2965,7 +2965,7 @@ pid_t kernel_clone(struct kernel_clone_args *args)
  * Create a kernel thread.
  */
 pid_t kernel_thread(int (*fn)(void *), void *arg, const char *name,
-		    unsigned long flags)
+		    unsigned long flags, int user)
 {
 	struct kernel_clone_args args = {
 		.flags		= ((lower_32_bits(flags) | CLONE_VM |
@@ -2974,23 +2974,7 @@ pid_t kernel_thread(int (*fn)(void *), void *arg, const char *name,
 		.fn		= fn,
 		.fn_arg		= arg,
 		.name		= name,
-		.kthread	= 1,
-	};
-
-	return kernel_clone(&args);
-}
-
-/*
- * Create a user mode thread.
- */
-pid_t user_mode_thread(int (*fn)(void *), void *arg, unsigned long flags)
-{
-	struct kernel_clone_args args = {
-		.flags		= ((lower_32_bits(flags) | CLONE_VM |
-				    CLONE_UNTRACED) & ~CSIGNAL),
-		.exit_signal	= (lower_32_bits(flags) & CSIGNAL),
-		.fn		= fn,
-		.fn_arg		= arg,
+		.kthread	= !user,
 	};
 
 	return kernel_clone(&args);
diff --git a/kernel/kthread.c b/kernel/kthread.c
index 490792b1066e..f4ac241c8d94 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -400,7 +400,7 @@ static void create_kthread(struct kthread_create_info *create)
 #endif
 	/* We want our own signal handler (we take no signals by default). */
 	pid = kernel_thread(kthread, create, create->full_name,
-			    CLONE_FS | CLONE_FILES | SIGCHLD);
+			    CLONE_FS | CLONE_FILES | SIGCHLD, 0);
 	if (pid < 0) {
 		/* Release the structure when caller killed by a fatal signal. */
 		struct completion *done = xchg(&create->done, NULL);
diff --git a/kernel/umh.c b/kernel/umh.c
index 60aa9e764a38..ec4ec12094b5 100644
--- a/kernel/umh.c
+++ b/kernel/umh.c
@@ -130,7 +130,7 @@ static void call_usermodehelper_exec_sync(struct subprocess_info *sub_info)
 
 	/* If SIGCLD is ignored do_wait won't populate the status. */
 	kernel_sigaction(SIGCHLD, SIG_DFL);
-	pid = user_mode_thread(call_usermodehelper_exec_async, sub_info, SIGCHLD);
+	pid = kernel_thread(call_usermodehelper_exec_async, sub_info, NULL, SIGCHLD, 1);
 	if (pid < 0)
 		sub_info->retval = pid;
 	else
@@ -169,8 +169,8 @@ static void call_usermodehelper_exec_work(struct work_struct *work)
 		 * want to pollute current->children, and we need a parent
 		 * that always ignores SIGCHLD to ensure auto-reaping.
 		 */
-		pid = user_mode_thread(call_usermodehelper_exec_async, sub_info,
-				       CLONE_PARENT | SIGCHLD);
+		pid = kernel_thread(call_usermodehelper_exec_async, sub_info,
+				       NULL, CLONE_PARENT | SIGCHLD, 1);
 		if (pid < 0) {
 			sub_info->retval = pid;
 			umh_complete(sub_info);
-- 
2.39.1

Re: [PATCH RFC] kthread: Unify kernel_thread() and user_mode_thread()

[Eric W. Biederman]

Huacai Chen <chenhuacai@loongson.cn> writes:

> Commit 343f4c49f2438d8 ("kthread: Don't allocate kthread_struct for init
> and umh") introduces a new function user_mode_thread() for init and umh.
> But the name is a bit confusing because init and umh are indeed kernel
> threads at creation time, the real difference is "they will become user
> processes".

No they are not "kernel threads" at creation time.  At creation time
init and umh are threads running in the kernel.

It is a very important distinction and you are loosing it.

Because they don't have a kthread_struct such tasks in the kernel
are not allowed to depend on anything that is ``kthread''.

Having this a separate function highlights the distinction.
Highlighting should hopefully cause people to ask why there is a
distinction, and what is going on.

> So let's unify the kernel_thread() and user_mode_thread() to
> kernel_thread() again, and add a new 'user' parameter for init and
> umh

Now that is confusing.

Eric

Re: [PATCH RFC] kthread: Unify kernel_thread() and user_mode_thread()

[Huacai Chen]

Hi, Eric,

On Wed, May 10, 2023 at 11:45 PM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Huacai Chen <chenhuacai@loongson.cn> writes:
>
> > Commit 343f4c49f2438d8 ("kthread: Don't allocate kthread_struct for init
> > and umh") introduces a new function user_mode_thread() for init and umh.
> > But the name is a bit confusing because init and umh are indeed kernel
> > threads at creation time, the real difference is "they will become user
> > processes".
>
> No they are not "kernel threads" at creation time.  At creation time
> init and umh are threads running in the kernel.
>
> It is a very important distinction and you are loosing it.
>
> Because they don't have a kthread_struct such tasks in the kernel
> are not allowed to depend on anything that is ``kthread''.
Hmm, traditionally, we call a "task" without userland address space
(i.e., the task_struct has no mm, it shares kernel's address space) as
a kernel thread, so init and umh are kernel threads until they call
kernel_execve(). Of course in your patch a kernel thread should have a
"kthread" struct (I can't grep "kthread_struct" so I suppose you are
saying "kthread"), but I think the traditional definition is more
natural for most people?

Huacai

>
> Having this a separate function highlights the distinction.
> Highlighting should hopefully cause people to ask why there is a
> distinction, and what is going on.
>
> > So let's unify the kernel_thread() and user_mode_thread() to
> > kernel_thread() again, and add a new 'user' parameter for init and
> > umh
>
> Now that is confusing.
>
> Eric

Re: [PATCH RFC] kthread: Unify kernel_thread() and user_mode_thread()

[Eric W. Biederman]

Huacai Chen <chenhuacai@kernel.org> writes:

> Hi, Eric,
>
> On Wed, May 10, 2023 at 11:45 PM Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> Huacai Chen <chenhuacai@loongson.cn> writes:
>>
>> > Commit 343f4c49f2438d8 ("kthread: Don't allocate kthread_struct for init
>> > and umh") introduces a new function user_mode_thread() for init and umh.
>> > But the name is a bit confusing because init and umh are indeed kernel
>> > threads at creation time, the real difference is "they will become user
>> > processes".
>>
>> No they are not "kernel threads" at creation time.  At creation time
>> init and umh are threads running in the kernel.
>>
>> It is a very important distinction and you are loosing it.
>>
>> Because they don't have a kthread_struct such tasks in the kernel
>> are not allowed to depend on anything that is ``kthread''.
> Hmm, traditionally, we call a "task" without userland address space
> (i.e., the task_struct has no mm, it shares kernel's address space) as
> a kernel thread, so init and umh are kernel threads until they call
> kernel_execve().

No.

The important distinction is not the userland address space.

The important distinction is how such tasks interact with the rest of
the system.

It is true the mm does not initially have userspace content but
that does not change the fact that it is a valid userspace mm.

For scheduling, for signal delivery, and for everything else
these tasks are userspace tasks.

The very important detail is that it is not at kernel_execve time that
the distinction is made, but that it is made earlier when the thread
is created.

This is a subtle change from the way things used to work once upon a
time.  But the way things used to work was buggy and racy.  Deciding at
thread creation time what the thread will be used for, what limitations
etc is much less error prone.

We had this concept of kthread_create that used to create a special
class of tasks.  What was special, and what extra could be done with
those tasks was defined by the presence "struct kthread" (my apologies
I mispoke when I said kthread_struct earlier).

Then because that specialness was needed on other tasks struct kthread
started to be added to tasks at run-time.  That runtime addition of
struct kthread introduced races that complicated the code, and had
bugs.

> Of course in your patch a kernel thread should have a
> "kthread" struct (I can't grep "kthread_struct" so I suppose you are
> saying "kthread"), but I think the traditional definition is more
> natural for most people?

Natural and traditional is a silly argument.  The fact is those are
tasks that ultimately run userspace code.  That ability needs to
be decided upon at creation time to make them race free.

Therefore the old code and definition are wrong.

Eric

Re: [PATCH RFC] kthread: Unify kernel_thread() and user_mode_thread()

[Huacai Chen]

Hi, Eric,


On Mon, May 15, 2023 at 10:42 PM Eric W. Biederman
<ebiederm@xmission.com> wrote:
>
> Huacai Chen <chenhuacai@kernel.org> writes:
>
> > Hi, Eric,
> >
> > On Wed, May 10, 2023 at 11:45 PM Eric W. Biederman
> > <ebiederm@xmission.com> wrote:
> >>
> >> Huacai Chen <chenhuacai@loongson.cn> writes:
> >>
> >> > Commit 343f4c49f2438d8 ("kthread: Don't allocate kthread_struct for init
> >> > and umh") introduces a new function user_mode_thread() for init and umh.
> >> > But the name is a bit confusing because init and umh are indeed kernel
> >> > threads at creation time, the real difference is "they will become user
> >> > processes".
> >>
> >> No they are not "kernel threads" at creation time.  At creation time
> >> init and umh are threads running in the kernel.
> >>
> >> It is a very important distinction and you are loosing it.
> >>
> >> Because they don't have a kthread_struct such tasks in the kernel
> >> are not allowed to depend on anything that is ``kthread''.
> > Hmm, traditionally, we call a "task" without userland address space
> > (i.e., the task_struct has no mm, it shares kernel's address space) as
> > a kernel thread, so init and umh are kernel threads until they call
> > kernel_execve().
>
> No.
>
> The important distinction is not the userland address space.
>
> The important distinction is how such tasks interact with the rest of
> the system.
>
> It is true the mm does not initially have userspace content but
> that does not change the fact that it is a valid userspace mm.
>
> For scheduling, for signal delivery, and for everything else
> these tasks are userspace tasks.
>
> The very important detail is that it is not at kernel_execve time that
> the distinction is made, but that it is made earlier when the thread
> is created.
>
> This is a subtle change from the way things used to work once upon a
> time.  But the way things used to work was buggy and racy.  Deciding at
> thread creation time what the thread will be used for, what limitations
> etc is much less error prone.
>
> We had this concept of kthread_create that used to create a special
> class of tasks.  What was special, and what extra could be done with
> those tasks was defined by the presence "struct kthread" (my apologies
> I mispoke when I said kthread_struct earlier).
>
> Then because that specialness was needed on other tasks struct kthread
> started to be added to tasks at run-time.  That runtime addition of
> struct kthread introduced races that complicated the code, and had
> bugs.
Thank you very much for providing so much background information. Now
I know that init and umh are different from typical kernel threads,
but on the other hand, they are also different from typical user mode
threads (have no mm struct at creation time). So from my point of
view, we can treat them as "special kernel thread".

Huacai

>
> > Of course in your patch a kernel thread should have a
> > "kthread" struct (I can't grep "kthread_struct" so I suppose you are
> > saying "kthread"), but I think the traditional definition is more
> > natural for most people?
>
> Natural and traditional is a silly argument.  The fact is those are
> tasks that ultimately run userspace code.  That ability needs to
> be decided upon at creation time to make them race free.
>
> Therefore the old code and definition are wrong.
>
> Eric

Re: [PATCH RFC] kthread: Unify kernel_thread() and user_mode_thread()

[Andrew Morton]

On Tue,  9 May 2023 18:41:27 +0800 Huacai Chen <chenhuacai@loongson.cn> wrote:

> Commit 343f4c49f2438d8 ("kthread: Don't allocate kthread_struct for init
> and umh") introduces a new function user_mode_thread() for init and umh.
> But the name is a bit confusing because init and umh are indeed kernel
> threads at creation time, the real difference is "they will become user
> processes". So let's unify the kernel_thread() and user_mode_thread() to
> kernel_thread() again, and add a new 'user' parameter for init and umh.
> 
> ...
>
>  5 files changed, 9 insertions(+), 26 deletions(-)
>

Less code is nice.

> -extern pid_t user_mode_thread(int (*fn)(void *), void *arg, unsigned long flags);
> +			    unsigned long flags, int user);

`bool user'?

Re: [PATCH RFC] kthread: Unify kernel_thread() and user_mode_thread()

[Huacai Chen]

Hi, Andrew,

On Sat, May 13, 2023 at 7:53 AM Andrew Morton <akpm@linux-foundation.org> wrote:
>
> On Tue,  9 May 2023 18:41:27 +0800 Huacai Chen <chenhuacai@loongson.cn> wrote:
>
> > Commit 343f4c49f2438d8 ("kthread: Don't allocate kthread_struct for init
> > and umh") introduces a new function user_mode_thread() for init and umh.
> > But the name is a bit confusing because init and umh are indeed kernel
> > threads at creation time, the real difference is "they will become user
> > processes". So let's unify the kernel_thread() and user_mode_thread() to
> > kernel_thread() again, and add a new 'user' parameter for init and umh.
> >
> > ...
> >
> >  5 files changed, 9 insertions(+), 26 deletions(-)
> >
>
> Less code is nice.
>
> > -extern pid_t user_mode_thread(int (*fn)(void *), void *arg, unsigned long flags);
> > +                         unsigned long flags, int user);
>
> `bool user'?
OK, I will do that in the next version if the whole patch is acceptable.

Huacai
>
>