Date: Sun, 09 Dec 2012 11:17:41 -0700 From: Ian Lepore <freebsd@damnhippie.dyndns.org> To: Jeff Roberson <jroberson@jroberson.net> Cc: powerpc@freebsd.org, marcel@freebsd.org, mips@freebsd.org, mav@freebsd.org, scottl@freebsd.org, attilio@freebsd.org, kib@freebsd.org, sparc64@freebsd.org, arm@freebsd.org Subject: Re: Call for testing and review, busdma changes Message-ID: <1355077061.87661.320.camel@revolution.hippie.lan> In-Reply-To: <alpine.BSF.2.00.1212080841370.4081@desktop> References: <alpine.BSF.2.00.1212080841370.4081@desktop>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sat, 2012-12-08 at 08:51 -1000, Jeff Roberson wrote: > Hello, > > http://people.freebsd.org/~jeff/physbio.diff > > I have a relative large patch that reforms the busdma API so that new > types may be added without modifying every architecture's > busdma_machdep.c. It does this by unifying the bus_dmamap_load_buffer() > routines so that they may be called from MI code. The MD busdma is then > given a chance to do any final processing in the complete() callback. > This patch also contains cam changes to unify the bus_dmamap_load* > handling in cam drivers. > > The arm and mips implementations saw the largest changes since they have > to track virtual addresses for sync(). Previously this was done in a type > specific way. Now it is done in a generic way by recording the list of > virtuals in the map. > > I have verified that this patch passes make universe which includes > several kernel builds from each architecture. I suspect that if I broke > anything your machine simply won't boot or will throw I/O errors. There > is little subtlety, it is mostly refactoring. > Testing on armv4/v5 platforms, the patches applied and compiled cleanly, but fault-abort with a NULL deref on what appears to be the first call to bus_dmamap_load(). I'll dig deeper into debugging that after browsing the new code so I can figure out where to start debugging. Just from an initial quick browsing of the new code for armv4, I notice these things... The new routine __bus_dmamap_mayblock() does (*flags) |= BUS_DMA_WAITOK; which is a no-op because BUS_DMA_WAITOK is zero. I'm not sure what the intent was, but I think any modification of the WAIT-related flags would be conceptually wrong because it overrides the caller's instructions on whether or not to do a deferred load when resources aren't available. The way you've refactored the mbuf-related load routines, the MD code is no longer able to tell that it's an mbuf operation. This comes back to what I said a few days ago... when you look at the current code on various platforms you see a lot of false similarities. The code isn't nearly identical because it really all needs to be the same, it's because it was all copied from the same source, and it doesn't currently work correctly on arm and mips. With your changes, the ability to correct the implementation on those platforms is now gone. Perhaps this can be fixed easily by defining some new flags that the MI routines can OR-in to give the MD routines more info on what's happening. (Correcting the mbuf sync handling on arm and mips requires that we know at sync time that the buffers involved are mbufs, which means we need to know at map-load time so we can set a flag or a type code or something in the map that we can examine at sync time.) > The next step is to allow for dma loading of physical addresses. This > will permit unmapped I/O. Which is a significant performance optimization > targeted for 10.0. This sounds scary for arm and mips, or more specifically for VIVT cache platforms that can only do a sync based on virtual addresses. Can you talk some more about the plans for this? -- Ian
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1355077061.87661.320.camel>