Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755253AbcJ0HTN (ORCPT ); Thu, 27 Oct 2016 03:19:13 -0400 Received: from mail-wm0-f67.google.com ([74.125.82.67]:35332 "EHLO mail-wm0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754887AbcJ0HTL (ORCPT ); Thu, 27 Oct 2016 03:19:11 -0400 Date: Thu, 27 Oct 2016 09:19:07 +0200 From: Daniel Vetter To: Tejun Heo Cc: Daniel Vetter , 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: <20161027071907.n5ifu2pkpse3aqpb@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> <20161026192525.qctwje64kyq7p3f3@phenom.ffwll.local> <20161026200725.GA19388@htj.duckdns.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161026200725.GA19388@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: 2194 Lines: 65 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