Re: [PATCH v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order

Re: [PATCH v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order

[Vineet Gupta]

Hi Mike,

On 5/4/20 8:39 AM, Mike Rapoport wrote:
> On Sun, May 03, 2020 at 11:43:00AM -0700, Guenter Roeck wrote:
>> On Sun, May 03, 2020 at 10:41:38AM -0700, Guenter Roeck wrote:
>>> Hi,
>>>
>>> On Wed, Apr 29, 2020 at 03:11:23PM +0300, Mike Rapoport wrote:
>>>> From: Mike Rapoport <rppt@linux.ibm.com>
>>>>
>>>> Some architectures (e.g. ARC) have the ZONE_HIGHMEM zone below the
>>>> ZONE_NORMAL. Allowing free_area_init() parse max_zone_pfn array even it is
>>>> sorted in descending order allows using free_area_init() on such
>>>> architectures.
>>>>
>>>> Add top -> down traversal of max_zone_pfn array in free_area_init() and use
>>>> the latter in ARC node/zone initialization.
>>>>
>>>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
>>>
>>> This patch causes my microblazeel qemu boot test in linux-next to fail.
>>> Reverting it fixes the problem.
>>>
>> The same problem is seen with s390 emulations.
> 
> Yeah, this patch breaks some others as well :(
> 
> My assumption that max_zone_pfn defines architectural limit for maximal
> PFN that can belong to a zone was over-optimistic. Several arches
> actually do that, but others do
> 
> 	max_zone_pfn[ZONE_DMA] = MAX_DMA_PFN;
> 	max_zone_pfn[ZONE_NORMAL] = max_pfn;
> 
> where MAX_DMA_PFN is build-time constrain and max_pfn is run time limit
> for the current system.
> 
> So, when max_pfn is lower than MAX_DMA_PFN, the free_init_area() will
> consider max_zone_pfn as descending and will wrongly calculate zone
> extents.
> 
> That said, instead of trying to create a generic way to special case
> ARC, I suggest to simply use the below patch instead.

Even for ARC it will be a bit more complicated. Highmem on ARC can be setup in 2
ways such that it is descending in one case, and ascending in other (w.r.t
"normal" mem) :-(

First some basic info about an ARC MMU based system

ARC logical address space (various addresses embedded in binaries)
 - translated (0 to 0x6FFF_FFFF)  - for userspace
 - untranslated (0x8000_0000 to 0xFFFF_FFFF) - kernel

ARC Physical address space is typically from 0x8000_0000 to 0xF000_0000.
Above translated space maps here via MMU. Untranslated is implicitly mapped (no
MMU involved).

The physical address in turn maps to a Bus address / memory (done at the
inter-connect/NoC). Typically Physical 0x8000_0000 map to DDR 0

Now,
- HIGHMEM w/o PAE40 adds Physical address space 0 to 0x7FFF_FFFF.
- HIGHMEM with PAE40 uses physical address space from 0x1_0000_0000 upwards.

But then you could also have a system which has both of above so the bimodal up/dn
won't work.

While I appreciate the effort to reduce complexity, it seems the current way of
setting things up allows for more flexibility in specifying the system memory map.

PS: I haven't looked at your series too carefully, the mention of ARC caught my
attention :-) I guess I need to read it more carefully to understand.


> 
> diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
> index 41eb9be1653c..386959bac3d2 100644
> --- a/arch/arc/mm/init.c
> +++ b/arch/arc/mm/init.c
> @@ -77,6 +77,11 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
>  		base, TO_MB(size), !in_use ? "Not used":"");
>  }
>  
> +bool arch_has_descending_max_zone_pfns(void)
> +{
> +	return true;
> +}
> +
>  /*
>   * First memory setup routine called from setup_arch()
>   * 1. setup swapper's mm @init_mm
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index b990e9734474..114f0e027144 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -7307,6 +7307,15 @@ static void check_for_memory(pg_data_t *pgdat, int nid)
>  	}
>  }
>  
> +/*
> + * Some architecturs, e.g. ARC may have ZONE_HIGHMEM below ZONE_NORMAL. For
> + * such cases we allow max_zone_pfn sorted in the descending order
> + */
> +bool __weak arch_has_descending_max_zone_pfns(void)
> +{
> +	return false;
> +}
> +
>  /**
>   * free_area_init - Initialise all pg_data_t and zone data
>   * @max_zone_pfn: an array of max PFNs for each zone
> @@ -7324,7 +7333,7 @@ void __init free_area_init(unsigned long *max_zone_pfn)
>  {
>  	unsigned long start_pfn, end_pfn;
>  	int i, nid, zone;
> -	bool descending = false;
> +	bool descending;
>  
>  	/* Record where the zone boundaries are */
>  	memset(arch_zone_lowest_possible_pfn, 0,
> @@ -7333,14 +7342,7 @@ void __init free_area_init(unsigned long *max_zone_pfn)
>  				sizeof(arch_zone_highest_possible_pfn));
>  
>  	start_pfn = find_min_pfn_with_active_regions();
> -
> -	/*
> -	 * Some architecturs, e.g. ARC may have ZONE_HIGHMEM below
> -	 * ZONE_NORMAL. For such cases we allow max_zone_pfn sorted in the
> -	 * descending order
> -	 */
> -	if (MAX_NR_ZONES > 1 && max_zone_pfn[0] > max_zone_pfn[1])
> -		descending = true;
> +	descending = arch_has_descending_max_zone_pfns();
>  
>  	for (i = 0; i < MAX_NR_ZONES; i++) {
>  		if (descending)
> 

Re: [PATCH v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order

[Mike Rapoport]

Hi Vineet,

On Tue, May 05, 2020 at 06:23:37AM +0000, Vineet Gupta wrote:
> Hi Mike,
> 
> On 5/4/20 8:39 AM, Mike Rapoport wrote:
> > On Sun, May 03, 2020 at 11:43:00AM -0700, Guenter Roeck wrote:
> >> On Sun, May 03, 2020 at 10:41:38AM -0700, Guenter Roeck wrote:
> >>> Hi,
> >>>
> >>> On Wed, Apr 29, 2020 at 03:11:23PM +0300, Mike Rapoport wrote:
> >>>> From: Mike Rapoport <rppt@linux.ibm.com>
> >>>>
> >>>> Some architectures (e.g. ARC) have the ZONE_HIGHMEM zone below the
> >>>> ZONE_NORMAL. Allowing free_area_init() parse max_zone_pfn array even it is
> >>>> sorted in descending order allows using free_area_init() on such
> >>>> architectures.
> >>>>
> >>>> Add top -> down traversal of max_zone_pfn array in free_area_init() and use
> >>>> the latter in ARC node/zone initialization.
> >>>>
> >>>> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
> >>>
> >>> This patch causes my microblazeel qemu boot test in linux-next to fail.
> >>> Reverting it fixes the problem.
> >>>
> >> The same problem is seen with s390 emulations.
> > 
> > Yeah, this patch breaks some others as well :(
> > 
> > My assumption that max_zone_pfn defines architectural limit for maximal
> > PFN that can belong to a zone was over-optimistic. Several arches
> > actually do that, but others do
> > 
> > 	max_zone_pfn[ZONE_DMA] = MAX_DMA_PFN;
> > 	max_zone_pfn[ZONE_NORMAL] = max_pfn;
> > 
> > where MAX_DMA_PFN is build-time constrain and max_pfn is run time limit
> > for the current system.
> > 
> > So, when max_pfn is lower than MAX_DMA_PFN, the free_init_area() will
> > consider max_zone_pfn as descending and will wrongly calculate zone
> > extents.
> > 
> > That said, instead of trying to create a generic way to special case
> > ARC, I suggest to simply use the below patch instead.
> 
> Even for ARC it will be a bit more complicated. Highmem on ARC can be setup in 2
> ways such that it is descending in one case, and ascending in other (w.r.t
> "normal" mem) :-(

Yeah, and this makes ARC really special :)

> First some basic info about an ARC MMU based system
> 
> ARC logical address space (various addresses embedded in binaries)
>  - translated (0 to 0x6FFF_FFFF)  - for userspace
>  - untranslated (0x8000_0000 to 0xFFFF_FFFF) - kernel
> 
> ARC Physical address space is typically from 0x8000_0000 to 0xF000_0000.
> Above translated space maps here via MMU. Untranslated is implicitly mapped (no
> MMU involved).
> 
> The physical address in turn maps to a Bus address / memory (done at the
> inter-connect/NoC). Typically Physical 0x8000_0000 map to DDR 0
> 
> Now,
> - HIGHMEM w/o PAE40 adds Physical address space 0 to 0x7FFF_FFFF.
> - HIGHMEM with PAE40 uses physical address space from 0x1_0000_0000 upwards.
> 
> But then you could also have a system which has both of above so the bimodal up/dn
> won't work.

From the code I've got the impression that it is either one of them. I.e
the physical memory is either at

0x8000_0000 - <end of DDR 0 bank>
0x0000_0000 - <end of DDR 1 bank>

or

0x0_8000_0000 - <end of DDR 0 bank>
0x1_0000_0000 - <end of DDR 1 bank>

Is this possible to have a system with three live ranges? Like

0x0_0000_0000 - <end of DDR 1 bank>
0x0_8000_0000 - <end of DDR 0 bank>
0x1_0000_0000 - <end of DDR 2 bank>

> While I appreciate the effort to reduce complexity, it seems the
> current way of
> setting things up allows for more flexibility in specifying the system memory map.
>
> PS: I haven't looked at your series too carefully, the mention of ARC caught my
> attention :-) I guess I need to read it more carefully to understand.
 
That would be cool :)

> > 
> > diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
> > index 41eb9be1653c..386959bac3d2 100644
> > --- a/arch/arc/mm/init.c
> > +++ b/arch/arc/mm/init.c
> > @@ -77,6 +77,11 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
> >  		base, TO_MB(size), !in_use ? "Not used":"");
> >  }
> >  
> > +bool arch_has_descending_max_zone_pfns(void)
> > +{
> > +	return true;
> > +}
> > +
> >  /*
> >   * First memory setup routine called from setup_arch()
> >   * 1. setup swapper's mm @init_mm
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index b990e9734474..114f0e027144 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -7307,6 +7307,15 @@ static void check_for_memory(pg_data_t *pgdat, int nid)
> >  	}
> >  }
> >  
> > +/*
> > + * Some architecturs, e.g. ARC may have ZONE_HIGHMEM below ZONE_NORMAL. For
> > + * such cases we allow max_zone_pfn sorted in the descending order
> > + */
> > +bool __weak arch_has_descending_max_zone_pfns(void)
> > +{
> > +	return false;
> > +}
> > +
> >  /**
> >   * free_area_init - Initialise all pg_data_t and zone data
> >   * @max_zone_pfn: an array of max PFNs for each zone
> > @@ -7324,7 +7333,7 @@ void __init free_area_init(unsigned long *max_zone_pfn)
> >  {
> >  	unsigned long start_pfn, end_pfn;
> >  	int i, nid, zone;
> > -	bool descending = false;
> > +	bool descending;
> >  
> >  	/* Record where the zone boundaries are */
> >  	memset(arch_zone_lowest_possible_pfn, 0,
> > @@ -7333,14 +7342,7 @@ void __init free_area_init(unsigned long *max_zone_pfn)
> >  				sizeof(arch_zone_highest_possible_pfn));
> >  
> >  	start_pfn = find_min_pfn_with_active_regions();
> > -
> > -	/*
> > -	 * Some architecturs, e.g. ARC may have ZONE_HIGHMEM below
> > -	 * ZONE_NORMAL. For such cases we allow max_zone_pfn sorted in the
> > -	 * descending order
> > -	 */
> > -	if (MAX_NR_ZONES > 1 && max_zone_pfn[0] > max_zone_pfn[1])
> > -		descending = true;
> > +	descending = arch_has_descending_max_zone_pfns();
> >  
> >  	for (i = 0; i < MAX_NR_ZONES; i++) {
> >  		if (descending)
> > 

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order

[Vineet Gupta]

On 5/5/20 2:19 AM, Mike Rapoport wrote:
> From the code I've got the impression that it is either one of them. I.e
> the physical memory is either at
>
> 0x8000_0000 - <end of DDR 0 bank>
> 0x0000_0000 - <end of DDR 1 bank>
>
> or
>
> 0x0_8000_0000 - <end of DDR 0 bank>
> 0x1_0000_0000 - <end of DDR 1 bank>
>
> Is this possible to have a system with three live ranges? Like
>
> 0x0_0000_0000 - <end of DDR 1 bank>
> 0x0_8000_0000 - <end of DDR 0 bank>
> 0x1_0000_0000 - <end of DDR 2 bank>

We don't have such a system, but it is indeed possible in theory. The question is
 - Can other arches have such a setup too
 - Is it not better to have the core retain the flexibility just in case

Thx,
-Vineet

Re: [PATCH v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order

[Mike Rapoport]

On Tue, May 05, 2020 at 06:07:46PM +0000, Vineet Gupta wrote:
> On 5/5/20 2:19 AM, Mike Rapoport wrote:
> > From the code I've got the impression that it is either one of them. I.e
> > the physical memory is either at
> >
> > 0x8000_0000 - <end of DDR 0 bank>
> > 0x0000_0000 - <end of DDR 1 bank>
> >
> > or
> >
> > 0x0_8000_0000 - <end of DDR 0 bank>
> > 0x1_0000_0000 - <end of DDR 1 bank>
> >
> > Is this possible to have a system with three live ranges? Like
> >
> > 0x0_0000_0000 - <end of DDR 1 bank>
> > 0x0_8000_0000 - <end of DDR 0 bank>
> > 0x1_0000_0000 - <end of DDR 2 bank>
> 
> We don't have such a system, but it is indeed possible in theory. The question is
>  - Can other arches have such a setup too

At the moment all architectures that support HIGHMEM have it above
DMA/NORMAL. I'm not sure if such a setup is theoretically possible for
other architectures, but as of now none of them support it in Linux.

The general case is somewhat like

	max_dma_pfn <= max_normal_pfn < max_high_pfn

And of course, either max_dma_pfn or max_high_pfn or both may be not
needed for an architecture.

>  - Is it not better to have the core retain the flexibility just in case

Hmm, there is indeed flexibility in the nodes and zones initialization,
but if you'd look more closely to free_area_init*() and friends, there
is a lot of cruft and retrofitting ;-)

What we have is two mutually exclusive paths, one that relies on the
architecture to calculate zone sizes and find the holes between the
zones (!CONFIG_HAVE_MEMBLOCK_NODE_MAP) and the other one that only
requires the architectures to pass possible limit for each zone and
detects the actual zone spans based on the knowlegde about the actual
physical memory layout that comes from memblock.

These patches attempt to drop the older method and switch all the
architectures to use newer and simpler one.

If the requirement to have support for 3-banks is a theoretical
possibility, I would prefer to adjust ARC's version of
arch_has_descending_max_zone_pfns() to cope with either of 2-banks
configuration (PAE40 and non-PAE40) and deal with the third bank when/if
it actually materializes.

> Thx,
> -Vineet

-- 
Sincerely yours,
Mike.

Re: [PATCH v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order

[Mike Rapoport]

On Tue, May 05, 2020 at 11:15:22PM +0300, Mike Rapoport wrote:
> On Tue, May 05, 2020 at 06:07:46PM +0000, Vineet Gupta wrote:
> > On 5/5/20 2:19 AM, Mike Rapoport wrote:
> 
> >  - Is it not better to have the core retain the flexibility just in case
> 
> If the requirement to have support for 3-banks is a theoretical
> possibility, I would prefer to adjust ARC's version of
> arch_has_descending_max_zone_pfns() to cope with either of 2-banks
> configuration (PAE40 and non-PAE40) and deal with the third bank when/if
> it actually materializes.
> 
> > Thx,
> > -Vineet
> 

The fix below should take care of any 2-bank configurations. 
This is vs. current mmotm.

From eb8124fb3584607d1036b7ae00c8092ae43e480d Mon Sep 17 00:00:00 2001
From: Mike Rapoport <rppt@linux.ibm.com>
Date: Thu, 7 May 2020 23:44:15 +0300
Subject: [PATCH] arc: free_area_init(): take into account PAE40 mode

The arch_has_descending_max_zone_pfns() does not take into account physical
memory layout for PAE40 configuration.
With PAE40 enabled, the HIGHMEM is actually higher than NORMAL and
arch_has_descending_max_zone_pfns() should return false in this case.

Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>
---
 arch/arc/mm/init.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
index 386959bac3d2..e7bdc2ac1c87 100644
--- a/arch/arc/mm/init.c
+++ b/arch/arc/mm/init.c
@@ -79,7 +79,7 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
 
 bool arch_has_descending_max_zone_pfns(void)
 {
-	return true;
+	return !IS_ENABLED(CONFIG_ARC_HAS_PAE40);
 }
 
 /*
-- 
2.26.1


-- 
Sincerely yours,
Mike.

Re: [PATCH v2 17/20] mm: free_area_init: allow defining max_zone_pfn in descending order

[Vineet Gupta]

On 5/7/20 1:59 PM, Mike Rapoport wrote:
> On Tue, May 05, 2020 at 11:15:22PM +0300, Mike Rapoport wrote:
>> On Tue, May 05, 2020 at 06:07:46PM +0000, Vineet Gupta wrote:
>>> On 5/5/20 2:19 AM, Mike Rapoport wrote:
>>>  - Is it not better to have the core retain the flexibility just in case
>> If the requirement to have support for 3-banks is a theoretical
>> possibility, I would prefer to adjust ARC's version of
>> arch_has_descending_max_zone_pfns() to cope with either of 2-banks
>> configuration (PAE40 and non-PAE40) and deal with the third bank when/if
>> it actually materializes.

Fair enough.

> The fix below should take care of any 2-bank configurations. 
> This is vs. current mmotm.
>
> From eb8124fb3584607d1036b7ae00c8092ae43e480d Mon Sep 17 00:00:00 2001
> From: Mike Rapoport <rppt@linux.ibm.com>
> Date: Thu, 7 May 2020 23:44:15 +0300
> Subject: [PATCH] arc: free_area_init(): take into account PAE40 mode
>
> The arch_has_descending_max_zone_pfns() does not take into account physical
> memory layout for PAE40 configuration.
> With PAE40 enabled, the HIGHMEM is actually higher than NORMAL and
> arch_has_descending_max_zone_pfns() should return false in this case.
>
> Signed-off-by: Mike Rapoport <rppt@linux.ibm.com>

LGTM.

Acked-by: Vineet Gupta <vgupta@synopsys.com>

Thx,

> ---
>  arch/arc/mm/init.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/arc/mm/init.c b/arch/arc/mm/init.c
> index 386959bac3d2..e7bdc2ac1c87 100644
> --- a/arch/arc/mm/init.c
> +++ b/arch/arc/mm/init.c
> @@ -79,7 +79,7 @@ void __init early_init_dt_add_memory_arch(u64 base, u64 size)
>  
>  bool arch_has_descending_max_zone_pfns(void)
>  {
> -	return true;
> +	return !IS_ENABLED(CONFIG_ARC_HAS_PAE40);
>  }
>  
>  /*