Re: [PATCH v2 2/2] treewide: Add the __GFP_PACKED flag to several non-DMA kmalloc() allocations

[Catalin Marinas <catalin.marinas@arm.com>]

On Tue, Nov 01, 2022 at 11:59:19AM +0100, Christoph Hellwig wrote:
> On Sun, Oct 30, 2022 at 04:43:21PM +0000, Catalin Marinas wrote:
> > The alternative is for dma_map_*() to (dynamically) verify at the point
> > of DMA setup rather than at the point of allocation (and bounce).
> 
> One issue with dma_map_* is that is can't report errors very well.
> So I'd really prefer to avoid adding that kind of checking there.

By checking I meant bouncing on condition fail rather than reporting
errors.

> > We have a few options here:
> > 
> > a) Require that the dma_map_*() functions (including sg) get a size
> >    multiple of cache_line_size() or the static ARCH_DMA_MINALIGN.
> 
> I don't think that is going to work, there are just too many instances
> all over the tree that pass smaller sizes.
> 
> > b) Always bounce small sizes as they may have come from a small kmalloc
> >    cache. The alignment of both ptr and length is ignored. This assumes
> >    that in-struct alignments (crypto) use ARCH_DMA_MINALIGN and we can
> >    reduce ARCH_KMALLOC_MINALIGN independently.
> 
> I think the start must be very much aligned.  So what is the problem
> with just checking the size?
> 
> Otherwise I think this is the way to go.  These tiny maps tend to be
> various aux setup path thing and not performance critical (and caring
> about performance for not DMA coherent devices isn't something we do
> all that much anyway).

The check is a bit weird as it needs some awareness of the kmalloc
caches to avoid unnecessary bouncing. We can't just check for smaller
than 128 since 192 is another kmalloc cache that is not aligned to an
ARCH_DMA_MINALIGN of 128:

https://git.kernel.org/arm64/c/3508d0d4d5e458e43916bf4f61b29d2a6f15c2bb

I can probably make it a bit nicer by checking the alignment of
kmalloc_size_roundup(size) (this function doesn't seem to have any
callers).

The main downside of bouncing is mobile phones where those vendors are
in the habit of passing noswiotlb on the kernel command line or they
want a very small bounce buffer (hard to pick a universally small size).
I guess we can go ahead with this and, depending on how bad the problem
is, we can look at optimising swiotlb to use a kmem_cache (or aligned
kmalloc) as fall-back for bouncing.

> > (b) is what I attempted on one of my branches (until I got stuck on
> > bouncing for iommu). A slight overhead on dma_map_*() to check the
> > length and we may keep a check on the start alignment (not length
> > though).
> 
> When was that?  These days dma-iommu already has bouncing code for
> the untrusted device case, so handling the bouncing there for unaligned
> request on non-coherent devices shouldn't be all that horrible.

The bouncing currently is all or nothing with iommu_dma_map_sg(), unlike
dma_direct_map_sg() which ends up calling dma_direct_map_page() and we
can do the bouncing per element. So I was looking to untangle
iommu_dma_map_sg() in a similar way but postponed it as too complicated.

As a less than optimal solution, we can force bouncing for the whole
list if any of the sg elements is below the alignment size. Hopefully we
won't have many such mixed size cases.

-- 
Catalin