Re: [PATCH -mm] pidns-guarantee-that-the-pidns-init-will-be-the-last-pidns-process-r eaped-v2-fix-fix

[ebiederm@xmission.com (Eric W. Biederman)]

Oleg Nesterov <oleg@redhat.com> writes:

> So. Eric, Andrew, will you agree with this cleanup on top of
> pidns-guarantee-that-the-pidns-init-will-be-the-last-pidns-process-reaped-v2-fix.patch
> ?
>
> 1. Update the comments in zap_pid_ns_processes() and __unhash_process()

In zap_pid_ns_processes I wonder if we should update the big block
comment with a little more of the theory.  AKA we want as many children
to self-reap and become EXIT_DEAD children as possible becasue it
enables more parallelism and is thus faster.

> 2. Move the wake-up-reaper code in __unhash_process() under IS_ENABLED()

I don't really care, it ceartainly looks better than an #ifdef block.
However come to think of it, it is about time to just plain start
removing those config options.  The original point was so that there
would be a simple hammer people could throw while we were implementing
the namespaces to easily avoid any issues.  At this point with the
namespaces being about as stable as the rest of the kernel I don't know
that there is any advantage is having in having a config option.

> 3. Re-structure the wait-for-empty-children code in zap_pid_ns_processes()
The restructuring seems basically sane.

> @@ -76,13 +74,16 @@ static void __unhash_process(struct task_struct *p, bool group_dead)
>  
>  		/*
>  		 * If we are the last child process in a pid namespace to be
> -		 * reaped, notify the child_reaper.
> +		 * reaped, notify the child_reaper, see zap_pid_ns_processes().
>  		 */


How about instead:
>  		/*
>  		 * If we are the last child process in a pid namespace to be
> -		 * reaped, notify the child_reaper.
> +		 * reaped, wake up the child_reaper sleeping in zap_pid_ns_processes().
>  		 */


> -		parent = p->real_parent;
> -		if ((task_active_pid_ns(p)->child_reaper == parent) &&
> -		    list_empty(&parent->children) &&
> -		    (parent->flags & PF_EXITING))
> -			wake_up_process(parent);
> +		if (IS_ENABLED(CONFIG_PID_NS)) {
> +			struct task_struct *parent = p->real_parent;
> +
> +			if ((task_active_pid_ns(p)->child_reaper == parent) &&
> +			    list_empty(&parent->children) &&
> +			    (parent->flags & PF_EXITING))
> +				wake_up_process(parent);
> +		}
>  	}
>  	detach_pid(p, PIDTYPE_PID);
>  	list_del_rcu(&p->thread_group);

Eric