[PATCH] arm64: mm: fix zone_dma_limit calculation

[PATCH] arm64: mm: fix zone_dma_limit calculation

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

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: <stable@vger.kernel.org>    [6.12+]
Reported-by: Yutang Jiang <jiangyutang@os.amperecomputing.com>
Tested-by: Yutang Jiang <jiangyutang@os.amperecomputing.com>
Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
---
 arch/arm64/mm/init.c | 17 ++++++++---------
 1 file changed, 8 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index d21f67d67cf5..ccdef53872a0 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -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
-- 
2.47.0

Re: [PATCH] arm64: mm: fix zone_dma_limit calculation

[Baruch Siach]

Hi Yang,

On Mon, Nov 25 2024, Yang Shi wrote:
> 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?

> 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

> Fixes: ba0fb44aed47 ("dma-mapping: replace zone_dma_bits by zone_dma_limit")
> Cc: <stable@vger.kernel.org>    [6.12+]
> Reported-by: Yutang Jiang <jiangyutang@os.amperecomputing.com>
> Tested-by: Yutang Jiang <jiangyutang@os.amperecomputing.com>
> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
> ---
>  arch/arm64/mm/init.c | 17 ++++++++---------
>  1 file changed, 8 insertions(+), 9 deletions(-)
>
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index d21f67d67cf5..ccdef53872a0 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -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

-- 
                                                     ~. .~   Tk Open Systems
=}------------------------------------------------ooO--U--Ooo------------{=
   - baruch@tkos.co.il - tel: +972.52.368.4656, http://www.tkos.co.il -

Re: [PATCH] arm64: mm: fix zone_dma_limit calculation

[Yang Shi]

On 11/25/24 10:27 PM, Baruch Siach wrote:
> Hi Yang,
>
> On Mon, Nov 25 2024, Yang Shi wrote:
>> 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.

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

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, Yang
>
> Thanks,
> baruch
>
>> Fixes: ba0fb44aed47 ("dma-mapping: replace zone_dma_bits by zone_dma_limit")
>> Cc: <stable@vger.kernel.org>    [6.12+]
>> Reported-by: Yutang Jiang <jiangyutang@os.amperecomputing.com>
>> Tested-by: Yutang Jiang <jiangyutang@os.amperecomputing.com>
>> Signed-off-by: Yang Shi <yang@os.amperecomputing.com>
>> ---
>>   arch/arm64/mm/init.c | 17 ++++++++---------
>>   1 file changed, 8 insertions(+), 9 deletions(-)
>>
>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>> index d21f67d67cf5..ccdef53872a0 100644
>> --- a/arch/arm64/mm/init.c
>> +++ b/arch/arm64/mm/init.c
>> @@ -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

Re: [PATCH] arm64: mm: fix zone_dma_limit calculation

[Catalin Marinas]

+ Robin

On Tue, Nov 26, 2024 at 09:38:22AM -0800, Yang Shi wrote:
> On 11/25/24 10:27 PM, Baruch Siach wrote:
> > On Mon, Nov 25 2024, Yang Shi wrote:
> > > 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.

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:

> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index d21f67d67cf5..ccdef53872a0 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -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;
>   }

This part is fine.

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

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

diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
index d21f67d67cf5..ffaf5bd8d0a1 100644
--- a/arch/arm64/mm/init.c
+++ b/arch/arm64/mm/init.c
@@ -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).

-- 
Catalin

Re: [PATCH] arm64: mm: fix zone_dma_limit calculation

[Robin Murphy]

On 2024-11-27 5:49 pm, Catalin Marinas wrote:
> + Robin
> 
> On Tue, Nov 26, 2024 at 09:38:22AM -0800, Yang Shi wrote:
>> On 11/25/24 10:27 PM, Baruch Siach wrote:
>>> On Mon, Nov 25 2024, Yang Shi wrote:
>>>> 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.
> 
> 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.

Oof, indeed the original min3() logic did also result in the 
32-bit-clamped value being written back to zone_dma_bits itself, so 
that's another subtlety overlooked in ba0fb44aed47 *and* the subsequent 
fixes reinstating that clamping (conditionally) within max_zone_phys().
  > 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. I'm yet 
to hear of any platform having a general offset with no significant 
upper limit, but if we did want to support such a thing then we would 
need to revisit the previously-discussed notion of an accurate 
of_dma_get_min_cpu_address() rather than the assumption based on 
memblock_start_of_DRAM().

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.

Thanks,
Robin.

> 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:
> 
>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
>> index d21f67d67cf5..ccdef53872a0 100644
>> --- a/arch/arm64/mm/init.c
>> +++ b/arch/arm64/mm/init.c
>> @@ -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;
>>    }
> 
> This part is fine.
> 
>> @@ -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
> 
> 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):
> 
> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c
> index d21f67d67cf5..ffaf5bd8d0a1 100644
> --- a/arch/arm64/mm/init.c
> +++ b/arch/arm64/mm/init.c
> @@ -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).
> 

Re: [PATCH] arm64: mm: fix zone_dma_limit calculation

[Catalin Marinas]

On Fri, Nov 29, 2024 at 06:06:50PM +0000, Robin Murphy wrote:
> On 2024-11-27 5:49 pm, Catalin Marinas wrote:
> > 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.

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

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

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

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

-- 
Catalin

Re: [PATCH] arm64: mm: fix zone_dma_limit calculation

[Yang Shi]

On 11/29/24 11:38 AM, Catalin Marinas wrote:
> On Fri, Nov 29, 2024 at 06:06:50PM +0000, Robin Murphy wrote:
>> On 2024-11-27 5:49 pm, Catalin Marinas wrote:
>>> 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.
> Fine by me to keep ZONE_DMA in the low range always. I was thinking of
> only doing this if ZONE_DMA32 is enabled.
>
>> 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?
>
>> 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.
>
>> 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.

Hi Catalin and Robin,

Thanks for moving this forward.

How's about the below commit message?

We failed to properly update the actual zone_dma_limit variable. Now it is
the memsize limit in IORT or device tree instead of U32_MAX if the 
memsize limit
is greater than U32_MAX.

The zone_dma_limit is used to determine whether GFP_DMA should be used 
or not
when allocating DMA buffers.  The wrong zone_dma_limit resulted in DMA 
allocations
use GFP_DMA even though the devices don't require it then fall into DMA 
zone on
node 0.  It caused regression on our two sockets systems due to 
excessive remote
memory access.

>

Re: [PATCH] arm64: mm: fix zone_dma_limit calculation

[Catalin Marinas]

On Mon, 25 Nov 2024 09:16:50 -0800, Yang Shi wrote:
> 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.
> 
> [...]

Applied to arm64 (for-next/fixes), thanks!

[1/1] arm64: mm: fix zone_dma_limit calculation
      https://git.kernel.org/arm64/c/56a708742a8b

-- 
Catalin