RE: [PATCH 1/6] freepgt: free_pgtables use vma list

RE: [PATCH 1/6] freepgt: free_pgtables use vma list

[Luck, Tony]

+	 * Why all these "- 1"s?  Because 0 represents both the bottom
+	 * of the address space and the top of it (using -1 for the
+	 * top wouldn't help much: the masks would do the wrong thing).
+	 * The rule is that addr 0 and floor 0 refer to the bottom of
+	 * the address space, but end 0 and ceiling 0 refer to the top
+	 * Comparisons need to use "end - 1" and "ceiling - 1" (though
+	 * that end 0 case should be mythical).

Can we legislate that "end==0" isn't possible.  On ia64 this is
certainly true (user virtual space is confined to regions 0-4, so
the max value of end is 0xa000000000000000[*]).  Same applies on x86
where the max user address is 0xc0000000 (assuming a standard 3G/1G
split, and ignoring the 4G-4G patch).  What do the other architectures
do?  Does anyone allow:
	mmap((void *)-PAGE_SIZE, PAGE_SIZE, MAP_FIXED, ...)
to succeed?

Otherwise throw in some extra macros to hide the computation needed
to make the masks work on ceiling values that represent the last byte
of the vma rather than the address beyond.  Presumably we can use a
few cycles on some extra arithmetic to help us save gazillions of
cycles for all the cache misses that we currently expend traversing
empty areas of page tables at each level.

Latest patches run on ia64 ... I did at least throw a fork-tester at
it to make sure that we aren't leaking pagetables ... it is still
running after a few million forks, so the simple cases are not doing
anything completely bogus.

-Tony

[*] Since a three level page table doesn't give us enough bits, the
actual limit (with a 16k page size) is 0x8000100000000000 with a hole
for the rest of region 4).

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[David S. Miller]

On Wed, 23 Mar 2005 11:16:27 -0800
"Luck, Tony" <tony.luck@intel.com> wrote:

> Can we legislate that "end==0" isn't possible.

It is possible with some out-of-tree patches.

I could certainly support it on sparc64, the only
glitch is that this is where the virtually mapped
linear page tables sit right now :-)

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Hugh Dickins]

On Wed, 23 Mar 2005, David S. Miller wrote:
> On Wed, 23 Mar 2005 11:16:27 -0800
> "Luck, Tony" <tony.luck@intel.com> wrote:
> 
> > Can we legislate that "end==0" isn't possible.
> 
> It is possible with some out-of-tree patches.
> 
> I could certainly support it on sparc64, the only
> glitch is that this is where the virtually mapped
> linear page tables sit right now :-)

Though my knowledge of out-of-tree patches is very limited,
I believe "end == 0" is not possible on any arch - when "end"
originates from vma->vm_end (or vm_struct->addr + size).  There
are plenty of "BUG_ON(addr >= end)"s dotted around to support that,
and other places that would be confused by vm_start > vm_end.

(And when Linus first proposed the sysenter page at 0xfffff000,
I protested, and he brought it down to 0xffffe000: I think we'll
do well ever to keep that last virtual page invalid.)

But certainly "ceiling == 0" is possible and common, and "rounded-up
end" may well be 0 with out-of-tree patches.  When I did those
free_pgtables tests, it seemed simpler to treat "end" in the same
way as "ceiling", implicitly allowing it the 0 case.  Perhaps
that's not so in Nick's version, I've yet to think through it.

Hugh

RE: [PATCH 1/6] freepgt: free_pgtables use vma list

[Paul Mackerras]

Luck, Tony writes:

> Can we legislate that "end==0" isn't possible.

I think this is only likely to be a problem on 32-bit platforms with
hardware support for separate user and kernel address spaces.  m68k
and sparc32 come to mind, though I might be mistaken.

Paul.

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Andreas Schwab]

Paul Mackerras <paulus@samba.org> writes:

> Luck, Tony writes:
>
>> Can we legislate that "end==0" isn't possible.
>
> I think this is only likely to be a problem on 32-bit platforms with
> hardware support for separate user and kernel address spaces.  m68k
> and sparc32 come to mind, though I might be mistaken.

On m68k we don't allow addresses above 0xF0000000.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

RE: [PATCH 1/6] freepgt: free_pgtables use vma list

[Luck, Tony]

>Though my knowledge of out-of-tree patches is very limited,
>I believe "end == 0" is not possible on any arch - when "end"
>originates from vma->vm_end (or vm_struct->addr + size).  There
>are plenty of "BUG_ON(addr >= end)"s dotted around to support that,
>and other places that would be confused by vm_start > vm_end.
>
>(And when Linus first proposed the sysenter page at 0xfffff000,
>I protested, and he brought it down to 0xffffe000: I think we'll
>do well ever to keep that last virtual page invalid.)

IS_ERR(ptr) and PTR_ERR(ptr) would also yield some interesting bizarre
errors if the last page (last 1000 bytes in the current implementation
of IS_ERR) were valid!

>But certainly "ceiling == 0" is possible and common, and "rounded-up
>end" may well be 0 with out-of-tree patches.  When I did those
>free_pgtables tests, it seemed simpler to treat "end" in the same
>way as "ceiling", implicitly allowing it the 0 case.  Perhaps
>that's not so in Nick's version, I've yet to think through it.

Yes ... rounding 'end' up to pmd/pud/pgd boundaries can certainly
wrap around to zero ... giving up the last page of address space
seems reasonable.  Giving up the last PGD_SIZE just to make some
math a bit easier sounds like overkill.

-Tony

[PATCH 0/6] freepgt: free_pgtables shakeup

[Hugh Dickins]

Here's the recut of those patches, including David Miller's vital fixes.
I'm addressing these to Nick rather than Andrew, because they're perhaps
not fit for -mm until more testing done and the x86_64 32-bit vdso issue
handled.  I'm unlikely to be responsive until next week, sorry: over to
you, Nick - thanks.

Hugh

 arch/i386/mm/hugetlbpage.c    |   11 --
 arch/i386/mm/pgtable.c        |    2 
 arch/ia64/mm/hugetlbpage.c    |   60 ++++---------
 arch/ppc64/mm/hugetlbpage.c   |   47 ----------
 include/asm-generic/pgtable.h |    8 -
 include/asm-ia64/page.h       |    2 
 include/asm-ia64/pgtable.h    |   30 ------
 include/asm-ia64/processor.h  |    8 -
 include/asm-ppc64/pgtable.h   |   12 +-
 include/asm-ppc64/processor.h |    4 
 include/asm-s390/processor.h  |    2 
 include/asm-sparc64/pgtable.h |   15 ---
 include/linux/hugetlb.h       |    6 -
 include/linux/mm.h            |   14 +--
 mm/memory.c                   |  190 ++++++++++++++++++++++++++++++------------
 mm/mmap.c                     |  139 ++++++++----------------------
 16 files changed, 226 insertions(+), 324 deletions(-)

[PATCH 1/6] freepgt: free_pgtables use vma list

[Hugh Dickins]

Recent woes with some arches needing their own pgd_addr_end macro; and
4-level clear_page_range regression since 2.6.10's clear_page_tables;
and its long-standing well-known inefficiency in searching throughout
the higher-level page tables for those few entries to clear and free:
all can be blamed on ignoring the list of vmas when we free page tables.

Replace exit_mmap's clear_page_range of the total user address space by
free_pgtables operating on the mm's vma list; unmap_region use it in the
same way, giving floor and ceiling beyond which it may not free tables.
This brings lmbench fork/exec/sh numbers back to 2.6.10 (unless preempt
is enabled, in which case latency fixes spoil unmap_vmas throughput).

Beware: the do_mmap_pgoff driver failure case must now use unmap_region
instead of zap_page_range, since a page table might have been allocated,
and can only be freed while it is touched by some vma.

Move free_pgtables from mmap.c to memory.c, where its lower levels are
adapted from the clear_page_range levels.  (Most of free_pgtables' old
code was actually for a non-existent case, prev not properly set up,
dating from before hch gave us split_vma.)  Pass mmu_gather** in the
public interfaces, since we might want to add latency lockdrops later;
but no attempt to do so yet, going by vma should itself reduce latency.

But what if is_hugepage_only_range?  Those ia64 and ppc64 cases need
careful examination: put that off until a later patch of the series.

What of x86_64's 32bit vdso page __map_syscall32 maps outside any vma?

And the range to sparc64's flush_tlb_pgtables?  It's less clear to me
now that we need to do more than is done here - every PMD_SIZE ever
occupied will be flushed, do we really have to flush every PGDIR_SIZE
ever partially occupied?  A shame to complicate it unnecessarily.

Special thanks to David Miller for time spent repairing my ceilings.

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

 arch/i386/mm/pgtable.c     |    2 
 arch/ia64/mm/hugetlbpage.c |   37 ----------
 include/linux/mm.h         |    3 
 mm/memory.c                |  152 ++++++++++++++++++++++++++++++++++-----------
 mm/mmap.c                  |  102 ++++++------------------------
 5 files changed, 141 insertions(+), 155 deletions(-)

--- 2.6.12-rc1-bk1/arch/i386/mm/pgtable.c	2005-03-02 07:39:18.000000000 +0000
+++ freepgt1/arch/i386/mm/pgtable.c	2005-03-21 19:06:35.000000000 +0000
@@ -255,6 +255,6 @@ void pgd_free(pgd_t *pgd)
 	if (PTRS_PER_PMD > 1)
 		for (i = 0; i < USER_PTRS_PER_PGD; ++i)
 			kmem_cache_free(pmd_cache, (void *)__va(pgd_val(pgd[i])-1));
-	/* in the non-PAE case, clear_page_range() clears user pgd entries */
+	/* in the non-PAE case, free_pgtables() clears user pgd entries */
 	kmem_cache_free(pgd_cache, pgd);
 }
--- 2.6.12-rc1-bk1/arch/ia64/mm/hugetlbpage.c	2005-03-18 10:22:18.000000000 +0000
+++ freepgt1/arch/ia64/mm/hugetlbpage.c	2005-03-21 19:06:35.000000000 +0000
@@ -187,45 +187,12 @@ follow_huge_pmd(struct mm_struct *mm, un
 }
 
 /*
- * Same as generic free_pgtables(), except constant PGDIR_* and pgd_offset
- * are hugetlb region specific.
+ * Do nothing, until we've worked out what to do!  To allow build, we
+ * must remove reference to clear_page_range since it no longer exists.
  */
 void hugetlb_free_pgtables(struct mmu_gather *tlb, struct vm_area_struct *prev,
 	unsigned long start, unsigned long end)
 {
-	unsigned long first = start & HUGETLB_PGDIR_MASK;
-	unsigned long last = end + HUGETLB_PGDIR_SIZE - 1;
-	struct mm_struct *mm = tlb->mm;
-
-	if (!prev) {
-		prev = mm->mmap;
-		if (!prev)
-			goto no_mmaps;
-		if (prev->vm_end > start) {
-			if (last > prev->vm_start)
-				last = prev->vm_start;
-			goto no_mmaps;
-		}
-	}
-	for (;;) {
-		struct vm_area_struct *next = prev->vm_next;
-
-		if (next) {
-			if (next->vm_start < start) {
-				prev = next;
-				continue;
-			}
-			if (last > next->vm_start)
-				last = next->vm_start;
-		}
-		if (prev->vm_end > first)
-			first = prev->vm_end;
-		break;
-	}
-no_mmaps:
-	if (last < first)	/* for arches with discontiguous pgd indices */
-		return;
-	clear_page_range(tlb, first, last);
 }
 
 void unmap_hugepage_range(struct vm_area_struct *vma, unsigned long start, unsigned long end)
--- 2.6.12-rc1-bk1/include/linux/mm.h	2005-03-18 10:22:43.000000000 +0000
+++ freepgt1/include/linux/mm.h	2005-03-21 19:06:35.000000000 +0000
@@ -591,7 +591,8 @@ int unmap_vmas(struct mmu_gather **tlbp,
 		struct vm_area_struct *start_vma, unsigned long start_addr,
 		unsigned long end_addr, unsigned long *nr_accounted,
 		struct zap_details *);
-void clear_page_range(struct mmu_gather *tlb, unsigned long addr, unsigned long end);
+void free_pgtables(struct mmu_gather **tlb, struct vm_area_struct *vma,
+		unsigned long floor, unsigned long ceiling);
 int copy_page_range(struct mm_struct *dst, struct mm_struct *src,
 			struct vm_area_struct *vma);
 int zeromap_page_range(struct vm_area_struct *vma, unsigned long from,
--- 2.6.12-rc1-bk1/mm/memory.c	2005-03-18 10:22:45.000000000 +0000
+++ freepgt1/mm/memory.c	2005-03-23 15:27:19.000000000 +0000
@@ -110,87 +110,165 @@ void pmd_clear_bad(pmd_t *pmd)
  * Note: this doesn't free the actual pages themselves. That
  * has been handled earlier when unmapping all the memory regions.
  */
-static inline void clear_pte_range(struct mmu_gather *tlb, pmd_t *pmd,
-				unsigned long addr, unsigned long end)
+static void free_pte_range(struct mmu_gather *tlb, pmd_t *pmd)
 {
-	if (!((addr | end) & ~PMD_MASK)) {
-		/* Only free fully aligned ranges */
-		struct page *page = pmd_page(*pmd);
-		pmd_clear(pmd);
-		dec_page_state(nr_page_table_pages);
-		tlb->mm->nr_ptes--;
-		pte_free_tlb(tlb, page);
-	}
+	struct page *page = pmd_page(*pmd);
+	pmd_clear(pmd);
+	pte_free_tlb(tlb, page);
+	dec_page_state(nr_page_table_pages);
+	tlb->mm->nr_ptes--;
 }
 
-static inline void clear_pmd_range(struct mmu_gather *tlb, pud_t *pud,
-				unsigned long addr, unsigned long end)
+static inline void free_pmd_range(struct mmu_gather *tlb, pud_t *pud,
+				unsigned long addr, unsigned long end,
+				unsigned long floor, unsigned long ceiling)
 {
 	pmd_t *pmd;
 	unsigned long next;
-	pmd_t *empty_pmd = NULL;
+	unsigned long start;
 
+	start = addr;
 	pmd = pmd_offset(pud, addr);
-
-	/* Only free fully aligned ranges */
-	if (!((addr | end) & ~PUD_MASK))
-		empty_pmd = pmd;
 	do {
 		next = pmd_addr_end(addr, end);
 		if (pmd_none_or_clear_bad(pmd))
 			continue;
-		clear_pte_range(tlb, pmd, addr, next);
+		free_pte_range(tlb, pmd);
 	} while (pmd++, addr = next, addr != end);
 
-	if (empty_pmd) {
-		pud_clear(pud);
-		pmd_free_tlb(tlb, empty_pmd);
+	start &= PUD_MASK;
+	if (start < floor)
+		return;
+	if (ceiling) {
+		ceiling &= PUD_MASK;
+		if (!ceiling)
+			return;
 	}
+	if (end - 1 > ceiling - 1)
+		return;
+
+	pmd = pmd_offset(pud, start);
+	pud_clear(pud);
+	pmd_free_tlb(tlb, pmd);
 }
 
-static inline void clear_pud_range(struct mmu_gather *tlb, pgd_t *pgd,
-				unsigned long addr, unsigned long end)
+static inline void free_pud_range(struct mmu_gather *tlb, pgd_t *pgd,
+				unsigned long addr, unsigned long end,
+				unsigned long floor, unsigned long ceiling)
 {
 	pud_t *pud;
 	unsigned long next;
-	pud_t *empty_pud = NULL;
+	unsigned long start;
 
+	start = addr;
 	pud = pud_offset(pgd, addr);
-
-	/* Only free fully aligned ranges */
-	if (!((addr | end) & ~PGDIR_MASK))
-		empty_pud = pud;
 	do {
 		next = pud_addr_end(addr, end);
 		if (pud_none_or_clear_bad(pud))
 			continue;
-		clear_pmd_range(tlb, pud, addr, next);
+		free_pmd_range(tlb, pud, addr, next, floor, ceiling);
 	} while (pud++, addr = next, addr != end);
 
-	if (empty_pud) {
-		pgd_clear(pgd);
-		pud_free_tlb(tlb, empty_pud);
+	start &= PGDIR_MASK;
+	if (start < floor)
+		return;
+	if (ceiling) {
+		ceiling &= PGDIR_MASK;
+		if (!ceiling)
+			return;
 	}
+	if (end - 1 > ceiling - 1)
+		return;
+
+	pud = pud_offset(pgd, start);
+	pgd_clear(pgd);
+	pud_free_tlb(tlb, pud);
 }
 
 /*
- * This function clears user-level page tables of a process.
- * Unlike other pagetable walks, some memory layouts might give end 0.
+ * This function frees user-level page tables of a process.
+ *
  * Must be called with pagetable lock held.
  */
-void clear_page_range(struct mmu_gather *tlb,
-				unsigned long addr, unsigned long end)
+static inline void free_pgd_range(struct mmu_gather *tlb,
+			unsigned long addr, unsigned long end,
+			unsigned long floor, unsigned long ceiling)
 {
 	pgd_t *pgd;
 	unsigned long next;
+	unsigned long start;
 
+	/*
+	 * The next few lines have given us lots of grief...
+	 *
+	 * Why are we testing PMD* at this top level?  Because often
+	 * there will be no work to do at all, and we'd prefer not to
+	 * go all the way down to the bottom just to discover that.
+	 *
+	 * Why all these "- 1"s?  Because 0 represents both the bottom
+	 * of the address space and the top of it (using -1 for the
+	 * top wouldn't help much: the masks would do the wrong thing).
+	 * The rule is that addr 0 and floor 0 refer to the bottom of
+	 * the address space, but end 0 and ceiling 0 refer to the top
+	 * Comparisons need to use "end - 1" and "ceiling - 1" (though
+	 * that end 0 case should be mythical).
+	 *
+	 * Wherever addr is brought up or ceiling brought down, we must
+	 * be careful to reject "the opposite 0" before it confuses the
+	 * subsequent tests.  But what about where end is brought down
+	 * by PMD_SIZE below? no, end can't go down to 0 there.
+	 *
+	 * Whereas we round start (addr) and ceiling down, by different
+	 * masks at different levels, in order to test whether a table
+	 * now has no other vmas using it, so can be freed, we don't
+	 * bother to round floor or end up - the tests don't need that.
+	 */
+
+	addr &= PMD_MASK;
+	if (addr < floor) {
+		addr += PMD_SIZE;
+		if (!addr)
+			return;
+	}
+	if (ceiling) {
+		ceiling &= PMD_MASK;
+		if (!ceiling)
+			return;
+	}
+	if (end - 1 > ceiling - 1)
+		end -= PMD_SIZE;
+	if (addr > end - 1)
+		return;
+
+	start = addr;
 	pgd = pgd_offset(tlb->mm, addr);
 	do {
 		next = pgd_addr_end(addr, end);
 		if (pgd_none_or_clear_bad(pgd))
 			continue;
-		clear_pud_range(tlb, pgd, addr, next);
+		free_pud_range(tlb, pgd, addr, next, floor, ceiling);
 	} while (pgd++, addr = next, addr != end);
+
+	if (!tlb_is_full_mm(tlb))
+		flush_tlb_pgtables(tlb->mm, start, end);
+}
+
+void free_pgtables(struct mmu_gather **tlb, struct vm_area_struct *vma,
+				unsigned long floor, unsigned long ceiling)
+{
+	while (vma) {
+		struct vm_area_struct *next = vma->vm_next;
+		unsigned long addr = vma->vm_start;
+
+		/* Optimization: gather nearby vmas into a single call down */
+		while (next && next->vm_start <= vma->vm_end + PMD_SIZE) {
+			vma = next;
+			next = vma->vm_next;
+		}
+		free_pgd_range(*tlb, addr, vma->vm_end,
+				floor, next? next->vm_start: ceiling);
+		vma = next;
+	}
 }
 
 pte_t fastcall * pte_alloc_map(struct mm_struct *mm, pmd_t *pmd, unsigned long address)
--- 2.6.12-rc1-bk1/mm/mmap.c	2005-03-18 10:22:45.000000000 +0000
+++ freepgt1/mm/mmap.c	2005-03-21 19:06:35.000000000 +0000
@@ -29,6 +29,10 @@
 #include <asm/cacheflush.h>
 #include <asm/tlb.h>
 
+static void unmap_region(struct mm_struct *mm,
+		struct vm_area_struct *vma, struct vm_area_struct *prev,
+		unsigned long start, unsigned long end);
+
 /*
  * WARNING: the debugging will use recursive algorithms so never enable this
  * unless you know what you are doing.
@@ -1129,7 +1133,8 @@ unmap_and_free_vma:
 	fput(file);
 
 	/* Undo any partial mapping done by a device driver. */
-	zap_page_range(vma, vma->vm_start, vma->vm_end - vma->vm_start, NULL);
+	unmap_region(mm, vma, prev, vma->vm_start, vma->vm_end);
+	charged = 0;
 free_vma:
 	kmem_cache_free(vm_area_cachep, vma);
 unacct_error:
@@ -1572,66 +1577,6 @@ find_extend_vma(struct mm_struct * mm, u
 }
 #endif
 
-/*
- * Try to free as many page directory entries as we can,
- * without having to work very hard at actually scanning
- * the page tables themselves.
- *
- * Right now we try to free page tables if we have a nice
- * PGDIR-aligned area that got free'd up. We could be more
- * granular if we want to, but this is fast and simple,
- * and covers the bad cases.
- *
- * "prev", if it exists, points to a vma before the one
- * we just free'd - but there's no telling how much before.
- */
-static void free_pgtables(struct mmu_gather *tlb, struct vm_area_struct *prev,
-	unsigned long start, unsigned long end)
-{
-	unsigned long first = start & PGDIR_MASK;
-	unsigned long last = end + PGDIR_SIZE - 1;
-	struct mm_struct *mm = tlb->mm;
-
-	if (last > MM_VM_SIZE(mm) || last < end)
-		last = MM_VM_SIZE(mm);
-
-	if (!prev) {
-		prev = mm->mmap;
-		if (!prev)
-			goto no_mmaps;
-		if (prev->vm_end > start) {
-			if (last > prev->vm_start)
-				last = prev->vm_start;
-			goto no_mmaps;
-		}
-	}
-	for (;;) {
-		struct vm_area_struct *next = prev->vm_next;
-
-		if (next) {
-			if (next->vm_start < start) {
-				prev = next;
-				continue;
-			}
-			if (last > next->vm_start)
-				last = next->vm_start;
-		}
-		if (prev->vm_end > first)
-			first = prev->vm_end;
-		break;
-	}
-no_mmaps:
-	if (last < first)	/* for arches with discontiguous pgd indices */
-		return;
-	if (first < FIRST_USER_PGD_NR * PGDIR_SIZE)
-		first = FIRST_USER_PGD_NR * PGDIR_SIZE;
-	/* No point trying to free anything if we're in the same pte page */
-	if ((first & PMD_MASK) < (last & PMD_MASK)) {
-		clear_page_range(tlb, first, last);
-		flush_tlb_pgtables(mm, first, last);
-	}
-}
-
 /* Normal function to fix up a mapping
  * This function is the default for when an area has no specific
  * function.  This may be used as part of a more specific routine.
@@ -1674,24 +1619,22 @@ static void unmap_vma_list(struct mm_str
  * Called with the page table lock held.
  */
 static void unmap_region(struct mm_struct *mm,
-	struct vm_area_struct *vma,
-	struct vm_area_struct *prev,
-	unsigned long start,
-	unsigned long end)
+		struct vm_area_struct *vma, struct vm_area_struct *prev,
+		unsigned long start, unsigned long end)
 {
+	struct vm_area_struct *next = prev? prev->vm_next: mm->mmap;
 	struct mmu_gather *tlb;
 	unsigned long nr_accounted = 0;
 
 	lru_add_drain();
+	spin_lock(&mm->page_table_lock);
 	tlb = tlb_gather_mmu(mm, 0);
 	unmap_vmas(&tlb, mm, vma, start, end, &nr_accounted, NULL);
 	vm_unacct_memory(nr_accounted);
-
-	if (is_hugepage_only_range(start, end - start))
-		hugetlb_free_pgtables(tlb, prev, start, end);
-	else
-		free_pgtables(tlb, prev, start, end);
+	free_pgtables(&tlb, vma, prev? prev->vm_end: 0,
+				 next? next->vm_start: 0);
 	tlb_finish_mmu(tlb, start, end);
+	spin_unlock(&mm->page_table_lock);
 }
 
 /*
@@ -1823,9 +1766,7 @@ int do_munmap(struct mm_struct *mm, unsi
 	 * Remove the vma's, and unmap the actual pages
 	 */
 	detach_vmas_to_be_unmapped(mm, mpnt, prev, end);
-	spin_lock(&mm->page_table_lock);
 	unmap_region(mm, mpnt, prev, start, end);
-	spin_unlock(&mm->page_table_lock);
 
 	/* Fix up all other VM information */
 	unmap_vma_list(mm, mpnt);
@@ -1957,25 +1898,21 @@ EXPORT_SYMBOL(do_brk);
 void exit_mmap(struct mm_struct *mm)
 {
 	struct mmu_gather *tlb;
-	struct vm_area_struct *vma;
+	struct vm_area_struct *vma = mm->mmap;
 	unsigned long nr_accounted = 0;
 
 	lru_add_drain();
 
 	spin_lock(&mm->page_table_lock);
 
-	tlb = tlb_gather_mmu(mm, 1);
 	flush_cache_mm(mm);
-	/* Use ~0UL here to ensure all VMAs in the mm are unmapped */
-	mm->map_count -= unmap_vmas(&tlb, mm, mm->mmap, 0,
-					~0UL, &nr_accounted, NULL);
+	tlb = tlb_gather_mmu(mm, 1);
+	/* Use -1 here to ensure all VMAs in the mm are unmapped */
+	mm->map_count -= unmap_vmas(&tlb, mm, vma, 0, -1, &nr_accounted, NULL);
 	vm_unacct_memory(nr_accounted);
-	BUG_ON(mm->map_count);	/* This is just debugging */
-	clear_page_range(tlb, FIRST_USER_PGD_NR * PGDIR_SIZE, MM_VM_SIZE(mm));
-	
+	free_pgtables(&tlb, vma, 0, 0);
 	tlb_finish_mmu(tlb, 0, MM_VM_SIZE(mm));
 
-	vma = mm->mmap;
 	mm->mmap = mm->mmap_cache = NULL;
 	mm->mm_rb = RB_ROOT;
 	mm->rss = 0;
@@ -1993,6 +1930,9 @@ void exit_mmap(struct mm_struct *mm)
 		remove_vm_struct(vma);
 		vma = next;
 	}
+
+	BUG_ON(mm->map_count);	/* This is just debugging */
+	BUG_ON(mm->nr_ptes);	/* This is just debugging */
 }
 
 /* Insert vm structure into process list sorted by address

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Andi Kleen]

On Wed, Mar 23, 2005 at 05:11:34PM +0000, Hugh Dickins wrote:
> Recent woes with some arches needing their own pgd_addr_end macro; and
> 4-level clear_page_range regression since 2.6.10's clear_page_tables;
> and its long-standing well-known inefficiency in searching throughout
> the higher-level page tables for those few entries to clear and free:
> all can be blamed on ignoring the list of vmas when we free page tables.
> 
> Replace exit_mmap's clear_page_range of the total user address space by
> free_pgtables operating on the mm's vma list; unmap_region use it in the
> same way, giving floor and ceiling beyond which it may not free tables.
> This brings lmbench fork/exec/sh numbers back to 2.6.10 (unless preempt
> is enabled, in which case latency fixes spoil unmap_vmas throughput).
> 
> Beware: the do_mmap_pgoff driver failure case must now use unmap_region
> instead of zap_page_range, since a page table might have been allocated,
> and can only be freed while it is touched by some vma.
> 
> Move free_pgtables from mmap.c to memory.c, where its lower levels are
> adapted from the clear_page_range levels.  (Most of free_pgtables' old
> code was actually for a non-existent case, prev not properly set up,
> dating from before hch gave us split_vma.)  Pass mmu_gather** in the
> public interfaces, since we might want to add latency lockdrops later;
> but no attempt to do so yet, going by vma should itself reduce latency.
> 
> But what if is_hugepage_only_range?  Those ia64 and ppc64 cases need
> careful examination: put that off until a later patch of the series.

Sorry for late answer. Nice approach.... It will not work as well
on large sparse mappings as the bit vectors, but that may be tolerable.

> 
> What of x86_64's 32bit vdso page __map_syscall32 maps outside any vma?

Everything. It could be easily changed though, but I was too lazy for 
it so far. Do you think it is needed for your patch?

-Andi

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Hugh Dickins]

On Thu, 24 Mar 2005, Andi Kleen wrote:
> On Wed, Mar 23, 2005 at 05:11:34PM +0000, Hugh Dickins wrote:
> 
> Sorry for late answer.

Ditto!  Sorry, I've been away a few days.

> Nice approach....

Thanks.

> It will not work as well
> on large sparse mappings as the bit vectors, but that may be tolerable.

Exactly.  It's simply what what we should be doing first, making use of
the infrastructure we already have.  If that proves inadequate, add on top.

> > What of x86_64's 32bit vdso page __map_syscall32 maps outside any vma?
> 
> Everything. It could be easily changed though, but I was too lazy for 
> it so far. Do you think it is needed for your patch?

I do.  I'll resend you an earlier mail I wrote about it, I think x86_64
is liable to leak pagetables or conversely rip pagetables out from under
the vsyscall page - in the 32-bit emulation case, with my patches, if
that vsyscall page has been mapped.  That it'll be fine or unnoticed
most of the time, but really not right.

I'll also resend you Ben's mail on the subject, what he does on ppc64.

Ah, you do SetPageReserved on that page.  That's good, rmap would have
a problem with it, since it doesn't belong to a file, yet is shared
between all tasks, so is quite unlike an anonymous page.  I suggest
you make the vma VM_RESERVED too, but that doesn't really matter yet.

Hugh

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Andi Kleen]

On Tue, Mar 29, 2005 at 11:03:02PM +0100, Hugh Dickins wrote:
> 
> > Nice approach....
> 
> Thanks.
> 
> > It will not work as well
> > on large sparse mappings as the bit vectors, but that may be tolerable.
> 
> Exactly.  It's simply what what we should be doing first, making use of
> the infrastructure we already have.  If that proves inadequate, add on top.

Ok. I will defer the bitvector patch now. 

I had it mostly working with hacks, but than I ran into
a nasty include ordering problem that scared me off so far.

> I do.  I'll resend you an earlier mail I wrote about it, I think x86_64
> is liable to leak pagetables or conversely rip pagetables out from under
> the vsyscall page - in the 32-bit emulation case, with my patches, if
> that vsyscall page has been mapped.  That it'll be fine or unnoticed
> most of the time, but really not right.
> 
> I'll also resend you Ben's mail on the subject, what he does on ppc64.

Thanks.
> 
> Ah, you do SetPageReserved on that page.  That's good, rmap would have
> a problem with it, since it doesn't belong to a file, yet is shared
> between all tasks, so is quite unlike an anonymous page.  I suggest
> you make the vma VM_RESERVED too, but that doesn't really matter yet.

Ok. I will change it to a VMA.

Only bad thing is that this has to be done at program startup.
At fault time we cannot upgrade the read lock on mmap sem to a write
lock that is needed to insert the VMA :/ But I guess that is ok
because with modern glibc basically all programs will use vsyscsall.

-Andi

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Hugh Dickins]

On Wed, 30 Mar 2005, Andi Kleen wrote:
> 
> Ok. I will defer the bitvector patch now. 
> 
> I had it mostly working with hacks, but than I ran into
> a nasty include ordering problem that scared me off so far.

Hah, you too!  I knew Ben and Nick had designs in that kind
of direction, and meant to leave the field clear for them; but
once the vma idea struck me, it seemed silly not to pursue it.

> Ok. I will change it to a VMA.

Thanks.  (It's only the 32-bit emulation case I'm caring about,
that poses a problem for free_pgtables: I'm not sure whether you're
meaning to VMA-ize the 64-bit one too, that's entirely up to you.)

> Only bad thing is that this has to be done at program startup.
> At fault time we cannot upgrade the read lock on mmap sem to a write
> lock that is needed to insert the VMA :/ But I guess that is ok
> because with modern glibc basically all programs will use vsyscsall.

Sorry for bumping you into this, but I think it's the right approach.
ARM can justify its special FIRST_USER_ADDRESS treatment because (as
I understaned it) the low vectors just have to be set up earlier and
cleared later, but your case is at a higher level.

Hugh

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Andi Kleen]

> > Ok. I will change it to a VMA.
> 
> Thanks.  (It's only the 32-bit emulation case I'm caring about,

I did the patch now and it works, but due to some technical problems I can only
upload it next week. Surprisingly the new code is actually shorter
than the old one and cleaner too.

> that poses a problem for free_pgtables: I'm not sure whether you're
> meaning to VMA-ize the 64-bit one too, that's entirely up to you.)

64bit is beyond __PAGE_OFFSET and mapped by the kernel, there are no page 
tables to free. I dont see any sense in making it a VMA.


-Andi

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Nick Piggin]

Hugh Dickins wrote:

> And the range to sparc64's flush_tlb_pgtables?  It's less clear to me
> now that we need to do more than is done here - every PMD_SIZE ever
> occupied will be flushed, do we really have to flush every PGDIR_SIZE
> ever partially occupied?  A shame to complicate it unnecessarily.

It looks like sparc64 is the only user of this, so it is up to
you Dave.

I don't think I'd be able to decipher how sparc64 implements this.

I think Hugh and I interpreted your message different ways.

So, to make the question more concrete: if a pgd_t is freed due
to freeing the single pmd_t contained within it (which was the
only part of the pgd's address space that contained a valid mapping)
Then do you need the full PGDIR_SIZE width passed to
flush_tlb_pgtables, or just the PMD_SIZE'd start,end that covered
the freed pmd_t?

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Nick Piggin]

Nick Piggin wrote:
> Hugh Dickins wrote:
> 
>> And the range to sparc64's flush_tlb_pgtables?  It's less clear to me
>> now that we need to do more than is done here - every PMD_SIZE ever
>> occupied will be flushed, do we really have to flush every PGDIR_SIZE
>> ever partially occupied?  A shame to complicate it unnecessarily.
> 
> 
> It looks like sparc64 is the only user of this, so it is up to
> you Dave.
> 

Oh - one other question too. Doing the unmap and page table freeing in
the same pass will put freed pagecache pages in the same mmu_gather as
the freed page table pages. This looks like it may be a problem for
sparc64?

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[David S. Miller]

On Fri, 25 Mar 2005 16:35:12 +1100
Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> Oh - one other question too. Doing the unmap and page table freeing in
> the same pass will put freed pagecache pages in the same mmu_gather as
> the freed page table pages. This looks like it may be a problem for
> sparc64?

No, that's fine, the code can handle this situation.

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[David S. Miller]

On Fri, 25 Mar 2005 16:32:07 +1100
Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> So, to make the question more concrete: if a pgd_t is freed due
> to freeing the single pmd_t contained within it (which was the
> only part of the pgd's address space that contained a valid mapping)
> Then do you need the full PGDIR_SIZE width passed to
> flush_tlb_pgtables, or just the PMD_SIZE'd start,end that covered
> the freed pmd_t?

Just the PMD_SIZE'd start,end is necessary.

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[David S. Miller]

On Fri, 25 Mar 2005 09:23:12 -0800
"David S. Miller" <davem@davemloft.net> wrote:

> On Fri, 25 Mar 2005 16:32:07 +1100
> Nick Piggin <nickpiggin@yahoo.com.au> wrote:
> 
> > So, to make the question more concrete: if a pgd_t is freed due
> > to freeing the single pmd_t contained within it (which was the
> > only part of the pgd's address space that contained a valid mapping)
> > Then do you need the full PGDIR_SIZE width passed to
> > flush_tlb_pgtables, or just the PMD_SIZE'd start,end that covered
> > the freed pmd_t?
> 
> Just the PMD_SIZE'd start,end is necessary.

Since sparc64 is the only user of this thing...

Let's make it so that the flush can be queued up
at pmd_clear() time, as that's what we really want.

Something like:

	pmd_clear(mm, vaddr, pmdp);

I'll try to play with something like this later.

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Hugh Dickins]

On Fri, 25 Mar 2005, David S. Miller wrote:

[ of flush_tlb_pgtables ]

> Since sparc64 is the only user of this thing...

Not quite.  sparc64 is the only user which makes any use of the
addresses passed to it, but frv does a little assembler with it,
and arm26 does a printk - eh? I'd take that to mean that it never
gets called there, but I don't see what prevents it, before or now.
Ian, does current -mm give you "flush_tlb_pgtables" printks?

> Let's make it so that the flush can be queued up
> at pmd_clear() time, as that's what we really want.
> 
> Something like:
> 
> 	pmd_clear(mm, vaddr, pmdp);
> 
> I'll try to play with something like this later.

Depends really on what DavidH wants there, not clear to me.
I suspect Ian can live without his printk!

Hugh

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[David Howells]

Hugh Dickins <hugh@veritas.com> wrote:

> On Fri, 25 Mar 2005, David S. Miller wrote:
> 
> [ of flush_tlb_pgtables ]
> 
> > Since sparc64 is the only user of this thing...
> 
> Not quite.  sparc64 is the only user which makes any use of the
> addresses passed to it, but frv does a little assembler with it,
> and arm26 does a printk - eh? I'd take that to mean that it never
> gets called there, but I don't see what prevents it, before or now.
> Ian, does current -mm give you "flush_tlb_pgtables" printks?

That bit of assembly invalidates some data cached by the TLB-miss handler in
registers SCR0 and SCR1 to improve performance in the next TLB-miss event.

What happens is that the TLB-miss handler sets a static mapping for a page
table and stores the base virtual address for the region covered by that page
table in SCR0 (ITLB) or SCR1 (DTLB). Then when dealing with the next TLB-miss
event we can do:

	((SCRx ^ virtaddr) >> 26) == 0

to very quickly work out whether we can re-use the static mapping left from
the previous TLB-miss event.

However, if the mapping from virtual address to page table changes, then the
cached static mappings may no longer be valid. We can invalidate them simply
by zapping SCR0 and SCR1.

It occurs to me that:

	#define flush_tlb_pgtables(mm,start,end) \
		asm volatile("movgs gr0,scr0 ! movgs gr0,scr1");

is actually wrong. It doesn't actually invalidate anything; it just changes
the virtual range for that page table to be 0x00000000-0x04000000, no matter
whether that page table should be backing that region or not. It should
instead be:

	#define flush_tlb_pgtables(mm,start,end) \
		asm volatile("movgs %0,scr0 ! movgs %0,scr1" :: "r"(-1));

Because the addresses in the range 0xFC000000-0xFFFFFFFF are all statically
mapped to the Boot ROM chip select.

This doesn't matter for most programs as they never use more than the bottom
page table anyway (which covers 64MB).

> > Let's make it so that the flush can be queued up
> > at pmd_clear() time, as that's what we really want.
> > 
> > Something like:
> > 
> > 	pmd_clear(mm, vaddr, pmdp);
> > 
> > I'll try to play with something like this later.
> 
> Depends really on what DavidH wants there, not clear to me.
> I suspect Ian can live without his printk!

I could do the zapping in pmd_clear() instead, I suppose. It's just that it
only needs to be done once when tearing down the page tables; not for every
PMD.

David

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Ian Molton]

David Howells wrote:

>>I suspect Ian can live without his printk!

I expect so, since arm26 doesnt boot yet. Hopefully once I get my 
current load of arm32 stuff done I'll get some time to revisit it.

arm26 mm is quite broken right now.

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[Hugh Dickins]

On Wed, 30 Mar 2005, David Howells wrote:
> Hugh Dickins <hugh@veritas.com> wrote:
> > On Fri, 25 Mar 2005, David S. Miller wrote:
> > 
> > [ of flush_tlb_pgtables ]
> 
> > > Let's make it so that the flush can be queued up
> > > at pmd_clear() time, as that's what we really want.
> > > 
> > > Something like:
> > > 
> > > 	pmd_clear(mm, vaddr, pmdp);
> > > 
> > > I'll try to play with something like this later.
> > 
> > Depends really on what DavidH wants there, not clear to me.
> > I suspect Ian can live without his printk!
> 
> I could do the zapping in pmd_clear() instead, I suppose. It's just that it
> only needs to be done once when tearing down the page tables; not for every
> PMD.

Sounds like we should leave flush_tlb_pgtables as it is
(apart from the issue in its frv implementation that you noticed).

Hugh

Re: [PATCH 1/6] freepgt: free_pgtables use vma list

[David S. Miller]

On Wed, 30 Mar 2005 13:22:53 +0100 (BST)
Hugh Dickins <hugh@veritas.com> wrote:

> Sounds like we should leave flush_tlb_pgtables as it is
> (apart from the issue in its frv implementation that you noticed).

Ok.  I may still adjust the pmd_clear() args.