| Message ID | 915ec9ff5e9cc1fae0b36bf7d4c4cb115439e15d.1442263512.git.lduncan@suse.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Tue, 2015-09-15 at 09:46 -0700, Lee Duncan wrote: > Clients of the ida and idr index-management routines > tend to use the same calling sequences much of the time, > so this change adds helper functions for allocating and > releasing indexes of either flavor, i.e. with or > without pointer management. > > Inline functions added for idr: > idr_get_index_in_range > idr_get_index (in range 0,0) > idr_put_index > And for ida: > ida_get_index > ida_put_index Every consumer of this I've seen seems to have the pattern of allocating the ida and the protecting spinlock together. If that's the case, why not move the spinlock into struct ida so it doesn't have to be separately allocated and passed in to all the helpers? Also, you need a cc of Tejun (added on this one) because he's the one who last did significant work in ida/idr. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Tue, Sep 15, 2015 at 09:46:01AM -0700, Lee Duncan wrote: > +/** > + * ida_get_index - allocate a ida index value > + * @ida idr handle > + * @lock spinlock handle protecting this index > + * @p_id pointer to allocated index value > + * > + * A helper function for safely allocating an index value (id), > + * returning a negative errno value on failure, else 0. > + */ > +static inline int ida_get_index(struct ida *ida, spinlock_t *lock, int *p_id) > +{ > + int error = -ENOMEM; > + > + do { > + if (!ida_pre_get(ida, GFP_KERNEL)) > + break; > + spin_lock(lock); > + error = ida_get_new(ida, p_id); > + spin_unlock(lock); > + } while (error == -EAGAIN); > + > + return error; > +} Obviously ida allocation doesn't need to be synchronized against anything else. Why not just use ida_simple_get/remove()? Thanks.
On Tue, 2015-09-15 at 14:27 -0400, Tejun Heo wrote: > Hello, > > On Tue, Sep 15, 2015 at 09:46:01AM -0700, Lee Duncan wrote: > > +/** > > + * ida_get_index - allocate a ida index value > > + * @ida idr handle > > + * @lock spinlock handle protecting this index > > + * @p_id pointer to allocated index value > > + * > > + * A helper function for safely allocating an index value (id), > > + * returning a negative errno value on failure, else 0. > > + */ > > +static inline int ida_get_index(struct ida *ida, spinlock_t *lock, int *p_id) > > +{ > > + int error = -ENOMEM; > > + > > + do { > > + if (!ida_pre_get(ida, GFP_KERNEL)) > > + break; > > + spin_lock(lock); > > + error = ida_get_new(ida, p_id); > > + spin_unlock(lock); > > + } while (error == -EAGAIN); > > + > > + return error; > > +} > > Obviously ida allocation doesn't need to be synchronized against > anything else. Why not just use ida_simple_get/remove()? For most of the SCSI stuff, yes. I'm less sure about the sd numbers. They go up very high and get hammered a lot during system bring up and hot plug. I think having their own lock rather than wrapping everything around simple_ida_lock makes more sense here just because the system is heavily contended on getting indexes at bring up. To continue the thought, why not move simple_ida_lock into struct ida so we don't have to worry about the contention and can sue ida_simple_... everywhere? James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Tue, Sep 15, 2015 at 11:38:42AM -0700, James Bottomley wrote: > For most of the SCSI stuff, yes. I'm less sure about the sd numbers. > They go up very high and get hammered a lot during system bring up and > hot plug. I think having their own lock rather than wrapping everything > around simple_ida_lock makes more sense here just because the system is > heavily contended on getting indexes at bring up. > > To continue the thought, why not move simple_ida_lock into struct ida so > we don't have to worry about the contention and can sue ida_simple_... > everywhere? We sure can do that if necessary but I'm rather doubtful that even with sd number hammering this is likely to be a problem. Let's convert the users to the simple interface and make the lock per-ida if we actually see contention on the lock. Thanks.
On 09/15/2015 11:41 AM, Tejun Heo wrote: > Hello, > > On Tue, Sep 15, 2015 at 11:38:42AM -0700, James Bottomley wrote: >> For most of the SCSI stuff, yes. I'm less sure about the sd numbers. >> They go up very high and get hammered a lot during system bring up and >> hot plug. I think having their own lock rather than wrapping everything >> around simple_ida_lock makes more sense here just because the system is >> heavily contended on getting indexes at bring up. >> >> To continue the thought, why not move simple_ida_lock into struct ida so >> we don't have to worry about the contention and can sue ida_simple_... >> everywhere? > > We sure can do that if necessary but I'm rather doubtful that even > with sd number hammering this is likely to be a problem. Let's > convert the users to the simple interface and make the lock per-ida if > we actually see contention on the lock. > > Thanks. > To be clear: you would like a patch series that converts the users of the ida_* routines in my patches to instead use the ida_simple_* routines, correct? And of course the ida_* helper routines I was adding in idr.h would not be needed. If this is correct, I will supply a version 2 patch series that addresses this issue as well as the two patch-naming issues that were raised.
Hello, On Fri, Sep 18, 2015 at 08:42:26AM -0700, Lee Duncan wrote: > To be clear: you would like a patch series that converts the users of > the ida_* routines in my patches to instead use the ida_simple_* > routines, correct? And of course the ida_* helper routines I was adding > in idr.h would not be needed. Yeap. > If this is correct, I will supply a version 2 patch series that > addresses this issue as well as the two patch-naming issues that were > raised. Sounds good to me. If the shared lock blows up, we can make that per-ida. Thanks.
diff --git a/include/linux/idr.h b/include/linux/idr.h index 013fd9bc4cb6..341c4f2d9874 100644 --- a/include/linux/idr.h +++ b/include/linux/idr.h @@ -16,6 +16,8 @@ #include <linux/bitops.h> #include <linux/init.h> #include <linux/rcupdate.h> +#include <linux/spinlock.h> +#include <linux/gfp.h> /* * We want shallower trees and thus more bits covered at each layer. 8 @@ -183,4 +185,104 @@ static inline int ida_get_new(struct ida *ida, int *p_id) void __init idr_init_cache(void); +/** + * ida_get_index - allocate a ida index value + * @ida idr handle + * @lock spinlock handle protecting this index + * @p_id pointer to allocated index value + * + * A helper function for safely allocating an index value (id), + * returning a negative errno value on failure, else 0. + */ +static inline int ida_get_index(struct ida *ida, spinlock_t *lock, int *p_id) +{ + int error = -ENOMEM; + + do { + if (!ida_pre_get(ida, GFP_KERNEL)) + break; + spin_lock(lock); + error = ida_get_new(ida, p_id); + spin_unlock(lock); + } while (error == -EAGAIN); + + return error; +} + +/** + * ida_put_index - free an allocated ida index value + * @ida idr handle + * @lock spinlock handle protecting this index + * @id the value of the allocated index + * + * A helper function that goes with @ida_get_index, which safely + * frees a previously-allocated index value. + */ +static inline void ida_put_index(struct ida *ida, spinlock_t *lock, int id) +{ + spin_lock(lock); + ida_remove(ida, id); + spin_unlock(lock); +} + +/** + * idr_get_index_in_range - allocate a new index, with locking + * within a range + * @idr: idr handle + * @lock: spin lock handle protecting the index + * @ptr: pointer to associate with allocated index + * @start: starting index (see idr_alloc) + * @end: ending index (0 -> use default max) + * + * This is a helper routine meant to make the common + * calling sequence to allocate an idr index easier. + * It uses a spin lock, and allocates a positive index + * in the range of [start,end), returning a negative + * errno on failure, else 0. + */ +static inline int idr_get_index_in_range(struct idr *idr, spinlock_t *lock, + void *ptr, int start, int end) +{ + int ret; + + idr_preload(GFP_KERNEL); + spin_lock(lock); + ret = idr_alloc(idr, ptr, start, end, GFP_NOWAIT); + spin_unlock(lock); + idr_preload_end(); + + return ret; +} + +/** + * idr_get_index - allocate new index, with locking, using the + * default range (zero to max-1) + * @idr: idr handle + * @lock: spin lock handle protecting the index + * @ptr: pointer to associate with allocated index + * + * Simple wrapper around idr_get_index_in_range() w/ @start and + * @end of 0, since this is a common case + */ +static inline int idr_get_index(struct idr *idr, spinlock_t *lock, void *ptr) +{ + return idr_get_index_in_range(idr, lock, ptr, 0, 0); +} + +/** + * idr_put_index - free an allocated idr index value + * @idr idr handle + * @lock spinlock handle protecting this index + * @id the value of the allocated index + * + * A helper function that goes with @idr_get_index, which safely + * frees a previously-allocated index value. + */ +static inline void idr_put_index(struct idr *idr, spinlock_t *lock, int id) +{ + spin_lock(lock); + idr_remove(idr, id); + spin_unlock(lock); +} + #endif /* __IDR_H__ */
Clients of the ida and idr index-management routines tend to use the same calling sequences much of the time, so this change adds helper functions for allocating and releasing indexes of either flavor, i.e. with or without pointer management. Inline functions added for idr: idr_get_index_in_range idr_get_index (in range 0,0) idr_put_index And for ida: ida_get_index ida_put_index Signed-off-by: Lee Duncan <lduncan@suse.com> --- include/linux/idr.h | 102 ++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 102 insertions(+)