Date: Thu, 5 Nov 2015 10:45:25 -0800 From: Jason Harmening <jason.harmening@gmail.com> To: Ian Lepore <ian@freebsd.org> Cc: Svatopluk Kraus <onwahe@gmail.com>, src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r289759 - in head/sys/arm: arm include Message-ID: <CAM=8qan2=aTR=9kmTxRia4bH6WiwBfVmrNaP=gys2ni5ErSk-Q@mail.gmail.com> In-Reply-To: <1446744103.91534.399.camel@freebsd.org> References: <201510221638.t9MGc1cc053885@repo.freebsd.org> <CAGtf9xOx_iSNqsPT7OdTmAy-ER_wFzOC9YxFigpQ%2B0wLj7=ytQ@mail.gmail.com> <56348FF8.3010602@gmail.com> <CAM=8qa=4EqPvRoUnMytQBe32GtvPRZRE_s34tOzmVNCLe8c=Fw@mail.gmail.com> <1446394311.91534.236.camel@freebsd.org> <CAM=8qa=GX3yZmcXmMZz-nfmwOFhXyyKh_6%2BLVR=y7P5pURYvpQ@mail.gmail.com> <CAFHCsPWE3YLcO_GKL7pGkB5wU342WDZ5T695S_DPG7eBQNmqaQ@mail.gmail.com> <CAM=8qakYSkY-8_z7D%2Bg=ctip8h9qrb-k_dLRyZVYLg5AvTZbEw@mail.gmail.com> <CAFHCsPX2cGn5EFg1M9mnpsnq=ikUfnAb%2BhxF1zRx8ckDsivR_w@mail.gmail.com> <CAM=8qanoZotHXsiDyUCKe5jszzPsLE07uxM1vnOBnKowvvSvYQ@mail.gmail.com> <1446744103.91534.399.camel@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, Nov 5, 2015 at 9:21 AM, Ian Lepore <ian@freebsd.org> wrote: > Just as an FYI for you guys, since we're all splashing around in busdma > code lately.... The things on my near-term (next several weeks) to-do > list for busdma are: > > Add "small bounce" support to help reduce the bounce preallocation > overhead for handling bounces due to cacheline alignment. Almost all > arm and mips bounces are triggered by cache alignment restrictions and > involve buffers smaller than a page (usually 256 bytes or less), and we > currently allocate full pages for bouncing. Instead we can use the > same small-buffer uma pools that are used for bus_dmamem_alloc(). > I like that. Along similar (but definitely not the same) lines, I had a suggestion in https://reviews.freebsd.org/D888 to coalesce bounce buffers, similar to how addseg already coalesces adjacent segments. Would something like that help here (in addition to small-bounce) to cut down on bounce buffer overhead, when the total buffer size is still a page or larger? > > The armv6 busdma implementation can be shared by armv4, armv6, and > mips. The only thing blocking this right now is the small-bounce > stuff; armv4 and mips boards often have only 32-128MB of ram, and they > can't afford the bounce-page allocation overhead of the current armv6 > implementation. Once the small-bounce support is in place, we can use > this implementation for all these platforms (and basically for any > platform that comes along that has a similar requirement for software > -assisted cache coherency for DMA). > Do you plan to merge armv6 and mips soonish? If so, then https://reviews.freebsd.org/D3986 can go away. That will also fix the problems mips busdma has with the cacheline copies in sync_buf() not being threadsafe. Code duplication in busdma is a huge pain right now. It seems like we have 3 broad classes of busdma support: bounce/coherent, bounce/non-coherent, and iommu. bounce/coherent could probably share all the same code. You might need a light dusting of platform-specific code (e.g. powerpc issues a sync instruction at the end of bus_dmamap_sync), but otherwise it should be the same. bounce/noncoherent could share the same code, as long as you define a set of macros or inline funcs that abstract the cache maintenance. That'll already be needed to cover the cache differences between armv4, armv6, and mips anyway. iommu is always going to be hardware-specific and need its own implementation for each kind of iommu. Then there's some logic that could probably be shared everywhere: a lot of the bus_dmamem_alloc()/bufalloc code strikes me as falling into that category. You might need MD wrappers that call MI helper functions, so for example the MD code can decide whether BUS_DMA_COHERENT means VM_MEMATTR_UNCACHEABLE, or whether allocated buffers really need to be physically contiguous (not needed for iommu). On top of all that, it makes sense to generalize the vtable approach already used for x86 and arm64. In a lot of cases the vtable would just point to the common bounce/coherent or bounce/noncoherent functions, or in some cases lightweight MD wrappers around those. Then for example it would be easy to add bounce/noncoherent support for arm64 if we end up needing it, and somewhat easier to add arm/mips/ppc iommu support. I'm definitely not suggesting you do all that stuff right now, just throwing ideas out there :) > > -- Ian > > On Thu, 2015-11-05 at 08:08 -0800, Jason Harmening wrote: > > Userspace buffers in load_buffer() also need temporary mappings, so > > it > > might be nice to keep the panic/KASSERT there for completeness. I > > don't > > see how anything coming from userspace would be outside > > vm_page_array > > though, so that is up to you and Michal. > > > > Since Michal's already made the initial patch, I think he should make > > the > > commit. That seems only right, plus I'm still busy trying to get > > everything set up at my new place. I'd like to be on the phabricator > > review, though. > > > > On Thu, Nov 5, 2015 at 3:58 AM, Svatopluk Kraus <onwahe@gmail.com> > > wrote: > > > > > On Thu, Nov 5, 2015 at 1:11 AM, Jason Harmening > > > <jason.harmening@gmail.com> wrote: > > > > > > > > On Wed, Nov 4, 2015 at 2:17 PM, Svatopluk Kraus <onwahe@gmail.com > > > > > > > > wrote: > > > > > > > > > > On Tue, Nov 3, 2015 at 8:45 AM, Jason Harmening > > > > > <jason.harmening@gmail.com> wrote: > > > > > > > > > > > > On Sun, Nov 1, 2015 at 8:11 AM, Ian Lepore <ian@freebsd.org> > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > It's almost certainly not related to sysinit ordering. > > > > > > > This > > > exception > > > > > > > is happening during mmc probing after interrupts are > > > > > > > enabled. > > > > > > > > > > > > > > It appears that the problem is the faulting code is running > > > > > > > on one of > > > > > > > the very early pre-allocated kernel stacks (perhaps in an > > > > > > > interrupt > > > > > > > handler on an idle thread stack), and these stacks are not > > > > > > > in memory > > > > > > > represented in the vm page arrays. The mmc code is using a > > > > > > > 64-byte > > > > > > > buffer on the stack and mapping it for DMA. Normally that > > > > > > > causes a > > > > > > > bounce for cacheline alignment, but unluckily in this case > > > > > > > that > > > buffer > > > > > > > on the stack just happened to be aligned to a cacheline > > > > > > > boundary and > > > a > > > > > > > multiple of the cacheline size, so no bounce. That causes > > > > > > > the new > > > sync > > > > > > > logic that is based on keeping vm_page_t pointers and > > > > > > > offsets to get > > > a > > > > > > > NULL pointer back from PHYS_TO_VM_PAGE when mapping, then > > > > > > > it dies at > > > > > > > sync time trying to dereference that. It used to work > > > > > > > because the > > > sync > > > > > > > logic used to use the vaddr, not a page pointer. > > > > > > > > > > > > > > Michal was working on a patch yesterday. > > > > > > > > > > > > > > > > > > > Ah, thanks for pointing that out Ian. I was left scratching > > > > > > my head > > > > > > (admittedly on the road and w/o easy access to the code) > > > > > > wondering > > > what > > > > > > on > > > > > > earth would be trying to do DMA during SI_SUB_CPU. > > > > > > > > > > > > > > > > > > > > > Using of fictitious pages is not so easy here as in case > > > > > pointed by > > > > > kib@ where they are allocated and freed inside one function. > > > > > For sync > > > > > list sake, they must be allocated when a buffer is loaded and > > > > > freed > > > > > when is unloaded. > > > > > > > > > > Michal uses pmap_kextract() in case when KVA of buffer is not > > > > > zero in > > > > > his proof-of-concept patch: > > > > > https://gist.github.com/strejda/d5ca3ed202427a2e0557 > > > > > When KVA of buffer is not zero, temporary mapping is not used > > > > > at all, > > > > > so vm_page_t array is not needed too. > > > > > > > > > > IMO, buffer's physical address can be saved in sync list to be > > > > > used > > > > > when its KVA is not zero. Thus pmap_kextract() won't be called > > > > > in > > > > > dma_dcache_sync(). > > > > > > > > > > > > I like the idea of storing off the physical address. If you want > > > > to save > > > > space in the sync list, I think you can place busaddr and pages > > > > in a > > > union, > > > > using vaddr == 0 to select which field to use. Some people frown > > > > upon > > > use > > > > of unions, though. > > > > > > > > Any reason the panics on PHYS_TO_VM_PAGE in load_buffer() and > > > > load_phys() > > > > shouldn't be KASSERTs instead? > > > > > > > > > KASSERTs are fine. I have looked at Michal's patch again and only > > > KASSERT should be in load_phys() as such buffers are unmapped, > > > temporary mapping must be used and so, they must be in > > > vm_page_array. > > > And maybe some inline function for getting PA from sync list would > > > be > > > nice too. I would like to review final patch if you are going to > > > make > > > it. And it would be nice if Ganbold will test it. Phabricator? > > > >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAM=8qan2=aTR=9kmTxRia4bH6WiwBfVmrNaP=gys2ni5ErSk-Q>