Date: Wed, 26 Oct 2016 10:39:29 -0400
From: Tejun Heo <tj@kernel.org>
To: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: 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: <20161026143929.GA23927@htj.duckdns.org>
References: <20161026142739.20266-1-daniel.vetter@ffwll.ch>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20161026142739.20266-1-daniel.vetter@ffwll.ch>
User-Agent: Mutt/1.7.0 (2016-08-17)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1573
Lines: 40

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