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

[Daniel Vetter <daniel@ffwll.ch>]

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