Re: [PATCH] x86: avoid unnecessary low zone allocation in AMD IOMMU's alloc_coherent

FUJITA Tomonori
2008-09-10T13:38:18Z

On Wed, 10 Sep 2008 15:10:32 +0200
Joerg Roedel <joerg.roedel@amd.com> wrote:

> On Wed, Sep 10, 2008 at 10:03:32PM +0900, FUJITA Tomonori wrote:
>
> Ok, I see.
>
> > Hmm, I'm not sure what code you look at. Here's dma_alloc_coherent()
> > in tip/x86/iommu:
> >
> > dma_alloc_coherent(struct device *dev, size_t size, dma_addr_t *dma_handle,
> > gfp_t gfp)
> > {
> > struct dma_mapping_ops *ops = get_dma_ops(dev);
> > void *memory;
> >
> > gfp &= ~(__GFP_DMA | __GFP_HIGHMEM | __GFP_DMA32);
> >
> > Surely we here clear the flag but...
> >
> > if (dma_alloc_from_coherent(dev, size, dma_handle, &memory))
> > return memory;
> >
> > if (!dev) {
> > dev = &x86_dma_fallback_dev;
> > gfp |= GFP_DMA;
> > }
> >
> > we play with it here though (not happens with pci devices),
> >
> > if (!dev->dma_mask)
> > return NULL;
> >
> > if (!ops->alloc_coherent)
> > return NULL;
> >
> > Then dma_alloc_coherent_gfp_flags() sets it again according to
> > device->coherent_dma_mask and gfp before ops->alloc_coherent hook:
> >
> > return ops->alloc_coherent(dev, size, dma_handle,
> > dma_alloc_coherent_gfp_flags(dev, gfp));
> >
> >
> > This code can set up the exact same gfp flag for swiotbl and nommu as
> > before.
>
> So its possible that alloc_coherent is called with region specifiers in
> the gfp flags.

Yes, that's how we solved the ZONE_DMA exhaustion problem in
swiotlb. And we don't duplicate the gfp hack in both swiotlb and
pci-nommu.

> Can't we simply make the gfp hacks depend on
> dma_ops->is_phys and avoid further gfp hacks in the hardware iommu
> implementations?

I thought about it but adding a new dma_ops->we_don't_want_gfp_flag
hook doesn't make the code simpler much. Currently, we have the gfp
setting hack in just one place. It's not bad. Adding such new hook
means adding more lines than we can remove.

Yeah, I was against your patch to adding the gfp setting hack to
swiotlb but it's because gfp is kinda architecture specific stuff and
swiotlb should not. It's the bad design IMO. It's ok for me that
architecture specific IOMMUs can do the architecture specific stuff
(and it's about just clearing the gfp flag).

Intel IOMMU already clears up the flag so I'll send a patch to do that
for Calgary shortly then I think that we can finish x86 alloc_coherent
rewrite.