All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND
@ 2014-04-13 19:56 Oleg Nesterov
  2014-04-14 11:05 ` Peter Zijlstra
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Oleg Nesterov @ 2014-04-13 19:56 UTC (permalink / raw)
  To: Andrew Morton
  Cc: Frederic Weisbecker, Ingo Molnar, Mathieu Desnoyers,
	Peter Zijlstra, Steven Rostedt, linux-kernel

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 <oleg@redhat.com>
---
 include/linux/sched.h |    6 ------
 init/main.c           |    2 +-
 2 files changed, 1 insertions(+), 7 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index a781dec..e44888e 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -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
diff --git a/init/main.c b/init/main.c
index 9c7fd4c..3815895 100644
--- a/init/main.c
+++ b/init/main.c
@@ -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();
-- 
1.5.5.1



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND
  2014-04-13 19:56 [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND Oleg Nesterov
@ 2014-04-14 11:05 ` Peter Zijlstra
  2014-04-14 13:51 ` Steven Rostedt
  2014-05-16 14:25 ` Sasha Levin
  2 siblings, 0 replies; 6+ messages in thread
From: Peter Zijlstra @ 2014-04-14 11:05 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Frederic Weisbecker, Ingo Molnar,
	Mathieu Desnoyers, Steven Rostedt, linux-kernel

On Sun, Apr 13, 2014 at 09:56:17PM +0200, Oleg Nesterov 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().
> 
>    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 <oleg@redhat.com>

Seems good;

Acked-by: Peter Zijlstra <peterz@infradead.org>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND
  2014-04-13 19:56 [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND Oleg Nesterov
  2014-04-14 11:05 ` Peter Zijlstra
@ 2014-04-14 13:51 ` Steven Rostedt
  2014-05-16 14:25 ` Sasha Levin
  2 siblings, 0 replies; 6+ messages in thread
From: Steven Rostedt @ 2014-04-14 13:51 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Frederic Weisbecker, Ingo Molnar,
	Mathieu Desnoyers, Peter Zijlstra, linux-kernel

On Sun, 13 Apr 2014 21:56:17 +0200
Oleg Nesterov <oleg@redhat.com> 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().
> 
>    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 <oleg@redhat.com>
> ---

Looks fine to me.

-- Steve

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND
  2014-04-13 19:56 [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND Oleg Nesterov
  2014-04-14 11:05 ` Peter Zijlstra
  2014-04-14 13:51 ` Steven Rostedt
@ 2014-05-16 14:25 ` Sasha Levin
  2014-05-16 15:35   ` Oleg Nesterov
  2 siblings, 1 reply; 6+ messages in thread
From: Sasha Levin @ 2014-05-16 14:25 UTC (permalink / raw)
  To: Oleg Nesterov, Andrew Morton
  Cc: Frederic Weisbecker, Ingo Molnar, Mathieu Desnoyers,
	Peter Zijlstra, Steven Rostedt, linux-kernel

On 04/13/2014 03:56 PM, Oleg Nesterov 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().
> 
>    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 <oleg@redhat.com>

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

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND
  2014-05-16 14:25 ` Sasha Levin
@ 2014-05-16 15:35   ` Oleg Nesterov
  2014-05-19 15:21     ` Sasha Levin
  0 siblings, 1 reply; 6+ messages in thread
From: Oleg Nesterov @ 2014-05-16 15:35 UTC (permalink / raw)
  To: Sasha Levin
  Cc: Andrew Morton, Frederic Weisbecker, Ingo Molnar,
	Mathieu Desnoyers, Peter Zijlstra, Steven Rostedt, linux-kernel

On 05/16, Sasha Levin wrote:
>
> On 04/13/2014 03:56 PM, Oleg Nesterov 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().
> >
> >    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 <oleg@redhat.com>
>
> Hi Oleg,
>
> This patch triggers a hang during boot in my KVM guest.

Hmm... How??? ;)

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

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

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

kernel_thread(kernel_init) doesn't use CLONE_FILES ?

Oleg.


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND
  2014-05-16 15:35   ` Oleg Nesterov
@ 2014-05-19 15:21     ` Sasha Levin
  0 siblings, 0 replies; 6+ messages in thread
From: Sasha Levin @ 2014-05-19 15:21 UTC (permalink / raw)
  To: Oleg Nesterov
  Cc: Andrew Morton, Frederic Weisbecker, Ingo Molnar,
	Mathieu Desnoyers, Peter Zijlstra, Steven Rostedt, linux-kernel

On 05/16/2014 11:35 AM, Oleg Nesterov wrote:
> On 05/16, Sasha Levin wrote:
>>
>> On 04/13/2014 03:56 PM, Oleg Nesterov 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().
>>>
>>>    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 <oleg@redhat.com>
>>
>> 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

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2014-05-19 15:22 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-04-13 19:56 [PATCH] kthreads: kill CLONE_KERNEL, change kernel_thread(kernel_init) to avoid CLONE_SIGHAND Oleg Nesterov
2014-04-14 11:05 ` Peter Zijlstra
2014-04-14 13:51 ` Steven Rostedt
2014-05-16 14:25 ` Sasha Levin
2014-05-16 15:35   ` Oleg Nesterov
2014-05-19 15:21     ` Sasha Levin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.