I wrote a test program. It does clone(CLONE_NEWPID | CLONE_VM) and
sleep(), a new task repeates the same actions. This program creates
4000 tasks. When I tried to kill all this processes, a system was
inaccessible for some minutes.
The system is inaccessible, because each process calls
zap_pid_ns_processes, which tries to kill subprocesses under
tasklist_lock. The most time are required for find_vpid().
I suggest to mark sub-namespaces in zap_pid_ns_processes.
zap_pid_ns_processes for marked pidns doesn't kill tasks,
it only waits them.
I am not sure, that this idea is correct, but it helps.
Maybe we should restrict depth of pidns?
Why can't we enumerate task->children instead of using find_vpid()?
Cc: Oleg Nesterov <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Serge Hallyn <[email protected]>
Cc: Paul Gortmaker <[email protected]>
Cc: "Eric W. Biederman" <[email protected]>
Cc: Vasiliy Kulikov <[email protected]>
Cc: Cyrill Gorcunov <[email protected]>
Cc: Pavel Emelyanov <[email protected]>
Signed-off-by: Andrew Vagin <[email protected]>
---
include/linux/pid_namespace.h | 1 +
kernel/pid_namespace.c | 14 ++++++++++++--
2 files changed, 13 insertions(+), 2 deletions(-)
diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
index 00474b0..28073a0 100644
--- a/include/linux/pid_namespace.h
+++ b/include/linux/pid_namespace.h
@@ -34,6 +34,7 @@ struct pid_namespace {
kgid_t pid_gid;
int hide_pid;
int reboot; /* group exit code if this pidns was rebooted */
+ atomic_t zapped; /* non zero if all process were killed */
};
extern struct pid_namespace init_pid_ns;
diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
index b051fa6..7db7dcd 100644
--- a/kernel/pid_namespace.c
+++ b/kernel/pid_namespace.c
@@ -177,21 +177,31 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
* maintain a tasklist for each pid namespace.
*
*/
+
+ if (atomic_read(&pid_ns->zapped))
+ goto wait; /* All processes were already killed */
+
read_lock(&tasklist_lock);
nr = next_pidmap(pid_ns, 1);
while (nr > 0) {
rcu_read_lock();
task = pid_task(find_vpid(nr), PIDTYPE_PID);
- if (task && !__fatal_signal_pending(task))
+ if (task && !__fatal_signal_pending(task)) {
+ struct pid_namespace *ns;
+
send_sig_info(SIGKILL, SEND_SIG_FORCED, task);
+ ns = task_active_pid_ns(task);
+ if (unlikely(ns->child_reaper == task))
+ atomic_set(&ns->zapped, 1);
+ }
rcu_read_unlock();
nr = next_pidmap(pid_ns, nr);
}
read_unlock(&tasklist_lock);
-
+wait:
/* Firstly reap the EXIT_ZOMBIE children we may have. */
do {
clear_thread_flag(TIF_SIGPENDING);
--
1.7.1
The test program is attached.
On Sun, Oct 07, 2012 at 01:49:18PM +0400, Andrew Vagin wrote:
> I wrote a test program. It does clone(CLONE_NEWPID | CLONE_VM) and
> sleep(), a new task repeates the same actions. This program creates
> 4000 tasks. When I tried to kill all this processes, a system was
> inaccessible for some minutes.
>
> The system is inaccessible, because each process calls
> zap_pid_ns_processes, which tries to kill subprocesses under
> tasklist_lock. The most time are required for find_vpid().
>
> I suggest to mark sub-namespaces in zap_pid_ns_processes.
> zap_pid_ns_processes for marked pidns doesn't kill tasks,
> it only waits them.
>
> I am not sure, that this idea is correct, but it helps.
>
> Maybe we should restrict depth of pidns?
> Why can't we enumerate task->children instead of using find_vpid()?
>
> Cc: Oleg Nesterov <[email protected]>
> Cc: Andrew Morton <[email protected]>
> Cc: Serge Hallyn <[email protected]>
> Cc: Paul Gortmaker <[email protected]>
> Cc: "Eric W. Biederman" <[email protected]>
> Cc: Vasiliy Kulikov <[email protected]>
> Cc: Cyrill Gorcunov <[email protected]>
> Cc: Pavel Emelyanov <[email protected]>
> Signed-off-by: Andrew Vagin <[email protected]>
> ---
> include/linux/pid_namespace.h | 1 +
> kernel/pid_namespace.c | 14 ++++++++++++--
> 2 files changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/pid_namespace.h b/include/linux/pid_namespace.h
> index 00474b0..28073a0 100644
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -34,6 +34,7 @@ struct pid_namespace {
> kgid_t pid_gid;
> int hide_pid;
> int reboot; /* group exit code if this pidns was rebooted */
> + atomic_t zapped; /* non zero if all process were killed */
> };
>
> extern struct pid_namespace init_pid_ns;
> diff --git a/kernel/pid_namespace.c b/kernel/pid_namespace.c
> index b051fa6..7db7dcd 100644
> --- a/kernel/pid_namespace.c
> +++ b/kernel/pid_namespace.c
> @@ -177,21 +177,31 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
> * maintain a tasklist for each pid namespace.
> *
> */
> +
> + if (atomic_read(&pid_ns->zapped))
> + goto wait; /* All processes were already killed */
> +
> read_lock(&tasklist_lock);
> nr = next_pidmap(pid_ns, 1);
> while (nr > 0) {
> rcu_read_lock();
>
> task = pid_task(find_vpid(nr), PIDTYPE_PID);
> - if (task && !__fatal_signal_pending(task))
> + if (task && !__fatal_signal_pending(task)) {
> + struct pid_namespace *ns;
> +
> send_sig_info(SIGKILL, SEND_SIG_FORCED, task);
> + ns = task_active_pid_ns(task);
> + if (unlikely(ns->child_reaper == task))
> + atomic_set(&ns->zapped, 1);
> + }
>
> rcu_read_unlock();
>
> nr = next_pidmap(pid_ns, nr);
> }
> read_unlock(&tasklist_lock);
> -
> +wait:
> /* Firstly reap the EXIT_ZOMBIE children we may have. */
> do {
> clear_thread_flag(TIF_SIGPENDING);
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
On 10/07, Andrew Vagin wrote:
>
> I wrote a test program. It does clone(CLONE_NEWPID | CLONE_VM) and
> sleep(), a new task repeates the same actions. This program creates
> 4000 tasks. When I tried to kill all this processes, a system was
> inaccessible for some minutes.
So this creates 4000 nested namespaces? Not sure this really needs the
fix... The size of pid would be more than 4000 * sizeof(struct upid).
Perhaps we should MAX_PID_NS_LEVEL instead?
As for the patch, it looks correct at first glance. But,
> --- a/include/linux/pid_namespace.h
> +++ b/include/linux/pid_namespace.h
> @@ -34,6 +34,7 @@ struct pid_namespace {
> kgid_t pid_gid;
> int hide_pid;
> int reboot; /* group exit code if this pidns was rebooted */
> + atomic_t zapped; /* non zero if all process were killed */
> };
atomic_t buys nothing. In this case atomic_set/read doesn't differ
from plain STORE/LOAD.
> @@ -177,21 +177,31 @@ void zap_pid_ns_processes(struct pid_namespace *pid_ns)
> * maintain a tasklist for each pid namespace.
> *
> */
> +
> + if (atomic_read(&pid_ns->zapped))
> + goto wait; /* All processes were already killed */
> +
OK, but if we try to speedup, then probably the main loop should
check ->zapped too and stop. Multiple reapers can start
zap_pid_ns_processes() at the same time.
So, probably,
> read_lock(&tasklist_lock);
> nr = next_pidmap(pid_ns, 1);
> while (nr > 0) {
should be "while (nr > 0 && !zapped)", and
> rcu_read_lock();
>
> task = pid_task(find_vpid(nr), PIDTYPE_PID);
> - if (task && !__fatal_signal_pending(task))
> + if (task && !__fatal_signal_pending(task)) {
> + struct pid_namespace *ns;
> +
> send_sig_info(SIGKILL, SEND_SIG_FORCED, task);
> + ns = task_active_pid_ns(task);
> + if (unlikely(ns->child_reaper == task))
> + atomic_set(&ns->zapped, 1);
This should be unconditional. Even if the task is not child_reaper,
we are going to kill the whole namespace. So I think
if (task_active_pid_ns(task) != task_active_pid_ns(current))
ns->zapped = 1;
except it should be optimized.
I am wondering if we can do for_each_pid_in_this_ns(pid) which skips
the pids from the sub-namespaces. Note that zap_pid_ns_processes()
doesn't really need to kill the tasks from sub-namespace, its init
will take care anyway. In this case we do not nee ns->zapped.
Probably not...
Oleg.
2012/10/7 Oleg Nesterov <[email protected]>:
> On 10/07, Andrew Vagin wrote:
>>
>> I wrote a test program. It does clone(CLONE_NEWPID | CLONE_VM) and
>> sleep(), a new task repeates the same actions. This program creates
>> 4000 tasks. When I tried to kill all this processes, a system was
>> inaccessible for some minutes.
>
> So this creates 4000 nested namespaces? Not sure this really needs the
> fix... The size of pid would be more than 4000 * sizeof(struct upid).
>
> Perhaps we should MAX_PID_NS_LEVEL instead?
Yes, we can.
Could I just define MAX_PID_NS_LEVEL in a code:
#define MAX_PID_NS_LEVEL ((PAGE_SIZE - offsetof(struct pid, numbers))
/ sizeof(struct upid))
Or should it be added in a config?
My opinion is that MAX_PID_NS_LEVEL can be defined, it will be 126 on
x86_64. I don't know a usecase for which, it will be not enough. When
someone finds a reasonable use case, it can be changed.
>
> As for the patch, it looks correct at first glance. But,
I agree with all your comments.
Thanks.
On 10/08, Andrey Wagin wrote:
>
> 2012/10/7 Oleg Nesterov <[email protected]>:
> >
> > Perhaps we should MAX_PID_NS_LEVEL instead?
>
> Yes, we can.
>
> Could I just define MAX_PID_NS_LEVEL in a code:
> #define MAX_PID_NS_LEVEL ((PAGE_SIZE - offsetof(struct pid, numbers))
> / sizeof(struct upid))
Or even less. But looks reasonable.
> Or should it be added in a config?
Personally I think that "define" is fine, we can add config/sysctl
later if needed.
Hmm. This is off-topic, but...
create_pid_namespace:
unsigned int level = parent_pid_ns->level + 1;
ns->pid_cachep = create_pid_cachep(level + 1);
is it correct? is seems that only one "+ 1" is needed?
Oleg.
2012/10/9 Oleg Nesterov <[email protected]>:
> On 10/08, Andrey Wagin wrote:
>>
>> 2012/10/7 Oleg Nesterov <[email protected]>:
>> >
>> > Perhaps we should MAX_PID_NS_LEVEL instead?
>>
>> Yes, we can.
>>
>> Could I just define MAX_PID_NS_LEVEL in a code:
>> #define MAX_PID_NS_LEVEL ((PAGE_SIZE - offsetof(struct pid, numbers))
>> / sizeof(struct upid))
>
> Or even less. But looks reasonable.
>
>> Or should it be added in a config?
>
> Personally I think that "define" is fine, we can add config/sysctl
> later if needed.
Ok, I'm going to send a patch.
>
>
> Hmm. This is off-topic, but...
>
> create_pid_namespace:
>
> unsigned int level = parent_pid_ns->level + 1;
> ns->pid_cachep = create_pid_cachep(level + 1);
Yes, it's correct, because pid->numbers[ns->level] should be valid, so
a size of an array pid->numbers should be (level + 1).
/*
....
* @nr_ids: the number of numerical ids this pid will have to carry
*/
static struct kmem_cache *create_pid_cachep(int nr_ids)
>
> is it correct? is seems that only one "+ 1" is needed?
>
> Oleg.
>
On 10/09, Andrey Wagin wrote:
>
> 2012/10/9 Oleg Nesterov <[email protected]>:
> > Hmm. This is off-topic, but...
> >
> > create_pid_namespace:
> >
> > unsigned int level = parent_pid_ns->level + 1;
> > ns->pid_cachep = create_pid_cachep(level + 1);
>
> Yes, it's correct, because pid->numbers[ns->level] should be valid, so
> a size of an array pid->numbers should be (level + 1).
Ah, yes. I missed that ns->level is "last index", not "array size".
Indeed, init_pid_ns.level = 0.
Thanks Andrey.
Oleg.