Re: [PATCH 3/3] pid: only take pidmap_lock once on alloc

[Oleg Nesterov <oleg@redhat.com>]

On 11/23, Mateusz Guzik wrote:
>
> This reduces contention on the lock during parallel clone/exit.
>
> It remains the primary bottleneck in such a case.
>
> While here tidy up the code.

Not sure I can review... But FWIW this patch looks good to me after the
very quick glance. I'll try to actually read it tomorrow.

But please find a couple of minor "can't resist" nits below.

> +	for (tmp = ns, i = ns->level; i >= 0; i--) {
> +		int tid = set_tid[ns->level - i];
>
>  		if (tid) {
>  			nr = idr_alloc(&tmp->idr, NULL, tid,
> @@ -235,10 +261,8 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *set_tid,
>  			 * a partially initialized PID (see below).
>  			 */
>  			nr = idr_alloc_cyclic(&tmp->idr, NULL, pid_min,
> -					      pid_max, GFP_ATOMIC);
> +					      pid_max[ns->level - i], GFP_ATOMIC);
>  		}
> -		spin_unlock(&pidmap_lock);
> -		idr_preload_end();
>
>  		if (nr < 0) {
>  			retval = (nr == -ENOSPC) ? -EAGAIN : nr;

So. With or without this patch we have

	if (tid) {
		nr = idr_alloc(...);

		if (nr == -ENOSPC)
			nr = -EEXIST;
	} else {
		nr = idr_alloc_cyclic(...);
	}

	if (nr < 0) {
		retval = (nr == -ENOSPC) ? -EAGAIN : nr;
		goto out_free;
	}

and somehow this looks annoying to me... Perhaps it makes sense to make this
code more symmetric (and imo more readable) ?

	if (tid) {
		nr = idr_alloc(...);

		if (nr == -ENOSPC)
			nr = -EEXIST;
	} else {
		nr = idr_alloc_cyclic(...);

		if (nr == -ENOSPC)
			nr = -EAGAIN;
	}

	if (nr < 0)
		retval = nr;
		goto out_free;
	}

> -	idr_preload(GFP_KERNEL);
> -	spin_lock(&pidmap_lock);
> -	if (!(ns->pid_allocated & PIDNS_ADDING))
> -		goto out_unlock;
> +	if (unlikely(!(ns->pid_allocated & PIDNS_ADDING)))
> +		goto out_free;
>  	pidfs_add_pid(pid);
> -	for ( ; upid >= pid->numbers; --upid) {
> +	for (upid = pid->numbers + ns->level; upid >= pid->numbers; --upid) {
>  		/* Make the PID visible to find_pid_ns. */
>  		idr_replace(&upid->ns->idr, pid, upid->nr);
>  		upid->ns->pid_allocated++;

So.. unless I am totally confused the current code has another
idr_preload + idr_preload_end around pidfs_add_pid().

AFAICS, this makes no sense, and your patch removes it. But perhaps this
deserves a note in the changelog or even a separate patch?


And another stupid question... I don't understand fs/pidfs.c, but it looks
a bit strange to me that pidfs_add_pid() is called before the

	for (...)
		idr_replace(...);

loop. I don't see any problem, but to me it would look a bit better to do
pidfs_add_pid(pid) when this pid is fully initialized...

Oleg.