linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
* [patch 0/4] mm: bootmem / page allocator bootstrap fixlets
@ 2011-12-13 13:58 Johannes Weiner
  2011-12-13 13:58 ` [patch 1/4] mm: page_alloc: remove order assumption from __free_pages_bootmem() Johannes Weiner
                   ` (3 more replies)
  0 siblings, 4 replies; 10+ messages in thread
From: Johannes Weiner @ 2011-12-13 13:58 UTC (permalink / raw)
  To: Andrew Morton, Uwe Kleine-König; +Cc: linux-mm, linux-kernel

Hi Uwe,

here is a follow-up to your bootmem micro optimizations.  3 and 4
directly relate to the discussion, 1 and 2 are cleanups I had sitting
around anyway.

Unfortunately, I can't test them as x86 kernels no longer build with
CONFIG_NO_BOOTMEM=n, but I suspect that you might have access to
non-x86 machines ;-) so if you can, please give this a spin - I don't
want this stuff to go in untested.

[ Fun fact: nobootmem.c is 400 lines of bootmem API emulation that is
  just incompatible enough that one can not switch between bootmem and
  nobootmem without touching callsites. ]

 mm/bootmem.c    |   22 ++++++++++------------
 mm/page_alloc.c |   33 ++++++++++++---------------------
 2 files changed, 22 insertions(+), 33 deletions(-)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* [patch 1/4] mm: page_alloc: remove order assumption from __free_pages_bootmem()
  2011-12-13 13:58 [patch 0/4] mm: bootmem / page allocator bootstrap fixlets Johannes Weiner
@ 2011-12-13 13:58 ` Johannes Weiner
  2011-12-13 22:05   ` Andrew Morton
  2011-12-13 13:58 ` [patch 2/4] mm: page_alloc: generalize order handling in __free_pages_bootmem() Johannes Weiner
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 10+ messages in thread
From: Johannes Weiner @ 2011-12-13 13:58 UTC (permalink / raw)
  To: Andrew Morton, Uwe Kleine-König; +Cc: linux-mm, linux-kernel

Even though bootmem passes an order with the page to be freed,
__free_pages_bootmem() assumes that 1 << order is always BITS_PER_LONG
if non-zero.  While this happens to be true, it's not really robust.
Remove that assumption and use 1 << order instead.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/page_alloc.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 2b8ba3a..4d5e91c 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -703,13 +703,14 @@ void __meminit __free_pages_bootmem(struct page *page, unsigned int order)
 		set_page_refcounted(page);
 		__free_page(page);
 	} else {
-		int loop;
+		unsigned int nr_pages = 1 << order;
+		unsigned int loop;
 
 		prefetchw(page);
-		for (loop = 0; loop < BITS_PER_LONG; loop++) {
+		for (loop = 0; loop < nr_pages; loop++) {
 			struct page *p = &page[loop];
 
-			if (loop + 1 < BITS_PER_LONG)
+			if (loop + 1 < nr_pages)
 				prefetchw(p + 1);
 			__ClearPageReserved(p);
 			set_page_count(p, 0);
-- 
1.7.7.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [patch 2/4] mm: page_alloc: generalize order handling in __free_pages_bootmem()
  2011-12-13 13:58 [patch 0/4] mm: bootmem / page allocator bootstrap fixlets Johannes Weiner
  2011-12-13 13:58 ` [patch 1/4] mm: page_alloc: remove order assumption from __free_pages_bootmem() Johannes Weiner
@ 2011-12-13 13:58 ` Johannes Weiner
  2011-12-13 13:58 ` [patch 3/4] mm: bootmem: drop superfluous range check when freeing pages in bulk Johannes Weiner
  2011-12-13 13:58 ` [patch 4/4] mm: bootmem: try harder to free " Johannes Weiner
  3 siblings, 0 replies; 10+ messages in thread
From: Johannes Weiner @ 2011-12-13 13:58 UTC (permalink / raw)
  To: Andrew Morton, Uwe Kleine-König; +Cc: linux-mm, linux-kernel

__free_pages_bootmem() used to special-case higher-order frees to save
individual page checking with free_pages_bulk().

Nowadays, both zero order and non-zero order frees use free_pages(),
which checks each individual page anyway, and so there is little point
in making the distinction anymore.  The higher-order loop will work
just fine for zero order pages.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/page_alloc.c |   34 ++++++++++++----------------------
 1 files changed, 12 insertions(+), 22 deletions(-)

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 4d5e91c..1efacb3 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -692,33 +692,23 @@ static void __free_pages_ok(struct page *page, unsigned int order)
 	local_irq_restore(flags);
 }
 
-/*
- * permit the bootmem allocator to evade page validation on high-order frees
- */
 void __meminit __free_pages_bootmem(struct page *page, unsigned int order)
 {
-	if (order == 0) {
-		__ClearPageReserved(page);
-		set_page_count(page, 0);
-		set_page_refcounted(page);
-		__free_page(page);
-	} else {
-		unsigned int nr_pages = 1 << order;
-		unsigned int loop;
-
-		prefetchw(page);
-		for (loop = 0; loop < nr_pages; loop++) {
-			struct page *p = &page[loop];
+	unsigned int nr_pages = 1 << order;
+	unsigned int loop;
 
-			if (loop + 1 < nr_pages)
-				prefetchw(p + 1);
-			__ClearPageReserved(p);
-			set_page_count(p, 0);
-		}
+	prefetchw(page);
+	for (loop = 0; loop < nr_pages; loop++) {
+		struct page *p = &page[loop];
 
-		set_page_refcounted(page);
-		__free_pages(page, order);
+		if (loop + 1 < nr_pages)
+			prefetchw(p + 1);
+		__ClearPageReserved(p);
+		set_page_count(p, 0);
 	}
+
+	set_page_refcounted(page);
+	__free_pages(page, order);
 }
 
 
-- 
1.7.7.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [patch 3/4] mm: bootmem: drop superfluous range check when freeing pages in bulk
  2011-12-13 13:58 [patch 0/4] mm: bootmem / page allocator bootstrap fixlets Johannes Weiner
  2011-12-13 13:58 ` [patch 1/4] mm: page_alloc: remove order assumption from __free_pages_bootmem() Johannes Weiner
  2011-12-13 13:58 ` [patch 2/4] mm: page_alloc: generalize order handling in __free_pages_bootmem() Johannes Weiner
@ 2011-12-13 13:58 ` Johannes Weiner
  2011-12-13 15:28   ` Uwe Kleine-König
  2011-12-13 13:58 ` [patch 4/4] mm: bootmem: try harder to free " Johannes Weiner
  3 siblings, 1 reply; 10+ messages in thread
From: Johannes Weiner @ 2011-12-13 13:58 UTC (permalink / raw)
  To: Andrew Morton, Uwe Kleine-König; +Cc: linux-mm, linux-kernel

The area node_bootmem_map represents is aligned to BITS_PER_LONG, and
all bits in any aligned word of that map valid.  When the represented
area extends beyond the end of the node, the non-existant pages will
be marked as reserved.

As a result, when freeing a page block, doing an explicit range check
for whether that block is within the node's range is redundant as the
bitmap is consulted anyway to see whether all pages in the block are
unreserved.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/bootmem.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/mm/bootmem.c b/mm/bootmem.c
index 3e6f152..1aea171 100644
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -197,7 +197,7 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
 		idx = start - bdata->node_min_pfn;
 		vec = ~map[idx / BITS_PER_LONG];
 
-		if (aligned && vec == ~0UL && start + BITS_PER_LONG <= end) {
+		if (aligned && vec == ~0UL) {
 			int order = ilog2(BITS_PER_LONG);
 
 			__free_pages_bootmem(pfn_to_page(start), order);
-- 
1.7.7.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* [patch 4/4] mm: bootmem: try harder to free pages in bulk
  2011-12-13 13:58 [patch 0/4] mm: bootmem / page allocator bootstrap fixlets Johannes Weiner
                   ` (2 preceding siblings ...)
  2011-12-13 13:58 ` [patch 3/4] mm: bootmem: drop superfluous range check when freeing pages in bulk Johannes Weiner
@ 2011-12-13 13:58 ` Johannes Weiner
  2011-12-14 20:20   ` Uwe Kleine-König
  3 siblings, 1 reply; 10+ messages in thread
From: Johannes Weiner @ 2011-12-13 13:58 UTC (permalink / raw)
  To: Andrew Morton, Uwe Kleine-König; +Cc: linux-mm, linux-kernel

The loop that frees pages to the page allocator while bootstrapping
tries to free higher-order blocks only when the starting address is
aligned to that block size.  Otherwise it will free all pages on that
node one-by-one.

Change it to free individual pages up to the first aligned block and
then try higher-order frees from there.

Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
---
 mm/bootmem.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/mm/bootmem.c b/mm/bootmem.c
index 1aea171..668e94d 100644
--- a/mm/bootmem.c
+++ b/mm/bootmem.c
@@ -171,7 +171,6 @@ void __init free_bootmem_late(unsigned long addr, unsigned long size)
 
 static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
 {
-	int aligned;
 	struct page *page;
 	unsigned long start, end, pages, count = 0;
 
@@ -181,14 +180,8 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
 	start = bdata->node_min_pfn;
 	end = bdata->node_low_pfn;
 
-	/*
-	 * If the start is aligned to the machines wordsize, we might
-	 * be able to free pages in bulks of that order.
-	 */
-	aligned = !(start & (BITS_PER_LONG - 1));
-
-	bdebug("nid=%td start=%lx end=%lx aligned=%d\n",
-		bdata - bootmem_node_data, start, end, aligned);
+	bdebug("nid=%td start=%lx end=%lx\n",
+		bdata - bootmem_node_data, start, end);
 
 	while (start < end) {
 		unsigned long *map, idx, vec;
@@ -196,12 +189,17 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
 		map = bdata->node_bootmem_map;
 		idx = start - bdata->node_min_pfn;
 		vec = ~map[idx / BITS_PER_LONG];
-
-		if (aligned && vec == ~0UL) {
+		/*
+		 * If we have a properly aligned and fully unreserved
+		 * BITS_PER_LONG block of pages in front of us, free
+		 * it in one go.
+		 */
+		if (IS_ALIGNED(start, BITS_PER_LONG) && vec == ~0UL) {
 			int order = ilog2(BITS_PER_LONG);
 
 			__free_pages_bootmem(pfn_to_page(start), order);
 			count += BITS_PER_LONG;
+			start += BITS_PER_LONG;
 		} else {
 			unsigned long off = 0;
 
@@ -214,8 +212,8 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
 				vec >>= 1;
 				off++;
 			}
+			start = ALIGN(start + 1, BITS_PER_LONG);
 		}
-		start += BITS_PER_LONG;
 	}
 
 	page = virt_to_page(bdata->node_bootmem_map);
-- 
1.7.7.3

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [patch 3/4] mm: bootmem: drop superfluous range check when freeing pages in bulk
  2011-12-13 13:58 ` [patch 3/4] mm: bootmem: drop superfluous range check when freeing pages in bulk Johannes Weiner
@ 2011-12-13 15:28   ` Uwe Kleine-König
  2011-12-13 15:44     ` Johannes Weiner
  0 siblings, 1 reply; 10+ messages in thread
From: Uwe Kleine-König @ 2011-12-13 15:28 UTC (permalink / raw)
  To: Johannes Weiner; +Cc: Andrew Morton, linux-mm, linux-kernel

Hello Johannes,

On Tue, Dec 13, 2011 at 02:58:30PM +0100, Johannes Weiner wrote:
> The area node_bootmem_map represents is aligned to BITS_PER_LONG, and
> all bits in any aligned word of that map valid.  When the represented
> area extends beyond the end of the node, the non-existant pages will
> be marked as reserved.
> 
> As a result, when freeing a page block, doing an explicit range check
> for whether that block is within the node's range is redundant as the
> bitmap is consulted anyway to see whether all pages in the block are
> unreserved.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
I suggest to drop my patch then and add something like

	Reported-by: $me

to this one instead.

Other than that I will give your series a spin on my ARM machine later
today.

Best regards
Uwe

> ---
>  mm/bootmem.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/bootmem.c b/mm/bootmem.c
> index 3e6f152..1aea171 100644
> --- a/mm/bootmem.c
> +++ b/mm/bootmem.c
> @@ -197,7 +197,7 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
>  		idx = start - bdata->node_min_pfn;
>  		vec = ~map[idx / BITS_PER_LONG];
>  
> -		if (aligned && vec == ~0UL && start + BITS_PER_LONG <= end) {
> +		if (aligned && vec == ~0UL) {
>  			int order = ilog2(BITS_PER_LONG);
>  
>  			__free_pages_bootmem(pfn_to_page(start), order);
> -- 
> 1.7.7.3
> 
> 

-- 
Pengutronix e.K.                           | Uwe Kleine-Konig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch 3/4] mm: bootmem: drop superfluous range check when freeing pages in bulk
  2011-12-13 15:28   ` Uwe Kleine-König
@ 2011-12-13 15:44     ` Johannes Weiner
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Weiner @ 2011-12-13 15:44 UTC (permalink / raw)
  To: Uwe Kleine-König; +Cc: Andrew Morton, linux-mm, linux-kernel

On Tue, Dec 13, 2011 at 04:28:43PM +0100, Uwe Kleine-Konig wrote:
> Hello Johannes,
> 
> On Tue, Dec 13, 2011 at 02:58:30PM +0100, Johannes Weiner wrote:
> > The area node_bootmem_map represents is aligned to BITS_PER_LONG, and
> > all bits in any aligned word of that map valid.  When the represented
> > area extends beyond the end of the node, the non-existant pages will
> > be marked as reserved.
> > 
> > As a result, when freeing a page block, doing an explicit range check
> > for whether that block is within the node's range is redundant as the
> > bitmap is consulted anyway to see whether all pages in the block are
> > unreserved.
> > 
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> I suggest to drop my patch then and add something like
> 
> 	Reported-by: $me
> 
> to this one instead.

Your patch is a real and obvious fix, while mine is just a cleanup but
has more obscure dependencies on how the bitmap is managed.

If you don't mind, I would prefer to keep them as separate changes.

> Other than that I will give your series a spin on my ARM machine later
> today.

Thanks a lot!

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch 1/4] mm: page_alloc: remove order assumption from __free_pages_bootmem()
  2011-12-13 13:58 ` [patch 1/4] mm: page_alloc: remove order assumption from __free_pages_bootmem() Johannes Weiner
@ 2011-12-13 22:05   ` Andrew Morton
  0 siblings, 0 replies; 10+ messages in thread
From: Andrew Morton @ 2011-12-13 22:05 UTC (permalink / raw)
  To: Johannes Weiner; +Cc: Uwe Kleine-König, linux-mm, linux-kernel, Tejun Heo

On Tue, 13 Dec 2011 14:58:28 +0100
Johannes Weiner <hannes@cmpxchg.org> wrote:

> Even though bootmem passes an order with the page to be freed,
> __free_pages_bootmem() assumes that 1 << order is always BITS_PER_LONG
> if non-zero.  While this happens to be true, it's not really robust.
> Remove that assumption and use 1 << order instead.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> ---
>  mm/page_alloc.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 2b8ba3a..4d5e91c 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -703,13 +703,14 @@ void __meminit __free_pages_bootmem(struct page *page, unsigned int order)
>  		set_page_refcounted(page);
>  		__free_page(page);
>  	} else {
> -		int loop;
> +		unsigned int nr_pages = 1 << order;
> +		unsigned int loop;
>  
>  		prefetchw(page);
> -		for (loop = 0; loop < BITS_PER_LONG; loop++) {
> +		for (loop = 0; loop < nr_pages; loop++) {
>  			struct page *p = &page[loop];
>  
> -			if (loop + 1 < BITS_PER_LONG)
> +			if (loop + 1 < nr_pages)
>  				prefetchw(p + 1);
>  			__ClearPageReserved(p);
>  			set_page_count(p, 0);

Tejun has recently secretly snuck the below (rather old) patch into
linux-next's page_alloc.c.  I think I got everything fixed up right -
please check.

commit 53348f27168534561c0c814843bbf181314374f4
Author:     Tejun Heo <tj@kernel.org>
AuthorDate: Tue Jul 12 09:58:06 2011 +0200
Commit:     H. Peter Anvin <hpa@linux.intel.com>
CommitDate: Wed Jul 13 16:35:56 2011 -0700

    bootmem: Fix __free_pages_bootmem() to use @order properly
    
    a226f6c899 (FRV: Clean up bootmem allocator's page freeing algorithm)
    separated out __free_pages_bootmem() from free_all_bootmem_core().
    __free_pages_bootmem() takes @order argument but it assumes @order is
    either 0 or ilog2(BITS_PER_LONG).  Note that all the current users
    match that assumption and this doesn't cause actual problems.
    
    Fix it by using 1 << order instead of BITS_PER_LONG.
    
    Signed-off-by: Tejun Heo <tj@kernel.org>
    Link: http://lkml.kernel.org/r/1310457490-3356-3-git-send-email-tj@kernel.org
    Cc: David Howells <dhowells@redhat.com>
    Signed-off-by: H. Peter Anvin <hpa@linux.intel.com>

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 9119faa..b6da6ed 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -705,10 +705,10 @@ void __meminit __free_pages_bootmem(struct page *page, unsigned int order)
 		int loop;
 
 		prefetchw(page);
-		for (loop = 0; loop < BITS_PER_LONG; loop++) {
+		for (loop = 0; loop < (1 << order); loop++) {
 			struct page *p = &page[loop];
 
-			if (loop + 1 < BITS_PER_LONG)
+			if (loop + 1 < (1 << order))
 				prefetchw(p + 1);
 			__ClearPageReserved(p);
 			set_page_count(p, 0);

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply related	[flat|nested] 10+ messages in thread

* Re: [patch 4/4] mm: bootmem: try harder to free pages in bulk
  2011-12-13 13:58 ` [patch 4/4] mm: bootmem: try harder to free " Johannes Weiner
@ 2011-12-14 20:20   ` Uwe Kleine-König
  2011-12-14 20:42     ` Johannes Weiner
  0 siblings, 1 reply; 10+ messages in thread
From: Uwe Kleine-König @ 2011-12-14 20:20 UTC (permalink / raw)
  To: Johannes Weiner; +Cc: Andrew Morton, linux-mm, linux-kernel

On Tue, Dec 13, 2011 at 02:58:31PM +0100, Johannes Weiner wrote:
> The loop that frees pages to the page allocator while bootstrapping
> tries to free higher-order blocks only when the starting address is
> aligned to that block size.  Otherwise it will free all pages on that
> node one-by-one.
> 
> Change it to free individual pages up to the first aligned block and
> then try higher-order frees from there.
> 
> Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
I gave all four patches a try now on my ARM machine and it still works
fine. But note that this patch isn't really tested, because for me
free_all_bootmem_core is only called once and that with an aligned
address.
But at least you didn't broke that case :-)
Having said that, I wonder if the code does the right thing for
unaligned start. (That is, it's wrong to start testing for bit 0 of
map[idx / BITS_PER_LONG], isn't it?) But if that's the case that's not
something you introduced in this series.

One more comment below.

> ---
>  mm/bootmem.c |   22 ++++++++++------------
>  1 files changed, 10 insertions(+), 12 deletions(-)
> 
> diff --git a/mm/bootmem.c b/mm/bootmem.c
> index 1aea171..668e94d 100644
> --- a/mm/bootmem.c
> +++ b/mm/bootmem.c
> @@ -171,7 +171,6 @@ void __init free_bootmem_late(unsigned long addr, unsigned long size)
>  
>  static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
>  {
> -	int aligned;
>  	struct page *page;
>  	unsigned long start, end, pages, count = 0;
>  
> @@ -181,14 +180,8 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
>  	start = bdata->node_min_pfn;
>  	end = bdata->node_low_pfn;
>  
> -	/*
> -	 * If the start is aligned to the machines wordsize, we might
> -	 * be able to free pages in bulks of that order.
> -	 */
> -	aligned = !(start & (BITS_PER_LONG - 1));
> -
> -	bdebug("nid=%td start=%lx end=%lx aligned=%d\n",
> -		bdata - bootmem_node_data, start, end, aligned);
> +	bdebug("nid=%td start=%lx end=%lx\n",
> +		bdata - bootmem_node_data, start, end);
>  
>  	while (start < end) {
>  		unsigned long *map, idx, vec;
> @@ -196,12 +189,17 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
>  		map = bdata->node_bootmem_map;
>  		idx = start - bdata->node_min_pfn;
>  		vec = ~map[idx / BITS_PER_LONG];
> -
> -		if (aligned && vec == ~0UL) {
> +		/*
> +		 * If we have a properly aligned and fully unreserved
> +		 * BITS_PER_LONG block of pages in front of us, free
> +		 * it in one go.
> +		 */
> +		if (IS_ALIGNED(start, BITS_PER_LONG) && vec == ~0UL) {
>  			int order = ilog2(BITS_PER_LONG);
>  
>  			__free_pages_bootmem(pfn_to_page(start), order);
>  			count += BITS_PER_LONG;
> +			start += BITS_PER_LONG;
>  		} else {
>  			unsigned long off = 0;
>  
> @@ -214,8 +212,8 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
>  				vec >>= 1;
>  				off++;
>  			}
> +			start = ALIGN(start + 1, BITS_PER_LONG);
>  		}
> -		start += BITS_PER_LONG;
I don't know if the compiler would be more happy if you would just use

	start = ALIGN(start + 1, BITS_PER_LONG);

unconditionally and drop

	start += BITS_PER_LONG

in the if block?!

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-Konig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: [patch 4/4] mm: bootmem: try harder to free pages in bulk
  2011-12-14 20:20   ` Uwe Kleine-König
@ 2011-12-14 20:42     ` Johannes Weiner
  0 siblings, 0 replies; 10+ messages in thread
From: Johannes Weiner @ 2011-12-14 20:42 UTC (permalink / raw)
  To: Uwe Kleine-König; +Cc: Andrew Morton, linux-mm, linux-kernel

On Wed, Dec 14, 2011 at 09:20:32PM +0100, Uwe Kleine-Konig wrote:
> On Tue, Dec 13, 2011 at 02:58:31PM +0100, Johannes Weiner wrote:
> > The loop that frees pages to the page allocator while bootstrapping
> > tries to free higher-order blocks only when the starting address is
> > aligned to that block size.  Otherwise it will free all pages on that
> > node one-by-one.
> > 
> > Change it to free individual pages up to the first aligned block and
> > then try higher-order frees from there.
> > 
> > Signed-off-by: Johannes Weiner <hannes@cmpxchg.org>
> I gave all four patches a try now on my ARM machine and it still works
> fine. But note that this patch isn't really tested, because for me
> free_all_bootmem_core is only called once and that with an aligned
> address.
> But at least you didn't broke that case :-)
> Having said that, I wonder if the code does the right thing for
> unaligned start. (That is, it's wrong to start testing for bit 0 of
> map[idx / BITS_PER_LONG], isn't it?) But if that's the case that's not
> something you introduced in this series.

We round up and cover area beyond the end of the node to the next
alignment boundary, but don't do the same for the beginning of the
node.  So map[0] is the first BITS_PER_LONG pages starting at start,
even when start is not aligned.

> > @@ -196,12 +189,17 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
> >  		map = bdata->node_bootmem_map;
> >  		idx = start - bdata->node_min_pfn;
> >  		vec = ~map[idx / BITS_PER_LONG];
> > -
> > -		if (aligned && vec == ~0UL) {
> > +		/*
> > +		 * If we have a properly aligned and fully unreserved
> > +		 * BITS_PER_LONG block of pages in front of us, free
> > +		 * it in one go.
> > +		 */
> > +		if (IS_ALIGNED(start, BITS_PER_LONG) && vec == ~0UL) {
> >  			int order = ilog2(BITS_PER_LONG);
> >  
> >  			__free_pages_bootmem(pfn_to_page(start), order);
> >  			count += BITS_PER_LONG;
> > +			start += BITS_PER_LONG;
> >  		} else {
> >  			unsigned long off = 0;
> >  
> > @@ -214,8 +212,8 @@ static unsigned long __init free_all_bootmem_core(bootmem_data_t *bdata)
> >  				vec >>= 1;
> >  				off++;
> >  			}
> > +			start = ALIGN(start + 1, BITS_PER_LONG);
> >  		}
> > -		start += BITS_PER_LONG;
> I don't know if the compiler would be more happy if you would just use
> 
> 	start = ALIGN(start + 1, BITS_PER_LONG);
> 
> unconditionally and drop
> 
> 	start += BITS_PER_LONG
> 
> in the if block?!

I thought it would be beneficial to have the simpler version for the
common case, which is freeing a full block.  Have you looked at the
object code?

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2011-12-14 20:42 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2011-12-13 13:58 [patch 0/4] mm: bootmem / page allocator bootstrap fixlets Johannes Weiner
2011-12-13 13:58 ` [patch 1/4] mm: page_alloc: remove order assumption from __free_pages_bootmem() Johannes Weiner
2011-12-13 22:05   ` Andrew Morton
2011-12-13 13:58 ` [patch 2/4] mm: page_alloc: generalize order handling in __free_pages_bootmem() Johannes Weiner
2011-12-13 13:58 ` [patch 3/4] mm: bootmem: drop superfluous range check when freeing pages in bulk Johannes Weiner
2011-12-13 15:28   ` Uwe Kleine-König
2011-12-13 15:44     ` Johannes Weiner
2011-12-13 13:58 ` [patch 4/4] mm: bootmem: try harder to free " Johannes Weiner
2011-12-14 20:20   ` Uwe Kleine-König
2011-12-14 20:42     ` Johannes Weiner

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).