From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752229AbbIOSiq (ORCPT ); Tue, 15 Sep 2015 14:38:46 -0400 Received: from bedivere.hansenpartnership.com ([66.63.167.143]:55650 "EHLO bedivere.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751708AbbIOSip (ORCPT ); Tue, 15 Sep 2015 14:38:45 -0400 Message-ID: <1442342322.2177.13.camel@HansenPartnership.com> Subject: Re: [PATCH 01/17] Add ida and idr helper routines. From: James Bottomley To: Tejun Heo Cc: Lee Duncan , linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, Hannes Reinecke , Johannes Thumshirn , Christoph Hellwig Date: Tue, 15 Sep 2015 11:38:42 -0700 In-Reply-To: <20150915182755.GA31484@htj.duckdns.org> References: <915ec9ff5e9cc1fae0b36bf7d4c4cb115439e15d.1442263512.git.lduncan@suse.com> <20150915182755.GA31484@htj.duckdns.org> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.11 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, 2015-09-15 at 14:27 -0400, Tejun Heo wrote: > Hello, > > On Tue, Sep 15, 2015 at 09:46:01AM -0700, Lee Duncan wrote: > > +/** > > + * ida_get_index - allocate a ida index value > > + * @ida idr handle > > + * @lock spinlock handle protecting this index > > + * @p_id pointer to allocated index value > > + * > > + * A helper function for safely allocating an index value (id), > > + * returning a negative errno value on failure, else 0. > > + */ > > +static inline int ida_get_index(struct ida *ida, spinlock_t *lock, int *p_id) > > +{ > > + int error = -ENOMEM; > > + > > + do { > > + if (!ida_pre_get(ida, GFP_KERNEL)) > > + break; > > + spin_lock(lock); > > + error = ida_get_new(ida, p_id); > > + spin_unlock(lock); > > + } while (error == -EAGAIN); > > + > > + return error; > > +} > > Obviously ida allocation doesn't need to be synchronized against > anything else. Why not just use ida_simple_get/remove()? For most of the SCSI stuff, yes. I'm less sure about the sd numbers. They go up very high and get hammered a lot during system bring up and hot plug. I think having their own lock rather than wrapping everything around simple_ida_lock makes more sense here just because the system is heavily contended on getting indexes at bring up. To continue the thought, why not move simple_ida_lock into struct ida so we don't have to worry about the contention and can sue ida_simple_... everywhere? James