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 - */ - 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); + + xa_unlock_irq(&tmp->pids); + if (retval >= 0) { + pid->numbers[i].ns = tmp; + tmp = tmp->parent; + continue; } - - pid->numbers[i].nr = nr; - pid->numbers[i].ns = tmp; - tmp = tmp->parent; + if (retval == -ENOSPC) + retval = -EAGAIN; + goto out_free; } 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: drivers/rapidio/rio_cm.c: XA_LIMIT(chstart, RIOCM_MAX_CHNUM), kernel/pid.c: NULL, XA_LIMIT(RESERVED_PIDS, pid_max), net/sched/cls_u32.c: XA_LIMIT(htid + 1, htid | 0xfff), &start, cls_u32 needs to explicitly initialise its 'start' anyway (it wants to start allocating from the middle of the range, for some reason). rapidio would benefit from the behaviour that you'd prefer: } else { /* Obtain channel ID from the dynamic allocation range */ - start = chstart; - end = RIOCM_MAX_CHNUM + 1; + xa_lock_bh(&channels); + if (channel_next < chstart) + channel_next = chstart; + err = __xa_alloc_cyclic(&channels, &ch_num, ch, + XA_LIMIT(chstart, RIOCM_MAX_CHNUM), + &channel_next, GFP_KERNEL); + xa_unlock_bh(&channels); } - idr_preload(GFP_KERNEL); - spin_lock_bh(&idr_lock); - id = idr_alloc_cyclic(&ch_idr, ch, start, end, GFP_NOWAIT); - spin_unlock_bh(&idr_lock); - idr_preload_end(); So with one "vote" from the PID allocator for the current way, one vote against from the RapidIO code and one "don't care" from the cls_u32 code, I haven't felt much need to change it.
