Re: [Bug 9880] dma_free_coherent in arcmsr when calling areca tools

[James Bottomley <James.Bottomley@HansenPartnership.com>]

On Sat, 2008-02-09 at 17:21 +0000, Russell King wrote:
> On Sat, Feb 09, 2008 at 10:51:16AM -0600, James Bottomley wrote:
> > On Mon, 2008-02-04 at 08:19 +0000, Russell King wrote:
> > > On Sun, Feb 03, 2008 at 09:55:37PM -0600, James Bottomley wrote:
> > > > I've cc'd the people responsible for this apparent bit of idiocy.  Since
> > > > the API addition to dma_alloc_coherent() was the GFP flags so you could
> > > > call it from interrupt context with GFP_ATOMIC if so desired,
> > > > (pci_dma_alloc_consistent always has GFP_ATOMIC semantics), why on earth
> > > > would the corresponding free routine require non-atomic semantics?
> > > 
> > > For the N-th time, when tearing down the MMU mappings which we need
> > > to setup for coherent mappings, we need to invalidate the TLB.  On
> > > SMP systems, that means doing an IPI to the other CPUs to tell them
> > > to invalidate their TLBs as well.
> > 
> > Actually it's the first time I'm seeing this.
> > 
> > > Now, look at the restrictions on calling smp_call_function_on_cpu()
> > > or equivalent function which is used to do the IPIs.  I don't care if
> > > you look that up for x86 or ARM, because the restrictions are the same
> > > - see the last two lines:
> > > 
> > > x86:
> > > /**
> > >  * smp_call_function_mask(): Run a function on a set of other CPUs.
> > >  * @mask: The set of cpus to run on.  Must not include the current cpu.
> > >  * @func: The function to run. This must be fast and non-blocking.
> > >  * @info: An arbitrary pointer to pass to the function.
> > >  * @wait: If true, wait (atomically) until function has completed on other CPUs. *
> > >   * Returns 0 on success, else a negative status code.
> > >  *
> > >  * If @wait is true, then returns once @func has returned; otherwise
> > >  * it returns just before the target cpu calls @func.
> > >  *
> > >  * You must not call this function with disabled interrupts or from a
> > >  * hardware interrupt handler or from a bottom half handler.
> > >  */
> > > 
> > > arm:
> > > /*
> > >  * You must not call this function with disabled interrupts, from a
> > >  * hardware interrupt handler, nor from a bottom half handler.
> > >  */
> > > 
> > > 
> > > So, if the architecture requires you to IPI the TLB flush to other CPUs
> > > this restriction has to propagate to dma_free_coherent.  Or we just don't
> > > provide *any* DMA coherent memory and dictate that such platforms can
> > > never use DMA.
> > > 
> > > Which is more idiotic?
> > 
> > Why are you actually going to the trouble of freeing the mappings?
> 
> If we don't free the mappings, we'll eventually end up running out of VM
> space for the coherent DMA.  We only have about 2MB of virtual space for
> these mappings on most machines.

No, free the memory back to the coherent DMA pool but don't free the
mappings; the next use takes a page with existing mappings, thus saving
the mapping setup/teardown cycle.  My contention is that the system
reaches steady state even for alloc/free drivers.

> > The MO of most consumers is either to allocate once on attach and free
> > on detach (effectively tying up the memory forever) or to alloc and free
> > descriptors as they come in and go out.  In the former case, there's no
> > need for free, in the latter the cycle of setup followed by teardown
> > will add a pretty big latency overhead.  Why not just use a pool
> > implementation, so you always add but never free ... it should reach a
> > steady state for the system and be a lot faster than the current
> > implementation.  Most architectures actually operate like this ... and
> > they tend to alloc and free in page size chunks to make it easy.
> >
> > If you're worried about memory, you can always reap the pool in the
> > background.
> 
> The existing implementation comes from the original PCI DMA code, which
> had the restriction that free was not called from interrupt contexts.
> 
> We now seem to be saying that the newer generic DMA API has different
> requirements, and these have not been reflected in the ARM implementation.
> If some of these folk who have the problem want to provide a patch to
> implement what you've outlined above, I'll certainly review it.

Being able to allocate from interrupt but not free from interrupt does
violate the principle of least surprise.

James