kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND

PATCH In review Oleg Nesterov 4 participants 6 messages Apr 13, 2014 – May 19, 2014 Scheduler

lore

Share & tools

lore

Oleg Nesterov

author

2014-04-13 19:56 UTC #0 lore raw

PATCH 1/1 kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND

1. Remove CLONE_KERNEL, it has no users and it is dangerous.

The (old) comment says "List of flags we want to share for kernel threads" but this is not true, we do not want to share ->sighand by default. This flag can only be used if the caller is sure that both parent/child will never play with signals (say, allow_signal/etc).

2. Change rest_init() to clone kernel_init() without CLONE_SIGHAND.

In this case CLONE_SIGHAND does not really hurt, and it looks like optimization because copy_sighand() can avoid kmem_cache_alloc().

But in fact this only adds the minor pessimization. kernel_init() is going to exec the init process, and de_thread() will need to unshare ->sighand and do kmem_cache_alloc(sighand_cachep) anyway, but it needs to do more work and take tasklist_lock and siglock.

Signed-off-by Oleg Nesterov <[email protected]>

---
 include/linux/sched.h |    6 ------
 init/main.c           |    2 +-
 2 files changed, 1 insertions(+), 7 deletions(-)

include/linux/sched.h +0 -6

index a781dec..e44888e 100644
@@ -131,12 +131,6 @@ struct blk_plug;
 struct filename;
 
 /*
- * List of flags we want to share for kernel threads,
- * if only because they are not used by them anyway.
- */
-#define CLONE_KERNEL	(CLONE_FS | CLONE_FILES | CLONE_SIGHAND)
-
-/*
  * These are the constant used to fake the fixed-point load-average
  * counting. Some notes:
  *  - 11 bit fractions expand to 22 bits by the multiplies: this gives

init/main.c +1 -1

index 9c7fd4c..3815895 100644
@@ -379,7 +379,7 @@ static noinline void __init_refok rest_init(void)
 	 * the init task will end up wanting to create kthreads, which, if
 	 * we schedule it before we create kthreadd, will OOPS.
 	 */
-	kernel_thread(kernel_init, NULL, CLONE_FS | CLONE_SIGHAND);
+	kernel_thread(kernel_init, NULL, CLONE_FS);
 	numa_default_policy();
 	pid = kernel_thread(kthreadd, NULL, CLONE_FS | CLONE_FILES);
 	rcu_read_lock();

Signature · 1 line

1.5.5.1

Peter Zijlstra

maintainer Acked-by on v1 1/1

2014-04-14 11:05 UTC #1 lore raw

Quoted 12 lines · Oleg Nesterov

1. Remove CLONE_KERNEL, it has no users and it is dangerous.
The (old) comment says "List of flags we want to share for kernel threads" but this is not true, we do not want to share ->sighand by default. This flag can only be used if the caller is sure that both parent/child will never play with signals (say, allow_signal/etc).
2. Change rest_init() to clone kernel_init() without CLONE_SIGHAND.
In this case CLONE_SIGHAND does not really hurt, and it looks like optimization because copy_sighand() can avoid kmem_cache_alloc().

Quoted 6 lines

But in fact this only adds the minor pessimization. kernel_init() is going to exec the init process, and de_thread() will need to unshare ->sighand and do kmem_cache_alloc(sighand_cachep) anyway, but it needs to do more work and take tasklist_lock and siglock.
Signed-off-by Oleg Nesterov <[email protected]>

Seems good;

Acked-by Peter Zijlstra <[email protected]>

I'm thinking the idea was for Andrew to pick this up?

Steven Rostedt

reviewer

2014-04-14 13:51 UTC #2 lore raw

In reply to Oleg Nesterov

Quoted 12 lines · On Sun, 13 Apr 2014 21:56:17 +0200 Oleg Nesterov <[email protected]> wrote:

1. Remove CLONE_KERNEL, it has no users and it is dangerous.
The (old) comment says "List of flags we want to share for kernel threads" but this is not true, we do not want to share ->sighand by default. This flag can only be used if the caller is sure that both parent/child will never play with signals (say, allow_signal/etc).
2. Change rest_init() to clone kernel_init() without CLONE_SIGHAND.
In this case CLONE_SIGHAND does not really hurt, and it looks like optimization because copy_sighand() can avoid kmem_cache_alloc().

Quoted 7 lines

But in fact this only adds the minor pessimization. kernel_init() is going to exec the init process, and de_thread() will need to unshare ->sighand and do kmem_cache_alloc(sighand_cachep) anyway, but it needs to do more work and take tasklist_lock and siglock.
Signed-off-by Oleg Nesterov <[email protected]>
---

Looks fine to me.

-- Steve

Sasha Levin

2014-05-16 14:26 UTC #3 lore raw

In reply to Oleg Nesterov

Quoted 12 lines · Oleg Nesterov

1. Remove CLONE_KERNEL, it has no users and it is dangerous.
The (old) comment says "List of flags we want to share for kernel threads" but this is not true, we do not want to share ->sighand by default. This flag can only be used if the caller is sure that both parent/child will never play with signals (say, allow_signal/etc).
2. Change rest_init() to clone kernel_init() without CLONE_SIGHAND.
In this case CLONE_SIGHAND does not really hurt, and it looks like optimization because copy_sighand() can avoid kmem_cache_alloc().

Quoted 6 lines

But in fact this only adds the minor pessimization. kernel_init() is going to exec the init process, and de_thread() will need to unshare ->sighand and do kmem_cache_alloc(sighand_cachep) anyway, but it needs to do more work and take tasklist_lock and siglock.
Signed-off-by Oleg Nesterov <[email protected]>

Hi Oleg,

This patch triggers a hang during boot in my KVM guest. There are no messages or anything, it just hangs right before init is supposed to start up.

I've narrowed it down a bit, and it's the removal of CLONE_SIGHAND that's bothering it. Removing CLONE_FS and CLONE_FILES doesn't cause the hang on boot.

Thanks,
Sasha

Oleg Nesterov

author

2014-05-16 15:36 UTC #4 lore raw

Quoted 19 lines · Sasha Levin

Quoted 17 lines · Oleg Nesterov
1. Remove CLONE_KERNEL, it has no users and it is dangerous.
The (old) comment says "List of flags we want to share for kernel threads" but this is not true, we do not want to share ->sighand by default. This flag can only be used if the caller is sure that both parent/child will never play with signals (say, allow_signal/etc).
2. Change rest_init() to clone kernel_init() without CLONE_SIGHAND.
In this case CLONE_SIGHAND does not really hurt, and it looks like optimization because copy_sighand() can avoid kmem_cache_alloc().
But in fact this only adds the minor pessimization. kernel_init() is going to exec the init process, and de_thread() will need to unshare ->sighand and do kmem_cache_alloc(sighand_cachep) anyway, but it needs to do more work and take tasklist_lock and siglock.

Quoted 5 lines

Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
Hi Oleg,
This patch triggers a hang during boot in my KVM guest.

Hmm... How??? ;)

Quoted 3 lines

There are no
messages or anything, it just hangs right before init is supposed to start up.

Do you mean kernel_init() hangs somewhere in run_init_process() paths?

Quoted 2 lines

I've narrowed it down a bit, and it's the removal of CLONE_SIGHAND that's bothering it.

This must not be possible, I bet there is something else which should be fixed.

Quoted 2 lines

Removing CLONE_FS and CLONE_FILES doesn't
cause the hang on boot.

kernel_thread(kernel_init) doesn't use CLONE_FILES ?

Oleg.

Sasha Levin

2014-05-19 15:22 UTC #5 lore raw

Quoted 20 lines · Oleg Nesterov

Quoted 19 lines · Sasha Levin
Quoted 17 lines · Oleg Nesterov
1. Remove CLONE_KERNEL, it has no users and it is dangerous.
The (old) comment says "List of flags we want to share for kernel threads" but this is not true, we do not want to share ->sighand by default. This flag can only be used if the caller is sure that both parent/child will never play with signals (say, allow_signal/etc).
2. Change rest_init() to clone kernel_init() without CLONE_SIGHAND.
In this case CLONE_SIGHAND does not really hurt, and it looks like optimization because copy_sighand() can avoid kmem_cache_alloc().
But in fact this only adds the minor pessimization. kernel_init() is going to exec the init process, and de_thread() will need to unshare ->sighand and do kmem_cache_alloc(sighand_cachep) anyway, but it needs to do more work and take tasklist_lock and siglock.

Quoted 7 lines

Quoted 5 lines
Quoted 1 line
Signed-off-by Oleg Nesterov <[email protected]>
Hi Oleg,
This patch triggers a hang during boot in my KVM guest.
Hmm... How??? ;)

I'm afraid this is just bisection gone bad. The real issue was the new goldfish code added, and not this patch.

Since apparently the boot issue was probabilistic I ended up blaming this commit by mistake. Sorry about that.

Thanks,
Sasha