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

[Mateusz Guzik <mjguzik@gmail.com>]

On Sun, Nov 23, 2025 at 9:10 PM Oleg Nesterov <oleg@redhat.com> wrote:
>
> 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) ?
>

I agree, but I also tried to not make non-perf changes to make it
easier to review.

There is tons of clean up which can be done here, maybe I'll add some later.

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

Oh heh, that was not intentional. After I was done massaging this I
just mindlessly removed the lock acquire + preload.

It does indeed look like it serves no purpose, I'm going to submit a
separate patch to remove it.

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

That's a question to Christian.