Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755516AbcJZTZc (ORCPT ); Wed, 26 Oct 2016 15:25:32 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:33998 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755348AbcJZTZ3 (ORCPT ); Wed, 26 Oct 2016 15:25:29 -0400 Date: Wed, 26 Oct 2016 21:25:25 +0200 From: Daniel Vetter To: Tejun Heo Cc: Daniel Vetter , LKML , Intel Graphics Development , Mel Gorman , Michal Hocko , Vlastimil Babka , Andrew Morton , Daniel Vetter Subject: Re: [PATCH] lib/ida: Document locking requirements a bit better Message-ID: <20161026192525.qctwje64kyq7p3f3@phenom.ffwll.local> Mail-Followup-To: Tejun Heo , LKML , Intel Graphics Development , Mel Gorman , Michal Hocko , Vlastimil Babka , Andrew Morton , Daniel Vetter References: <20161026142739.20266-1-daniel.vetter@ffwll.ch> <20161026143929.GA23927@htj.duckdns.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161026143929.GA23927@htj.duckdns.org> X-Operating-System: Linux phenom 4.6.0-1-amd64 User-Agent: NeoMutt/20161014 (1.7.1) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2164 Lines: 57 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