Date: Sun, 09 Dec 2012 13:34:10 -0700 From: Ian Lepore <freebsd@damnhippie.dyndns.org> To: Jeff Roberson <jroberson@jroberson.net> Cc: powerpc@freebsd.org, marcel@freebsd.org, mips@freebsd.org, John Baldwin <jhb@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: <1355085250.87661.345.camel@revolution.hippie.lan> In-Reply-To: <alpine.BSF.2.00.1212090840080.4081@desktop> References: <alpine.BSF.2.00.1212080841370.4081@desktop> <1355077061.87661.320.camel@revolution.hippie.lan> <alpine.BSF.2.00.1212090840080.4081@desktop>
next in thread | previous in thread | raw e-mail | index | archive | help
--=-jWdke4I5Nd/QjDc9IHG8 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit On Sun, 2012-12-09 at 08:48 -1000, Jeff Roberson wrote: > > On Sun, 9 Dec 2012, Ian Lepore wrote: > > > 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. > >> First an FYI, my replies to this thread aren't making it to the mailing lists, except when quoted by someone replying to them. They're being held for moderator approval because they're going to too many addresses. > > > > 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. > > Can you give me the file and line of the fault? > Better than that, I can give you patches (attached). There were two little glitches... the allocated slist wasn't getting attached to the map, and when building the slist in _bus_dmamap_load_buffer() the slist needs to participate in the coalescing logic near the end of the loop. I'm not positive the attached quick-fix is perfect, but it allows one of my arm units here to boot and run and pass some basic IO smoke tests. I'll be trying it on other arm platforms later today. > > > > 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 intent of the original load function seems to be that it is always > blocking. Most architectures force the flag. The mbuf and uio load > routines don't support blocking at all because the callback lacks the > information needed to restart the operation. > The manpage states that bus_dmamap_load() never blocks for any reason. The mbuf and uio flavors say they are variations of bus_dmapmap_load(); I take the implication of that to mean that they also are defined as never blocking. The docs then say that WAITOK vs. NOWAIT control the scheduling of a deferred load, and that NOWAIT is forced in the mbuf and uio cases. Your refactored code already handles that by adding NOWAIT to the caller-specified flags for mbufs and uio. So the code will do the right thing now (with your patches), but only because (*flags) |= BUS_DMA_WAITOK is a no-op. > > > > 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.) > > Why do you need to know it is an mbuf if you have a record of the virtual > addresses? The only type specific information was used to find the > addresses. See how arm's busdma_machdep-v6 ws implemented. > Because there is a special rule for mbufs on VIVT-cache architectures: the address of the data buffer is not aligned to a cacheline boundary, but the sync code is allowed to treat the buffer as if it is aligned and thus avoid invoking the partial cacheline flush algorithm. That algorithm was shown a few months ago to be fatally flawed, and we're on a path (albeit in super-slow-motion) to eliminate it. I have patches to fix the mbuf partial sync and other things related to partial sync problems, and my next step will be to try to rework them to fit in with what you've done. Based on what I've seen so far in your refactoring, I think just passing a flag to say it's an mbuf, from the MI to the MD mapping routine, will provide enough info. > > > >> 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? > > It will be for addresses which were never mapped into kva. To support > unmaapped io. There will only be a need for bounce pages, no syncing. > Can the pages be mapped into any non-kernel vmspaces? If they aren't mapped into any vmspace at all, then I think there'll be no implications for VIVT cache architectures. If they appear in any pmap at all then there may be problems. -- Ian --=-jWdke4I5Nd/QjDc9IHG8 Content-Disposition: inline; filename="physbio_armv4fix.diff" Content-Type: text/x-patch; name="physbio_armv4fix.diff"; charset="us-ascii" Content-Transfer-Encoding: 7bit diff -r bee560bf0159 sys/arm/arm/busdma_machdep.c --- a/sys/arm/arm/busdma_machdep.c Sun Dec 09 12:12:11 2012 -0700 +++ b/sys/arm/arm/busdma_machdep.c Sun Dec 09 13:01:12 2012 -0700 @@ -310,9 +310,10 @@ static __inline bus_dmamap_t map->flags = DMAMAP_ALLOCATED; } else map->flags = 0; - if (map != NULL) + if (map != NULL) { STAILQ_INIT(&map->bpages); - else + map->slist = slist; + } else free(slist, M_DEVBUF); return (map); } @@ -764,7 +765,6 @@ int { bus_size_t sgsize; bus_addr_t curaddr, baddr, bmask; - struct sync_list *sl; vm_offset_t vaddr = (vm_offset_t)buf; int seg; int error = 0; @@ -855,14 +855,6 @@ int if (((dmat->flags & BUS_DMA_COULD_BOUNCE) != 0) && map->pagesneeded != 0 && run_filter(dmat, curaddr)) { curaddr = add_bounce_page(dmat, map, vaddr, sgsize); - } else { - sl = &map->slist[map->sync_count]; - if (++map->sync_count > dmat->nsegments) - goto cleanup; - printf("sl %p count %d nseg %d\n", sl, map->sync_count, dmat->nsegments); - sl->vaddr = vaddr; - sl->datacount = sgsize; - sl->busaddr = curaddr; } if (dmat->ranges) { @@ -891,10 +883,15 @@ int (segs[seg].ds_addr & bmask) == (curaddr & bmask))) { segs[seg].ds_len += sgsize; + map->slist[seg].datacount += sgsize; goto segdone; } else { if (++seg >= dmat->nsegments) break; + ++map->sync_count; + map->slist[seg].vaddr = vaddr; + map->slist[seg].busaddr = curaddr; + map->slist[seg].datacount = sgsize; segs[seg].ds_addr = curaddr; segs[seg].ds_len = sgsize; } @@ -906,7 +903,6 @@ segdone: } *segp = seg; -cleanup: /* * Did we fit? */ --=-jWdke4I5Nd/QjDc9IHG8--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1355085250.87661.345.camel>