Skip site navigation (1)Skip section navigation (2)
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, 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:  <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>