While working on the conversion of the s390 port to use memblock and
nobootmem instead of bootmem I discovered two small bugs:
alloc_memory_core_early() in mm/nobootmem.c called memblock_reserve()
without forwarding the return value of memblock_reserve().
free_low_memory_core() (used by free_all_bootmem) in mm/nobootmem.c
already took care of releasing the memblock.reserved array in case
it has been allocated using memblock itself. This behaviour was
missing for memblock.memory.
Cases where memblock.memory grows bigger than the initial 128 entries
have been seen. So this should be supported as well.
Philipp Hachtmann (2):
mm, nobootmem: Add return value check in __alloc_memory_core_early()
mm: free memblock.memory in free_all_bootmem
include/linux/memblock.h | 1 +
mm/memblock.c | 12 ++++++++++++
mm/nobootmem.c | 11 +++++++++--
3 files changed, 22 insertions(+), 2 deletions(-)
--
1.8.4.5
When memblock_reserve() fails because memblock.reserved.regions cannot
be resized, the caller (e.g. alloc_bootmem()) is not informed of the
failed allocation. Therefore alloc_bootmem() silently returns the same
pointer again and again.
This patch adds a check for the return value of memblock_reserve() in
__alloc_memory_core().
Signed-off-by: Philipp Hachtmann <[email protected]>
---
mm/nobootmem.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/mm/nobootmem.c b/mm/nobootmem.c
index 2c254d3..3a7e14d 100644
--- a/mm/nobootmem.c
+++ b/mm/nobootmem.c
@@ -45,7 +45,9 @@ static void * __init __alloc_memory_core_early(int nid, u64 size, u64 align,
if (!addr)
return NULL;
- memblock_reserve(addr, size);
+ if (memblock_reserve(addr, size))
+ return NULL;
+
ptr = phys_to_virt(addr);
memset(ptr, 0, size);
/*
--
1.8.4.5
When calling free_all_bootmem() the free areas under memblock's
control are released to the buddy allocator. Additionally the
reserved list is freed if it was reallocated by memblock.
The same should apply for the memory list.
Signed-off-by: Philipp Hachtmann <[email protected]>
---
include/linux/memblock.h | 1 +
mm/memblock.c | 12 ++++++++++++
mm/nobootmem.c | 7 ++++++-
3 files changed, 19 insertions(+), 1 deletion(-)
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 77c60e5..d174922 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -52,6 +52,7 @@ phys_addr_t memblock_find_in_range_node(phys_addr_t start, phys_addr_t end,
phys_addr_t memblock_find_in_range(phys_addr_t start, phys_addr_t end,
phys_addr_t size, phys_addr_t align);
phys_addr_t get_allocated_memblock_reserved_regions_info(phys_addr_t *addr);
+phys_addr_t get_allocated_memblock_memory_regions_info(phys_addr_t *addr);
void memblock_allow_resize(void);
int memblock_add_node(phys_addr_t base, phys_addr_t size, int nid);
int memblock_add(phys_addr_t base, phys_addr_t size);
diff --git a/mm/memblock.c b/mm/memblock.c
index 53e477b..1a11d04 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -271,6 +271,18 @@ phys_addr_t __init_memblock get_allocated_memblock_reserved_regions_info(
memblock.reserved.max);
}
+phys_addr_t __init_memblock get_allocated_memblock_memory_regions_info(
+ phys_addr_t *addr)
+{
+ if (memblock.memory.regions == memblock_memory_init_regions)
+ return 0;
+
+ *addr = __pa(memblock.memory.regions);
+
+ return PAGE_ALIGN(sizeof(struct memblock_region) *
+ memblock.memory.max);
+}
+
/**
* memblock_double_array - double the size of the memblock regions array
* @type: memblock type of the regions array being doubled
diff --git a/mm/nobootmem.c b/mm/nobootmem.c
index 3a7e14d..83f36d3 100644
--- a/mm/nobootmem.c
+++ b/mm/nobootmem.c
@@ -122,11 +122,16 @@ static unsigned long __init free_low_memory_core_early(void)
for_each_free_mem_range(i, MAX_NUMNODES, &start, &end, NULL)
count += __free_memory_core(start, end);
- /* free range that is used for reserved array if we allocate it */
+ /* Free memblock.reserved array if it was allocated */
size = get_allocated_memblock_reserved_regions_info(&start);
if (size)
count += __free_memory_core(start, start + size);
+ /* Free memblock.memory array if it was allocated */
+ size = get_allocated_memblock_memory_regions_info(&start);
+ if (size)
+ count += __free_memory_core(start, start + size);
+
return count;
}
--
1.8.4.5
On Tue, Jan 07, 2014 at 04:16:13PM +0100, Philipp Hachtmann wrote:
> When memblock_reserve() fails because memblock.reserved.regions cannot
> be resized, the caller (e.g. alloc_bootmem()) is not informed of the
> failed allocation. Therefore alloc_bootmem() silently returns the same
> pointer again and again.
> This patch adds a check for the return value of memblock_reserve() in
> __alloc_memory_core().
>
> Signed-off-by: Philipp Hachtmann <[email protected]>
Reviewed-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
On Tue, Jan 07, 2014 at 04:16:14PM +0100, Philipp Hachtmann wrote:
> When calling free_all_bootmem() the free areas under memblock's
> control are released to the buddy allocator. Additionally the
> reserved list is freed if it was reallocated by memblock.
> The same should apply for the memory list.
>
> Signed-off-by: Philipp Hachtmann <[email protected]>
Reviewed-by: Tejun Heo <[email protected]>
Thanks.
--
tejun
On 2014/1/7 23:16, Philipp Hachtmann wrote:
> When calling free_all_bootmem() the free areas under memblock's
> control are released to the buddy allocator. Additionally the
> reserved list is freed if it was reallocated by memblock.
> The same should apply for the memory list.
>
> Signed-off-by: Philipp Hachtmann <[email protected]>
> ---
> include/linux/memblock.h | 1 +
> mm/memblock.c | 12 ++++++++++++
> mm/nobootmem.c | 7 ++++++-
> 3 files changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 77c60e5..d174922 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -52,6 +52,7 @@ phys_addr_t memblock_find_in_range_node(phys_addr_t start, phys_addr_t end,
> phys_addr_t memblock_find_in_range(phys_addr_t start, phys_addr_t end,
> phys_addr_t size, phys_addr_t align);
> phys_addr_t get_allocated_memblock_reserved_regions_info(phys_addr_t *addr);
> +phys_addr_t get_allocated_memblock_memory_regions_info(phys_addr_t *addr);
> void memblock_allow_resize(void);
> int memblock_add_node(phys_addr_t base, phys_addr_t size, int nid);
> int memblock_add(phys_addr_t base, phys_addr_t size);
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 53e477b..1a11d04 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -271,6 +271,18 @@ phys_addr_t __init_memblock get_allocated_memblock_reserved_regions_info(
> memblock.reserved.max);
> }
>
> +phys_addr_t __init_memblock get_allocated_memblock_memory_regions_info(
> + phys_addr_t *addr)
> +{
> + if (memblock.memory.regions == memblock_memory_init_regions)
> + return 0;
> +
> + *addr = __pa(memblock.memory.regions);
> +
> + return PAGE_ALIGN(sizeof(struct memblock_region) *
> + memblock.memory.max);
> +}
> +
> /**
> * memblock_double_array - double the size of the memblock regions array
> * @type: memblock type of the regions array being doubled
> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> index 3a7e14d..83f36d3 100644
> --- a/mm/nobootmem.c
> +++ b/mm/nobootmem.c
> @@ -122,11 +122,16 @@ static unsigned long __init free_low_memory_core_early(void)
> for_each_free_mem_range(i, MAX_NUMNODES, &start, &end, NULL)
> count += __free_memory_core(start, end);
>
> - /* free range that is used for reserved array if we allocate it */
> + /* Free memblock.reserved array if it was allocated */
> size = get_allocated_memblock_reserved_regions_info(&start);
> if (size)
> count += __free_memory_core(start, start + size);
>
> + /* Free memblock.memory array if it was allocated */
> + size = get_allocated_memblock_memory_regions_info(&start);
> + if (size)
> + count += __free_memory_core(start, start + size);
> +
Hi Philipp,
For some archs, like arm64, would use memblock.memory after system booting,
so we can not simply released to the buddy allocator, maybe need !defined(CONFIG_ARCH_DISCARD_MEMBLOCK).
#ifdef CONFIG_HAVE_ARCH_PFN_VALID
int pfn_valid(unsigned long pfn)
{
return memblock_is_memory(pfn << PAGE_SHIFT);
}
EXPORT_SYMBOL(pfn_valid);
Thanks,
Jianguo Wu
> return count;
> }
>
Am Wed, 8 Jan 2014 12:08:04 +0800
schrieb Jianguo Wu <[email protected]>:
> For some archs, like arm64, would use memblock.memory after system
> booting, so we can not simply released to the buddy allocator, maybe
> need !defined(CONFIG_ARCH_DISCARD_MEMBLOCK).
Oh, I see. I have added some ifdefs to prevent memblock.memory from
being freed when CONFIG_ARCH_DISCARD_MEMBLOCK is not set.
Here is a replacement for the patch.
Kind regards
Philipp
>From aca95bcb9d79388b68bf18e7bae4353259b6758f Mon Sep 17 00:00:00 2001
From: Philipp Hachtmann <[email protected]>
Date: Thu, 19 Dec 2013 15:53:46 +0100
Subject: [PATCH 2/2] mm: free memblock.memory in free_all_bootmem
When calling free_all_bootmem() the free areas under memblock's
control are released to the buddy allocator. Additionally the
reserved list is freed if it was reallocated by memblock.
The same should apply for the memory list.
Signed-off-by: Philipp Hachtmann <[email protected]>
---
include/linux/memblock.h | 1 +
mm/memblock.c | 16 ++++++++++++++++
mm/nobootmem.c | 10 +++++++++-
3 files changed, 26 insertions(+), 1 deletion(-)
diff --git a/include/linux/memblock.h b/include/linux/memblock.h
index 77c60e5..d174922 100644
--- a/include/linux/memblock.h
+++ b/include/linux/memblock.h
@@ -52,6 +52,7 @@ phys_addr_t memblock_find_in_range_node(phys_addr_t start, phys_addr_t end,
phys_addr_t memblock_find_in_range(phys_addr_t start, phys_addr_t end,
phys_addr_t size, phys_addr_t align);
phys_addr_t get_allocated_memblock_reserved_regions_info(phys_addr_t *addr);
+phys_addr_t get_allocated_memblock_memory_regions_info(phys_addr_t *addr);
void memblock_allow_resize(void);
int memblock_add_node(phys_addr_t base, phys_addr_t size, int nid);
int memblock_add(phys_addr_t base, phys_addr_t size);
diff --git a/mm/memblock.c b/mm/memblock.c
index 53e477b..a78b2e9 100644
--- a/mm/memblock.c
+++ b/mm/memblock.c
@@ -271,6 +271,22 @@ phys_addr_t __init_memblock get_allocated_memblock_reserved_regions_info(
memblock.reserved.max);
}
+#ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
+
+phys_addr_t __init_memblock get_allocated_memblock_memory_regions_info(
+ phys_addr_t *addr)
+{
+ if (memblock.memory.regions == memblock_memory_init_regions)
+ return 0;
+
+ *addr = __pa(memblock.memory.regions);
+
+ return PAGE_ALIGN(sizeof(struct memblock_region) *
+ memblock.memory.max);
+}
+
+#endif
+
/**
* memblock_double_array - double the size of the memblock regions array
* @type: memblock type of the regions array being doubled
diff --git a/mm/nobootmem.c b/mm/nobootmem.c
index 3a7e14d..63ff3f6 100644
--- a/mm/nobootmem.c
+++ b/mm/nobootmem.c
@@ -122,11 +122,19 @@ static unsigned long __init free_low_memory_core_early(void)
for_each_free_mem_range(i, MAX_NUMNODES, &start, &end, NULL)
count += __free_memory_core(start, end);
- /* free range that is used for reserved array if we allocate it */
+ /* Free memblock.reserved array if it was allocated */
size = get_allocated_memblock_reserved_regions_info(&start);
if (size)
count += __free_memory_core(start, start + size);
+#ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
+
+ /* Free memblock.memory array if it was allocated */
+ size = get_allocated_memblock_memory_regions_info(&start);
+ if (size)
+ count += __free_memory_core(start, start + size);
+#endif
+
return count;
}
--
1.8.4.5
On Wed, Jan 8, 2014 at 5:42 AM, Philipp Hachtmann
<[email protected]> wrote:
> Am Wed, 8 Jan 2014 12:08:04 +0800
> schrieb Jianguo Wu <[email protected]>:
>
>> For some archs, like arm64, would use memblock.memory after system
>> booting, so we can not simply released to the buddy allocator, maybe
>> need !defined(CONFIG_ARCH_DISCARD_MEMBLOCK).
>
> Oh, I see. I have added some ifdefs to prevent memblock.memory from
> being freed when CONFIG_ARCH_DISCARD_MEMBLOCK is not set.
>
> Here is a replacement for the patch.
>
> Kind regards
>
> Philipp
>
> From aca95bcb9d79388b68bf18e7bae4353259b6758f Mon Sep 17 00:00:00 2001
> From: Philipp Hachtmann <[email protected]>
> Date: Thu, 19 Dec 2013 15:53:46 +0100
> Subject: [PATCH 2/2] mm: free memblock.memory in free_all_bootmem
>
> When calling free_all_bootmem() the free areas under memblock's
> control are released to the buddy allocator. Additionally the
> reserved list is freed if it was reallocated by memblock.
> The same should apply for the memory list.
>
> Signed-off-by: Philipp Hachtmann <[email protected]>
> ---
> include/linux/memblock.h | 1 +
> mm/memblock.c | 16 ++++++++++++++++
> mm/nobootmem.c | 10 +++++++++-
> 3 files changed, 26 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> index 77c60e5..d174922 100644
> --- a/include/linux/memblock.h
> +++ b/include/linux/memblock.h
> @@ -52,6 +52,7 @@ phys_addr_t memblock_find_in_range_node(phys_addr_t start, phys_addr_t end,
> phys_addr_t memblock_find_in_range(phys_addr_t start, phys_addr_t end,
> phys_addr_t size, phys_addr_t align);
> phys_addr_t get_allocated_memblock_reserved_regions_info(phys_addr_t *addr);
> +phys_addr_t get_allocated_memblock_memory_regions_info(phys_addr_t *addr);
> void memblock_allow_resize(void);
> int memblock_add_node(phys_addr_t base, phys_addr_t size, int nid);
> int memblock_add(phys_addr_t base, phys_addr_t size);
> diff --git a/mm/memblock.c b/mm/memblock.c
> index 53e477b..a78b2e9 100644
> --- a/mm/memblock.c
> +++ b/mm/memblock.c
> @@ -271,6 +271,22 @@ phys_addr_t __init_memblock get_allocated_memblock_reserved_regions_info(
> memblock.reserved.max);
> }
>
> +#ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
> +
> +phys_addr_t __init_memblock get_allocated_memblock_memory_regions_info(
> + phys_addr_t *addr)
> +{
> + if (memblock.memory.regions == memblock_memory_init_regions)
> + return 0;
> +
> + *addr = __pa(memblock.memory.regions);
> +
> + return PAGE_ALIGN(sizeof(struct memblock_region) *
> + memblock.memory.max);
> +}
> +
> +#endif
> +
> /**
> * memblock_double_array - double the size of the memblock regions array
> * @type: memblock type of the regions array being doubled
> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> index 3a7e14d..63ff3f6 100644
> --- a/mm/nobootmem.c
> +++ b/mm/nobootmem.c
> @@ -122,11 +122,19 @@ static unsigned long __init free_low_memory_core_early(void)
> for_each_free_mem_range(i, MAX_NUMNODES, &start, &end, NULL)
> count += __free_memory_core(start, end);
>
> - /* free range that is used for reserved array if we allocate it */
> + /* Free memblock.reserved array if it was allocated */
> size = get_allocated_memblock_reserved_regions_info(&start);
> if (size)
> count += __free_memory_core(start, start + size);
>
> +#ifdef CONFIG_ARCH_DISCARD_MEMBLOCK
> +
> + /* Free memblock.memory array if it was allocated */
> + size = get_allocated_memblock_memory_regions_info(&start);
> + if (size)
> + count += __free_memory_core(start, start + size);
> +#endif
> +
> return count;
> }
I sent similar before.
http://www.gossamer-threads.com/lists/engine?do=post_attachment;postatt_id=49616;list=linux
http://www.gossamer-threads.com/lists/linux/kernel/1556026?do=post_view_threaded#1556026
Also for arches that do not free memblock, do they still need to access
memblock.reserved.regions ?
Yinghai