max_low_pfn reflect the number of _pages_ in the system,
not the maximum PFN. You can easily find that fact in init_bootmem().
So fix it.
Additionally, if 'start_pfn == end_pfn', we don't need to go futher,
so change range check.
Signed-off-by: Joonsoo Kim <[email protected]>
diff --git a/mm/nobootmem.c b/mm/nobootmem.c
index 5e07d36..4711e91 100644
--- a/mm/nobootmem.c
+++ b/mm/nobootmem.c
@@ -110,9 +110,9 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
{
unsigned long start_pfn = PFN_UP(start);
unsigned long end_pfn = min_t(unsigned long,
- PFN_DOWN(end), max_low_pfn);
+ PFN_DOWN(end), min_low_pfn);
- if (start_pfn > end_pfn)
+ if (start_pfn >= end_pfn)
return 0;
__free_pages_memory(start_pfn, end_pfn);
--
1.7.9.5
Remove unused argument and make function static,
because there is no user outside of nobootmem.c
Signed-off-by: Joonsoo Kim <[email protected]>
diff --git a/include/linux/bootmem.h b/include/linux/bootmem.h
index cdc3bab..5f0b0e1 100644
--- a/include/linux/bootmem.h
+++ b/include/linux/bootmem.h
@@ -44,7 +44,6 @@ extern unsigned long init_bootmem_node(pg_data_t *pgdat,
unsigned long endpfn);
extern unsigned long init_bootmem(unsigned long addr, unsigned long memend);
-extern unsigned long free_low_memory_core_early(int nodeid);
extern unsigned long free_all_bootmem_node(pg_data_t *pgdat);
extern unsigned long free_all_bootmem(void);
diff --git a/mm/nobootmem.c b/mm/nobootmem.c
index 4711e91..589c673 100644
--- a/mm/nobootmem.c
+++ b/mm/nobootmem.c
@@ -120,7 +120,7 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
return end_pfn - start_pfn;
}
-unsigned long __init free_low_memory_core_early(int nodeid)
+static unsigned long __init free_low_memory_core_early()
{
unsigned long count = 0;
phys_addr_t start, end, size;
@@ -170,7 +170,7 @@ unsigned long __init free_all_bootmem(void)
* because in some case like Node0 doesn't have RAM installed
* low ram will be on Node1
*/
- return free_low_memory_core_early(MAX_NUMNODES);
+ return free_low_memory_core_early();
}
/**
--
1.7.9.5
Currently, we do memset() before reserving the area.
This may not cause any problem, but it is somewhat weird.
So change execution order.
Signed-off-by: Joonsoo Kim <[email protected]>
diff --git a/mm/nobootmem.c b/mm/nobootmem.c
index 589c673..f11ec1c 100644
--- a/mm/nobootmem.c
+++ b/mm/nobootmem.c
@@ -46,8 +46,8 @@ static void * __init __alloc_memory_core_early(int nid, u64 size, u64 align,
return NULL;
ptr = phys_to_virt(addr);
- memset(ptr, 0, size);
memblock_reserve(addr, size);
+ memset(ptr, 0, size);
/*
* The min_count is set to 0 so that bootmem allocated blocks
* are never reported as leaks.
--
1.7.9.5
On Mon, Mar 18, 2013 at 10:15 PM, Joonsoo Kim <[email protected]> wrote:
> max_low_pfn reflect the number of _pages_ in the system,
> not the maximum PFN. You can easily find that fact in init_bootmem().
> So fix it.
I'm confused. for x86, we have max_low_pfn defined in ...
#ifdef CONFIG_X86_32
/* max_low_pfn get updated here */
find_low_pfn_range();
#else
num_physpages = max_pfn;
check_x2apic();
/* How many end-of-memory variables you have, grandma! */
/* need this before calling reserve_initrd */
if (max_pfn > (1UL<<(32 - PAGE_SHIFT)))
max_low_pfn = e820_end_of_low_ram_pfn();
else
max_low_pfn = max_pfn;
and under max_low_pfn is bootmem.
>
> Additionally, if 'start_pfn == end_pfn', we don't need to go futher,
> so change range check.
>
> Signed-off-by: Joonsoo Kim <[email protected]>
>
> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> index 5e07d36..4711e91 100644
> --- a/mm/nobootmem.c
> +++ b/mm/nobootmem.c
> @@ -110,9 +110,9 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
> {
> unsigned long start_pfn = PFN_UP(start);
> unsigned long end_pfn = min_t(unsigned long,
> - PFN_DOWN(end), max_low_pfn);
> + PFN_DOWN(end), min_low_pfn);
what is min_low_pfn ? is it 0 for x86?
Thanks
Yinghai
On Tue, Mar 19, 2013 at 02:16:00PM +0900, Joonsoo Kim wrote:
> Remove unused argument and make function static,
> because there is no user outside of nobootmem.c
>
> Signed-off-by: Joonsoo Kim <[email protected]>
>
> diff --git a/include/linux/bootmem.h b/include/linux/bootmem.h
> index cdc3bab..5f0b0e1 100644
> --- a/include/linux/bootmem.h
> +++ b/include/linux/bootmem.h
> @@ -44,7 +44,6 @@ extern unsigned long init_bootmem_node(pg_data_t *pgdat,
> unsigned long endpfn);
> extern unsigned long init_bootmem(unsigned long addr, unsigned long memend);
>
> -extern unsigned long free_low_memory_core_early(int nodeid);
> extern unsigned long free_all_bootmem_node(pg_data_t *pgdat);
> extern unsigned long free_all_bootmem(void);
>
> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> index 4711e91..589c673 100644
> --- a/mm/nobootmem.c
> +++ b/mm/nobootmem.c
> @@ -120,7 +120,7 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
> return end_pfn - start_pfn;
> }
>
> -unsigned long __init free_low_memory_core_early(int nodeid)
> +static unsigned long __init free_low_memory_core_early()
> {
> unsigned long count = 0;
> phys_addr_t start, end, size;
> @@ -170,7 +170,7 @@ unsigned long __init free_all_bootmem(void)
> * because in some case like Node0 doesn't have RAM installed
> * low ram will be on Node1
> */
> - return free_low_memory_core_early(MAX_NUMNODES);
> + return free_low_memory_core_early();
> }
>
> /**
> --
> 1.7.9.5
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
Sorry, this patch makes build warning.
Below is fixed version.
-------------------->&------------------------
>From 05f4a768dd5c514113916908f4710f8863704ed9 Mon Sep 17 00:00:00 2001
From: Joonsoo Kim <[email protected]>
Date: Mon, 18 Mar 2013 14:17:57 +0900
Subject: [PATCH] mm, nobootmem: clean-up of free_low_memory_core_early()
Remove unused argument and make function static,
because there is no user outside of nobootmem.c
Signed-off-by: Joonsoo Kim <[email protected]>
diff --git a/include/linux/bootmem.h b/include/linux/bootmem.h
index cdc3bab..5f0b0e1 100644
--- a/include/linux/bootmem.h
+++ b/include/linux/bootmem.h
@@ -44,7 +44,6 @@ extern unsigned long init_bootmem_node(pg_data_t *pgdat,
unsigned long endpfn);
extern unsigned long init_bootmem(unsigned long addr, unsigned long memend);
-extern unsigned long free_low_memory_core_early(int nodeid);
extern unsigned long free_all_bootmem_node(pg_data_t *pgdat);
extern unsigned long free_all_bootmem(void);
diff --git a/mm/nobootmem.c b/mm/nobootmem.c
index 4711e91..9c38698 100644
--- a/mm/nobootmem.c
+++ b/mm/nobootmem.c
@@ -120,7 +120,7 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
return end_pfn - start_pfn;
}
-unsigned long __init free_low_memory_core_early(int nodeid)
+static unsigned long __init free_low_memory_core_early(void)
{
unsigned long count = 0;
phys_addr_t start, end, size;
@@ -170,7 +170,7 @@ unsigned long __init free_all_bootmem(void)
* because in some case like Node0 doesn't have RAM installed
* low ram will be on Node1
*/
- return free_low_memory_core_early(MAX_NUMNODES);
+ return free_low_memory_core_early();
}
/**
--
1.7.9.5
On Mon, Mar 18, 2013 at 10:16 PM, Joonsoo Kim <[email protected]> wrote:
> Remove unused argument and make function static,
> because there is no user outside of nobootmem.c
>
> Signed-off-by: Joonsoo Kim <[email protected]>
>
> diff --git a/include/linux/bootmem.h b/include/linux/bootmem.h
> index cdc3bab..5f0b0e1 100644
> --- a/include/linux/bootmem.h
> +++ b/include/linux/bootmem.h
> @@ -44,7 +44,6 @@ extern unsigned long init_bootmem_node(pg_data_t *pgdat,
> unsigned long endpfn);
> extern unsigned long init_bootmem(unsigned long addr, unsigned long memend);
>
> -extern unsigned long free_low_memory_core_early(int nodeid);
> extern unsigned long free_all_bootmem_node(pg_data_t *pgdat);
> extern unsigned long free_all_bootmem(void);
>
> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> index 4711e91..589c673 100644
> --- a/mm/nobootmem.c
> +++ b/mm/nobootmem.c
> @@ -120,7 +120,7 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
> return end_pfn - start_pfn;
> }
>
> -unsigned long __init free_low_memory_core_early(int nodeid)
> +static unsigned long __init free_low_memory_core_early()
(void) ?
On Mon, Mar 18, 2013 at 10:16 PM, Joonsoo Kim <[email protected]> wrote:
> Currently, we do memset() before reserving the area.
> This may not cause any problem, but it is somewhat weird.
> So change execution order.
>
> Signed-off-by: Joonsoo Kim <[email protected]>
>
> diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> index 589c673..f11ec1c 100644
> --- a/mm/nobootmem.c
> +++ b/mm/nobootmem.c
> @@ -46,8 +46,8 @@ static void * __init __alloc_memory_core_early(int nid, u64 size, u64 align,
> return NULL;
>
> ptr = phys_to_virt(addr);
> - memset(ptr, 0, size);
> memblock_reserve(addr, size);
> + memset(ptr, 0, size);
move down ptr = ... too ?
> /*
> * The min_count is set to 0 so that bootmem allocated blocks
> * are never reported as leaks.
> --
> 1.7.9.5
>
On Mon, Mar 18, 2013 at 10:53:04PM -0700, Yinghai Lu wrote:
> On Mon, Mar 18, 2013 at 10:16 PM, Joonsoo Kim <[email protected]> wrote:
> > Currently, we do memset() before reserving the area.
> > This may not cause any problem, but it is somewhat weird.
> > So change execution order.
> >
> > Signed-off-by: Joonsoo Kim <[email protected]>
> >
> > diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> > index 589c673..f11ec1c 100644
> > --- a/mm/nobootmem.c
> > +++ b/mm/nobootmem.c
> > @@ -46,8 +46,8 @@ static void * __init __alloc_memory_core_early(int nid, u64 size, u64 align,
> > return NULL;
> >
> > ptr = phys_to_virt(addr);
> > - memset(ptr, 0, size);
> > memblock_reserve(addr, size);
> > + memset(ptr, 0, size);
>
> move down ptr = ... too ?
Okay.
I will send v2 soon.
>
> > /*
> > * The min_count is set to 0 so that bootmem allocated blocks
> > * are never reported as leaks.
> > --
> > 1.7.9.5
> >
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
On Mon, Mar 18, 2013 at 10:51:43PM -0700, Yinghai Lu wrote:
> On Mon, Mar 18, 2013 at 10:16 PM, Joonsoo Kim <[email protected]> wrote:
> > Remove unused argument and make function static,
> > because there is no user outside of nobootmem.c
> >
> > Signed-off-by: Joonsoo Kim <[email protected]>
> >
> > diff --git a/include/linux/bootmem.h b/include/linux/bootmem.h
> > index cdc3bab..5f0b0e1 100644
> > --- a/include/linux/bootmem.h
> > +++ b/include/linux/bootmem.h
> > @@ -44,7 +44,6 @@ extern unsigned long init_bootmem_node(pg_data_t *pgdat,
> > unsigned long endpfn);
> > extern unsigned long init_bootmem(unsigned long addr, unsigned long memend);
> >
> > -extern unsigned long free_low_memory_core_early(int nodeid);
> > extern unsigned long free_all_bootmem_node(pg_data_t *pgdat);
> > extern unsigned long free_all_bootmem(void);
> >
> > diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> > index 4711e91..589c673 100644
> > --- a/mm/nobootmem.c
> > +++ b/mm/nobootmem.c
> > @@ -120,7 +120,7 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
> > return end_pfn - start_pfn;
> > }
> >
> > -unsigned long __init free_low_memory_core_early(int nodeid)
> > +static unsigned long __init free_low_memory_core_early()
>
> (void) ?
Yes, fixed version is already sent.
Thanks.
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
On Mon, Mar 18, 2013 at 10:47:41PM -0700, Yinghai Lu wrote:
> On Mon, Mar 18, 2013 at 10:15 PM, Joonsoo Kim <[email protected]> wrote:
> > max_low_pfn reflect the number of _pages_ in the system,
> > not the maximum PFN. You can easily find that fact in init_bootmem().
> > So fix it.
>
> I'm confused. for x86, we have max_low_pfn defined in ...
Below is queote from Russell King in 'https://lkml.org/lkml/2013/3/13/123'
Now, max_low_pfn is initialized this way:
/**
* init_bootmem - register boot memory
* @start: pfn where the bitmap is to be placed
* @pages: number of available physical pages
*
* Returns the number of bytes needed to hold the bitmap.
*/
unsigned long __init init_bootmem(unsigned long start, unsigned long pages)
{
max_low_pfn = pages;
min_low_pfn = start;
return init_bootmem_core(NODE_DATA(0)->bdata, start, 0, pages);
}
So, min_low_pfn is the PFN offset of the start of physical memory (so
3GB >> PAGE_SHIFT) and max_low_pfn ends up being the number of pages,
_not_ the maximum PFN value
So, if physical address doesn't start at 0, max_low_pfn doesn't represent
the maximum PFN value. This is a case for ARM.
>
> #ifdef CONFIG_X86_32
> /* max_low_pfn get updated here */
> find_low_pfn_range();
> #else
> num_physpages = max_pfn;
>
> check_x2apic();
>
> /* How many end-of-memory variables you have, grandma! */
> /* need this before calling reserve_initrd */
> if (max_pfn > (1UL<<(32 - PAGE_SHIFT)))
> max_low_pfn = e820_end_of_low_ram_pfn();
> else
> max_low_pfn = max_pfn;
>
> and under max_low_pfn is bootmem.
>
> >
> > Additionally, if 'start_pfn == end_pfn', we don't need to go futher,
> > so change range check.
> >
> > Signed-off-by: Joonsoo Kim <[email protected]>
> >
> > diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> > index 5e07d36..4711e91 100644
> > --- a/mm/nobootmem.c
> > +++ b/mm/nobootmem.c
> > @@ -110,9 +110,9 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
> > {
> > unsigned long start_pfn = PFN_UP(start);
> > unsigned long end_pfn = min_t(unsigned long,
> > - PFN_DOWN(end), max_low_pfn);
> > + PFN_DOWN(end), min_low_pfn);
>
> what is min_low_pfn ? is it 0 for x86?
My implementation is totally wrong. :)
min_low_pfn is not proper value for this purpose.
I will fix it.
Sorry for noise.
Thanks.
>
> Thanks
>
> Yinghai
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
On Tue, Mar 19, 2013 at 03:25:22PM +0900, Joonsoo Kim wrote:
> On Mon, Mar 18, 2013 at 10:47:41PM -0700, Yinghai Lu wrote:
> > On Mon, Mar 18, 2013 at 10:15 PM, Joonsoo Kim <[email protected]> wrote:
> > > max_low_pfn reflect the number of _pages_ in the system,
> > > not the maximum PFN. You can easily find that fact in init_bootmem().
> > > So fix it.
> >
> > I'm confused. for x86, we have max_low_pfn defined in ...
>
> Below is queote from Russell King in 'https://lkml.org/lkml/2013/3/13/123'
>
>
> Now, max_low_pfn is initialized this way:
>
> /**
> * init_bootmem - register boot memory
> * @start: pfn where the bitmap is to be placed
> * @pages: number of available physical pages
> *
> * Returns the number of bytes needed to hold the bitmap.
> */
> unsigned long __init init_bootmem(unsigned long start, unsigned long pages)
> {
> max_low_pfn = pages;
> min_low_pfn = start;
> return init_bootmem_core(NODE_DATA(0)->bdata, start, 0, pages);
> }
> So, min_low_pfn is the PFN offset of the start of physical memory (so
> 3GB >> PAGE_SHIFT) and max_low_pfn ends up being the number of pages,
> _not_ the maximum PFN value
>
> So, if physical address doesn't start at 0, max_low_pfn doesn't represent
> the maximum PFN value. This is a case for ARM.
>
> >
> > #ifdef CONFIG_X86_32
> > /* max_low_pfn get updated here */
> > find_low_pfn_range();
> > #else
> > num_physpages = max_pfn;
> >
> > check_x2apic();
> >
> > /* How many end-of-memory variables you have, grandma! */
> > /* need this before calling reserve_initrd */
> > if (max_pfn > (1UL<<(32 - PAGE_SHIFT)))
> > max_low_pfn = e820_end_of_low_ram_pfn();
> > else
> > max_low_pfn = max_pfn;
> >
> > and under max_low_pfn is bootmem.
> >
> > >
> > > Additionally, if 'start_pfn == end_pfn', we don't need to go futher,
> > > so change range check.
> > >
> > > Signed-off-by: Joonsoo Kim <[email protected]>
> > >
> > > diff --git a/mm/nobootmem.c b/mm/nobootmem.c
> > > index 5e07d36..4711e91 100644
> > > --- a/mm/nobootmem.c
> > > +++ b/mm/nobootmem.c
> > > @@ -110,9 +110,9 @@ static unsigned long __init __free_memory_core(phys_addr_t start,
> > > {
> > > unsigned long start_pfn = PFN_UP(start);
> > > unsigned long end_pfn = min_t(unsigned long,
> > > - PFN_DOWN(end), max_low_pfn);
> > > + PFN_DOWN(end), min_low_pfn);
> >
> > what is min_low_pfn ? is it 0 for x86?
>
> My implementation is totally wrong. :)
> min_low_pfn is not proper value for this purpose.
>
> I will fix it.
> Sorry for noise.
>
> Thanks.
How about using "memblock.current_limit"?
unsigned long end_pfn = min_t(unsigned long, PFN_DOWN(end),
memblock.current_limit);
Thanks.
>
> >
> > Thanks
> >
> > Yinghai
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to [email protected]. For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to [email protected]. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"[email protected]"> [email protected] </a>
On Tue, Mar 19, 2013 at 12:35:45AM -0700, Yinghai Lu wrote:
> Can you check why sparc do not need to change interface during converting
> to use memblock to replace bootmem?
Sure.
According to my understanding to sparc32 code(arch/sparc/mm/init_32.c),
they already use max_low_pfn as the maximum PFN value,
not as the number of pages.
On Tue, Mar 19, 2013 at 05:07:21PM +0900, Joonsoo Kim wrote:
> On Tue, Mar 19, 2013 at 12:35:45AM -0700, Yinghai Lu wrote:
> > Can you check why sparc do not need to change interface during converting
> > to use memblock to replace bootmem?
>
> Sure.
> According to my understanding to sparc32 code(arch/sparc/mm/init_32.c),
> they already use max_low_pfn as the maximum PFN value,
> not as the number of pages.
I assume you already know...
sparc64 uses memblock, but sparc32 does not.
I looked at using memblock for sparc32 some time ago but got
distracted by other stuff.
I recall from back then that these ackward named variables confused me,
and some of my confusion was likely rooted in sparc32 using
max_low_pfn for something elase than others do.
I have no plans to look into adding memblock support for sparc32
right now. But may eventually do so when I get some spare time.
Sam