[PATCH] pid: cosmetic changes to alloc_pid()

[PATCH] pid: cosmetic changes to alloc_pid()

[Mateusz Guzik]

Commit 6d864a1b182532e7 ("pid: only take pidmap_lock once on alloc")
landed v2 of the patch instead of v3. This patch remedies the problem.

No functional changes.

Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
---
 kernel/pid.c | 19 +++++++++----------
 1 file changed, 9 insertions(+), 10 deletions(-)

diff --git a/kernel/pid.c b/kernel/pid.c
index 2f1dbcbc2349..dbe82062e683 100644
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -177,7 +177,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
 	 * for a process in all nested PID namespaces but arg_set_tid_size must
 	 * never be greater than the current ns->level + 1.
 	 */
-	if (arg_set_tid_size > ns->level + 1)
+	if (unlikely(arg_set_tid_size > ns->level + 1))
 		return ERR_PTR(-EINVAL);
 
 	/*
@@ -186,7 +186,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
 	 * 1. allocate and fill in pid struct
 	 */
 	pid = kmem_cache_alloc(ns->pid_cachep, GFP_KERNEL);
-	if (!pid)
+	if (unlikely(!pid))
 		return ERR_PTR(retval);
 
 	get_pid_ns(ns);
@@ -205,7 +205,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
 	 * This stores found pid_max to make sure the used value is the same should
 	 * later code need it.
 	 */
-	for (tmp = ns, i = ns->level; i >= 0; i--) {
+	for (tmp = ns, i = ns->level; i >= 0;) {
 		pid_max[ns->level - i] = READ_ONCE(tmp->pid_max);
 
 		if (arg_set_tid_size) {
@@ -227,6 +227,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
 		}
 
 		tmp = tmp->parent;
+		i--;
 	}
 
 	/*
@@ -247,10 +248,9 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
 				       tid + 1, GFP_ATOMIC);
 			/*
 			 * If ENOSPC is returned it means that the PID is
-			 * alreay in use. Return EEXIST in that case.
+			 * already in use. Return EEXIST in that case.
 			 */
 			if (nr == -ENOSPC)
-
 				nr = -EEXIST;
 		} else {
 			int pid_min = 1;
@@ -276,12 +276,11 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
 			 * Preload more memory if idr_alloc{,cyclic} failed with -ENOMEM.
 			 *
 			 * The IDR API only allows us to preload memory for one call, while we may end
-			 * up doing several under pidmap_lock with GFP_ATOMIC. The situation may be
-			 * salvageable with GFP_KERNEL. But make sure to not loop indefinitely if preload
-			 * did not help (the routine unfortunately returns void, so we have no idea
-			 * if it got anywhere).
+			 * up doing several with GFP_ATOMIC. It may be the situation is salvageable with
+			 * GFP_KERNEL. But make sure to not loop indefinitely if preload did not help
+			 * (the routine unfortunately returns void, so we have no idea if it got anywhere).
 			 *
-			 * The lock can be safely dropped and picked up as historically pid allocation
+			 * The pidmap lock can be safely dropped and picked up as historically pid allocation
 			 * for different namespaces was *not* atomic -- we try to hold on to it the
 			 * entire time only for performance reasons.
 			 */
-- 
2.48.1

Re: [PATCH] pid: cosmetic changes to alloc_pid()

[Oleg Nesterov]

On 03/06, Mateusz Guzik wrote:
>
> Commit 6d864a1b182532e7 ("pid: only take pidmap_lock once on alloc")
> landed v2 of the patch instead of v3. This patch remedies the problem.
>
> No functional changes.
>
> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>

The patch looks obviously good, but it conflicts with Pavel's

	[PATCH v4 0/4] pid_namespace: make init creation more flexible
	https://lore.kernel.org/all/20260225133229.550302-1-ptikhomirov@virtuozzo.com/#t

In particular, this change:

 -     for (tmp = ns, i = ns->level; i >= 0; i--) {
 +     for (tmp = ns, i = ns->level; i >= 0;) {

With 2/4 from Pavel we need to decrement "i" before the ->child_reaper
check.

Pavel, it seems you need to resend your series anyway, may be on top
of this patch, I dunno. And probably this all should be routed via
-mm tree?

Oleg.

> ---
>  kernel/pid.c | 19 +++++++++----------
>  1 file changed, 9 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/pid.c b/kernel/pid.c
> index 2f1dbcbc2349..dbe82062e683 100644
> --- a/kernel/pid.c
> +++ b/kernel/pid.c
> @@ -177,7 +177,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>  	 * for a process in all nested PID namespaces but arg_set_tid_size must
>  	 * never be greater than the current ns->level + 1.
>  	 */
> -	if (arg_set_tid_size > ns->level + 1)
> +	if (unlikely(arg_set_tid_size > ns->level + 1))
>  		return ERR_PTR(-EINVAL);
>  
>  	/*
> @@ -186,7 +186,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>  	 * 1. allocate and fill in pid struct
>  	 */
>  	pid = kmem_cache_alloc(ns->pid_cachep, GFP_KERNEL);
> -	if (!pid)
> +	if (unlikely(!pid))
>  		return ERR_PTR(retval);
>  
>  	get_pid_ns(ns);
> @@ -205,7 +205,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>  	 * This stores found pid_max to make sure the used value is the same should
>  	 * later code need it.
>  	 */
> -	for (tmp = ns, i = ns->level; i >= 0; i--) {
> +	for (tmp = ns, i = ns->level; i >= 0;) {
>  		pid_max[ns->level - i] = READ_ONCE(tmp->pid_max);
>  
>  		if (arg_set_tid_size) {
> @@ -227,6 +227,7 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>  		}
>  
>  		tmp = tmp->parent;
> +		i--;
>  	}
>  
>  	/*
> @@ -247,10 +248,9 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>  				       tid + 1, GFP_ATOMIC);
>  			/*
>  			 * If ENOSPC is returned it means that the PID is
> -			 * alreay in use. Return EEXIST in that case.
> +			 * already in use. Return EEXIST in that case.
>  			 */
>  			if (nr == -ENOSPC)
> -
>  				nr = -EEXIST;
>  		} else {
>  			int pid_min = 1;
> @@ -276,12 +276,11 @@ struct pid *alloc_pid(struct pid_namespace *ns, pid_t *arg_set_tid,
>  			 * Preload more memory if idr_alloc{,cyclic} failed with -ENOMEM.
>  			 *
>  			 * The IDR API only allows us to preload memory for one call, while we may end
> -			 * up doing several under pidmap_lock with GFP_ATOMIC. The situation may be
> -			 * salvageable with GFP_KERNEL. But make sure to not loop indefinitely if preload
> -			 * did not help (the routine unfortunately returns void, so we have no idea
> -			 * if it got anywhere).
> +			 * up doing several with GFP_ATOMIC. It may be the situation is salvageable with
> +			 * GFP_KERNEL. But make sure to not loop indefinitely if preload did not help
> +			 * (the routine unfortunately returns void, so we have no idea if it got anywhere).
>  			 *
> -			 * The lock can be safely dropped and picked up as historically pid allocation
> +			 * The pidmap lock can be safely dropped and picked up as historically pid allocation
>  			 * for different namespaces was *not* atomic -- we try to hold on to it the
>  			 * entire time only for performance reasons.
>  			 */
> -- 
> 2.48.1
> 

Re: [PATCH] pid: cosmetic changes to alloc_pid()

[Mateusz Guzik]

On Sat, Mar 7, 2026 at 10:23 AM Oleg Nesterov <oleg@redhat.com> wrote:
>
> On 03/06, Mateusz Guzik wrote:
> >
> > Commit 6d864a1b182532e7 ("pid: only take pidmap_lock once on alloc")
> > landed v2 of the patch instead of v3. This patch remedies the problem.
> >
> > No functional changes.
> >
> > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
>
> The patch looks obviously good, but it conflicts with Pavel's
>
>         [PATCH v4 0/4] pid_namespace: make init creation more flexible
>         https://lore.kernel.org/all/20260225133229.550302-1-ptikhomirov@virtuozzo.com/#t
>
> In particular, this change:
>
>  -     for (tmp = ns, i = ns->level; i >= 0; i--) {
>  +     for (tmp = ns, i = ns->level; i >= 0;) {
>
> With 2/4 from Pavel we need to decrement "i" before the ->child_reaper
> check.
>

That patch will be fine as it modifies the other loop which already
had i-- towards the end.

So I don't think there is any need to rebase.

Re: [PATCH] pid: cosmetic changes to alloc_pid()

[Oleg Nesterov]

On 03/07, Mateusz Guzik wrote:
>
> On Sat, Mar 7, 2026 at 10:23 AM Oleg Nesterov <oleg@redhat.com> wrote:
> >
> > On 03/06, Mateusz Guzik wrote:
> > >
> > > Commit 6d864a1b182532e7 ("pid: only take pidmap_lock once on alloc")
> > > landed v2 of the patch instead of v3. This patch remedies the problem.
> > >
> > > No functional changes.
> > >
> > > Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
> >
> > The patch looks obviously good, but it conflicts with Pavel's
> >
> >         [PATCH v4 0/4] pid_namespace: make init creation more flexible
> >         https://lore.kernel.org/all/20260225133229.550302-1-ptikhomirov@virtuozzo.com/#t
> >
> > In particular, this change:
> >
> >  -     for (tmp = ns, i = ns->level; i >= 0; i--) {
> >  +     for (tmp = ns, i = ns->level; i >= 0;) {
> >
> > With 2/4 from Pavel we need to decrement "i" before the ->child_reaper
> > check.
> >
>
> That patch will be fine as it modifies the other loop which already
> had i-- towards the end.
>
> So I don't think there is any need to rebase.

Indeed, I misread your patch, sorry for noise.

Oleg.

Re: [PATCH] pid: cosmetic changes to alloc_pid()

[Pavel Tikhomirov]

On 3/7/26 13:11, Oleg Nesterov wrote:
> On 03/07, Mateusz Guzik wrote:
>>
>> On Sat, Mar 7, 2026 at 10:23 AM Oleg Nesterov <oleg@redhat.com> wrote:
>>>
>>> On 03/06, Mateusz Guzik wrote:
>>>>
>>>> Commit 6d864a1b182532e7 ("pid: only take pidmap_lock once on alloc")
>>>> landed v2 of the patch instead of v3. This patch remedies the problem.
>>>>
>>>> No functional changes.
>>>>
>>>> Signed-off-by: Mateusz Guzik <mjguzik@gmail.com>
>>>
>>> The patch looks obviously good, but it conflicts with Pavel's
>>>
>>>         [PATCH v4 0/4] pid_namespace: make init creation more flexible
>>>         https://lore.kernel.org/all/20260225133229.550302-1-ptikhomirov@virtuozzo.com/#t
>>>
>>> In particular, this change:
>>>
>>>  -     for (tmp = ns, i = ns->level; i >= 0; i--) {
>>>  +     for (tmp = ns, i = ns->level; i >= 0;) {
>>>
>>> With 2/4 from Pavel we need to decrement "i" before the ->child_reaper
>>> check.
>>>
>>
>> That patch will be fine as it modifies the other loop which already
>> had i-- towards the end.
>>
>> So I don't think there is any need to rebase.
> 
> Indeed, I misread your patch, sorry for noise.

I looked into this patch and also confirm that there is no conflict, with my series. And in general patch looks good:

Reviewed-by: Pavel Tikhomirov <ptikhomirov@virtuozzo.com>

side note: I've sent v5 of my series, since I found one more place to add READ_ONCE, also based it on top of mm tree. https://lore.kernel.org/all/20260313111014.2068913-1-ptikhomirov@virtuozzo.com/

> 
> Oleg.
> 

-- 
Best regards, Pavel Tikhomirov
Senior Software Developer, Virtuozzo.