Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1509481 > unrolled thread

[PATCH] lib/ida: Document locking requirements a bit better

Back to article view | Back to linux.kernel

Contents

  [PATCH] lib/ida: Document locking requirements a bit better Daniel Vetter <daniel.vetter@ffwll.ch> - 2016-10-26 16:30 +0200
    Re: [PATCH] lib/ida: Document locking requirements a bit better Tejun Heo <tj@kernel.org> - 2016-10-26 16:40 +0200
      Re: [PATCH] lib/ida: Document locking requirements a bit better Daniel Vetter <daniel@ffwll.ch> - 2016-10-26 21:30 +0200
        Re: [PATCH] lib/ida: Document locking requirements a bit better Tejun Heo <tj@kernel.org> - 2016-10-26 22:10 +0200
          Re: [PATCH] lib/ida: Document locking requirements a bit better Daniel Vetter <daniel@ffwll.ch> - 2016-10-27 09:20 +0200

#1509481 — [PATCH] lib/ida: Document locking requirements a bit better

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(+)

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)
 {
-- 
2.10.1

[toc] | [next] | [standalone]

#1509485

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.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1509725

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [next] | [standalone]

#1509777

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.

-- 
tejun

[toc] | [prev] | [next] | [standalone]

#1510110

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
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web