[PATCH 1/2 v2] bootmem/powerpc: Unify bootmem initialization

[PATCH 1/2 v2] bootmem/powerpc: Unify bootmem initialization

[Emil Medve]

Unify the low/highmem code path from do_init_bootmem() by using (the)
lowmem related variables/parameters even when the low/highmem split
is not needed (64-bit) or configured. In such cases the "lowmem"
variables/parameters continue to observe the definition by referring
to memory directly mapped by the kernel

Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
---

v2: Rebased, no changes

 arch/powerpc/mm/mem.c | 36 ++++++++++++++++--------------------
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index 32202c9..eaf5d1d8 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -188,27 +188,31 @@ EXPORT_SYMBOL_GPL(walk_system_ram_range);
 void __init do_init_bootmem(void)
 {
 	unsigned long start, bootmap_pages;
-	unsigned long total_pages;
 	struct memblock_region *reg;
 	int boot_mapsize;
+	phys_addr_t _total_lowmem;
+	phys_addr_t _lowmem_end_addr;
 
-	max_low_pfn = max_pfn = memblock_end_of_DRAM() >> PAGE_SHIFT;
-	total_pages = (memblock_end_of_DRAM() - memstart_addr) >> PAGE_SHIFT;
-#ifdef CONFIG_HIGHMEM
-	total_pages = total_lowmem >> PAGE_SHIFT;
-	max_low_pfn = lowmem_end_addr >> PAGE_SHIFT;
+#ifndef CONFIG_HIGHMEM
+	_lowmem_end_addr = memblock_end_of_DRAM();
+#else
+	_lowmem_end_addr = lowmem_end_addr;
 #endif
 
+	max_pfn = memblock_end_of_DRAM() >> PAGE_SHIFT;
+	max_low_pfn = _lowmem_end_addr >> PAGE_SHIFT;
+	min_low_pfn = MEMORY_START >> PAGE_SHIFT;
+
 	/*
 	 * Find an area to use for the bootmem bitmap.  Calculate the size of
 	 * bitmap required as (Total Memory) / PAGE_SIZE / BITS_PER_BYTE.
 	 * Add 1 additional page in case the address isn't page-aligned.
 	 */
-	bootmap_pages = bootmem_bootmap_pages(total_pages);
+	_total_lowmem = _lowmem_end_addr - memstart_addr;
+	bootmap_pages = bootmem_bootmap_pages(_total_lowmem >> PAGE_SHIFT);
 
 	start = memblock_alloc(bootmap_pages << PAGE_SHIFT, PAGE_SIZE);
 
-	min_low_pfn = MEMORY_START >> PAGE_SHIFT;
 	boot_mapsize = init_bootmem_node(NODE_DATA(0), start >> PAGE_SHIFT, min_low_pfn, max_low_pfn);
 
 	/* Place all memblock_regions in the same node and merge contiguous
@@ -219,26 +223,18 @@ void __init do_init_bootmem(void)
 	/* Add all physical memory to the bootmem map, mark each area
 	 * present.
 	 */
-#ifdef CONFIG_HIGHMEM
-	free_bootmem_with_active_regions(0, lowmem_end_addr >> PAGE_SHIFT);
+	free_bootmem_with_active_regions(0, max_low_pfn);
 
 	/* reserve the sections we're already using */
 	for_each_memblock(reserved, reg) {
-		unsigned long top = reg->base + reg->size - 1;
-		if (top < lowmem_end_addr)
+		if (reg->base + reg->size - 1 < _lowmem_end_addr)
 			reserve_bootmem(reg->base, reg->size, BOOTMEM_DEFAULT);
-		else if (reg->base < lowmem_end_addr) {
-			unsigned long trunc_size = lowmem_end_addr - reg->base;
+		else if (reg->base < _lowmem_end_addr) {
+			unsigned long trunc_size = _lowmem_end_addr - reg->base;
 			reserve_bootmem(reg->base, trunc_size, BOOTMEM_DEFAULT);
 		}
 	}
-#else
-	free_bootmem_with_active_regions(0, max_pfn);
 
-	/* reserve the sections we're already using */
-	for_each_memblock(reserved, reg)
-		reserve_bootmem(reg->base, reg->size, BOOTMEM_DEFAULT);
-#endif
 	/* XXX need to clip this if using highmem? */
 	sparse_memory_present_with_active_regions(0);
 
-- 
1.9.2

[PATCH 2/2 v2] powerpc: Enable NO_BOOTMEM

[Emil Medve]

Currently bootmem is just a wrapper around memblock. This gets rid of
the wrapper code just as other ARHC(es) did: x86, arm, etc.

For now only cover !NUMA systems/builds

Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
---

v2: Acknowledge that NUMA systems/builds are not covered by this patch

 arch/powerpc/Kconfig  | 3 +++
 arch/powerpc/mm/mem.c | 8 ++++++++
 2 files changed, 11 insertions(+)

diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index e099899..07b164b 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -475,6 +475,9 @@ config SYS_SUPPORTS_HUGETLBFS
 
 source "mm/Kconfig"
 
+config NO_BOOTMEM
+	def_bool !NUMA
+
 config ARCH_MEMORY_PROBE
 	def_bool y
 	depends on MEMORY_HOTPLUG
diff --git a/arch/powerpc/mm/mem.c b/arch/powerpc/mm/mem.c
index eaf5d1d8..d3e1d5f 100644
--- a/arch/powerpc/mm/mem.c
+++ b/arch/powerpc/mm/mem.c
@@ -187,10 +187,12 @@ EXPORT_SYMBOL_GPL(walk_system_ram_range);
 #ifndef CONFIG_NEED_MULTIPLE_NODES
 void __init do_init_bootmem(void)
 {
+#ifndef CONFIG_NO_BOOTMEM
 	unsigned long start, bootmap_pages;
 	struct memblock_region *reg;
 	int boot_mapsize;
 	phys_addr_t _total_lowmem;
+#endif
 	phys_addr_t _lowmem_end_addr;
 
 #ifndef CONFIG_HIGHMEM
@@ -203,6 +205,7 @@ void __init do_init_bootmem(void)
 	max_low_pfn = _lowmem_end_addr >> PAGE_SHIFT;
 	min_low_pfn = MEMORY_START >> PAGE_SHIFT;
 
+#ifndef CONFIG_NO_BOOTMEM
 	/*
 	 * Find an area to use for the bootmem bitmap.  Calculate the size of
 	 * bitmap required as (Total Memory) / PAGE_SIZE / BITS_PER_BYTE.
@@ -214,12 +217,14 @@ void __init do_init_bootmem(void)
 	start = memblock_alloc(bootmap_pages << PAGE_SHIFT, PAGE_SIZE);
 
 	boot_mapsize = init_bootmem_node(NODE_DATA(0), start >> PAGE_SHIFT, min_low_pfn, max_low_pfn);
+#endif
 
 	/* Place all memblock_regions in the same node and merge contiguous
 	 * memblock_regions
 	 */
 	memblock_set_node(0, (phys_addr_t)ULLONG_MAX, &memblock.memory, 0);
 
+#ifndef CONFIG_NO_BOOTMEM
 	/* Add all physical memory to the bootmem map, mark each area
 	 * present.
 	 */
@@ -234,11 +239,14 @@ void __init do_init_bootmem(void)
 			reserve_bootmem(reg->base, trunc_size, BOOTMEM_DEFAULT);
 		}
 	}
+#endif
 
 	/* XXX need to clip this if using highmem? */
 	sparse_memory_present_with_active_regions(0);
 
+#ifndef CONFIG_NO_BOOTMEM
 	init_bootmem_done = 1;
+#endif
 }
 
 /* mark pages that don't exist as nosave */
-- 
1.9.2

Re: [PATCH 2/2 v2] powerpc: Enable NO_BOOTMEM

[Scott Wood]

On Tue, 2014-05-06 at 13:48 -0500, Emil Medve wrote:
> Currently bootmem is just a wrapper around memblock. This gets rid of
> the wrapper code just as other ARHC(es) did: x86, arm, etc.
> 
> For now only cover !NUMA systems/builds
> 
> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
> ---
> 
> v2: Acknowledge that NUMA systems/builds are not covered by this patch
> 
>  arch/powerpc/Kconfig  | 3 +++
>  arch/powerpc/mm/mem.c | 8 ++++++++
>  2 files changed, 11 insertions(+)
> 
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index e099899..07b164b 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -475,6 +475,9 @@ config SYS_SUPPORTS_HUGETLBFS
>  
>  source "mm/Kconfig"
>  
> +config NO_BOOTMEM
> +	def_bool !NUMA

This will allow a user to manually turn on CONFIG_NO_BOOTMEM in the
presence of NUMA.  From the changelog it sounds like this is not what
you intended.

What are the issues with NUMA?  As is, you're not getting rid of wrapper
code -- only adding ifdefs.

-Scott

Re: [PATCH 2/2 v2] powerpc: Enable NO_BOOTMEM

[Scott Wood]

On Tue, 2014-05-06 at 16:49 -0500, Scott Wood wrote:
> On Tue, 2014-05-06 at 13:48 -0500, Emil Medve wrote:
> > Currently bootmem is just a wrapper around memblock. This gets rid of
> > the wrapper code just as other ARHC(es) did: x86, arm, etc.
> > 
> > For now only cover !NUMA systems/builds
> > 
> > Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
> > ---
> > 
> > v2: Acknowledge that NUMA systems/builds are not covered by this patch
> > 
> >  arch/powerpc/Kconfig  | 3 +++
> >  arch/powerpc/mm/mem.c | 8 ++++++++
> >  2 files changed, 11 insertions(+)
> > 
> > diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> > index e099899..07b164b 100644
> > --- a/arch/powerpc/Kconfig
> > +++ b/arch/powerpc/Kconfig
> > @@ -475,6 +475,9 @@ config SYS_SUPPORTS_HUGETLBFS
> >  
> >  source "mm/Kconfig"
> >  
> > +config NO_BOOTMEM
> > +	def_bool !NUMA
> 
> This will allow a user to manually turn on CONFIG_NO_BOOTMEM in the
> presence of NUMA.  From the changelog it sounds like this is not what
> you intended.

Ignore this part -- I see it doesn't have an option string for it to
show up to the user.

-Scott

Re: [PATCH 2/2 v2] powerpc: Enable NO_BOOTMEM

[Emil Medve]

Hello Scott,


On 05/06/2014 04:49 PM, Scott Wood wrote:
> On Tue, 2014-05-06 at 13:48 -0500, Emil Medve wrote:
>> Currently bootmem is just a wrapper around memblock. This gets rid of
>> the wrapper code just as other ARHC(es) did: x86, arm, etc.
>>
>> For now only cover !NUMA systems/builds
>>
>> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
>> ---
>>
>> v2: Acknowledge that NUMA systems/builds are not covered by this patch
>>
>>  arch/powerpc/Kconfig  | 3 +++
>>  arch/powerpc/mm/mem.c | 8 ++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index e099899..07b164b 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -475,6 +475,9 @@ config SYS_SUPPORTS_HUGETLBFS
>>  
>>  source "mm/Kconfig"
>>  
>> +config NO_BOOTMEM
>> +	def_bool !NUMA
> 
> This will allow a user to manually turn on CONFIG_NO_BOOTMEM in the
> presence of NUMA.  From the changelog it sounds like this is not what
> you intended.
> 
> What are the issues with NUMA?

Well, I don't have access to a NUMA box/board. I could enable NUMA for a
!NUMA board but I'd feel better if I could actually test/debug on a
relevant system

> As is, you're not getting rid of wrapper code -- only adding ifdefs.

First, you're talking about the bootmem initialization wrapper code for
powerpc. The actual bootmem code is in include/linux/bootmem.h and
mm/bootmem.c. We can't remove those files as they are still used by
other arches. Also, the word wrapper is somewhat imprecise as in powerpc
land bootmem sort of runs on top of memblock

When NO_BOOTMEM is configured the mm/nobootmem.c is used that is the
bootmem API actually re-implemented with memblock. The bootmem API is
used in various places in the arch independent code

This patch wants to isolate for removal the bootmem initialization code
for powerpc and to exclude mm/bootmem.c from being built. This being the
first step I didn't want to actually remove the code, so it will be easy
to debug if some issues crop up. Also, people that want the use the
bootmem code for some reason can easily do that. Once this change spends
some time in the tree, we can actually remove the bootmem initialization
code


Cheers,

Re: [PATCH 2/2 v2] powerpc: Enable NO_BOOTMEM

[Scott Wood]

On Tue, 2014-05-06 at 19:16 -0500, Emil Medve wrote:
> Hello Scott,
> 
> 
> On 05/06/2014 04:49 PM, Scott Wood wrote:
> > On Tue, 2014-05-06 at 13:48 -0500, Emil Medve wrote:
> >> Currently bootmem is just a wrapper around memblock. This gets rid of
> >> the wrapper code just as other ARHC(es) did: x86, arm, etc.
> >>
> >> For now only cover !NUMA systems/builds
> >>
> >> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
> >> ---
> >>
> >> v2: Acknowledge that NUMA systems/builds are not covered by this patch
> >>
> >>  arch/powerpc/Kconfig  | 3 +++
> >>  arch/powerpc/mm/mem.c | 8 ++++++++
> >>  2 files changed, 11 insertions(+)
> >>
> >> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> >> index e099899..07b164b 100644
> >> --- a/arch/powerpc/Kconfig
> >> +++ b/arch/powerpc/Kconfig
> >> @@ -475,6 +475,9 @@ config SYS_SUPPORTS_HUGETLBFS
> >>  
> >>  source "mm/Kconfig"
> >>  
> >> +config NO_BOOTMEM
> >> +	def_bool !NUMA
> > 
> > This will allow a user to manually turn on CONFIG_NO_BOOTMEM in the
> > presence of NUMA.  From the changelog it sounds like this is not what
> > you intended.
> > 
> > What are the issues with NUMA?
> 
> Well, I don't have access to a NUMA box/board. I could enable NUMA for a
> !NUMA board but I'd feel better if I could actually test/debug on a
> relevant system

You could first test with NUMA on a non-NUMA board, and then if that
works ask the list for help testing on NUMA hardware (and various
non-Freescale non-NUMA hardware, for that matter).

Is there a specific issue that would need to be addressed to make it
work on NUMA?

> > As is, you're not getting rid of wrapper code -- only adding ifdefs.
> 
> First, you're talking about the bootmem initialization wrapper code for
> powerpc. The actual bootmem code is in include/linux/bootmem.h and
> mm/bootmem.c. We can't remove those files as they are still used by
> other arches. Also, the word wrapper is somewhat imprecise as in powerpc
> land bootmem sort of runs on top of memblock

My point was just that the changelog says "This gets rid of wrapper
code" when it actually removes no source code, and adds configuration
complexity.

> When NO_BOOTMEM is configured the mm/nobootmem.c is used that is the
> bootmem API actually re-implemented with memblock. The bootmem API is
> used in various places in the arch independent code
> 
> This patch wants to isolate for removal the bootmem initialization code
> for powerpc and to exclude mm/bootmem.c from being built. This being the
> first step I didn't want to actually remove the code, so it will be easy
> to debug if some issues crop up. Also, people that want the use the
> bootmem code for some reason can easily do that. Once this change spends
> some time in the tree, we can actually remove the bootmem initialization
> code

Is there a plausible reason someone would "want to use the bootmem
code"?

While the "ifdef it for a while" approach is sometimes sensible, usually
it's better to just make the change rather than ifdef it.  Consider what
the code would look like if there were ifdefs for a ton of random
changes, half of which nobody ever bothered to go back and clean up
after the change got widespread testing.  Why is this patch risky enough
to warrant such an approach?  Shouldn't boot-time issues be pretty
obvious?

-Scott

Re: [PATCH 2/2 v2] powerpc: Enable NO_BOOTMEM

[Aneesh Kumar K.V]

Emil Medve <Emilian.Medve@Freescale.com> writes:

> Currently bootmem is just a wrapper around memblock. This gets rid of
> the wrapper code just as other ARHC(es) did: x86, arm, etc.
>
> For now only cover !NUMA systems/builds
>
> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
> ---
>
> v2: Acknowledge that NUMA systems/builds are not covered by this patch
>
>  arch/powerpc/Kconfig  | 3 +++
>  arch/powerpc/mm/mem.c | 8 ++++++++
>  2 files changed, 11 insertions(+)
>
> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
> index e099899..07b164b 100644
> --- a/arch/powerpc/Kconfig
> +++ b/arch/powerpc/Kconfig
> @@ -475,6 +475,9 @@ config SYS_SUPPORTS_HUGETLBFS
>  
>  source "mm/Kconfig"
>  
> +config NO_BOOTMEM
> +	def_bool !NUMA


There is actually one in  mm/Kconfig

So I guess you should make the platform that you are interested
/tested just do select No_BOOTMEM like we do in arch/arm/Kconfig etc ?

-aneesh

Re: [PATCH 2/2 v2] powerpc: Enable NO_BOOTMEM

[Emil Medve]

Hello Aneesh,


On 05/07/2014 01:35 AM, Aneesh Kumar K.V wrote:
> Emil Medve <Emilian.Medve@Freescale.com> writes:
> 
>> Currently bootmem is just a wrapper around memblock. This gets rid of
>> the wrapper code just as other ARHC(es) did: x86, arm, etc.
>>
>> For now only cover !NUMA systems/builds
>>
>> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
>> ---
>>
>> v2: Acknowledge that NUMA systems/builds are not covered by this patch
>>
>>  arch/powerpc/Kconfig  | 3 +++
>>  arch/powerpc/mm/mem.c | 8 ++++++++
>>  2 files changed, 11 insertions(+)
>>
>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>> index e099899..07b164b 100644
>> --- a/arch/powerpc/Kconfig
>> +++ b/arch/powerpc/Kconfig
>> @@ -475,6 +475,9 @@ config SYS_SUPPORTS_HUGETLBFS
>>  
>>  source "mm/Kconfig"
>>  
>> +config NO_BOOTMEM
>> +	def_bool !NUMA
> 
> 
> There is actually one in  mm/Kconfig
> 
> So I guess you should make the platform that you are interested
> /tested just do select No_BOOTMEM like we do in arch/arm/Kconfig etc ?

I had a look at what was done for x86 and I missed why the mm/Kconfig
symbol was added. Will include your suggestion in the next version


Cheers,

Re: [PATCH 2/2 v2] powerpc: Enable NO_BOOTMEM

[Emil Medve]

Hello Scott,


On 05/06/2014 09:44 PM, Scott Wood wrote:
> On Tue, 2014-05-06 at 19:16 -0500, Emil Medve wrote:
>> Hello Scott,
>>
>>
>> On 05/06/2014 04:49 PM, Scott Wood wrote:
>>> On Tue, 2014-05-06 at 13:48 -0500, Emil Medve wrote:
>>>> Currently bootmem is just a wrapper around memblock. This gets rid of
>>>> the wrapper code just as other ARHC(es) did: x86, arm, etc.
>>>>
>>>> For now only cover !NUMA systems/builds
>>>>
>>>> Signed-off-by: Emil Medve <Emilian.Medve@Freescale.com>
>>>> ---
>>>>
>>>> v2: Acknowledge that NUMA systems/builds are not covered by this patch
>>>>
>>>>  arch/powerpc/Kconfig  | 3 +++
>>>>  arch/powerpc/mm/mem.c | 8 ++++++++
>>>>  2 files changed, 11 insertions(+)
>>>>
>>>> diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
>>>> index e099899..07b164b 100644
>>>> --- a/arch/powerpc/Kconfig
>>>> +++ b/arch/powerpc/Kconfig
>>>> @@ -475,6 +475,9 @@ config SYS_SUPPORTS_HUGETLBFS
>>>>  
>>>>  source "mm/Kconfig"
>>>>  
>>>> +config NO_BOOTMEM
>>>> +	def_bool !NUMA
>>>
>>> This will allow a user to manually turn on CONFIG_NO_BOOTMEM in the
>>> presence of NUMA.  From the changelog it sounds like this is not what
>>> you intended.
>>>
>>> What are the issues with NUMA?
>>
>> Well, I don't have access to a NUMA box/board. I could enable NUMA for a
>> !NUMA board but I'd feel better if I could actually test/debug on a
>> relevant system
> 
> You could first test with NUMA on a non-NUMA board, and then if that
> works ask the list for help testing on NUMA hardware (and various
> non-Freescale non-NUMA hardware, for that matter).
> 
> Is there a specific issue that would need to be addressed to make it
> work on NUMA?

Nope. Just different code/file(s)... with NUMA specific details...

>>> As is, you're not getting rid of wrapper code -- only adding ifdefs.
>>
>> First, you're talking about the bootmem initialization wrapper code for
>> powerpc. The actual bootmem code is in include/linux/bootmem.h and
>> mm/bootmem.c. We can't remove those files as they are still used by
>> other arches. Also, the word wrapper is somewhat imprecise as in powerpc
>> land bootmem sort of runs on top of memblock
> 
> My point was just that the changelog says "This gets rid of wrapper
> code" when it actually removes no source code, and adds configuration
> complexity.

The patch gets rid of the wrapper code, bootmem itself and arch specific
initialization, from the build/kernel image. I guess I'll re-word that
so it doesn't sound so literal

>> When NO_BOOTMEM is configured the mm/nobootmem.c is used that is the
>> bootmem API actually re-implemented with memblock. The bootmem API is
>> used in various places in the arch independent code
>>
>> This patch wants to isolate for removal the bootmem initialization code
>> for powerpc and to exclude mm/bootmem.c from being built. This being the
>> first step I didn't want to actually remove the code, so it will be easy
>> to debug if some issues crop up. Also, people that want the use the
>> bootmem code for some reason can easily do that. Once this change spends
>> some time in the tree, we can actually remove the bootmem initialization
>> code
> 
> Is there a plausible reason someone would "want to use the bootmem
> code"?

Don't know, but as before it wasn't even possible to make a build with
NO_BOOTMEM I decided to err on the side of caution

> While the "ifdef it for a while" approach is sometimes sensible, usually
> it's better to just make the change rather than ifdef it.

I felt it was sensible in this case

> Consider what
> the code would look like if there were ifdefs for a ton of random
> changes, half of which nobody ever bothered to go back and clean up
> after the change got widespread testing.

Well, this is not a random change, but I certainly don't disagree with
the principle of what you're saying

> Why is this patch risky enough to warrant such an approach?

I don't think it's risky and we've been using it for months on various
SoC(s)/boards. Just didn't want to be very abrupt in removing the option
of using bootmem

> Shouldn't boot-time issues be pretty obvious?

Gross errors should be obvious. But the devil is in the details, and
even tough I've debugged/compared the memory layout/allocations with
bootmem and memblock only, I'm prepared to admit I might have missed
something (especially in the first patch of the sequence)


Cheers,