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

[Christoph Hellwig <hch@lst.de>]

On Sun, Oct 30, 2022 at 04:43:21PM +0000, Catalin Marinas wrote:
> I've been trying to come up with what the semantics of __dma should be
> but couldn't get to any clear conclusion. We can avoid the noderef part
> as it doesn't make sense but it still implies a different address space.
> Once I made the ptr in dma_map_single() a 'void __dma *', sparse
> complained not just about the callers of this function but the callees
> like is_vmalloc_addr(). So I have a suspicion we'd end up with __dma
> annotations in lots of places unrelated to DMA or drivers. Then we have
> structures like 'catc' with a 'ctrl_dr' that ends up in the DMA API (so
> far as TO_DEVICE, so not that problematic).

Yes, it's all a bit of a mess.

> 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.

> That's definitely less daunting if we find the right solution. The
> problem with checking the alignment of both start and length is that
> there are valid cases where the start is aligned but the length isn't.

Yes.
> They don't need bouncing since they come from a sufficiently aligned
> slab. 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).

> (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.
That leaves the non-dma_iommu non-coherent cases.  Arm should really
switch to dma-iommu, and Robin has been doing work on that which
has been going on for a while.  MIPS/jazz are ISA based boards from
the early 90s and have like 3 drivers that work on them and should
not be affected.  sparc/sun4u only does the to_cpu syncs and IIRC
those are just an optimization and not required for correctness.