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 :) > - if (idr_get_cursor(&tmp->idr) > RESERVED_PIDS) > - pid_min = RESERVED_PIDS; > - > /* > * Store a null pointer so find_pid_ns does not find > * a partially initialized PID (see below). > */ > - nr = idr_alloc_cyclic(&tmp->idr, NULL, pid_min, > - pid_max, GFP_ATOMIC); > - spin_unlock_irq(&pidmap_lock); > - idr_preload_end(); > - > - if (nr < 0) { > - retval = (nr == -ENOSPC) ? -EAGAIN : nr; > - goto out_free; > + xa_lock_irq(&tmp->pids); > + retval = __xa_alloc_cyclic(&tmp->pids, &pid->numbers[i].nr, > + NULL, XA_LIMIT(RESERVED_PIDS, pid_max), > + &tmp->last_pid, GFP_KERNEL); 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); It is looks important for the pid algorithm to have this shift in behavior, so it shouldn't be subtly hidden in the xarray code.. > Now, most users aren't going to care about it. They want to cycle back > around to 0 or 1, and they're either using zero or one based allocation. > grepping for users of XA_LIMIT() shows only these non-zero users: 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. Since it doesn't seem difficult for the caller that needs this behavior to get it, I'd prefer to see the xarray API be really clear and well specified.. Jason
