b1a000d3b8ec ("block: relax direct io memory alignment")

Regression Ming Lei 4 participants 12 messages Oct 15, 2024 – Oct 23, 2024 Block layer

Share & tools

Ming Lei

author

2024-10-15 16:40 UTC #0 lore raw

Hello Guys,

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.

Thanks,
Ming

#define _GNU_SOURCE
#include <stdio.h>
#include <fcntl.h>
#include <string.h>
#include <stdlib.h>
#include <libaio.h>
#include <errno.h>
#include <unistd.h>

int main(int argc, char *argv[])

{
	const char *outputfile=argv[1];
	io_context_t ctx;
	int output_fd;
	const int nr = 4;
	struct iocb _io[nr];
	struct iocb *io[16] = {
		&_io[0],
		&_io[1],
		&_io[2],
		&_io[3],
	};
	struct io_event e[nr];
	struct timespec timeout;
	int ret;
	char *content;
	unsigned size = 2*1024 * 1024;

	posix_memalign((void **)&content, 4096, nr * size + 512);

	memset(&ctx,0,sizeof(ctx));
	if(io_setup(10, &ctx) != 0) {
		printf("io_setup error\n");
		return -1;
	}

	if((output_fd = open(outputfile, O_DIRECT, 0644)) < 0) {
		perror("open error");
		io_destroy(ctx);
		return -1;
	}
	io_prep_pread(io[0], output_fd, content + 4, size, 0);
	io_prep_pread(io[1], output_fd, content + size, size, size * 2);
	io_prep_pread(io[2], output_fd, content + size * 2, size, size * 4);
	io_prep_pread(io[3], output_fd, content + size * 3, size, size * 8);

	ret = io_submit(ctx, nr, io);
	if(ret != nr) {
		io_destroy(ctx);
		printf("io_submit error %d\n", ret);
		return -1;
	}

	while(1) {
		timeout.tv_sec=0;
	        timeout.tv_nsec=500000000;
	        if (io_getevents(ctx, nr, nr, e, &timeout) == nr) {
	            close(output_fd);
	            break;
	        }
	        printf("haven't done\n");
	        sleep(1);
	}
	io_destroy(ctx);
	return 0;
}

Christoph Hellwig

2024-10-16 08:04 UTC #1 lore raw

Quoted 9 lines · Ming Lei

Hello Guys,
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.

Ming Lei

author

2024-10-16 08:32 UTC #2 lore raw

Quoted 7 lines · Christoph Hellwig

Quoted 6 lines · Ming Lei
Hello Guys,
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".

Quoted 6 lines

Quoted 3 lines
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.

Yes, something like the following change:

block/blk-settings.c +3 -1

index a446654ddee5..26bd0e72c68e 100644
@@ -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;
 
 	if (lim->alignment_offset) {

Thanks,
Ming

Christoph Hellwig

2024-10-16 12:31 UTC #3 lore raw

Quoted 8 lines · Ming Lei

Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Ming Lei
Hello Guys,
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".

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
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.
Yes, something like the following change:

We only really need this if the architecture support cache incoherent DMA. Maybe even as a runtime setting.

Ming Lei

author

2024-10-22 01:21 UTC #4 lore raw

Quoted 13 lines · Christoph Hellwig

Quoted 12 lines · Ming Lei
Quoted 11 lines · Christoph Hellwig
Quoted 9 lines · Ming Lei
Hello Guys,
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.

Quoted 7 lines

Quoted 4 lines
Quoted 2 lines
We should not allow smaller than cache line alignment on architectures that are not cache coherent indeed.
Yes, something like the following change:
We only really need this if the architecture support cache incoherent DMA. Maybe even as a runtime setting.

Can you take coherent DMA into account on kernel/dma/debug.c first? Otherwise the warning still may be triggered on coherent DMA.

thanks,
Ming

Christoph Hellwig

2024-10-22 07:25 UTC #5 lore raw

Quoted 1 line · Ming Lei

Can you take coherent DMA into account on kernel/dma/debug.c first?

We intentionally don't to force people to write portable code.

Quoted 1 line

Otherwise the warning still may be triggered on coherent DMA.

Well, it is a valid warning for the above reason.

Jens Axboe

maintainer

2024-10-22 02:15 UTC #6 lore raw

In reply to Ming Lei

Quoted 22 lines · Ming Lei

Quoted 7 lines · Christoph Hellwig
Quoted 6 lines · Ming Lei
Hello Guys,
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".
Quoted 6 lines
Quoted 3 lines
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.
Yes, something like the following change:
block/blk-settings.c +0 -0
index a446654ddee5..26bd0e72c68e 100644

block/blk-settings.c static int blk_validate_limits(struct queue_limits *lim) · line 356

Quoted 10 lines

block/blk-settings.c +3 -1

@@ -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;
 
 	if (lim->alignment_offset) {

This will break existing applications, running on an architecture that are cache coherent.

Signature · 1 line

Jens Axboe

Catalin Marinas

2024-10-22 10:24 UTC #7 lore raw

In reply to Ming Lei

Quoted 11 lines · Ming Lei

Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · Ming Lei
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.

Quoted 7 lines

Yes, something like the following change:
block/blk-settings.c +0 -0
index a446654ddee5..26bd0e72c68e 100644

block/blk-settings.c static int blk_validate_limits(struct queue_limits *lim) · line 354

Quoted 8 lines

block/blk-settings.c +3 -1

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

Signature · 1 line

Catalin

Ming Lei

author

2024-10-23 00:50 UTC #8 lore raw

Quoted 10 lines · Catalin Marinas

Quoted 9 lines · Ming Lei
Quoted 8 lines · Christoph Hellwig
Quoted 6 lines · Ming Lei
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.

Quoted 6 lines

Quoted 2 lines
Quoted 2 lines
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.

Yeah, looks the issue has to be fixed from driver side, only driver has 'struct device' info.

Quoted 35 lines

Quoted 8 lines
Yes, something like the following change:
block/blk-settings.c +0 -0
index a446654ddee5..26bd0e72c68e 100644
block/blk-settings.c static int blk_validate_limits(struct queue_limits *lim) · line 354
Quoted 8 lines
block/blk-settings.c +3 -1
@@ -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.

Quoted 5 lines

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

I did see report on warning of "cacheline tracking EEXIST, overlapping mappings aren't supported" on USB several times, since it is often treated as same with this one.

Quoted 6 lines

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.

Or dma debug log needs to be improved by showing the warning is just a hint.

Thanks,
Ming

Christoph Hellwig

2024-10-23 06:12 UTC #9 lore raw

In reply to Catalin Marinas

Quoted 6 lines · Catalin Marinas

Quoted 2 lines
Quoted 2 lines
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.

Well, devices set the queue limits. So this would be a fix in the drivers that set the queue limits. SCSI already does this in the midlayer code, so the main places to fix are nvme und ublk.

I cant take care of nvme by copying the scsi pattern.

Quoted 12 lines

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.

Quoted 5 lines

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

I don't think there's too many of these indeed.

Quoted 5 lines

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.

Sounds reasonable.

Ming Lei

author

2024-10-23 08:14 UTC #10 lore raw

Quoted 11 lines · Christoph Hellwig

Quoted 6 lines · Catalin Marinas
Quoted 2 lines
Quoted 2 lines
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.
Well, devices set the queue limits. So this would be a fix in the drivers that set the queue limits. SCSI already does this in the midlayer code,

I guess it isn't true:

[linux]# cat /sys/block/sda/queue/dma_alignment
3

Quoted 16 lines

so the main places to fix are nvme und ublk.
I cant take care of nvme by copying the scsi pattern.
Quoted 12 lines
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.

Quoted 7 lines

Quoted 5 lines
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).
I don't think there's too many of these indeed.

Usually it is assumed that it is safe to DMA over kmalloc() buffer...

thanks,
Ming

Christoph Hellwig

2024-10-23 12:23 UTC #11 lore raw

Quoted 9 lines · Ming Lei

Quoted 8 lines · Christoph Hellwig
Quoted 6 lines · Catalin Marinas
Quoted 2 lines
Quoted 2 lines
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.

Quoted 7 lines

Quoted 3 lines
Well, devices set the queue limits. So this would be a fix in the drivers that set the queue limits. SCSI already does this in the midlayer code,
I guess it isn't true:
[linux]# cat /sys/block/sda/queue/dma_alignment

Is that a SCSI HBA that is on a not DMA coherent bus? If not that is expected.