Re: 2.6.22-rc6 bad page error

On Wed, 11 Jul 2007, Ivan Kokshaysky wrote:
> On Tue, Jul 10, 2007 at 09:25:50PM +0100, Hugh Dickins
wrote:
> > On Mon, 9 Jul 2007, Bob Tracy wrote:
> > > That seems to have done the trick.  Normally,
I get the "bad page"
> > > errors on the second NX session, but I'm on
the third session of the
> > > day (thus far), and everything seems to be ok
as far as I can tell.
> > 
> > Thanks a lot for the feedback, that tells us we've
made the right
> > guesses, and my __GFP_COMP patch should be a good
bandaid for you;
> > but for the right patch, let's wait to hear from
Ivan.
> 
> Here it is. Bob, can you test this one?
> 
> ---
> 
> Take care of DMA flags passed to dma_alloc_coherent().
> Particularly ALSA needs that.
> 
> Signed-off-by: Ivan Kokshaysky <inkjurassic.park.msu.ru>

Yes, I agree that's the right way to go.  Aside from fixing
Bob's
crash, it also liberates ALSA from needing GFP_ATOMIC there
on Alpha.
Couple of GFP_ notes below.

Hugh

> 
> Ivan.
> 
> --- 2.6.22/arch/alpha/kernel/pci_iommu.c	Mon Jul  9
03:32:17 2007
> +++ linux/arch/alpha/kernel/pci_iommu.c	Wed Jul 11
14:40:37 2007
>  -398,11 +398,13 
EXPORT_SYMBOL(pci_unmap_page);
>     else DMA_ADDRP is undefined.  */
>  
>  void *
> -pci_alloc_consistent(struct pci_dev *pdev, size_t
size, dma_addr_t *dma_addrp)
> +__pci_alloc_consistent(struct pci_dev *pdev, size_t
size,
> +		       dma_addr_t *dma_addrp, gfp_t gfp)
>  {
>  	void *cpu_addr;
>  	long order = get_order(size);
> -	gfp_t gfp = GFP_ATOMIC;
> +
> +	gfp &= ~GFP_DMA;

The canonical form for this masking, even on arches without
HIGHMEM,
appears to be:
	gfp &= ~(__GFP_DMA | __GFP_HIGHMEM);
But I guess that won't make any actual difference.

>  
>  try_again:
>  	cpu_addr = (void *)__get_free_pages(gfp, order);
>  -432,7 +434,7  try_again:
>  
>  	return cpu_addr;
>  }
> -EXPORT_SYMBOL(pci_alloc_consistent);
> +EXPORT_SYMBOL(__pci_alloc_consistent);
>  
>  /* Free and unmap a consistent DMA buffer.  CPU_ADDR
and DMA_ADDR must
>     be values that were returned from
pci_alloc_consistent.  SIZE must
> --- 2.6.22/include/asm-alpha/pci.h	Mon Jul  9 03:32:17
2007
> +++ linux/include/asm-alpha/pci.h	Wed Jul 11 14:40:37
2007
>  -75,7 +75,13  extern inline void pcibios_penalize_isa_
>     successful and sets *DMA_ADDRP to the pci side dma
address as well,
>     else DMA_ADDRP is undefined.  */
>  
> -extern void *pci_alloc_consistent(struct pci_dev *,
size_t, dma_addr_t *);
> +extern void *__pci_alloc_consistent(struct pci_dev *,
size_t,
> +				    dma_addr_t *, gfp_t);
> +static inline void *
> +pci_alloc_consistent(struct pci_dev *dev, size_t size,
dma_addr_t *dma)
> +{
> +	return __pci_alloc_consistent(dev, size, dma,
GFP_ATOMIC);

I was going to ask you why that needs to be GFP_ATOMIC on
Alpha.
But find you're following the example of asm-generic and
others.
So really should be asking Sparc how it gets away with
GFP_KERNEL.

> +}
>  
>  /* Free and unmap a consistent DMA buffer.  CPU_ADDR
and DMA_ADDR must
>     be values that were returned from
pci_alloc_consistent.  SIZE must
> --- 2.6.22/include/asm-alpha/dma-mapping.h	Mon Jul  9
03:32:17 2007
> +++ linux/include/asm-alpha/dma-mapping.h	Wed Jul 11
14:40:37 2007
>  -11,7 +11,7 
>  #define dma_unmap_single(dev, addr, size, dir)		
>  		pci_unmap_single(alpha_gendev_to_pci(dev), addr,
size, dir)
>  #define dma_alloc_coherent(dev, size, addr, gfp)	
> -		pci_alloc_consistent(alpha_gendev_to_pci(dev), size,
addr)
> +	     
__pci_alloc_consistent(alpha_gendev_to_pci(dev), size, addr,
gfp)
>  #define dma_free_coherent(dev, size, va, addr)		
>  		pci_free_consistent(alpha_gendev_to_pci(dev), size,
va, addr)
>  #define dma_map_page(dev, page, off, size, dir)		
-
To unsubscribe from this list: send the line
"unsubscribe linux-alpha" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html

On Wed, Jul 11, 2007 at 06:07:59PM +0100, Hugh Dickins
wrote:
> > +	gfp &= ~GFP_DMA;
> 
> The canonical form for this masking, even on arches
without HIGHMEM,
> appears to be:
> 	gfp &= ~(__GFP_DMA | __GFP_HIGHMEM);
> But I guess that won't make any actual difference.

As far as I can see, none of 64-bit architectures masks the
HIGHMEM
(or any other) bits. It's just not obvious because most of
their
"alloc_coherent" routines are hidden in iommu_ops
or something like that.

> > +	return __pci_alloc_consistent(dev, size, dma,
GFP_ATOMIC);
> 
> I was going to ask you why that needs to be GFP_ATOMIC
on Alpha.
> But find you're following the example of asm-generic
and others.
> So really should be asking Sparc how it gets away with
GFP_KERNEL.

According to DMA-mapping.txt GFP_ATOMIC is mandatory for
pci_alloc_consistent() since it can be called from interrupt
context.
So sparc code is probably incorrect...

Ivan.
-
To unsubscribe from this list: send the line
"unsubscribe linux-alpha" in
the body of a message to majordomovger.kernel.org
More majordomo info at  http://vge
r.kernel.org/majordomo-info.html