scsi: remove dma_is_consistent usage in 53c700
in [Kernel]
|
Prev: block: defer the use of inline biovecs for discard requests
Next: SysRq commands trigger without holding sysrq.
From: James Bottomley on 27 Jun 2010 11:10 On Sun, 2010-06-27 at 19:10 +0900, FUJITA Tomonori wrote: > 53c700 is the only user of dma_is_consistent(): > > BUG_ON(!dma_is_consistent(hostdata->dev, pScript) && L1_CACHE_BYTES < dma_get_cache_alignment()); > > The above code tries to see if the system can allocate coherent memory > or not. It's for some old systems that can't allocate coherent memory > at all (e.g some parisc systems). Actually, that's not the right explanation. The BUG_ON is because of an efficiency in the driver ... it's nothing to do with the architecture. The driver uses a set of mailboxes, but for efficiency's sake, it packs them into a single coherent area and separates the different usages by a L1 cache stride). On architectures capable of manufacturing coherent memory, this is a nice speed up in the DMA infrastructure. However, for incoherent architectures, it's fatal if the dma coherence stride is greater than the L1 cache size, because now we'll get data corruption due to cacheline interference. That's what the BUG_ON is checking for. > I think that we can safely remove the above usage: > > - such old systems haven't triger the above checking for long. > > - the above condition is important for systems that can't allocate > coherent memory if these systems do DMA. So probably it would be > better to have such checking in arch's DMA initialization code > instead of a driver. Well, we can't check in the architecture because it's a driver specific thing ... I suppose making it a rule that dma_get_cache_alignment() *must* be <= L1_CACHE_BYTES fixes it ... we seem to have no architecture violating that, so just add it to the documentation, and the check can go. James -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: FUJITA Tomonori on 29 Jun 2010 22:50
On Tue, 29 Jun 2010 08:37:35 -0500 James Bottomley <James.Bottomley(a)HansenPartnership.com> wrote: > > How about using ARCH_KMALLOC_MINALIGN instead of L1_CACHE_BYTES? (snip) > Actually, I'd rather not do this. The reason is that L1_CACHE_ALIGN is > quite a big performance optimisation on x86 for the driver. Without it, > it's functionally correct, but the DMA use of the mailboxes really > thrashes the cache which damages performance (x86 has > ARCH_KMALLOC_MINALIGN set to 8 ... the default) Ah, I see. If slab.h doesn't define ARCH_KMALLOC_MINALIGN for architectures that don't define it, the driver could do something like: #ifdef ARCH_KMALLOC_MINALIGN #define DMA_ALIGN(x) ALIGN(x, ARCH_KMALLOC_MINALIGN) #else #define DMA_ALIGN(x) ALIGN(x, L1_CACHE_BYTES) #endif Seems that it's better to rename ARCH_KMALLOC_MINALIGN to something like ARCH_DMA_MINALIGN and make ARCH_KMALLOC_MINALIGN the slab internal thing. > The only correctness problem, which the BUG is checking for is mismatch > in dma alignment ... as I said, I'm happy just to rely on that being > correct on every incoherent platform the driver operates on. Ok, it's fine by me too. let's simply remove the BUG_ON. I think that you want to document that dma_get_cache_alignment() cannot be greater than the L1 cache stride. However, seems that dma_get_cache_alignment() is greater than L1_CACHE_BYTES on some architectures (they have some reasons, I assume). So I'll just remove the BUG_ON. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |