[PATCH] nommu page refcount bug fixing

[PATCH] nommu page refcount bug fixing

[Luke Yang]

[-- Attachment #1: Type: text/plain, Size: 2769 bytes --]

Hi all,

   The previous "nommu use compound pages" patch has a problem: when
the pages allocated is not compound page (eg: slab allocator), the
refcount value of every page still need to be set, otherwise the
get/put_page() would free a single page improperly, such as in
access_process_vm().

Signed-off-by: Luke Yang <luke.adi@gmail.com>

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b7f14a4..fc8b544 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -436,6 +436,14 @@ static void __free_pages_ok(struct page
                mutex_debug_check_no_locks_freed(page_address(page),
                                                 PAGE_SIZE<<order);

+#ifndef CONFIG_MMU
+       if (!PageCompound(page)) {
+               for (i = 1 ; i < (1 << order) ; ++i) {
+                       __put_page(page + i);
+               }
+       }
+#endif
+
        for (i = 0 ; i < (1 << order) ; ++i)
                reserved += free_pages_check(page + i);
        if (reserved)
@@ -453,6 +461,8 @@ static void __free_pages_ok(struct page
  */
 void fastcall __init __free_pages_bootmem(struct page *page, unsigned
int order)
 {
+       int i;
+
        if (order == 0) {
                __ClearPageReserved(page);
                set_page_count(page, 0);
@@ -472,6 +482,11 @@ void fastcall __init __free_pages_bootme
                }

                set_page_refcounted(page);
+
+#ifndef CONFIG_MMU
+               for (i = 1; i < (1 << order); i++)
+                       set_page_refcounted(page + i);
+#endif
                __free_pages(page, order);
        }
 }
@@ -512,6 +527,8 @@ static inline void expand(struct zone *z
  */
 static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
 {
+       int i;
+
        if (unlikely(page_mapcount(page) |
                (page->mapping != NULL)  |
                (page_count(page) != 0)  |
@@ -539,7 +556,21 @@ static int prep_new_page(struct page *pa
                        1 << PG_referenced | 1 << PG_arch_1 |
                        1 << PG_checked | 1 << PG_mappedtodisk);
        set_page_private(page, 0);
+
        set_page_refcounted(page);
+
+#ifndef CONFIG_MMU
+       if (!(gfp_flags & __GFP_COMP)) {
+               /*
+                * Reference all the pages for this order, otherwise if
+                * anyone accesses one of the pages with (get/put) it
+                * will be freed. - eg: access_process_vm()
+                */
+               for (i = 1; i < (1 << order); i++)
+                       set_page_refcounted(page + i);
+       }
+#endif
+
        kernel_map_pages(page, 1 << order, 1);

        if (gfp_flags & __GFP_ZERO)

--
Best regards,
Luke Yang
luke.adi@gmail.com

[-- Attachment #2: nommu_page_count_fix.patch --]
[-- Type: text/x-patch, Size: 1833 bytes --]

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index b7f14a4..fc8b544 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -436,6 +436,14 @@ static void __free_pages_ok(struct page 
 		mutex_debug_check_no_locks_freed(page_address(page),
 						 PAGE_SIZE<<order);
 
+#ifndef CONFIG_MMU
+	if (!PageCompound(page)) {
+		for (i = 1 ; i < (1 << order) ; ++i) {
+			__put_page(page + i);
+		}
+	}
+#endif
+
 	for (i = 0 ; i < (1 << order) ; ++i)
 		reserved += free_pages_check(page + i);
 	if (reserved)
@@ -453,6 +461,8 @@ static void __free_pages_ok(struct page 
  */
 void fastcall __init __free_pages_bootmem(struct page *page, unsigned int order)
 {
+	int i;
+
 	if (order == 0) {
 		__ClearPageReserved(page);
 		set_page_count(page, 0);
@@ -472,6 +482,11 @@ void fastcall __init __free_pages_bootme
 		}
 
 		set_page_refcounted(page);
+
+#ifndef CONFIG_MMU
+		for (i = 1; i < (1 << order); i++)
+			set_page_refcounted(page + i);
+#endif
 		__free_pages(page, order);
 	}
 }
@@ -512,6 +527,8 @@ static inline void expand(struct zone *z
  */
 static int prep_new_page(struct page *page, int order, gfp_t gfp_flags)
 {
+	int i;
+
 	if (unlikely(page_mapcount(page) |
 		(page->mapping != NULL)  |
 		(page_count(page) != 0)  |
@@ -539,7 +556,21 @@ static int prep_new_page(struct page *pa
 			1 << PG_referenced | 1 << PG_arch_1 |
 			1 << PG_checked | 1 << PG_mappedtodisk);
 	set_page_private(page, 0);
+
 	set_page_refcounted(page);
+
+#ifndef CONFIG_MMU
+	if (!(gfp_flags & __GFP_COMP)) {
+		/*
+		 * Reference all the pages for this order, otherwise if
+		 * anyone accesses one of the pages with (get/put) it
+		 * will be freed. - eg: access_process_vm()
+		 */
+		for (i = 1; i < (1 << order); i++)
+			set_page_refcounted(page + i);
+	}
+#endif
+
 	kernel_map_pages(page, 1 << order, 1);
 
 	if (gfp_flags & __GFP_ZERO)

Re: [PATCH] nommu page refcount bug fixing

[Nick Piggin]

Luke Yang wrote:

>Hi all,
>
>   The previous "nommu use compound pages" patch has a problem: when
>the pages allocated is not compound page (eg: slab allocator), the
>refcount value of every page still need to be set, otherwise the
>get/put_page() would free a single page improperly, such as in
>access_process_vm().
>
>

Yep, sorry this slipped into the kernel. It's my fault for not giving
Andrew a fix for it.

As you might know, page refcounting in nommu was already broken, so
I'm working on a proper solution to fix it.

In the meantime though, this is a step backwards and reintroduces
NOMMU special-casing in page refcounting. As a temporary fix, what I
think should happen is simply for all slab allocations to ask for
__GFP_COMP pages.

Could you check that fixes your problem?

Thanks,
Nick

--

Send instant messages to your online friends http://au.messenger.yahoo.com 

Re: [PATCH] nommu page refcount bug fixing

[Luke Yang]

[-- Attachment #1: Type: text/plain, Size: 1339 bytes --]

On 3/30/06, Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> Luke Yang wrote:
>
> Yep, sorry this slipped into the kernel. It's my fault for not giving
> Andrew a fix for it.
>
> As you might know, page refcounting in nommu was already broken, so
> I'm working on a proper solution to fix it.
>
> In the meantime though, this is a step backwards and reintroduces
> NOMMU special-casing in page refcounting. As a temporary fix, what I
> think should happen is simply for all slab allocations to ask for
> __GFP_COMP pages.
>
> Could you check that fixes your problem?
  It works.  What's your plan to modify nommu mm? I would like to
help. And I am also interested in implementing the "non-power-of-2"
allocator in 2.6.

  New patch:
Signed-off-by: Luke Yang <luke.adi@gmail.com>

diff --git a/mm/slab.c b/mm/slab.c
index 4cbf8bb..f93d3d5 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1454,7 +1454,7 @@ static void *kmem_getpages(struct kmem_c
        int i;

        flags |= cachep->gfpflags;
-       page = alloc_pages_node(nodeid, flags, cachep->gfporder);
+       page = alloc_pages_node(nodeid, (flags | __GFP_COMP), cachep->gfporder);
        if (!page)
                return NULL;
        addr = page_address(page);

--
Best regards,
Luke Yang (Kernel for Blackfin maintainer)
luke.adi@gmail.com

[-- Attachment #2: nommu_use_compound_page_in_slab.patch --]
[-- Type: text/x-patch, Size: 440 bytes --]

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
diff --git a/mm/slab.c b/mm/slab.c
index 4cbf8bb..f93d3d5 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1454,7 +1454,7 @@ static void *kmem_getpages(struct kmem_c
 	int i;
 
 	flags |= cachep->gfpflags;
-	page = alloc_pages_node(nodeid, flags, cachep->gfporder);
+	page = alloc_pages_node(nodeid, (flags | __GFP_COMP), cachep->gfporder);
 	if (!page)
 		return NULL;
 	addr = page_address(page);

Re: [PATCH] nommu page refcount bug fixing

[Luke Yang]

[-- Attachment #1: Type: text/plain, Size: 1170 bytes --]

> > NOMMU special-casing in page refcounting. As a temporary fix, what I
> > think should happen is simply for all slab allocations to ask for
> > __GFP_COMP pages.
> >
> > Could you check that fixes your problem?
>   It works.  What's your plan to modify nommu mm? I would like to
> help. And I am also interested in implementing the "non-power-of-2"
> allocator in 2.6.
>
>   New patch:
  Sorry! Previous patch only works for nommu...  Here's the correct one:
Signed-off-by: Luke Yang <luke.adi@gmail.com>

diff --git a/mm/slab.c b/mm/slab.c
index 4cbf8bb..388a6a9 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1454,7 +1454,11 @@ static void *kmem_getpages(struct kmem_c
        int i;

        flags |= cachep->gfpflags;
+#ifdef CONFIG_MMU
        page = alloc_pages_node(nodeid, flags, cachep->gfporder);
+#else
+       page = alloc_pages_node(nodeid, (flags | __GFP_COMP), cachep->gfporder);
+#endif
        if (!page)
                return NULL;
        addr = page_address(page);

> --
> Best regards,
> Luke Yang (Kernel for Blackfin maintainer)
> luke.adi@gmail.com
>
>
>


--
Best regards,
Luke Yang
luke.adi@gmail.com

[-- Attachment #2: nommu_use_compound_page_in_slab.patch --]
[-- Type: text/x-patch, Size: 475 bytes --]

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
diff --git a/mm/slab.c b/mm/slab.c
index 4cbf8bb..388a6a9 100644
--- a/mm/slab.c
+++ b/mm/slab.c
@@ -1454,7 +1454,11 @@ static void *kmem_getpages(struct kmem_c
 	int i;
 
 	flags |= cachep->gfpflags;
+#ifdef CONFIG_MMU
 	page = alloc_pages_node(nodeid, flags, cachep->gfporder);
+#else
+	page = alloc_pages_node(nodeid, (flags | __GFP_COMP), cachep->gfporder);
+#endif
 	if (!page)
 		return NULL;
 	addr = page_address(page);

Re: [PATCH] nommu page refcount bug fixing

[Nick Piggin]

Luke Yang wrote:
>>>NOMMU special-casing in page refcounting. As a temporary fix, what I
>>>think should happen is simply for all slab allocations to ask for
>>>__GFP_COMP pages.
>>>
>>>Could you check that fixes your problem?
>>
>>  It works.  What's your plan to modify nommu mm? I would like to
>>help. And I am also interested in implementing the "non-power-of-2"
>>allocator in 2.6.
>>

I'll get it up to date and send it over to you, offline. It
would be great if you could help.

>>  New patch:
> 

Acked-by: Nick Piggin <npiggin@suse.de>

I'll write a changelog for you:

***
The earlier patch to consolidate mmu and nommu page allocation
and refcounting by using compound pages for nommu allocations
had a bug: kmalloc slabs who's pages were initially allocated
by a non-__GFP_COMP allocator could be passed into mm/nommu.c
kmalloc allocations which really wanted __GFP_COMP underlying
pages.

Fix that by having nommu pass __GFP_COMP to all higher order
slab allocations
***

Sound OK? Can you do it next time? ;)

-- 
SUSE Labs, Novell Inc.
Send instant messages to your online friends http://au.messenger.yahoo.com