[PATCH 1/3] compound page: use page[1].lru

[PATCH 1/3] compound page: use page[1].lru

[Hugh Dickins]

If a compound page has its own put_page_testzero destructor (the only
current example is free_huge_page), that is noted in page[1].mapping of
the compound page.  But that's rather a poor place to keep it: functions
which call set_page_dirty_lock after get_user_pages (e.g. Infiniband's
__ib_umem_release) ought to be checking first, otherwise set_page_dirty
is liable to crash on what's not the address of a struct address_space.

And now I'm about to make that worse: it turns out that every compound
page needs a destructor, so we can no longer rely on hugetlb pages going
their own special way, to avoid further problems of page->mapping reuse.
For example, not many people know that: on 50% of i386 -Os builds, the
first tail page of a compound page purports to be PageAnon (when its
destructor has an odd address), which surprises page_add_file_rmap.

Keep the compound page destructor in page[1].lru.next instead.  And to
free up the common pairing of mapping and index, also move compound page
order from index to lru.prev.  Slab reuses page->lru too: but if we ever
need slab to use compound pages, it can easily stack its use above this.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 mm/hugetlb.c    |    4 ++--
 mm/page_alloc.c |   15 ++++++---------
 mm/swap.c       |    2 +-
 3 files changed, 9 insertions(+), 12 deletions(-)

--- 2.6.16-rc2-git11/mm/hugetlb.c	2006-02-10 12:28:04.000000000 +0000
+++ 2.6.16-rc2-git11+/mm/hugetlb.c	2006-02-10 19:54:44.000000000 +0000
@@ -85,7 +85,7 @@ void free_huge_page(struct page *page)
 	BUG_ON(page_count(page));
 
 	INIT_LIST_HEAD(&page->lru);
-	page[1].mapping = NULL;
+	page[1].lru.next = NULL;			/* reset dtor */
 
 	spin_lock(&hugetlb_lock);
 	enqueue_huge_page(page);
@@ -105,7 +105,7 @@ struct page *alloc_huge_page(struct vm_a
 	}
 	spin_unlock(&hugetlb_lock);
 	set_page_count(page, 1);
-	page[1].mapping = (void *)free_huge_page;
+	page[1].lru.next = (void *)free_huge_page;	/* set dtor */
 	for (i = 0; i < (HPAGE_SIZE/PAGE_SIZE); ++i)
 		clear_user_highpage(&page[i], addr);
 	return page;
--- 2.6.16-rc2-git11/mm/page_alloc.c	2006-02-10 12:28:04.000000000 +0000
+++ 2.6.16-rc2-git11+/mm/page_alloc.c	2006-02-10 20:03:19.000000000 +0000
@@ -169,20 +169,17 @@ static void bad_page(struct page *page)
  * All pages have PG_compound set.  All pages have their ->private pointing at
  * the head page (even the head page has this).
  *
- * The first tail page's ->mapping, if non-zero, holds the address of the
- * compound page's put_page() function.
- *
- * The order of the allocation is stored in the first tail page's ->index
- * This is only for debug at present.  This usage means that zero-order pages
- * may not be compound.
+ * The first tail page's ->lru.next holds the address of the compound page's
+ * put_page() function.  Its ->lru.prev holds the order of allocation.
+ * This usage means that zero-order pages may not be compound.
  */
 static void prep_compound_page(struct page *page, unsigned long order)
 {
 	int i;
 	int nr_pages = 1 << order;
 
-	page[1].mapping = NULL;
-	page[1].index = order;
+	page[1].lru.next = NULL;			/* set dtor */
+	page[1].lru.prev = (void *)order;
 	for (i = 0; i < nr_pages; i++) {
 		struct page *p = page + i;
 
@@ -196,7 +193,7 @@ static void destroy_compound_page(struct
 	int i;
 	int nr_pages = 1 << order;
 
-	if (unlikely(page[1].index != order))
+	if (unlikely((unsigned long)page[1].lru.prev != order))
 		bad_page(page);
 
 	for (i = 0; i < nr_pages; i++) {
--- 2.6.16-rc2-git11/mm/swap.c	2006-02-10 12:28:04.000000000 +0000
+++ 2.6.16-rc2-git11+/mm/swap.c	2006-02-10 19:54:44.000000000 +0000
@@ -40,7 +40,7 @@ static void put_compound_page(struct pag
 	if (put_page_testzero(page)) {
 		void (*dtor)(struct page *page);
 
-		dtor = (void (*)(struct page *))page[1].mapping;
+		dtor = (void (*)(struct page *))page[1].lru.next;
 		(*dtor)(page);
 	}
 }

[PATCH 2/3] compound page: default destructor

[Hugh Dickins]

Somehow I imagined that calling a NULL destructor would free a compound
page rather than oopsing.  No, we must supply a default destructor,
__free_pages_ok using the order noted by prep_compound_page.  hugetlb
can still replace this as before with its own free_huge_page pointer.

The case that needs this is not common: rarely does put_compound_page's
put_page_testzero bring the count down to 0.  But if get_user_pages is
applied to some part of a compound page, without immediate release (e.g.
AIO or Infiniband), then it's possible for its put_page to come after
the containing vma has been unmapped and the driver done its free_pages.

That's just the kind of case compound pages are supposed to be guarding
against (but Nick points out, nor did PageReserved handle this right).

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 mm/page_alloc.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletion(-)

--- 2.6.16-rc2-git11+/mm/page_alloc.c	2006-02-10 20:03:19.000000000 +0000
+++ 2.6.16-rc2-git11++/mm/page_alloc.c	2006-02-10 20:07:05.000000000 +0000
@@ -56,6 +56,7 @@ long nr_swap_pages;
 int percpu_pagelist_fraction;
 
 static void fastcall free_hot_cold_page(struct page *page, int cold);
+static void __free_pages_ok(struct page *page, unsigned int order);
 
 /*
  * results with 256, 32 in the lowmem_reserve sysctl:
@@ -173,12 +174,18 @@ static void bad_page(struct page *page)
  * put_page() function.  Its ->lru.prev holds the order of allocation.
  * This usage means that zero-order pages may not be compound.
  */
+
+static void free_compound_page(struct page *page)
+{
+	__free_pages_ok(page, (unsigned long)page[1].lru.prev);
+}
+
 static void prep_compound_page(struct page *page, unsigned long order)
 {
 	int i;
 	int nr_pages = 1 << order;
 
-	page[1].lru.next = NULL;			/* set dtor */
+	page[1].lru.next = (void *)free_compound_page;	/* set dtor */
 	page[1].lru.prev = (void *)order;
 	for (i = 0; i < nr_pages; i++) {
 		struct page *p = page + i;

[PATCH 3/3] compound page: no access_process_vm check

[Hugh Dickins]

The PageCompound check before access_process_vm's set_page_dirty_lock is
no longer necessary, so remove it.  But leave the PageCompound checks in
bio_set_pages_dirty, dio_bio_complete and nfs_free_user_pages: at least
some of those were introduced as a little optimization on hugetlb pages.

Signed-off-by: Hugh Dickins <hugh@veritas.com>
---

 kernel/ptrace.c |    3 +--
 1 files changed, 1 insertion(+), 2 deletions(-)

--- 2.6.16-rc2-git11/kernel/ptrace.c	2006-02-03 09:32:51.000000000 +0000
+++ 2.6.16-rc2-git11+/kernel/ptrace.c	2006-02-10 20:07:05.000000000 +0000
@@ -242,8 +242,7 @@ int access_process_vm(struct task_struct
 		if (write) {
 			copy_to_user_page(vma, page, addr,
 					  maddr + offset, buf, bytes);
-			if (!PageCompound(page))
-				set_page_dirty_lock(page);
+			set_page_dirty_lock(page);
 		} else {
 			copy_from_user_page(vma, page, addr,
 					    buf, maddr + offset, bytes);

Re: [PATCH 1/3] compound page: use page[1].lru

[Andrew Morton]

Hugh Dickins <hugh@veritas.com> wrote:
>
> If a compound page has its own put_page_testzero destructor (the only
>  current example is free_huge_page), that is noted in page[1].mapping of
>  the compound page.  But that's rather a poor place to keep it: functions
>  which call set_page_dirty_lock after get_user_pages (e.g. Infiniband's
>  __ib_umem_release) ought to be checking first, otherwise set_page_dirty
>  is liable to crash on what's not the address of a struct address_space.
> 
>  And now I'm about to make that worse: it turns out that every compound
>  page needs a destructor, so we can no longer rely on hugetlb pages going
>  their own special way, to avoid further problems of page->mapping reuse.
>  For example, not many people know that: on 50% of i386 -Os builds, the
>  first tail page of a compound page purports to be PageAnon (when its
>  destructor has an odd address), which surprises page_add_file_rmap.
> 
>  Keep the compound page destructor in page[1].lru.next instead.  And to
>  free up the common pairing of mapping and index, also move compound page
>  order from index to lru.prev.  Slab reuses page->lru too: but if we ever
>  need slab to use compound pages, it can easily stack its use above this.

I'm scratching my head over flush_dcache_page() on, say, sparc64.  For
example, the one in fs/direct-io.c.  With this patch, we'll call
flush_dcache_page_impl(), which at least won't crash.  Before the patch I
think we'd just do random stuff.

But I'm not sure that flush_dcache_page(hugetlb tail page) will do the
right thing in aither case?

Re: [PATCH 1/3] compound page: use page[1].lru

[Hugh Dickins]

On Sun, 12 Feb 2006, Andrew Morton wrote:
> 
> I'm scratching my head over flush_dcache_page() on, say, sparc64.  For
> example, the one in fs/direct-io.c.  With this patch, we'll call
> flush_dcache_page_impl(), which at least won't crash.  Before the patch I
> think we'd just do random stuff.

A head-scratching business indeed.  I think your description is right.

I haven't looked up the intersection of the set of arches which have
hugetlb with the set of arches which do something in flush_dcache_page,
but maybe you have, and found sparc64 the only or most significant.

> But I'm not sure that flush_dcache_page(hugetlb tail page) will do the
> right thing in aither case?

Probably not; but nobody seems to have noticed.  Perhaps it all comes
right in the end somehow.  I haven't much of a clue.

Remember that 1/3 is only changing page[1].mapping i.e. it's making
the first tail page behave like all the other constituents of the
compound page, so it can hardly be making matters worse.

Hugh

Re: [PATCH 1/3] compound page: use page[1].lru

[Andrew Morton]

Hugh Dickins <hugh@veritas.com> wrote:
>
> On Sun, 12 Feb 2006, Andrew Morton wrote:
> > 
> > I'm scratching my head over flush_dcache_page() on, say, sparc64.  For
> > example, the one in fs/direct-io.c.  With this patch, we'll call
> > flush_dcache_page_impl(), which at least won't crash.  Before the patch I
> > think we'd just do random stuff.
> 
> A head-scratching business indeed.  I think your description is right.
> 
> I haven't looked up the intersection of the set of arches which have
> hugetlb with the set of arches which do something in flush_dcache_page,
> but maybe you have, and found sparc64 the only or most significant.

We have a page which has no ->mapping, but lo, it's mmapped by userspace
and can be MAP_SHARED between different CPUs and processes.

Yes, I suspect it'll do the wrong thing in unpleasantly subtle ways.

(cc's davem and runs away).

Re: [PATCH 1/3] compound page: use page[1].lru

[David S. Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Sun, 12 Feb 2006 18:13:12 -0800

> We have a page which has no ->mapping, but lo, it's mmapped by userspace
> and can be MAP_SHARED between different CPUs and processes.
> 
> Yes, I suspect it'll do the wrong thing in unpleasantly subtle ways.
> 
> (cc's davem and runs away).

The ->mapping check is there essentially to hit user mapped pages that
would be modified by the kernel using kernel space memory accesses
other than those done by copy_user_page() and clear_user_page() (and
their brothers copy_user_highpage() and clear_user_highpage() which
just call the former directly on a non-HIGHPAGE platform like
sparc64).

Hugepages actually have no D-cache aliasing issues by definition on
sparc64 because the smallest possible hugepage size is 64K which is
larger than the D-cache aliasing factor (which is 16K).

Re: [PATCH 1/3] compound page: use page[1].lru

[Andrew Morton]

"David S. Miller" <davem@davemloft.net> wrote:
>
> From: Andrew Morton <akpm@osdl.org>
> Date: Sun, 12 Feb 2006 18:13:12 -0800
> 
> > We have a page which has no ->mapping, but lo, it's mmapped by userspace
> > and can be MAP_SHARED between different CPUs and processes.
> > 
> > Yes, I suspect it'll do the wrong thing in unpleasantly subtle ways.
> > 
> > (cc's davem and runs away).
> 
> The ->mapping check is there essentially to hit user mapped pages that
> would be modified by the kernel using kernel space memory accesses
> other than those done by copy_user_page() and clear_user_page() (and
> their brothers copy_user_highpage() and clear_user_highpage() which
> just call the former directly on a non-HIGHPAGE platform like
> sparc64).

The direct-io.c code just does memset.  (That's very common - maybe
clear_user_highpage_partial() is needed?)

> Hugepages actually have no D-cache aliasing issues by definition on
> sparc64 because the smallest possible hugepage size is 64K which is
> larger than the D-cache aliasing factor (which is 16K).

OK..  So it would be a bug, except for this hardware quirk.  I'll check the
other architectures.

Re: [PATCH 1/3] compound page: use page[1].lru

[David S. Miller]

From: Andrew Morton <akpm@osdl.org>
Date: Sun, 12 Feb 2006 23:12:23 -0800

> "David S. Miller" <davem@davemloft.net> wrote:
> >
> > The ->mapping check is there essentially to hit user mapped pages that
> > would be modified by the kernel using kernel space memory accesses
> > other than those done by copy_user_page() and clear_user_page() (and
> > their brothers copy_user_highpage() and clear_user_highpage() which
> > just call the former directly on a non-HIGHPAGE platform like
> > sparc64).
> 
> The direct-io.c code just does memset.  (That's very common - maybe
> clear_user_highpage_partial() is needed?)

Yes, something like clear_user_higpage_partial() is definitely needed
for cases like direct-io.c.

We have similar handling for when ptrace() uses get_user_pages() and
tries to write to those pages, via copy_to_user_page() and
copy_from_user_page().  But those interfaces don't clear memory and
they only work in the ptrace path, and they are optimized for that
usage.