Date: Wed, 26 Oct 2016 16:07:25 -0400
From: Tejun Heo <tj@kernel.org>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
        LKML <linux-kernel@vger.kernel.org>,
        Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
        Mel Gorman <mgorman@techsingularity.net>,
        Michal Hocko <mhocko@suse.com>, Vlastimil Babka <vbabka@suse.cz>,
        Andrew Morton <akpm@linux-foundation.org>,
        Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH] lib/ida: Document locking requirements a bit better
Message-ID: <20161026200725.GA19388@htj.duckdns.org>
References: <20161026142739.20266-1-daniel.vetter@ffwll.ch>
 <20161026143929.GA23927@htj.duckdns.org>
 <20161026192525.qctwje64kyq7p3f3@phenom.ffwll.local>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161026192525.qctwje64kyq7p3f3@phenom.ffwll.local>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1843
Lines: 60

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