| Message ID | 20161026142739.20266-1-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hello, Daniel. On Wed, Oct 26, 2016 at 04:27:39PM +0200, Daniel Vetter wrote: > I wanted to wrap a bunch of ida_simple_get calls into their own > locking, until I dug around and read the original commit message. > Stuff like this should imo be added to the kernel doc, let's do that. Generally agreed but some nits below. > @@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get); > * and go back to the ida_pre_get() call. If the ida is full, it will > * return %-ENOSPC. > * > + * Note that callers must ensure that concurrent access to @ida is not possible. > + * When simplicity trumps concurrency needs look at ida_simple_get() instead. Maybe we can make it a bit less dramatic? > @@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy); > * Allocates an id in the range start <= id < end, or returns -ENOSPC. > * On memory allocation failure, returns -ENOMEM. > * > + * Compared to ida_get_new_above() this function does its own locking and hence > + * is recommended everywhere where simplicity is preferred over the last bit of > + * speed. Hmm... so, this isn't necessarily about speed. For example, id allocation might have to happen inside a spinlock which protects a larger scope. To guarantee GFP_KERNEL allocation behavior in such cases, the caller would have to call ida_pre_get() outside the said spinlock and then call ida_get_new_above() inside the lock. I think it'd be better to explain what the simple version does and expects and then say that unless there are specific requirements using the simple version is recommended. Thanks.
On Wed, Oct 26, 2016 at 10:39:29AM -0400, Tejun Heo wrote: > Hello, Daniel. > > On Wed, Oct 26, 2016 at 04:27:39PM +0200, Daniel Vetter wrote: > > I wanted to wrap a bunch of ida_simple_get calls into their own > > locking, until I dug around and read the original commit message. > > Stuff like this should imo be added to the kernel doc, let's do that. > > Generally agreed but some nits below. I value good docs but I suck at typing them ;-) > > @@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get); > > * and go back to the ida_pre_get() call. If the ida is full, it will > > * return %-ENOSPC. > > * > > + * Note that callers must ensure that concurrent access to @ida is not possible. > > + * When simplicity trumps concurrency needs look at ida_simple_get() instead. > > Maybe we can make it a bit less dramatic? What about? "Note that callers must ensure that concurrent access to @ida is not possible. See ida_simple_get() for a varaint which takes care of locking. > > > > @@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy); > > * Allocates an id in the range start <= id < end, or returns -ENOSPC. > > * On memory allocation failure, returns -ENOMEM. > > * > > + * Compared to ida_get_new_above() this function does its own locking and hence > > + * is recommended everywhere where simplicity is preferred over the last bit of > > + * speed. > > Hmm... so, this isn't necessarily about speed. For example, id > allocation might have to happen inside a spinlock which protects a > larger scope. To guarantee GFP_KERNEL allocation behavior in such > cases, the caller would have to call ida_pre_get() outside the said > spinlock and then call ida_get_new_above() inside the lock. Hm, ida_simple_get does that for you already ... > I think it'd be better to explain what the simple version does and > expects and then say that unless there are specific requirements using > the simple version is recommended. What about: "Compared to ida_get_new_above() this function does its own locking, and should be used unless there are special requirements." -Daniel
Hello, Daniel. On Wed, Oct 26, 2016 at 09:25:25PM +0200, Daniel Vetter wrote: > > > + * Note that callers must ensure that concurrent access to @ida is not possible. > > > + * When simplicity trumps concurrency needs look at ida_simple_get() instead. > > > > Maybe we can make it a bit less dramatic? > > What about? > > "Note that callers must ensure that concurrent access to @ida is not possible. > See ida_simple_get() for a varaint which takes care of locking. Yeah, that reads easier to me. > > Hmm... so, this isn't necessarily about speed. For example, id > > allocation might have to happen inside a spinlock which protects a > > larger scope. To guarantee GFP_KERNEL allocation behavior in such > > cases, the caller would have to call ida_pre_get() outside the said > > spinlock and then call ida_get_new_above() inside the lock. > > Hm, ida_simple_get does that for you already ... Here's an example. spin_lock(); do some stuff; something->id = ida_simple_get(some gfp flag); do some stuff; spin_unlock(); In this scenario, you can't use sleeping GFPs for ida_simple_get() because it does preloading inside it. What one has to do is... ida_pre_get(GFP_KERNEL); spin_lock(); do some stuff; something->id = ida_get_new_above(GFP_NOWAIT); do some stuff; spin_unlock(); So, I guess it can be sometimes about avoiding the extra locking overhead but it's more often about separating out allocation context into an earlier call. > > I think it'd be better to explain what the simple version does and > > expects and then say that unless there are specific requirements using > > the simple version is recommended. > > What about: > > "Compared to ida_get_new_above() this function does its own locking, and > should be used unless there are special requirements." Yeah, looks good to me. Thanks.
On Wed, Oct 26, 2016 at 04:07:25PM -0400, Tejun Heo wrote: > Hello, Daniel. > > On Wed, Oct 26, 2016 at 09:25:25PM +0200, Daniel Vetter wrote: > > > > + * Note that callers must ensure that concurrent access to @ida is not possible. > > > > + * When simplicity trumps concurrency needs look at ida_simple_get() instead. > > > > > > Maybe we can make it a bit less dramatic? > > > > What about? > > > > "Note that callers must ensure that concurrent access to @ida is not possible. > > See ida_simple_get() for a varaint which takes care of locking. > > Yeah, that reads easier to me. > > > > Hmm... so, this isn't necessarily about speed. For example, id > > > allocation might have to happen inside a spinlock which protects a > > > larger scope. To guarantee GFP_KERNEL allocation behavior in such > > > cases, the caller would have to call ida_pre_get() outside the said > > > spinlock and then call ida_get_new_above() inside the lock. > > > > Hm, ida_simple_get does that for you already ... > > Here's an example. > > spin_lock(); > do some stuff; > something->id = ida_simple_get(some gfp flag); > do some stuff; > spin_unlock(); > > In this scenario, you can't use sleeping GFPs for ida_simple_get() > because it does preloading inside it. What one has to do is... > > ida_pre_get(GFP_KERNEL); > spin_lock(); > do some stuff; > something->id = ida_get_new_above(GFP_NOWAIT); > do some stuff; > spin_unlock(); > > So, I guess it can be sometimes about avoiding the extra locking > overhead but it's more often about separating out allocation context > into an earlier call. Hm yeah, calling ida_simple_get in that case isn't recommend really. > > > I think it'd be better to explain what the simple version does and > > > expects and then say that unless there are specific requirements using > > > the simple version is recommended. > > > > What about: > > > > "Compared to ida_get_new_above() this function does its own locking, and > > should be used unless there are special requirements." > > Yeah, looks good to me. Ok, will respin, thanks for the review comments. -Daniel
diff --git a/lib/idr.c b/lib/idr.c index 6098336df267..5508d7f6d7be 100644 --- a/lib/idr.c +++ b/lib/idr.c @@ -927,6 +927,9 @@ EXPORT_SYMBOL(ida_pre_get); * and go back to the ida_pre_get() call. If the ida is full, it will * return %-ENOSPC. * + * Note that callers must ensure that concurrent access to @ida is not possible. + * When simplicity trumps concurrency needs look at ida_simple_get() instead. + * * @p_id returns a value in the range @starting_id ... %0x7fffffff. */ int ida_get_new_above(struct ida *ida, int starting_id, int *p_id) @@ -1073,6 +1076,10 @@ EXPORT_SYMBOL(ida_destroy); * Allocates an id in the range start <= id < end, or returns -ENOSPC. * On memory allocation failure, returns -ENOMEM. * + * Compared to ida_get_new_above() this function does its own locking and hence + * is recommended everywhere where simplicity is preferred over the last bit of + * speed. + * * Use ida_simple_remove() to get rid of an id. */ int ida_simple_get(struct ida *ida, unsigned int start, unsigned int end, @@ -1119,6 +1126,13 @@ EXPORT_SYMBOL(ida_simple_get); * ida_simple_remove - remove an allocated id. * @ida: the (initialized) ida. * @id: the id returned by ida_simple_get. + * + * Use to release an id allocated with ida_simple_get(). + * + * Compared to ida_get_new_above() this function does its own locking and hence + * is recommended everywhere where simplicity is preferred over the last bit of + * speed. + * */ void ida_simple_remove(struct ida *ida, unsigned int id) {
I wanted to wrap a bunch of ida_simple_get calls into their own locking, until I dug around and read the original commit message. Stuff like this should imo be added to the kernel doc, let's do that. Cc: Mel Gorman <mgorman@techsingularity.net> Cc: Michal Hocko <mhocko@suse.com> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Tejun Heo <tj@kernel.org> Cc: Andrew Morton <akpm@linux-foundation.org> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com> --- lib/idr.c | 14 ++++++++++++++ 1 file changed, 14 insertions(+)