arm64: mm: fix zone_dma_limit calculation

The commit ba0fb44aed47 ("dma-mapping: replace zone_dma_bits by zone_dma_limit") changed how zone_dma_limit was calculated. Now it returns the memsize limit in IORT or device tree instead of U32_MAX if the memsize limit is greater than U32_MAX.

This resulted in DMA allocations may use GFP_DMA even though the devices don't require it. It caused regression on our two sockets systems due to excessive remote memory access.

Fixes ba0fb44aed47 ("dma-mapping: replace zone_dma_bits by zone_dma_limit")

Cc <[email protected]> [6.12+]

Reported-by Yutang Jiang <[email protected]>

Tested-by Yutang Jiang <[email protected]>

Signed-off-by Yang Shi <[email protected]>

---
 arch/arm64/mm/init.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

arch/arm64/mm/init.c +8 -9

index d21f67d67cf5..ccdef53872a0 100644
@@ -117,15 +117,6 @@ static void __init arch_reserve_crashkernel(void)
 
 static phys_addr_t __init max_zone_phys(phys_addr_t zone_limit)
 {
-	/**
-	 * Information we get from firmware (e.g. DT dma-ranges) describe DMA
-	 * bus constraints. Devices using DMA might have their own limitations.
-	 * Some of them rely on DMA zone in low 32-bit memory. Keep low RAM
-	 * DMA zone on platforms that have RAM there.
-	 */
-	if (memblock_start_of_DRAM() < U32_MAX)
-		zone_limit = min(zone_limit, U32_MAX);
-
 	return min(zone_limit, memblock_end_of_DRAM() - 1) + 1;
 }
 

@@ -141,6 +132,14 @@ static void __init zone_sizes_init(void)
 	acpi_zone_dma_limit = acpi_iort_dma_get_max_cpu_address();
 	dt_zone_dma_limit = of_dma_get_max_cpu_address(NULL);
 	zone_dma_limit = min(dt_zone_dma_limit, acpi_zone_dma_limit);
+	/*
+	 * Information we get from firmware (e.g. DT dma-ranges) describe DMA
+	 * bus constraints. Devices using DMA might have their own limitations.
+	 * Some of them rely on DMA zone in low 32-bit memory. Keep low RAM
+	 * DMA zone on platforms that have RAM there.
+	 */
+	if (memblock_start_of_DRAM() < U32_MAX)
+		zone_dma_limit = min(zone_dma_limit, U32_MAX);
 	arm64_dma_phys_limit = max_zone_phys(zone_dma_limit);
 	max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
 #endif

Hi Yang,

Quoted 4 lines · Yang Shi

The commit ba0fb44aed47 ("dma-mapping: replace zone_dma_bits by zone_dma_limit") changed how zone_dma_limit was calculated. Now it returns the memsize limit in IORT or device tree instead of U32_MAX if the memsize limit is greater than U32_MAX.

Can you give a concrete example of memory layout and dma-ranges that demonstrates this issue?

Quoted 4 lines

This resulted in DMA allocations may use GFP_DMA even though the devices don't require it. It caused regression on our two sockets systems due to excessive remote memory access.

That is, DMA zone used to cover all memory before commit ba0fb44aed47, but now DMA zone is limited to the smallest dma-ranges. Is that correct?

Thanks,
baruch

Quoted 44 lines

Fixes ba0fb44aed47 ("dma-mapping: replace zone_dma_bits by zone_dma_limit")

Cc <[email protected]> [6.12+]

Reported-by Yutang Jiang <[email protected]>

Tested-by Yutang Jiang <[email protected]>

Signed-off-by Yang Shi <[email protected]>

---
  arch/arm64/mm/init.c | 17 ++++++++---------
  1 file changed, 8 insertions(+), 9 deletions(-)

arch/arm64/mm/init.c +8 -9

index d21f67d67cf5..ccdef53872a0 100644
@@ -117,15 +117,6 @@ static void __init arch_reserve_crashkernel(void)
 
  static phys_addr_t __init max_zone_phys(phys_addr_t zone_limit)
  {
-	/**
-	 * Information we get from firmware (e.g. DT dma-ranges) describe DMA
-	 * bus constraints. Devices using DMA might have their own limitations.
-	 * Some of them rely on DMA zone in low 32-bit memory. Keep low RAM
-	 * DMA zone on platforms that have RAM there.
-	 */
-	if (memblock_start_of_DRAM() < U32_MAX)
-		zone_limit = min(zone_limit, U32_MAX);
-
  	return min(zone_limit, memblock_end_of_DRAM() - 1) + 1;
  }
 

@@ -141,6 +132,14 @@ static void __init zone_sizes_init(void)
  	acpi_zone_dma_limit = acpi_iort_dma_get_max_cpu_address();
  	dt_zone_dma_limit = of_dma_get_max_cpu_address(NULL);
  	zone_dma_limit = min(dt_zone_dma_limit, acpi_zone_dma_limit);
+	/*
+	 * Information we get from firmware (e.g. DT dma-ranges) describe DMA
+	 * bus constraints. Devices using DMA might have their own limitations.
+	 * Some of them rely on DMA zone in low 32-bit memory. Keep low RAM
+	 * DMA zone on platforms that have RAM there.
+	 */
+	if (memblock_start_of_DRAM() < U32_MAX)
+		zone_dma_limit = min(zone_dma_limit, U32_MAX);
  	arm64_dma_phys_limit = max_zone_phys(zone_dma_limit);
  	max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
  #endif

Quoted 8 lines · Baruch Siach

Quoted 4 lines · Yang Shi

The commit ba0fb44aed47 ("dma-mapping: replace zone_dma_bits by zone_dma_limit") changed how zone_dma_limit was calculated. Now it returns the memsize limit in IORT or device tree instead of U32_MAX if the memsize limit is greater than U32_MAX.

Can you give a concrete example of memory layout and dma-ranges that demonstrates this issue?

Our 2 sockets system has physical memory starts at 0x0 on node 0 and 0x200000000000 on node 1. The memory size limit defined in IORT is 0x30 (48 bits).

The DMA zone is:

pages free     887722
        boost    0
        min      229
        low      1108
        high     1987
        promo    2866
        spanned  983040
        present  982034
        managed  903238
        cma      16384
        protection: (0, 0, 124824, 0, 0)
 start_pfn:           65536

When allocating DMA buffer, dma_direct_optimal_gfp_mask() is called to determine the proper zone constraints. If the phys_limit is less than zone_dma_limit, it will use GFP_DMA. But zone_dma_limit is 0xffffffffffff on v6.12 instead of 4G prior v6.12, it means all DMA buffer allocation will go to DMA zone even though the devices don't require it.

DMA zone is on node 0, so we saw excessive remote access on 2 sockets system.

The physical addr range for DMA zone is correct, the problem is wrong zone_dma_limit. Before commit ba0fb44aed47 zone_dma_limit was 4G, after it it is the whole memory even though DMA zone just covers low 4G.

arch/arm64/mm/init.c +0 -1

index d21f67d67cf5..ccdef53872a0 100644
@@ -117,15 +117,6 @@ static void __init arch_reserve_crashkernel(void)
  static phys_addr_t __init max_zone_phys(phys_addr_t zone_limit)
  {
-	/**

arch/arm64/mm/init.c +1 -0

@@ -141,6 +132,14 @@ static void __init zone_sizes_init(void)
  	acpi_zone_dma_limit = acpi_iort_dma_get_max_cpu_address();
  	dt_zone_dma_limit = of_dma_get_max_cpu_address(NULL);
  	zone_dma_limit = min(dt_zone_dma_limit, acpi_zone_dma_limit);
+	/*

+ Robin

Quoted 37 lines · Yang Shi

Quoted 8 lines · Baruch Siach

Quoted 4 lines · Yang Shi

The commit ba0fb44aed47 ("dma-mapping: replace zone_dma_bits by zone_dma_limit") changed how zone_dma_limit was calculated. Now it returns the memsize limit in IORT or device tree instead of U32_MAX if the memsize limit is greater than U32_MAX.

Can you give a concrete example of memory layout and dma-ranges that demonstrates this issue?

Our 2 sockets system has physical memory starts at 0x0 on node 0 and 0x200000000000 on node 1. The memory size limit defined in IORT is 0x30 (48 bits).

The DMA zone is:

pages free     887722
        boost    0
        min      229
        low      1108
        high     1987
        promo    2866
        spanned  983040
        present  982034
        managed  903238
        cma      16384
        protection: (0, 0, 124824, 0, 0)
 start_pfn:           65536

When allocating DMA buffer, dma_direct_optimal_gfp_mask() is called to determine the proper zone constraints. If the phys_limit is less than zone_dma_limit, it will use GFP_DMA. But zone_dma_limit is 0xffffffffffff on v6.12 instead of 4G prior v6.12, it means all DMA buffer allocation will go to DMA zone even though the devices don't require it.

DMA zone is on node 0, so we saw excessive remote access on 2 sockets system.

[...]

Quoted 3 lines

The physical addr range for DMA zone is correct, the problem is wrong zone_dma_limit. Before commit ba0fb44aed47 zone_dma_limit was 4G, after it it is the whole memory even though DMA zone just covers low 4G.

Thanks for the details. I agree that zone_dma_limit shouldn't be higher than the ZONE_DMA upper boundary, otherwise it gets confusing for functions like dma_direct_optimal_gfp_mask() and we may force allocations to a specific range unnecessarily.

If IORT or DT indicate a large mask covering the whole RAM (i.e. no restrictions), in an ideal world, we should normally extend ZONE_DMA to the same.

One problem is ZONE_DMA32 (and GFP_DMA32) and the fact that ZONE_DMA sits below it. Until we hear otherwise, we assume a DMA offset of 0 for such 32-bit devices and therefore define ZONE_DMA32 in the lower 4GB if RAM starts below this limit (and an empty ZONE_DMA32 if RAM starts above).

Another aspect to consider is that we don't always have DT or IORT information or some devices need a smaller limit than what's advertised in the firmware tables (typically 32-bit masks). This code went through a couple of fixes already:

833bd284a454 ("arm64: mm: fix DMA zone when dma-ranges is missing") 122c234ef4e1 ("arm64: mm: keep low RAM dma zone")

Since our current assumption is to assume ZONE_DMA within 32-bit if RAM below 4GB, I'm happy to make this conditional on CONFIG_ZONE_DMA32 also being enabled. So, from your patch below:

Quoted 8 lines

arch/arm64/mm/init.c +0 -1

index d21f67d67cf5..ccdef53872a0 100644
@@ -117,15 +117,6 @@ static void __init arch_reserve_crashkernel(void)
   static phys_addr_t __init max_zone_phys(phys_addr_t zone_limit)
   {
-	/**

arch/arm64/mm/init.c static void __init arch_reserve_crashkernel(void) · line 120

Quoted 10 lines

arch/arm64/mm/init.c +0 -8

-	 * Information we get from firmware (e.g. DT dma-ranges) describe DMA
-	 * bus constraints. Devices using DMA might have their own limitations.
-	 * Some of them rely on DMA zone in low 32-bit memory. Keep low RAM
-	 * DMA zone on platforms that have RAM there.
-	 */
-	if (memblock_start_of_DRAM() < U32_MAX)
-		zone_limit = min(zone_limit, U32_MAX);
-
   	return min(zone_limit, memblock_end_of_DRAM() - 1) + 1;
   }

This part is fine.

Quoted 5 lines

arch/arm64/mm/init.c +1 -0

@@ -141,6 +132,14 @@ static void __init zone_sizes_init(void)
   	acpi_zone_dma_limit = acpi_iort_dma_get_max_cpu_address();
   	dt_zone_dma_limit = of_dma_get_max_cpu_address(NULL);
   	zone_dma_limit = min(dt_zone_dma_limit, acpi_zone_dma_limit);
+	/*

arch/arm64/mm/init.c static void __init zone_sizes_init(void) · line 145

Quoted 10 lines

arch/arm64/mm/init.c +7 -0

+	 * Information we get from firmware (e.g. DT dma-ranges) describe DMA
+	 * bus constraints. Devices using DMA might have their own limitations.
+	 * Some of them rely on DMA zone in low 32-bit memory. Keep low RAM
+	 * DMA zone on platforms that have RAM there.
+	 */
+	if (memblock_start_of_DRAM() < U32_MAX)
+		zone_dma_limit = min(zone_dma_limit, U32_MAX);
   	arm64_dma_phys_limit = max_zone_phys(zone_dma_limit);
   	max_zone_pfns[ZONE_DMA] = PFN_DOWN(arm64_dma_phys_limit);
   #endif

But I'd move the zone_dma_limit update further down in the CONFIG_ZONE_DMA32 block. I think we only need to limit it to dma32_phys_limit and ignore the U32_MAX check. The former is already capped to 32-bit. For the second hunk, something like below (untested):

arch/arm64/mm/init.c +3 -1

index d21f67d67cf5..ffaf5bd8d0a1 100644
@@ -146,8 +146,10 @@ static void __init zone_sizes_init(void)
  #endif
  #ifdef CONFIG_ZONE_DMA32
  	max_zone_pfns[ZONE_DMA32] = PFN_DOWN(dma32_phys_limit);
-	if (!arm64_dma_phys_limit)
+	if (!arm64_dma_phys_limit || arm64_dma_phys_limit > dma32_phys_limit) {
  		arm64_dma_phys_limit = dma32_phys_limit;
+		zone_dma_limit = arm64_dma_phys_limit - 1;
+	}
  #endif
  	if (!arm64_dma_phys_limit)
  		arm64_dma_phys_limit = PHYS_MASK + 1;

With some comment on why we do this but most likely not the current comment in max_zone_phys() (more like keep ZONE_DMA below ZONE_DMA32).

Quoted 3 lines · Catalin Marinas

If IORT or DT indicate a large mask covering the whole RAM (i.e. no restrictions), in an ideal world, we should normally extend ZONE_DMA to the same.

That's not right, ZONE_DMA should still be relatively limited in size (unless we really do only have a tiny amount of RAM) - just because a DT dma-ranges property says the system interconnect can carry >32 address bits in general doesn't mean that individual device DMA masks can't still be 32-bit or smaller. IIRC we're still implicitly assuming that if DT does describe an offset range into "high" RAM, it must represent a suitable lowest common denominator for all relevant devices already, and therefore we can get away with sizing ZONE_DMA off it blindly.

After staring at it for long enough, I think $SUBJECT patch is actually correct as it is.

In fact I'm now wondering why the fix was put inside max_zone_phys() in the first place, since it's clearly pointless to clamp DMA_BIT_MASK(32) to U32_MAX in the dma32_phys_limit case...

However the commit message is perhaps not as clear as it could be - technically we are correctly *calculating* the appropriate effective zone_dma_limt value within the scope of zone_sizes_init(), we're just failing to properly update the actual zone_dma_limit variable for the benefit of other users.

Quoted 4 lines · Robin Murphy

Quoted 3 lines · Catalin Marinas

If IORT or DT indicate a large mask covering the whole RAM (i.e. no restrictions), in an ideal world, we should normally extend ZONE_DMA to the same.

Quoted 8 lines

That's not right, ZONE_DMA should still be relatively limited in size (unless we really do only have a tiny amount of RAM) - just because a DT dma-ranges property says the system interconnect can carry >32 address bits in general doesn't mean that individual device DMA masks can't still be 32-bit or smaller. IIRC we're still implicitly assuming that if DT does describe an offset range into "high" RAM, it must represent a suitable lowest common denominator for all relevant devices already, and therefore we can get away with sizing ZONE_DMA off it blindly.

Fine by me to keep ZONE_DMA in the low range always. I was thinking of only doing this if ZONE_DMA32 is enabled.

Quoted 2 lines

After staring at it for long enough, I think $SUBJECT patch is actually correct as it is.

Thanks Robin for having a look. Can I add your reviewed-by?

Quoted 3 lines

In fact I'm now wondering why the fix was put inside max_zone_phys() in the first place, since it's clearly pointless to clamp DMA_BIT_MASK(32) to U32_MAX in the dma32_phys_limit case...

I came to the same conclusion. I think it might have been some left-over from when we had a ZONE_DMA32 in the above 4GB (AMD Seattle?). Than we changed it a few times but only focused on this function for setting the limits.

Quoted 5 lines

However the commit message is perhaps not as clear as it could be - technically we are correctly *calculating* the appropriate effective zone_dma_limt value within the scope of zone_sizes_init(), we're just failing to properly update the actual zone_dma_limit variable for the benefit of other users.

I'll have a look next week at rewriting the commit message, unless Yang does it first. I'm planning to queue this patch for -rc2.

The commit ba0fb44aed47 ("dma-mapping: replace zone_dma_bits by zone_dma_limit") changed how zone_dma_limit was calculated. Now it returns the memsize limit in IORT or device tree instead of U32_MAX if the memsize limit is greater than U32_MAX.

This resulted in DMA allocations may use GFP_DMA even though the devices don't require it. It caused regression on our two sockets systems due to excessive remote memory access.

[...]