Re: [Regression] b1a000d3b8ec ("block: relax direct io memory alignment")

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

On Wed, Oct 16, 2024 at 04:31:45PM +0800, Ming Lei wrote:
> On Wed, Oct 16, 2024 at 10:04:19AM +0200, Christoph Hellwig wrote:
> > On Wed, Oct 16, 2024 at 12:40:13AM +0800, Ming Lei wrote:
> > > Turns out host controller's DMA alignment is often too relax, so two DMA
> > > buffers may cross same cache line easily, and trigger the warning of
> > > "cacheline tracking EEXIST, overlapping mappings aren't supported".
> > > 
> > > The attached test code can trigger the warning immediately with CONFIG_DMA_API_DEBUG
> > > enabled when reading from one scsi disk which queue DMA alignment is 3.
> > 
> > We should not allow smaller than cache line alignment on architectures
> > that are not cache coherent indeed.

Even on architectures that are not fully coherent, the coherency is a
property of the device. You may need to somehow pass this information in
struct queue_limits if you want it to be optimal.

> Yes, something like the following change:
> 
> diff --git a/block/blk-settings.c b/block/blk-settings.c
> index a446654ddee5..26bd0e72c68e 100644
> --- a/block/blk-settings.c
> +++ b/block/blk-settings.c
> @@ -348,7 +348,9 @@ static int blk_validate_limits(struct queue_limits *lim)
>  	 */
>  	if (!lim->dma_alignment)
>  		lim->dma_alignment = SECTOR_SIZE - 1;
> -	if (WARN_ON_ONCE(lim->dma_alignment > PAGE_SIZE))
> +	else if (lim->dma_alignment < L1_CACHE_BYTES - 1)
> +		lim->dma_alignment = L1_CACHE_BYTES - 1;
> +	else if (WARN_ON_ONCE(lim->dma_alignment > PAGE_SIZE))
>  		return -EINVAL;

L1_CACHE_BYTES is not the right check here since a level 2/3 cache may
have a larger cache line than level 1 (and we have such configurations
on arm64 where ARCH_DMA_MINALIGN is 128 and L1_CACHE_BYTES is 64). Use
dma_get_cache_alignment() instead. On fully coherent architectures like
x86 it should return 1.

That said, the DMA debug code also uses the static L1_CACHE_SHIFT and it
will trigger the warning anyway. Some discussion around the DMA API
debug came up during the small ARCH_KMALLOC_MINALIGN changes (don't
remember it was in private with Robin or on the list). Now kmalloc() can
return a small buffer (less than a cache line) that won't be bounced if
the device is coherent (see dma_kmalloc_safe()) but the DMA API debug
code only checks for direction == DMA_TO_DEVICE, not
dev_is_dma_coherent(). For arm64 I did not want to disable small
ARCH_KMALLOC_MINALIGN if CONFIG_DMA_API_DEBUG is enabled as this would
skew the testing by forcing all allocations to be ARCH_DMA_MINALIGN
aligned.

Maybe I'm missing something in those checks but I'm surprised that the
DMA API debug code doesn't complain about small kmalloc() buffers on x86
(which never had any bouncing for this specific case since it's fully
coherent). I suspect people just don't enable DMA debugging on x86 for
such devices (typically USB drivers have this issue).

So maybe the DMA API debug should have two modes: a generic one that
catches alignments irrespective of the coherency of the device and
another that's specific to the device/architecture coherency properties.
The former, if enabled, should also force a higher minimum kmalloc()
alignment and a dma_get_cache_alignment() > 1.

-- 
Catalin