On Wed, Jan 16, 2019 at 01:26:41PM -0700, Jason Gunthorpe wrote: > On Wed, Jan 16, 2019 at 12:00:16PM -0800, Matthew Wilcox wrote: > > On Tue, Jan 15, 2019 at 02:42:50PM -0700, Jason Gunthorpe wrote: > > > On Tue, Jan 15, 2019 at 01:22:43PM -0800, Matthew Wilcox wrote: > > > > On Tue, Jan 15, 2019 at 02:15:41PM -0700, Jason Gunthorpe wrote: > > > > > Matt, down the road can we get an xa_alloc_cyclic helper that handles > > > > > this and the required locking? I have another need in the works as > > > > > well.. > > > > > > > > http://git.infradead.org/users/willy/linux-dax.git/shortlog/refs/heads/xarray-conv > > > > > > > > all current users of idr_alloc_cyclic are converted to xa_alloc_cyclic() > > > > in that tree. > > > > There were actually two callers left. Those have now been converted > > and idr_alloc_cyclic() removed in that tree. > > > > More importantly, I've now updated it to my current vision of how XA_LIMIT > > should be used. > > > > > Maybe this is what your commit comment was talking about: > > > > > > int __xa_alloc_cyclic(struct xarray *xa, u32 *id, void *entry, > > > const struct xa_limit *limit, u32 *next, gfp_t gfp) > > > { > > > XA_LIMIT(tmp, *next, limit->max); > > > > > > I would have expected to be: > > > > > > XA_LIMIT(tmp, min(limit->min, *next), limit->max); > > > > > > I think it is a very surprising API design that the limit is not > > > respected in all cases. > > > > > > Since going below the limit also disables the 'cyclic' part of this > > > function, why shouldn't callers have to use straight xa_alloc (or > > > __xa_alloc if they need atomic next updates)?? > > > > Here's the code in question which prompted that change ... > > > > for (i = ns->level; i >= 0; i--) { > > - int pid_min = 1; > > - > > - idr_preload(GFP_KERNEL); > > - spin_lock_irq(&pidmap_lock); > > - > > - /* > > - * init really needs pid 1, but after reaching the maximum > > - * wrap back to RESERVED_PIDS > > - */ > > I don't think the comment explaining why the aglorithm is so special > should be deleted :) Fair, but I think it's in the wrong place. It should be up with the definition of RESERVED_PIDS, and it should probably say why! How about this: /* * We avoid reusing PIDs below this to prevent user processes receiving * signals which are intended for system daemons. */ #define RESERVED_PIDS 300 > I think this is clearer as to the intended special case: > > retval = __xa_alloc_cyclic(&tmp->pids, &pid->numbers[i].nr, > NULL, XA_LIMIT(tmp->last_pid < RESERVED_PIDS ? > 1 : RESERVED_PIDS, pid_max), > &tmp->last_pid, GFP_KERNEL); That's ... hard to read for me. How about this? for (i = ns->level; i >= 0; i--) { unsigned int min = RESERVED_PIDS; xa_lock_irq(&tmp->pids); /* Allow allocation below RESERVED_PID the first time */ if (min > tmp->last_pid) min = tmp->last_pid; /* * Store a null pointer so find_pid_ns does not find * a partially initialized PID (see below). */ retval = __xa_alloc_cyclic(&tmp->pids, &pid->numbers[i].nr, NULL, XA_LIMIT(min, pid_max), &tmp->last_pid, GFP_KERNEL); xa_unlock_irq(&tmp->pids); > The biggest 'don't like' I see is that it disables cyclic mode as > well, so not only is the limit not a limit, but the cyclic allocator > is not cyclic.. And that is too many things being described by the > wrong words for my taste. It is cyclic ... it just has a 'tail' (or rather a 'head'). Anyway, it seems like the PID allocator is probably the most unusual, so I'll make that change.
