[RFC 00/23] mm: Use memblock interface instead of bootmem

[RFC 00/23] mm: Use memblock interface instead of bootmem

[Santosh Shilimkar]

Tejun, Yinghai and others, 

Here is an attempt to convert the core kernel code to memblock allocator
APIs when used with NO_BOOTMEM. Based on discussion thread [1] and my
limited understanding of the topic, I tried to cook up this RFC with
help from Grygorii. I am counting on reviews, guidance and testing help
to move forward with the approach. This is one of the blocking item for
the ARM LPAE architecture on the physical memory starts after 4BG boundary
and hence needs the early memory allocators 

As outlined by Tejun, we would like to remove the use of nobootmem.c and
then eventually bootmem allocator once all arch switch to NO_BOOTMEM.
Not to break the existing architectures using bootmem, all the new
memblock interfaces fall back to bootmem layer with !NO_BOOTMEM

Testing is done on ARM architecture with 32 bit and ARM LAPE machines
with normal as well sparse(famed) memory model. To convert ARM to
NO_BOOTMEM, I have used Russell's work [2] and couple of patches
on top of that.

Comments/suggestions are welcome !!

Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: H. Peter Anvin <hpa@zytor.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Nicolas Pitre <nicolas.pitre@linaro.org>
Cc: Olof Johansson <olof@lixom.net>

Grygorii Strashko (9):
  mm/bootmem: remove duplicated declaration of __free_pages_bootmem()
  mm/block: remove unnecessary inclusion of bootmem.h
  mm/memory_hotplug: remove unnecessary inclusion of bootmem.h
  mm/staging: remove unnecessary inclusion of bootmem.h
  mm/char: remove unnecessary inclusion of bootmem.h
  mm/memblock: debug: correct displaying of upper memory boundary
  mm/memblock: debug: don't free reserved array if
    !ARCH_DISCARD_MEMBLOCK
  mm/hugetlb: Use memblock apis for early memory allocations
  mm/page_cgroup: Use memblock apis for early memory allocations

Santosh Shilimkar (14):
  mm/memblock: Add memblock early memory allocation apis
  mm/init: Use memblock apis for early memory allocations
  mm/printk: Use memblock apis for early memory allocations
  mm/page_alloc: Use memblock apis for early memory allocations
  mm/power: Use memblock apis for early memory allocations
  mm/lib: Use memblock apis for early memory allocations
  mm/lib: Use memblock apis for early memory allocations
  mm/sparse: Use memblock apis for early memory allocations
  mm/percpu: Use memblock apis for early memory allocations
  mm/memory_hotplug: Use memblock apis for early memory allocations
  mm/firmware: Use memblock apis for early memory allocations
  mm/ARM: kernel: Use memblock apis for early memory allocations
  mm/ARM: mm: Use memblock apis for early memory allocations
  mm/ARM: OMAP: Use memblock apis for early memory allocations

 arch/arm/kernel/devtree.c        |    2 +-
 arch/arm/kernel/setup.c          |    2 +-
 arch/arm/mach-omap2/omap_hwmod.c |    8 +--
 arch/arm/mm/init.c               |    2 +-
 block/blk-ioc.c                  |    1 -
 drivers/char/mem.c               |    1 -
 drivers/firmware/memmap.c        |    2 +-
 drivers/staging/speakup/main.c   |    2 -
 include/linux/bootmem.h          |   73 ++++++++++++++++++++++-
 init/main.c                      |    4 +-
 kernel/power/snapshot.c          |    2 +-
 kernel/printk/printk.c           |   10 +---
 lib/cpumask.c                    |    4 +-
 lib/swiotlb.c                    |   30 +++++-----
 mm/hugetlb.c                     |   10 ++--
 mm/memblock.c                    |  122 +++++++++++++++++++++++++++++++++++++-
 mm/memory_hotplug.c              |    3 +-
 mm/page_alloc.c                  |   26 ++++----
 mm/page_cgroup.c                 |    5 +-
 mm/percpu.c                      |   39 +++++++-----
 mm/sparse-vmemmap.c              |    5 +-
 mm/sparse.c                      |   24 ++++----
 22 files changed, 284 insertions(+), 93 deletions(-)

Regards,
Santosh

[1] https://lkml.org/lkml/2013/6/29/77
[2] http://lwn.net/Articles/561854/
-- 
1.7.9.5

[RFC 20/23] mm/firmware: Use memblock apis for early memory allocations

[Santosh Shilimkar]

Switch to memblock interfaces for early memory allocator

Cc: Yinghai Lu <yinghai@kernel.org>
Cc: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>

Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
---
 drivers/firmware/memmap.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/firmware/memmap.c b/drivers/firmware/memmap.c
index e2e04b0..fa8a789 100644
--- a/drivers/firmware/memmap.c
+++ b/drivers/firmware/memmap.c
@@ -324,7 +324,7 @@ int __init firmware_map_add_early(u64 start, u64 end, const char *type)
 {
 	struct firmware_map_entry *entry;
 
-	entry = alloc_bootmem(sizeof(struct firmware_map_entry));
+	entry = memblock_early_alloc(sizeof(struct firmware_map_entry));
 	if (WARN_ON(!entry))
 		return -ENOMEM;
 
-- 
1.7.9.5

[RFC 06/23] mm/memblock: Add memblock early memory allocation apis

[Tejun Heo]

Hello,

On Sat, Oct 12, 2013 at 05:58:49PM -0400, Santosh Shilimkar wrote:
> Introduce memblock early memory allocation APIs which allow to support
> LPAE extension on 32 bits archs. More over, this is the next step

LPAE isn't something people outside arm circle would understand.
Let's stick to highmem.

> to get rid of NO_BOOTMEM memblock wrapper(nobootmem.c) and directly use
> memblock APIs.
> 
> The proposed interface will became active if both CONFIG_HAVE_MEMBLOCK
> and CONFIG_NO_BOOTMEM are specified by arch. In case !CONFIG_NO_BOOTMEM,
> the memblock() wrappers will fallback to the existing bootmem apis so
> that arch's noy converted to NO_BOOTMEM continue to work as is.
              ^^^
             typo

> +/* FIXME: Move to memblock.h at a point where we remove nobootmem.c */
> +void *memblock_early_alloc_try_nid_nopanic(int nid, phys_addr_t size,
> +		phys_addr_t align, phys_addr_t from, phys_addr_t max_addr);
> +void *memblock_early_alloc_try_nid(int nid, phys_addr_t size,
> +		phys_addr_t align, phys_addr_t from, phys_addr_t max_addr);

Wouldn't it make more sense to put @nid at the end.  @size is the main
parameter here and it gets confusing with _alloc_node() interface as
the positions of paramters change.  Plus, kmalloc_node() puts @node at
the end too.

> +void __memblock_free_early(phys_addr_t base, phys_addr_t size);
> +void __memblock_free_late(phys_addr_t base, phys_addr_t size);

Would it be possible to drop "early"?  It's redundant and makes the
function names unnecessarily long.  When memblock is enabled, these
are basically doing about the same thing as memblock_alloc() and
friends, right?  Wouldn't it make more sense to define these as
memblock_alloc_XXX()?

> +#define memblock_early_alloc(x) \
> +	memblock_early_alloc_try_nid(MAX_NUMNODES, x, SMP_CACHE_BYTES, \
> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_align(x, align) \
> +	memblock_early_alloc_try_nid(MAX_NUMNODES, x, align, \
> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_nopanic(x) \
> +	memblock_early_alloc_try_nid_nopanic(MAX_NUMNODES, x, SMP_CACHE_BYTES, \
> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_pages(x) \
> +	memblock_early_alloc_try_nid(MAX_NUMNODES, x, PAGE_SIZE, \
> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_pages_nopanic(x) \
> +	memblock_early_alloc_try_nid_nopanic(MAX_NUMNODES, x, PAGE_SIZE, \
> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)

I always felt a bit weird about _pages() interface.  It says pages but
takes bytes in size.  Maybe we're better off just converting the
current _pages users to _alloc_align()?

> +#define memblock_early_alloc_node(nid, x) \
> +	memblock_early_alloc_try_nid(nid, x, SMP_CACHE_BYTES, \
> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> +#define memblock_early_alloc_node_nopanic(nid, x) \
> +	memblock_early_alloc_try_nid_nopanic(nid, x, SMP_CACHE_BYTES, \
> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)

Ditto as above.  Maybe @nid can be moved to the end?

> +static void * __init _memblock_early_alloc_try_nid_nopanic(int nid,
> +				phys_addr_t size, phys_addr_t align,
> +				phys_addr_t from, phys_addr_t max_addr)
> +{
> +	phys_addr_t alloc;
> +	void *ptr;
> +
> +	if (WARN_ON_ONCE(slab_is_available())) {
> +		if (nid == MAX_NUMNODES)

Shouldn't we be using NUMA_NO_NODE?

> +			return kzalloc(size, GFP_NOWAIT);
> +		else
> +			return kzalloc_node(size, GFP_NOWAIT, nid);

And kzalloc_node() understands NUMA_NO_NODE.

> +	}
> +
> +	if (WARN_ON(!align))
> +		align = __alignof__(long long);

Wouldn't SMP_CACHE_BYTES make more sense?  Also, I'm not sure we
actually want WARN on it.  Interpreting 0 as "default align" isn't
that weird.

> +	/* align @size to avoid excessive fragmentation on reserved array */
> +	size = round_up(size, align);
> +
> +again:
> +	alloc = memblock_find_in_range_node(from, max_addr, size, align, nid);
> +	if (alloc)
> +		goto done;
> +
> +	if (nid != MAX_NUMNODES) {
> +		alloc =
> +			memblock_find_in_range_node(from, max_addr, size,
> +						    align, MAX_NUMNODES);
> +		if (alloc)
> +			goto done;
> +	}
> +
> +	if (from) {
> +		from = 0;
> +		goto again;
> +	} else {
> +		goto error;
> +	}
> +
> +done:
> +	memblock_reserve(alloc, size);
> +	ptr = phys_to_virt(alloc);
> +	memset(ptr, 0, size);

What if the address is high?  Don't we need kmapping here?

> +
> +	/*
> +	 * The min_count is set to 0 so that bootmem allocated blocks
> +	 * are never reported as leaks.
> +	 */
> +	kmemleak_alloc(ptr, size, 0, 0);
> +
> +	return ptr;
> +
> +error:
> +	return NULL;
> +}
> +
> +void * __init memblock_early_alloc_try_nid_nopanic(int nid,
> +				phys_addr_t size, phys_addr_t align,
> +				phys_addr_t from, phys_addr_t max_addr)
> +{
> +	memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n",
> +			__func__, (u64)size, (u64)align, nid, (u64)from,
> +			(u64)max_addr, (void *)_RET_IP_);
> +	return _memblock_early_alloc_try_nid_nopanic(nid, size,
> +						align, from, max_addr);

Do we need the extra level of wrapping?  Just implement
alloc_try_nid_nopanic() here and make the panicky version call it?

Thanks.

-- 
tejun

[RFC 06/23] mm/memblock: Add memblock early memory allocation apis

[Russell King - ARM Linux]

On Sun, Oct 13, 2013 at 01:56:48PM -0400, Tejun Heo wrote:
> Hello,
> 
> On Sat, Oct 12, 2013 at 05:58:49PM -0400, Santosh Shilimkar wrote:
> > Introduce memblock early memory allocation APIs which allow to support
> > LPAE extension on 32 bits archs. More over, this is the next step
> 
> LPAE isn't something people outside arm circle would understand.
> Let's stick to highmem.

LPAE != highmem.  Two totally different things, unless you believe
system memory always starts at physical address zero, which is very
far from the case on the majority of ARM platforms.

So replacing LPAE with "highmem" is pure misrepresentation and is
inaccurate.  PAE might be a better term, and is also the x86 term
for this.

[RFC 06/23] mm/memblock: Add memblock early memory allocation apis

[Tejun Heo]

On Sun, Oct 13, 2013 at 07:00:59PM +0100, Russell King - ARM Linux wrote:
> On Sun, Oct 13, 2013 at 01:56:48PM -0400, Tejun Heo wrote:
> > Hello,
> > 
> > On Sat, Oct 12, 2013 at 05:58:49PM -0400, Santosh Shilimkar wrote:
> > > Introduce memblock early memory allocation APIs which allow to support
> > > LPAE extension on 32 bits archs. More over, this is the next step
> > 
> > LPAE isn't something people outside arm circle would understand.
> > Let's stick to highmem.
> 
> LPAE != highmem.  Two totally different things, unless you believe
> system memory always starts at physical address zero, which is very
> far from the case on the majority of ARM platforms.
> 
> So replacing LPAE with "highmem" is pure misrepresentation and is
> inaccurate.  PAE might be a better term, and is also the x86 term
> for this.

Ah, right, forgot about the base address.  Let's please spell out the
requirements then.  Briefly explaining both aspects (non-zero base
addr & highmem) and why the existing bootmem based interfaced can't
serve them would be helpful to later readers.

Thanks.

-- 
tejun

[RFC 06/23] mm/memblock: Add memblock early memory allocation apis

[Santosh Shilimkar]

On Sunday 13 October 2013 02:42 PM, Tejun Heo wrote:
> On Sun, Oct 13, 2013 at 07:00:59PM +0100, Russell King - ARM Linux wrote:
>> On Sun, Oct 13, 2013 at 01:56:48PM -0400, Tejun Heo wrote:
>>> Hello,
>>>
>>> On Sat, Oct 12, 2013 at 05:58:49PM -0400, Santosh Shilimkar wrote:
>>>> Introduce memblock early memory allocation APIs which allow to support
>>>> LPAE extension on 32 bits archs. More over, this is the next step
>>>
>>> LPAE isn't something people outside arm circle would understand.
>>> Let's stick to highmem.
>>
>> LPAE != highmem.  Two totally different things, unless you believe
>> system memory always starts at physical address zero, which is very
>> far from the case on the majority of ARM platforms.
>>
thanks Russell for clarification.

>> So replacing LPAE with "highmem" is pure misrepresentation and is
>> inaccurate.  PAE might be a better term, and is also the x86 term
>> for this.
> 
> Ah, right, forgot about the base address.  Let's please spell out the
> requirements then.  Briefly explaining both aspects (non-zero base
> addr & highmem) and why the existing bootmem based interfaced can't
> serve them would be helpful to later readers.
> 
OK. Will try to describe bit more in the next version.Cover letter had
some of the information on the requirement which I will also
mention in the patch commit in next version.

Regards,
Santosh

[RFC 06/23] mm/memblock: Add memblock early memory allocation apis

[Santosh Shilimkar]

On Sunday 13 October 2013 01:56 PM, Tejun Heo wrote:
> Hello,
> 
> On Sat, Oct 12, 2013 at 05:58:49PM -0400, Santosh Shilimkar wrote:
>> Introduce memblock early memory allocation APIs which allow to support
>> LPAE extension on 32 bits archs. More over, this is the next step
> 

[..]

>> +/* FIXME: Move to memblock.h at a point where we remove nobootmem.c */
>> +void *memblock_early_alloc_try_nid_nopanic(int nid, phys_addr_t size,
>> +		phys_addr_t align, phys_addr_t from, phys_addr_t max_addr);
>> +void *memblock_early_alloc_try_nid(int nid, phys_addr_t size,
>> +		phys_addr_t align, phys_addr_t from, phys_addr_t max_addr);
> 
> Wouldn't it make more sense to put @nid at the end.  @size is the main
> parameter here and it gets confusing with _alloc_node() interface as
> the positions of paramters change.  Plus, kmalloc_node() puts @node at
> the end too.
> 
Ok. Will make @nid as a last parameter.

>> +void __memblock_free_early(phys_addr_t base, phys_addr_t size);
>> +void __memblock_free_late(phys_addr_t base, phys_addr_t size);
> 
> Would it be possible to drop "early"?  It's redundant and makes the
> function names unnecessarily long.  When memblock is enabled, these
> are basically doing about the same thing as memblock_alloc() and
> friends, right?  Wouldn't it make more sense to define these as
> memblock_alloc_XXX()?
> 
A small a difference w.r.t existing memblock_alloc() vs these new
exports returns virtual mapped memory pointers. Actually I started
with memblock_alloc_xxx() but then memblock already exports memblock_alloc_xx()
returning physical memory pointer. So just wanted to make these interfaces
distinct and added "early". But I agree with you that the 'early' can
be dropped. Will fix it.

>> +#define memblock_early_alloc(x) \
>> +	memblock_early_alloc_try_nid(MAX_NUMNODES, x, SMP_CACHE_BYTES, \
>> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
>> +#define memblock_early_alloc_align(x, align) \
>> +	memblock_early_alloc_try_nid(MAX_NUMNODES, x, align, \
>> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
>> +#define memblock_early_alloc_nopanic(x) \
>> +	memblock_early_alloc_try_nid_nopanic(MAX_NUMNODES, x, SMP_CACHE_BYTES, \
>> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
>> +#define memblock_early_alloc_pages(x) \
>> +	memblock_early_alloc_try_nid(MAX_NUMNODES, x, PAGE_SIZE, \
>> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
>> +#define memblock_early_alloc_pages_nopanic(x) \
>> +	memblock_early_alloc_try_nid_nopanic(MAX_NUMNODES, x, PAGE_SIZE, \
>> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> 
> I always felt a bit weird about _pages() interface.  It says pages but
> takes bytes in size.  Maybe we're better off just converting the
> current _pages users to _alloc_align()?
> 
I thought the pages interfaces are more for asking the memory
allocations which are page aligned. So yes, we could convert
these users to make use of align interfaces.


>> +#define memblock_early_alloc_node(nid, x) \
>> +	memblock_early_alloc_try_nid(nid, x, SMP_CACHE_BYTES, \
>> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
>> +#define memblock_early_alloc_node_nopanic(nid, x) \
>> +	memblock_early_alloc_try_nid_nopanic(nid, x, SMP_CACHE_BYTES, \
>> +			BOOTMEM_LOW_LIMIT, BOOTMEM_ALLOC_ACCESSIBLE)
> 
> Ditto as above.  Maybe @nid can be moved to the end?
>
ok
 
>> +static void * __init _memblock_early_alloc_try_nid_nopanic(int nid,
>> +				phys_addr_t size, phys_addr_t align,
>> +				phys_addr_t from, phys_addr_t max_addr)
>> +{
>> +	phys_addr_t alloc;
>> +	void *ptr;
>> +
>> +	if (WARN_ON_ONCE(slab_is_available())) {
>> +		if (nid == MAX_NUMNODES)
> 
> Shouldn't we be using NUMA_NO_NODE?
> 
>> +			return kzalloc(size, GFP_NOWAIT);
>> +		else
>> +			return kzalloc_node(size, GFP_NOWAIT, nid);
> 
> And kzalloc_node() understands NUMA_NO_NODE.
> 
Will try this out.

>> +	}
>> +
>> +	if (WARN_ON(!align))
>> +		align = __alignof__(long long);
> 
> Wouldn't SMP_CACHE_BYTES make more sense?  Also, I'm not sure we
> actually want WARN on it.  Interpreting 0 as "default align" isn't
> that weird.
> 
Will drop that WARN and use SMP_CACHE_BYTES as a default.


>> +	/* align @size to avoid excessive fragmentation on reserved array */
>> +	size = round_up(size, align);
>> +
>> +again:
>> +	alloc = memblock_find_in_range_node(from, max_addr, size, align, nid);
>> +	if (alloc)
>> +		goto done;
>> +
>> +	if (nid != MAX_NUMNODES) {
>> +		alloc =
>> +			memblock_find_in_range_node(from, max_addr, size,
>> +						    align, MAX_NUMNODES);
>> +		if (alloc)
>> +			goto done;
>> +	}
>> +
>> +	if (from) {
>> +		from = 0;
>> +		goto again;
>> +	} else {
>> +		goto error;
>> +	}
>> +
>> +done:
>> +	memblock_reserve(alloc, size);
>> +	ptr = phys_to_virt(alloc);
>> +	memset(ptr, 0, size);
> 
> What if the address is high?  Don't we need kmapping here?
>
The current nobootmem code actually don't handle the high
addresses since the max memory is limited by memblock.current_limit
which is max_low_pfn. So I am assuming we don't need to support
it. __alloc_bootmem_node_high() interface underneath uses
__alloc_memory_core_early() and we tried to keep the same
functionality in new code.
 
>> +
>> +	/*
>> +	 * The min_count is set to 0 so that bootmem allocated blocks
>> +	 * are never reported as leaks.
>> +	 */
>> +	kmemleak_alloc(ptr, size, 0, 0);
>> +
>> +	return ptr;
>> +
>> +error:
>> +	return NULL;
>> +}
>> +
>> +void * __init memblock_early_alloc_try_nid_nopanic(int nid,
>> +				phys_addr_t size, phys_addr_t align,
>> +				phys_addr_t from, phys_addr_t max_addr)
>> +{
>> +	memblock_dbg("%s: %llu bytes align=0x%llx nid=%d from=0x%llx max_addr=0x%llx %pF\n",
>> +			__func__, (u64)size, (u64)align, nid, (u64)from,
>> +			(u64)max_addr, (void *)_RET_IP_);
>> +	return _memblock_early_alloc_try_nid_nopanic(nid, size,
>> +						align, from, max_addr);
> 
> Do we need the extra level of wrapping?  Just implement
> alloc_try_nid_nopanic() here and make the panicky version call it?
> 
It was useful to have caller information (_RET_IP_) for debug. But
it can be dropped if you insist.

Regards,
Santosh

[RFC 06/23] mm/memblock: Add memblock early memory allocation apis

[Tejun Heo]

Hello,

On Mon, Oct 14, 2013 at 10:39:54AM -0400, Santosh Shilimkar wrote:
> >> +void __memblock_free_early(phys_addr_t base, phys_addr_t size);
> >> +void __memblock_free_late(phys_addr_t base, phys_addr_t size);
> > 
> > Would it be possible to drop "early"?  It's redundant and makes the
> > function names unnecessarily long.  When memblock is enabled, these
> > are basically doing about the same thing as memblock_alloc() and
> > friends, right?  Wouldn't it make more sense to define these as
> > memblock_alloc_XXX()?
> > 
> A small a difference w.r.t existing memblock_alloc() vs these new
> exports returns virtual mapped memory pointers. Actually I started
> with memblock_alloc_xxx() but then memblock already exports memblock_alloc_xx()
> returning physical memory pointer. So just wanted to make these interfaces
> distinct and added "early". But I agree with you that the 'early' can
> be dropped. Will fix it.

Hmmm, so while this removes address limit on the base / limit side, it
keeps virt address on the result.  In that case, we probably want to
somehow distinguish the two sets of interfaces - one set dealing with
phys and the other dealing with virts.  Maybe we want to build the
base interface on phys address and add convenience wrappers for virts?
Would that make more sense?

Thanks.

-- 
tejun

[RFC 06/23] mm/memblock: Add memblock early memory allocation apis

[Santosh Shilimkar]

On Monday 14 October 2013 10:58 AM, Tejun Heo wrote:
> Hello,
> 
> On Mon, Oct 14, 2013 at 10:39:54AM -0400, Santosh Shilimkar wrote:
>>>> +void __memblock_free_early(phys_addr_t base, phys_addr_t size);
>>>> +void __memblock_free_late(phys_addr_t base, phys_addr_t size);
>>>
>>> Would it be possible to drop "early"?  It's redundant and makes the
>>> function names unnecessarily long.  When memblock is enabled, these
>>> are basically doing about the same thing as memblock_alloc() and
>>> friends, right?  Wouldn't it make more sense to define these as
>>> memblock_alloc_XXX()?
>>>
>> A small a difference w.r.t existing memblock_alloc() vs these new
>> exports returns virtual mapped memory pointers. Actually I started
>> with memblock_alloc_xxx() but then memblock already exports memblock_alloc_xx()
>> returning physical memory pointer. So just wanted to make these interfaces
>> distinct and added "early". But I agree with you that the 'early' can
>> be dropped. Will fix it.
> 
> Hmmm, so while this removes address limit on the base / limit side, it
> keeps virt address on the result.  In that case, we probably want to
> somehow distinguish the two sets of interfaces - one set dealing with
> phys and the other dealing with virts.  Maybe we want to build the
> base interface on phys address and add convenience wrappers for virts?
> Would that make more sense?
> 
Thats what more or less we are doing if you look at it. The only
additional code we have is to manage the virtual memory and checks
as such, just the same way initially done in nobootmem.c wrappers.

Not sure if adding 'virt' word in these APIs to make it explicit
would help to avoid any confusion.

Regards,
Santosh

[RFC 07/23] mm/memblock: debug: correct displaying of upper memory boundary

[Tejun Heo]

On Sat, Oct 12, 2013 at 05:58:50PM -0400, Santosh Shilimkar wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> When debugging is enabled (cmdline has "memblock=debug") the memblock
> will display upper memory boundary per each allocated/freed memory range
> wrongly. For example:
>  memblock_reserve: [0x0000009e7e8000-0x0000009e7ed000] _memblock_early_alloc_try_nid_nopanic+0xfc/0x12c
> 
> The 0x0000009e7ed000 is displayed instead of 0x0000009e7ecfff
> 
> Hence, correct this by changing formula used to calculate upper memory
> boundary to (u64)base + size - 1 instead of  (u64)base + size everywhere
> in the debug messages.

I kinda prefer base + size because it's easier to actually know the
size but yeah, it should have been [base, base + size) and other
places use base + size - 1 notation so it probably is better to stick
to that.  Maybe move this one to the beginning of the series?

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

[RFC 07/23] mm/memblock: debug: correct displaying of upper memory boundary

[Santosh Shilimkar]

On Sunday 13 October 2013 02:02 PM, Tejun Heo wrote:
> On Sat, Oct 12, 2013 at 05:58:50PM -0400, Santosh Shilimkar wrote:
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> When debugging is enabled (cmdline has "memblock=debug") the memblock
>> will display upper memory boundary per each allocated/freed memory range
>> wrongly. For example:
>>  memblock_reserve: [0x0000009e7e8000-0x0000009e7ed000] _memblock_early_alloc_try_nid_nopanic+0xfc/0x12c
>>
>> The 0x0000009e7ed000 is displayed instead of 0x0000009e7ecfff
>>
>> Hence, correct this by changing formula used to calculate upper memory
>> boundary to (u64)base + size - 1 instead of  (u64)base + size everywhere
>> in the debug messages.
> 
> I kinda prefer base + size because it's easier to actually know the
> size but yeah, it should have been [base, base + size) and other
> places use base + size - 1 notation so it probably is better to stick
> to that.  Maybe move this one to the beginning of the series?
> 
> Acked-by: Tejun Heo <tj@kernel.org>
> 
Thanks. Will do

[RFC 08/23] mm/memblock: debug: don't free reserved array if !ARCH_DISCARD_MEMBLOCK

[Tejun Heo]

On Sat, Oct 12, 2013 at 05:58:51PM -0400, Santosh Shilimkar wrote:
> From: Grygorii Strashko <grygorii.strashko@ti.com>
> 
> Now the Nobootmem allocator will always try to free memory allocated for
> reserved memory regions (free_low_memory_core_early()) without taking
> into to account current memblock debugging configuration
> (CONFIG_ARCH_DISCARD_MEMBLOCK and CONFIG_DEBUG_FS state).
> As result if:
>  - CONFIG_DEBUG_FS defined
>  - CONFIG_ARCH_DISCARD_MEMBLOCK not defined;
> -  reserved memory regions array have been resized during boot
> 
> then:
> - memory allocated for reserved memory regions array will be freed to
> buddy allocator;
> - debug_fs entry "sys/kernel/debug/memblock/reserved" will show garbage
> instead of state of memory reservations. like:
>    0: 0x98393bc0..0x9a393bbf
>    1: 0xff120000..0xff11ffff
>    2: 0x00000000..0xffffffff
> 
> Hence, do not free memory allocated for reserved memory regions if
> defined(CONFIG_DEBUG_FS) && !defined(CONFIG_ARCH_DISCARD_MEMBLOCK).
> 
> Cc: Yinghai Lu <yinghai@kernel.org>
> Cc: Tejun Heo <tj@kernel.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> 
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> ---
>  mm/memblock.c |    4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/mm/memblock.c b/mm/memblock.c
> index d903138..1bb2cc0 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -169,6 +169,10 @@ phys_addr_t __init_memblock get_allocated_memblock_reserved_regions_info(
>  	if (memblock.reserved.regions == memblock_reserved_init_regions)
>  		return 0;
>  

Please add comment explaining why the following test exists.  It's
pretty difficult to deduce the reason only from the code.

> +	if (IS_ENABLED(CONFIG_DEBUG_FS) &&
> +	    !IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
> +		return 0;
> +

Also, as this is another fix patch, can you please move this to the
head of the series?

Thanks.

-- 
tejun

[RFC 08/23] mm/memblock: debug: don't free reserved array if !ARCH_DISCARD_MEMBLOCK

[Santosh Shilimkar]

On Sunday 13 October 2013 03:51 PM, Tejun Heo wrote:
> On Sat, Oct 12, 2013 at 05:58:51PM -0400, Santosh Shilimkar wrote:
>> From: Grygorii Strashko <grygorii.strashko@ti.com>
>>
>> Now the Nobootmem allocator will always try to free memory allocated for
>> reserved memory regions (free_low_memory_core_early()) without taking
>> into to account current memblock debugging configuration
>> (CONFIG_ARCH_DISCARD_MEMBLOCK and CONFIG_DEBUG_FS state).
>> As result if:
>>  - CONFIG_DEBUG_FS defined
>>  - CONFIG_ARCH_DISCARD_MEMBLOCK not defined;
>> -  reserved memory regions array have been resized during boot
>>
>> then:
>> - memory allocated for reserved memory regions array will be freed to
>> buddy allocator;
>> - debug_fs entry "sys/kernel/debug/memblock/reserved" will show garbage
>> instead of state of memory reservations. like:
>>    0: 0x98393bc0..0x9a393bbf
>>    1: 0xff120000..0xff11ffff
>>    2: 0x00000000..0xffffffff
>>
>> Hence, do not free memory allocated for reserved memory regions if
>> defined(CONFIG_DEBUG_FS) && !defined(CONFIG_ARCH_DISCARD_MEMBLOCK).
>>
>> Cc: Yinghai Lu <yinghai@kernel.org>
>> Cc: Tejun Heo <tj@kernel.org>
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
>> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
>> ---
>>  mm/memblock.c |    4 ++++
>>  1 file changed, 4 insertions(+)
>>
>> diff --git a/mm/memblock.c b/mm/memblock.c
>> index d903138..1bb2cc0 100644
>> --- a/mm/memblock.c
>> +++ b/mm/memblock.c
>> @@ -169,6 +169,10 @@ phys_addr_t __init_memblock get_allocated_memblock_reserved_regions_info(
>>  	if (memblock.reserved.regions == memblock_reserved_init_regions)
>>  		return 0;
>>  
> 
> Please add comment explaining why the following test exists.  It's
> pretty difficult to deduce the reason only from the code.
> 
ok.

>> +	if (IS_ENABLED(CONFIG_DEBUG_FS) &&
>> +	    !IS_ENABLED(CONFIG_ARCH_DISCARD_MEMBLOCK))
>> +		return 0;
>> +
> 
> Also, as this is another fix patch, can you please move this to the
> head of the series?
> 
Sure

[RFC 09/23] mm/init: Use memblock apis for early memory allocations

[Tejun Heo]

On Sat, Oct 12, 2013 at 05:58:52PM -0400, Santosh Shilimkar wrote:
> Switch to memblock interfaces for early memory allocator

When posting actual (non-RFC) patches later, please cc the maintainers
of the target subsystem and briefly explain why the new interface is
needed and that this doesn't change visible behavior.

Thanks.

-- 
tejun

[RFC 09/23] mm/init: Use memblock apis for early memory allocations

[Santosh Shilimkar]

On Sunday 13 October 2013 03:54 PM, Tejun Heo wrote:
> On Sat, Oct 12, 2013 at 05:58:52PM -0400, Santosh Shilimkar wrote:
>> Switch to memblock interfaces for early memory allocator
> 
> When posting actual (non-RFC) patches later, please cc the maintainers
> of the target subsystem and briefly explain why the new interface is
> needed and that this doesn't change visible behavior.
> 
Sure. Thanks a lot for quick response on the series. I will give another
week or so to see if there are more comments and then start addressing
comments in next version.

Regards,
Santosh