2004-04-13 23:28:20

by Chen, Kenneth W

[permalink] [raw]
Subject: hugetlb demand paging patch part [3/3]

Part 3 of 3

3. hugetlb_demand_arch.patch - this adds additional arch specific fixes
for x86 and ia64 when generic demand paging is turned on. Also bulk
of the patch is to clean up with functions that no longer needed.


i386/mm/hugetlbpage.c | 148 +----------------------------------------------
ia64/mm/hugetlbpage.c | 93 -----------------------------
sh/mm/hugetlbpage.c | 94 -----------------------------
sparc64/mm/hugetlbpage.c | 94 -----------------------------
4 files changed, 10 insertions(+), 419 deletions(-)

diff -Nurp linux-2.6.5/arch/i386/mm/hugetlbpage.c linux-2.6.5.htlb/arch/i386/mm/hugetlbpage.c
--- linux-2.6.5/arch/i386/mm/hugetlbpage.c 2004-04-13 11:26:21.000000000 -0700
+++ linux-2.6.5.htlb/arch/i386/mm/hugetlbpage.c 2004-04-13 11:39:44.000000000 -0700
@@ -9,18 +9,14 @@
#include <linux/fs.h>
#include <linux/mm.h>
#include <linux/hugetlb.h>
-#include <linux/pagemap.h>
-#include <linux/smp_lock.h>
-#include <linux/slab.h>
#include <linux/module.h>
#include <linux/err.h>
-#include <linux/sysctl.h>
#include <asm/mman.h>
#include <asm/pgalloc.h>
#include <asm/tlb.h>
#include <asm/tlbflush.h>

-static pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr)
+pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr)
{
pgd_t *pgd;
pmd_t *pmd = NULL;
@@ -41,7 +37,8 @@ static pte_t *huge_pte_offset(struct mm_
return (pte_t *) pmd;
}

-static void set_huge_pte(struct mm_struct *mm, struct vm_area_struct *vma, struct page *page, pte_t * page_table, int write_access)
+void set_huge_pte(struct mm_struct *mm, struct vm_area_struct *vma, struct page *page,
+ pte_t * page_table, int write_access)
{
pte_t entry;

@@ -96,95 +93,6 @@ nomem:
return -ENOMEM;
}

-int
-follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
- struct page **pages, struct vm_area_struct **vmas,
- unsigned long *position, int *length, int i)
-{
- unsigned long vpfn, vaddr = *position;
- int remainder = *length;
-
- WARN_ON(!is_vm_hugetlb_page(vma));
-
- vpfn = vaddr/PAGE_SIZE;
- while (vaddr < vma->vm_end && remainder) {
-
- if (pages) {
- pte_t *pte;
- struct page *page;
-
- pte = huge_pte_offset(mm, vaddr);
-
- /* hugetlb should be locked, and hence, prefaulted */
- WARN_ON(!pte || pte_none(*pte));
-
- page = &pte_page(*pte)[vpfn % (HPAGE_SIZE/PAGE_SIZE)];
-
- WARN_ON(!PageCompound(page));
-
- get_page(page);
- pages[i] = page;
- }
-
- if (vmas)
- vmas[i] = vma;
-
- vaddr += PAGE_SIZE;
- ++vpfn;
- --remainder;
- ++i;
- }
-
- *length = remainder;
- *position = vaddr;
-
- return i;
-}
-
-#if 0 /* This is just for testing */
-struct page *
-follow_huge_addr(struct mm_struct *mm,
- struct vm_area_struct *vma, unsigned long address, int write)
-{
- unsigned long start = address;
- int length = 1;
- int nr;
- struct page *page;
-
- nr = follow_hugetlb_page(mm, vma, &page, NULL, &start, &length, 0);
- if (nr == 1)
- return page;
- return NULL;
-}
-
-/*
- * If virtual address `addr' lies within a huge page, return its controlling
- * VMA, else NULL.
- */
-struct vm_area_struct *hugepage_vma(struct mm_struct *mm, unsigned long addr)
-{
- if (mm->used_hugetlb) {
- struct vm_area_struct *vma = find_vma(mm, addr);
- if (vma && is_vm_hugetlb_page(vma))
- return vma;
- }
- return NULL;
-}
-
-int pmd_huge(pmd_t pmd)
-{
- return 0;
-}
-
-struct page *
-follow_huge_pmd(struct mm_struct *mm, unsigned long address,
- pmd_t *pmd, int write)
-{
- return NULL;
-}
-
-#else
-
struct page *
follow_huge_addr(struct mm_struct *mm,
struct vm_area_struct *vma, unsigned long address, int write)
@@ -209,13 +117,10 @@ follow_huge_pmd(struct mm_struct *mm, un
struct page *page;

page = pte_page(*(pte_t *)pmd);
- if (page) {
+ if (page)
page += ((address & ~HPAGE_MASK) >> PAGE_SHIFT);
- get_page(page);
- }
return page;
}
-#endif

void unmap_hugepage_range(struct vm_area_struct *vma,
unsigned long start, unsigned long end)
@@ -239,48 +144,3 @@ void unmap_hugepage_range(struct vm_area
mm->rss -= (end - start) >> PAGE_SHIFT;
flush_tlb_range(vma, start, end);
}
-
-int hugetlb_prefault(struct address_space *mapping, struct vm_area_struct *vma)
-{
- struct mm_struct *mm = current->mm;
- unsigned long addr;
- int ret = 0;
-
- BUG_ON(vma->vm_start & ~HPAGE_MASK);
- BUG_ON(vma->vm_end & ~HPAGE_MASK);
-
- spin_lock(&mm->page_table_lock);
- for (addr = vma->vm_start; addr < vma->vm_end; addr += HPAGE_SIZE) {
- unsigned long idx;
- pte_t *pte = huge_pte_alloc(mm, addr);
- struct page *page;
-
- if (!pte) {
- ret = -ENOMEM;
- goto out;
- }
- if (!pte_none(*pte))
- continue;
-
- idx = ((addr - vma->vm_start) >> HPAGE_SHIFT)
- + (vma->vm_pgoff >> (HPAGE_SHIFT - PAGE_SHIFT));
- page = find_get_page(mapping, idx);
- if (!page) {
- page = alloc_huge_page();
- if (!page) {
- ret = -ENOMEM;
- goto out;
- }
- ret = add_to_page_cache(page, mapping, idx, GFP_ATOMIC);
- unlock_page(page);
- if (ret) {
- free_huge_page(page);
- goto out;
- }
- }
- set_huge_pte(mm, vma, page, pte, vma->vm_flags & VM_WRITE);
- }
-out:
- spin_unlock(&mm->page_table_lock);
- return ret;
-}
diff -Nurp linux-2.6.5/arch/ia64/mm/hugetlbpage.c linux-2.6.5.htlb/arch/ia64/mm/hugetlbpage.c
--- linux-2.6.5/arch/ia64/mm/hugetlbpage.c 2004-04-13 11:26:21.000000000 -0700
+++ linux-2.6.5.htlb/arch/ia64/mm/hugetlbpage.c 2004-04-13 11:42:09.000000000 -0700
@@ -13,10 +13,6 @@
#include <linux/fs.h>
#include <linux/mm.h>
#include <linux/hugetlb.h>
-#include <linux/pagemap.h>
-#include <linux/smp_lock.h>
-#include <linux/slab.h>
-#include <linux/sysctl.h>
#include <asm/mman.h>
#include <asm/pgalloc.h>
#include <asm/tlb.h>
@@ -24,8 +20,7 @@

unsigned int hpage_shift=HPAGE_SHIFT_DEFAULT;

-static pte_t *
-huge_pte_alloc (struct mm_struct *mm, unsigned long addr)
+pte_t * huge_pte_alloc (struct mm_struct *mm, unsigned long addr)
{
unsigned long taddr = htlbpage_to_page(addr);
pgd_t *pgd;
@@ -58,8 +53,7 @@ huge_pte_offset (struct mm_struct *mm, u

#define mk_pte_huge(entry) { pte_val(entry) |= _PAGE_P; }

-static void
-set_huge_pte (struct mm_struct *mm, struct vm_area_struct *vma,
+void set_huge_pte (struct mm_struct *mm, struct vm_area_struct *vma,
struct page *page, pte_t * page_table, int write_access)
{
pte_t entry;
@@ -116,43 +110,6 @@ nomem:
return -ENOMEM;
}

-int
-follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
- struct page **pages, struct vm_area_struct **vmas,
- unsigned long *st, int *length, int i)
-{
- pte_t *ptep, pte;
- unsigned long start = *st;
- unsigned long pstart;
- int len = *length;
- struct page *page;
-
- do {
- pstart = start & HPAGE_MASK;
- ptep = huge_pte_offset(mm, start);
- pte = *ptep;
-
-back1:
- page = pte_page(pte);
- if (pages) {
- page += ((start & ~HPAGE_MASK) >> PAGE_SHIFT);
- get_page(page);
- pages[i] = page;
- }
- if (vmas)
- vmas[i] = vma;
- i++;
- len--;
- start += PAGE_SIZE;
- if (((start & HPAGE_MASK) == pstart) && len &&
- (start < vma->vm_end))
- goto back1;
- } while (len && start < vma->vm_end);
- *length = len;
- *st = start;
- return i;
-}
-
struct vm_area_struct *hugepage_vma(struct mm_struct *mm, unsigned long addr)
{
if (mm->used_hugetlb) {
@@ -175,7 +132,6 @@ struct page *follow_huge_addr(struct mm_
return NULL;
page = pte_page(*ptep);
page += ((addr & ~HPAGE_MASK) >> PAGE_SHIFT);
- get_page(page);
return page;
}
int pmd_huge(pmd_t pmd)
@@ -263,51 +219,6 @@ void unmap_hugepage_range(struct vm_area
flush_tlb_range(vma, start, end);
}

-int hugetlb_prefault(struct address_space *mapping, struct vm_area_struct *vma)
-{
- struct mm_struct *mm = current->mm;
- unsigned long addr;
- int ret = 0;
-
- BUG_ON(vma->vm_start & ~HPAGE_MASK);
- BUG_ON(vma->vm_end & ~HPAGE_MASK);
-
- spin_lock(&mm->page_table_lock);
- for (addr = vma->vm_start; addr < vma->vm_end; addr += HPAGE_SIZE) {
- unsigned long idx;
- pte_t *pte = huge_pte_alloc(mm, addr);
- struct page *page;
-
- if (!pte) {
- ret = -ENOMEM;
- goto out;
- }
- if (!pte_none(*pte))
- continue;
-
- idx = ((addr - vma->vm_start) >> HPAGE_SHIFT)
- + (vma->vm_pgoff >> (HPAGE_SHIFT - PAGE_SHIFT));
- page = find_get_page(mapping, idx);
- if (!page) {
- page = alloc_huge_page();
- if (!page) {
- ret = -ENOMEM;
- goto out;
- }
- ret = add_to_page_cache(page, mapping, idx, GFP_ATOMIC);
- unlock_page(page);
- if (ret) {
- free_huge_page(page);
- goto out;
- }
- }
- set_huge_pte(mm, vma, page, pte, vma->vm_flags & VM_WRITE);
- }
-out:
- spin_unlock(&mm->page_table_lock);
- return ret;
-}
-
unsigned long hugetlb_get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
unsigned long pgoff, unsigned long flags)
{
diff -Nurp linux-2.6.5/arch/sh/mm/hugetlbpage.c linux-2.6.5.htlb/arch/sh/mm/hugetlbpage.c
--- linux-2.6.5/arch/sh/mm/hugetlbpage.c 2004-04-13 11:26:21.000000000 -0700
+++ linux-2.6.5.htlb/arch/sh/mm/hugetlbpage.c 2004-04-13 11:42:53.000000000 -0700
@@ -13,10 +13,6 @@
#include <linux/fs.h>
#include <linux/mm.h>
#include <linux/hugetlb.h>
-#include <linux/pagemap.h>
-#include <linux/smp_lock.h>
-#include <linux/slab.h>
-#include <linux/sysctl.h>

#include <asm/mman.h>
#include <asm/pgalloc.h>
@@ -24,7 +20,7 @@
#include <asm/tlbflush.h>
#include <asm/cacheflush.h>

-static pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr)
+pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr)
{
pgd_t *pgd;
pmd_t *pmd;
@@ -56,7 +52,7 @@ static pte_t *huge_pte_offset(struct mm_

#define mk_pte_huge(entry) do { pte_val(entry) |= _PAGE_SZHUGE; } while (0)

-static void set_huge_pte(struct mm_struct *mm, struct vm_area_struct *vma,
+void set_huge_pte(struct mm_struct *mm, struct vm_area_struct *vma,
struct page *page, pte_t * page_table, int write_access)
{
unsigned long i;
@@ -124,47 +120,6 @@ nomem:
return -ENOMEM;
}

-int follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
- struct page **pages, struct vm_area_struct **vmas,
- unsigned long *position, int *length, int i)
-{
- unsigned long vaddr = *position;
- int remainder = *length;
-
- WARN_ON(!is_vm_hugetlb_page(vma));
-
- while (vaddr < vma->vm_end && remainder) {
- if (pages) {
- pte_t *pte;
- struct page *page;
-
- pte = huge_pte_offset(mm, vaddr);
-
- /* hugetlb should be locked, and hence, prefaulted */
- BUG_ON(!pte || pte_none(*pte));
-
- page = pte_page(*pte);
-
- WARN_ON(!PageCompound(page));
-
- get_page(page);
- pages[i] = page;
- }
-
- if (vmas)
- vmas[i] = vma;
-
- vaddr += PAGE_SIZE;
- --remainder;
- ++i;
- }
-
- *length = remainder;
- *position = vaddr;
-
- return i;
-}
-
struct page *follow_huge_addr(struct mm_struct *mm,
struct vm_area_struct *vma,
unsigned long address, int write)
@@ -214,48 +169,3 @@ void unmap_hugepage_range(struct vm_area
mm->rss -= (end - start) >> PAGE_SHIFT;
flush_tlb_range(vma, start, end);
}
-
-int hugetlb_prefault(struct address_space *mapping, struct vm_area_struct *vma)
-{
- struct mm_struct *mm = current->mm;
- unsigned long addr;
- int ret = 0;
-
- BUG_ON(vma->vm_start & ~HPAGE_MASK);
- BUG_ON(vma->vm_end & ~HPAGE_MASK);
-
- spin_lock(&mm->page_table_lock);
- for (addr = vma->vm_start; addr < vma->vm_end; addr += HPAGE_SIZE) {
- unsigned long idx;
- pte_t *pte = huge_pte_alloc(mm, addr);
- struct page *page;
-
- if (!pte) {
- ret = -ENOMEM;
- goto out;
- }
- if (!pte_none(*pte))
- continue;
-
- idx = ((addr - vma->vm_start) >> HPAGE_SHIFT)
- + (vma->vm_pgoff >> (HPAGE_SHIFT - PAGE_SHIFT));
- page = find_get_page(mapping, idx);
- if (!page) {
- page = alloc_huge_page();
- if (!page) {
- ret = -ENOMEM;
- goto out;
- }
- ret = add_to_page_cache(page, mapping, idx, GFP_ATOMIC);
- unlock_page(page);
- if (ret) {
- free_huge_page(page);
- goto out;
- }
- }
- set_huge_pte(mm, vma, page, pte, vma->vm_flags & VM_WRITE);
- }
-out:
- spin_unlock(&mm->page_table_lock);
- return ret;
-}
diff -Nurp linux-2.6.5/arch/sparc64/mm/hugetlbpage.c linux-2.6.5.htlb/arch/sparc64/mm/hugetlbpage.c
--- linux-2.6.5/arch/sparc64/mm/hugetlbpage.c 2004-04-13 11:26:21.000000000 -0700
+++ linux-2.6.5.htlb/arch/sparc64/mm/hugetlbpage.c 2004-04-13 11:44:06.000000000 -0700
@@ -9,10 +9,6 @@
#include <linux/fs.h>
#include <linux/mm.h>
#include <linux/hugetlb.h>
-#include <linux/pagemap.h>
-#include <linux/smp_lock.h>
-#include <linux/slab.h>
-#include <linux/sysctl.h>
#include <linux/module.h>

#include <asm/mman.h>
@@ -21,7 +17,7 @@
#include <asm/tlbflush.h>
#include <asm/cacheflush.h>

-static pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr)
+pte_t *huge_pte_alloc(struct mm_struct *mm, unsigned long addr)
{
pgd_t *pgd;
pmd_t *pmd;
@@ -53,7 +49,7 @@ static pte_t *huge_pte_offset(struct mm_

#define mk_pte_huge(entry) do { pte_val(entry) |= _PAGE_SZHUGE; } while (0)

-static void set_huge_pte(struct mm_struct *mm, struct vm_area_struct *vma,
+void set_huge_pte(struct mm_struct *mm, struct vm_area_struct *vma,
struct page *page, pte_t * page_table, int write_access)
{
unsigned long i;
@@ -121,47 +117,6 @@ nomem:
return -ENOMEM;
}

-int follow_hugetlb_page(struct mm_struct *mm, struct vm_area_struct *vma,
- struct page **pages, struct vm_area_struct **vmas,
- unsigned long *position, int *length, int i)
-{
- unsigned long vaddr = *position;
- int remainder = *length;
-
- WARN_ON(!is_vm_hugetlb_page(vma));
-
- while (vaddr < vma->vm_end && remainder) {
- if (pages) {
- pte_t *pte;
- struct page *page;
-
- pte = huge_pte_offset(mm, vaddr);
-
- /* hugetlb should be locked, and hence, prefaulted */
- BUG_ON(!pte || pte_none(*pte));
-
- page = pte_page(*pte);
-
- WARN_ON(!PageCompound(page));
-
- get_page(page);
- pages[i] = page;
- }
-
- if (vmas)
- vmas[i] = vma;
-
- vaddr += PAGE_SIZE;
- --remainder;
- ++i;
- }
-
- *length = remainder;
- *position = vaddr;
-
- return i;
-}
-
struct page *follow_huge_addr(struct mm_struct *mm,
struct vm_area_struct *vma,
unsigned long address, int write)
@@ -211,48 +166,3 @@ void unmap_hugepage_range(struct vm_area
mm->rss -= (end - start) >> PAGE_SHIFT;
flush_tlb_range(vma, start, end);
}
-
-int hugetlb_prefault(struct address_space *mapping, struct vm_area_struct *vma)
-{
- struct mm_struct *mm = current->mm;
- unsigned long addr;
- int ret = 0;
-
- BUG_ON(vma->vm_start & ~HPAGE_MASK);
- BUG_ON(vma->vm_end & ~HPAGE_MASK);
-
- spin_lock(&mm->page_table_lock);
- for (addr = vma->vm_start; addr < vma->vm_end; addr += HPAGE_SIZE) {
- unsigned long idx;
- pte_t *pte = huge_pte_alloc(mm, addr);
- struct page *page;
-
- if (!pte) {
- ret = -ENOMEM;
- goto out;
- }
- if (!pte_none(*pte))
- continue;
-
- idx = ((addr - vma->vm_start) >> HPAGE_SHIFT)
- + (vma->vm_pgoff >> (HPAGE_SHIFT - PAGE_SHIFT));
- page = find_get_page(mapping, idx);
- if (!page) {
- page = alloc_huge_page();
- if (!page) {
- ret = -ENOMEM;
- goto out;
- }
- ret = add_to_page_cache(page, mapping, idx, GFP_ATOMIC);
- unlock_page(page);
- if (ret) {
- free_huge_page(page);
- goto out;
- }
- }
- set_huge_pte(mm, vma, page, pte, vma->vm_flags & VM_WRITE);
- }
-out:
- spin_unlock(&mm->page_table_lock);
- return ret;
-}



2004-04-15 07:27:35

by David Gibson

[permalink] [raw]
Subject: Re: hugetlb demand paging patch part [3/3]

[snip]
> @@ -209,13 +117,10 @@ follow_huge_pmd(struct mm_struct *mm, un
> struct page *page;
>
> page = pte_page(*(pte_t *)pmd);
> - if (page) {
> + if (page)
> page += ((address & ~HPAGE_MASK) >> PAGE_SHIFT);
> - get_page(page);
> - }
> return page;
> }
> -#endif

[snip]

> @@ -175,7 +132,6 @@ struct page *follow_huge_addr(struct mm_
> return NULL;
> page = pte_page(*ptep);
> page += ((addr & ~HPAGE_MASK) >> PAGE_SHIFT);
> - get_page(page);
> return page;
> }

As far as I can tell, the removal of these get_page()s is also
unrelated to the demand paging per se. But afaict removing them is
correct - the corresponding logic in follow_page() for normal pages
doesn't appear to do a get_page(), nor do all archs do a get_page().

Does that sound right to you?

If so, the patch below ought to be safe (and indeed a bugfix) to apply
now:

Index: working-2.6/arch/ppc64/mm/hugetlbpage.c
===================================================================
--- working-2.6.orig/arch/ppc64/mm/hugetlbpage.c 2004-04-15 17:03:43.052825264 +1000
+++ working-2.6/arch/ppc64/mm/hugetlbpage.c 2004-04-15 17:25:11.450920656 +1000
@@ -314,10 +314,8 @@
BUG_ON(! pmd_hugepage(*pmd));

page = hugepte_page(*(hugepte_t *)pmd);
- if (page) {
+ if (page)
page += ((address & ~HPAGE_MASK) >> PAGE_SHIFT);
- get_page(page);
- }
return page;
}

Index: working-2.6/arch/i386/mm/hugetlbpage.c
===================================================================
--- working-2.6.orig/arch/i386/mm/hugetlbpage.c 2004-04-15 17:07:42.813857792 +1000
+++ working-2.6/arch/i386/mm/hugetlbpage.c 2004-04-15 17:25:40.837847480 +1000
@@ -111,7 +111,6 @@

WARN_ON(!PageCompound(page));

- get_page(page);
return page;
}

@@ -167,10 +166,8 @@
struct page *page;

page = pte_page(*(pte_t *)pmd);
- if (page) {
+ if (page)
page += ((address & ~HPAGE_MASK) >> PAGE_SHIFT);
- get_page(page);
- }
return page;
}
#endif
Index: working-2.6/arch/ia64/mm/hugetlbpage.c
===================================================================
--- working-2.6.orig/arch/ia64/mm/hugetlbpage.c 2004-04-15 17:08:30.667905672 +1000
+++ working-2.6/arch/ia64/mm/hugetlbpage.c 2004-04-15 17:26:02.309910776 +1000
@@ -133,7 +133,6 @@
ptep = huge_pte_offset(mm, addr);
page = pte_page(*ptep);
page += ((addr & ~HPAGE_MASK) >> PAGE_SHIFT);
- get_page(page);
return page;
}
int pmd_huge(pmd_t pmd)


--
David Gibson | For every complex problem there is a
david AT gibson.dropbear.id.au | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson

2004-04-15 17:20:31

by Chen, Kenneth W

[permalink] [raw]
Subject: RE: hugetlb demand paging patch part [3/3]

>>>>> David Gibson wrote on Thursday, April 15, 2004 12:26 AM
>
> > @@ -175,7 +132,6 @@ struct page *follow_huge_addr(struct mm_
> > return NULL;
> > page = pte_page(*ptep);
> > page += ((addr & ~HPAGE_MASK) >> PAGE_SHIFT);
> > - get_page(page);
> > return page;
> > }
>
> As far as I can tell, the removal of these get_page()s is also
> unrelated to the demand paging per se. But afaict removing them is
> correct - the corresponding logic in follow_page() for normal pages
> doesn't appear to do a get_page(), nor do all archs do a get_page().
>
> Does that sound right to you?

It's a bug in the code that was never exercised with prefaulting. See
get_user_pages() that short circuits the rest of faulting code with
is_vm_hugetlb_page() test.


> If so, the patch below ought to be safe (and indeed a bugfix) to
> apply now:

Yep, that's correct, I already did x86 and ia64 in one of the three
patches posted. ;-)


2004-04-16 02:36:45

by David Gibson

[permalink] [raw]
Subject: Re: hugetlb demand paging patch part [3/3]

On Thu, Apr 15, 2004 at 10:16:45AM -0700, Chen, Kenneth W wrote:
> >>>>> David Gibson wrote on Thursday, April 15, 2004 12:26 AM
> >
> > > @@ -175,7 +132,6 @@ struct page *follow_huge_addr(struct mm_
> > > return NULL;
> > > page = pte_page(*ptep);
> > > page += ((addr & ~HPAGE_MASK) >> PAGE_SHIFT);
> > > - get_page(page);
> > > return page;
> > > }
> >
> > As far as I can tell, the removal of these get_page()s is also
> > unrelated to the demand paging per se. But afaict removing them is
> > correct - the corresponding logic in follow_page() for normal pages
> > doesn't appear to do a get_page(), nor do all archs do a get_page().
> >
> > Does that sound right to you?
>
> It's a bug in the code that was never exercised with prefaulting. See
> get_user_pages() that short circuits the rest of faulting code with
> is_vm_hugetlb_page() test.

Erm.. it's not clear to me that it could never be exercise:
get_user_pages() is not the only caller of follow_page().

> > If so, the patch below ought to be safe (and indeed a bugfix) to
> > apply now:
>
> Yep, that's correct, I already did x86 and ia64 in one of the three
> patches posted. ;-)

Yes, I know, but I'm trying to separate which parts of your patches
are fixes/cleanups for pre-existing problems, and which are genuinely
new for demand paging.

--
David Gibson | For every complex problem there is a
david AT gibson.dropbear.id.au | solution which is simple, neat and
| wrong.
http://www.ozlabs.org/people/dgibson

2004-04-16 02:49:52

by Chen, Kenneth W

[permalink] [raw]
Subject: RE: hugetlb demand paging patch part [3/3]

>>>> David Gibson wrote on Thursday, April 15, 2004 6:41 PM
> > > > @@ -175,7 +132,6 @@ struct page *follow_huge_addr(struct mm_
> > > > return NULL;
> > > > page = pte_page(*ptep);
> > > > page += ((addr & ~HPAGE_MASK) >> PAGE_SHIFT);
> > > > - get_page(page);
> > > > return page;
> > > > }
> > >
> > > As far as I can tell, the removal of these get_page()s is also
> > > unrelated to the demand paging per se. But afaict removing them is
> > > correct - the corresponding logic in follow_page() for normal pages
> > > doesn't appear to do a get_page(), nor do all archs do a get_page().
> > >
> > > Does that sound right to you?
> >
> > It's a bug in the code that was never exercised with prefaulting. See
> > get_user_pages() that short circuits the rest of faulting code with
> > is_vm_hugetlb_page() test.
>
> Erm.. it's not clear to me that it could never be exercise:
> get_user_pages() is not the only caller of follow_page().

At least we all agree it's a bug :-)


> > > If so, the patch below ought to be safe (and indeed a bugfix) to
> > > apply now:
> >
> > Yep, that's correct, I already did x86 and ia64 in one of the three
> > patches posted. ;-)
>
> Yes, I know, but I'm trying to separate which parts of your patches
> are fixes/cleanups for pre-existing problems, and which are genuinely
> new for demand paging.

The only part I know that is bug fix is the extra get_page() reference
in follow_huge_addr(). All others are for demand paging.

- Ken