From mboxrd@z Thu Jan 1 00:00:00 1970 From: Joe Eykholt Subject: Re: [PATCH 1/8] scsi: Drop struct Scsi_Host->host_lock around SHT->queuecommand() Date: Fri, 17 Sep 2010 10:24:41 -0700 Message-ID: <4C93A459.1000009@cisco.com> References: <1284676529-10756-1-git-send-email-nab@linux-iscsi.org> <1284691571.26423.50.camel@mulgrave.site> <20100917072022.GB2644@gargoyle.ger.corp.intel.com> <1284725592.26423.60.camel@mulgrave.site> <4C9379AA.4000103@linux.intel.com> <1284735438.26423.81.camel@mulgrave.site> <1284741478.13344.154.camel@haakon2.linux-iscsi.org> Mime-Version: 1.0 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1284741478.13344.154.camel@haakon2.linux-iscsi.org> Sender: linux-kernel-owner@vger.kernel.org To: linux-iscsi-target-dev@googlegroups.com Cc: "Nicholas A. Bellinger" , James Bottomley , Andi Kleen , linux-scsi , linux-kernel , Vasu Dev , Tim Chen , Matthew Wilcox , Mike Christie , James Smart , Andrew Vasquez , FUJITA Tomonori , Hannes Reinecke , Christoph Hellwig List-Id: linux-scsi@vger.kernel.org On 9/17/10 9:37 AM, Nicholas A. Bellinger wrote: > On Fri, 2010-09-17 at 10:57 -0400, James Bottomley wrote: >> On Fri, 2010-09-17 at 16:22 +0200, Andi Kleen wrote: >>>> I don't disagree with the idea of removing it, especially as it has so >>>> few users, but replacing the host lock with an atomic here would still >>>> vastly reduce the contention, which is the initial complaint. The >>> >>> Actually the complaint is the overhead of the spin lock. This can be >>> either caused >>> by contention or by cache line bounce time. >> >> The original complaint was contention. My desire is to reduce the >> locked path coverage, so I saw an opportunity. >> >> What I was actually thinking of for the atomic is that we'd let it range >> [1..INT_MAX] so a zero was an indicator of no use of this. Then the >> actual code could become >> >> if (atomic_read(x)) { >> do { >> y = atomic_add_return(1, x); >> } while (y == 0); >> } A tiny trick I like to use is to start a serial number at 1 and increment by 2 so its always odd and then never wraps to 0. That eliminates the check for 0 (and the curly brackets). > The conversion of struct scsi_cmnd->serial_number to atomic_t and the > above code for scsi_cmd_get_serial() sounds perfectly reasonable to me. > > I will take a look at this conversion and respin a complete set of > patches for review a bit later today. > > Thanks! > > --nab > >> >> So "fast" cards not using the serial number set a zero there (we'd >> default initialise to one), the line is shared so no bouncing (because >> it's never updated). This should satisfy everyone. >> >>>> contention occurs because the host lock is so widely used for other >>>> things. The way to reduce that contention is firstly to reduce the >>>> length of code covered by the lock and also reduce the actual number of >>>> places where the lock is taken. Compared with host lock's current vast >>>> footprint, and atomic here is tiny. >>> >>> That assumes that it's contention that is the problem and not simply >>> bounce time. >> >> That's what the patch and data that started this whole thread showed, >> yes ... but I think actual bounce in the spinlock is also a problem ... >> we just don't have data to show it. >> >> James >> >> >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-scsi" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >