Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752917AbbD3OzP (ORCPT ); Thu, 30 Apr 2015 10:55:15 -0400 Received: from bedivere.hansenpartnership.com ([66.63.167.143]:40599 "EHLO bedivere.hansenpartnership.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752891AbbD3OzK (ORCPT ); Thu, 30 Apr 2015 10:55:10 -0400 Message-ID: <1430405707.2167.13.camel@HansenPartnership.com> Subject: Re: [PATCH] scatterlist: enable sg chaining for all architectures From: James Bottomley To: "Nicholas A. Bellinger" Cc: Akinobu Mita , Andrew Morton , LKML , Arnd Bergmann , linux-arch@vger.kernel.org, Christoph Hellwig , "linux-scsi@vger.kernel.org" , target-devel@vger.kernel.org, Parisc List Date: Thu, 30 Apr 2015 07:55:07 -0700 In-Reply-To: <1430380782.24121.99.camel@haakon3.risingtidesystems.com> References: <1429973776-7499-1-git-send-email-akinobu.mita@gmail.com> <20150428142743.578d1c930aca013b596d7546@linux-foundation.org> <1430259419.2181.26.camel@HansenPartnership.com> <1430273746.2181.49.camel@HansenPartnership.com> <1430380782.24121.99.camel@haakon3.risingtidesystems.com> 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 Content-Length: 4370 Lines: 90 On Thu, 2015-04-30 at 00:59 -0700, Nicholas A. Bellinger wrote: > On Tue, 2015-04-28 at 19:15 -0700, James Bottomley wrote: > > On Wed, 2015-04-29 at 09:34 +0900, Akinobu Mita wrote: > > > 2015-04-29 7:16 GMT+09:00 James Bottomley > > > : > > > > On Tue, 2015-04-28 at 14:27 -0700, Andrew Morton wrote: > > > >> On Sat, 25 Apr 2015 23:56:16 +0900 Akinobu Mita wrote: > > > >> > > > >> > Some architectures enable sg chaining option while others do not. > > > >> > > > > >> > The requirement to enable sg chaining is that pages must be aligned > > > >> > at a 32-bit boundary in order to overload the LSB of the pointer. > > > >> > Regardless of whether ARCH_HAS_SG_CHAIN is defined or not, the above > > > >> > requirement is always chacked by BUG_ON() in sg_assign_page. So > > > >> > all architectures can enable sg chaining. > > > >> > > > > >> > As you can see from the changes in drivers/target/target_core_rd.c, > > > >> > enabling SG chaining for all architectures allows us to allocate > > > >> > discontiguous scatterlist tables which can be traversed throughout > > > >> > by sg_next() without a special handling for some architectures. > > > >> > > > >> Thanks, I'll grab this. If anyone has concerns, speak now or hold both > > > >> pieces! > > > > > > > > It breaks a host of architectures doesn't it? I can specifically speak > > > > for PARISC: The problem is the way our iommus are consuming > > > > scatterlists. They're assuming we can dereference the scatterlist as an > > > > array (like this code in ccio-dma.c): > > > > > > > > static int > > > > ccio_map_sg(struct device *dev, struct scatterlist *sglist, int nents, > > > > enum dma_data_direction direction) > > > > [...] > > > > for(i = 0; i < nents; i++) > > > > prev_len += sglist[i].length; > > > > > > > > If you turn on sg chaining on our architecture, we'll run off the end of > > > > that array dereference and crash. > > > > > > > > This can all be fixed by making our architecture dma mapping code use > > > > iterators instead of array lists, but that needs more code than this > > > > patch provides. I assume there are similar issues on a lot of other > > > > architectures, so before you can contemplate a patch like this, surely > > > > all the architecture consumers have to be converted to iterator instead > > > > of array format? > > > > > > > > The first place to start would be a survey of who's still using the > > > > array format. > > > > > > Agreed. I could find similar issues in arch/m68k/kernel/dma.c. > > > (git grep '[^a-z]sg++' shows that there are a lot of similar issues) > > > > OK, so the original idea of the chained SG lists was that most of the > > older architectures have fixed length lists for their IOMMUs, or simply > > wouldn't see a benefit with IO lengths > 0.5MB (which was the default > > before chaining) so there wasn't much point converting them to chaining > > if they wouldn't see any benefit from it. > > > > ARCH_HAS_SG_CHAIN is supposed to be completely transparent to all driver > > side consumers, so there was never thought to be much point removing it. > > It looks like there's some sort of cockup going on in the target driver > > but otherwise, your removal patch is pretty empty, confirming this. > > > > Perhaps the best thing to do is just fix target and call it quits? > > > > So the ARCH_HAS_SG_CHAIN usage in target_core_rd.c was recently added so > target DIF emulation could use standard SGL iterators and correctly > handle boundaries across T10-PI metadata SGL tables in the ramdisk > backend. > > The SGLs in question are never actually mapped to a HW IOMMU, and > Akinobu's current changes in mainline do support both arch cases and > make common sbc_dif_copy_prot() code a bit simpler too. > > That said, I'd rather to keep the hack around for now so that both > ARCH_HAS_SG_CHAIN types can still work, short of a full arch conversion > of course.. It looks like you might not have needed the hack if you'd used the existing sg chain allocators .... James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/