[RFC][PATCH 1/5] pagemap: remove file header

[RFC][PATCH 1/5] pagemap: remove file header

[Dave Hansen]

The /proc/<pid>/pagemap file has a header containing:
 * first byte:   0 for big endian, 1 for little
 * second byte:  page shift (eg 12 for 4096 byte pages)
 * third byte:   entry size in bytes (currently either 4 or 8)
 * fourth byte:  header size

The endianness is only useful when examining a raw dump of
pagemap from a different machine when you don't know the
source of the file.  This is pretty rare, and the programs
or scripts doing the copying off-machine can certainly be
made to hold this information.

The page size is available in userspace at least with libc's
getpagesize().  This will also never vary across processes,
so putting it in a per-process file doesn't make any difference.
If we need a "kernel's page size" exported to userspace,
perhaps we can put it in /proc/meminfo.

The entry size is the really tricky one.  This can't just
be sizeof(unsigned long) from userspace because we can have
32-bit processes on 64-bit kernels.  But, userspace can
certainly derive this value if it lseek()s to the end of
the file, and divides the file position by the size of its
virtual address space.

In any case, I believe this information is redundant, and
can be removed.

Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
---

 lxc-dave/fs/proc/task_mmu.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

diff -puN fs/proc/task_mmu.c~pagemap-no-header fs/proc/task_mmu.c
--- lxc/fs/proc/task_mmu.c~pagemap-no-header	2007-08-07 15:30:52.000000000 -0700
+++ lxc-dave/fs/proc/task_mmu.c	2007-08-07 15:30:52.000000000 -0700
@@ -653,12 +653,12 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -EIO;
-	svpfn = src / sizeof(unsigned long) - 1;
+	svpfn = src / sizeof(unsigned long);
 	addr = PAGE_SIZE * svpfn;
-	if ((svpfn + 1) * sizeof(unsigned long) != src)
+	if (svpfn * sizeof(unsigned long) != src)
 		goto out;
 	evpfn = min((src + count) / sizeof(unsigned long),
-		    ((~0UL) >> PAGE_SHIFT) + 1) - 1;
+		    ((~0UL) >> PAGE_SHIFT) + 1);
 	count = (evpfn - svpfn) * sizeof(unsigned long);
 	end = PAGE_SIZE * evpfn;
 	//printk("src %ld svpfn %d evpfn %d count %d\n", src, svpfn, evpfn, count);
@@ -690,14 +690,6 @@ static ssize_t pagemap_read(struct file 
 	pm.count = count;
 	pm.out = (unsigned long __user *)buf;
 
-	if (svpfn == -1) {
-		add_to_pagemap(pm.next, 0, &pm);
-		put_user((char)(ntohl(1) != 1), buf);
-		put_user((char)PAGE_SHIFT, buf + 1);
-		put_user((char)sizeof(unsigned long), buf + 2);
-		put_user((char)sizeof(unsigned long), buf + 3);
-	}
-
 	down_read(&mm->mmap_sem);
 	vma = find_vma(mm, pm.next);
 	while (pm.count > 0 && vma) {
_

[RFC][PATCH 2/5] pagemap: use PAGE_MASK/PAGE_ALIGN()

[Dave Hansen]

Use existing macros (PAGE_MASK/PAGE_ALIGN()) instead of
open-coding them.

Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
---

 lxc-dave/fs/proc/task_mmu.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff -puN fs/proc/task_mmu.c~pagemap-use-PAGE_MASK fs/proc/task_mmu.c
--- lxc/fs/proc/task_mmu.c~pagemap-use-PAGE_MASK	2007-08-07 15:30:52.000000000 -0700
+++ lxc-dave/fs/proc/task_mmu.c	2007-08-07 15:30:52.000000000 -0700
@@ -669,9 +669,9 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -ENOMEM;
-	uaddr = (unsigned long)buf & ~(PAGE_SIZE-1);
+	uaddr = (unsigned long)buf & PAGE_MASK;
 	uend = (unsigned long)(buf + count);
-	pagecount = (uend - uaddr + PAGE_SIZE-1) / PAGE_SIZE;
+	pagecount = (uend - PAGE_ALIGN(uaddr)) / PAGE_SIZE;
 	pages = kmalloc(pagecount * sizeof(struct page *), GFP_KERNEL);
 	if (!pages)
 		goto out_task;
_

[RFC][PATCH 3/5] pagemap: remove open-coded sizeof(unsigned long)

[Dave Hansen]

I think the code gets easier to read when we give symbolic names
to some of the operations we're performing.  I was sure we needed
this when I saw the header being built like this:

	...
	buf[2] = sizeof(unsigned long)
	buf[3] = sizeof(unsigned long)

I really couldn't remember what either field did ;(

Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
---

 lxc-dave/fs/proc/task_mmu.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff -puN fs/proc/task_mmu.c~pagemap-use-ENTRY_SIZE fs/proc/task_mmu.c
--- lxc/fs/proc/task_mmu.c~pagemap-use-ENTRY_SIZE	2007-08-07 15:30:53.000000000 -0700
+++ lxc-dave/fs/proc/task_mmu.c	2007-08-07 15:30:53.000000000 -0700
@@ -560,14 +560,15 @@ struct pagemapread {
 	unsigned long __user *out;
 };
 
+#define PAGEMAP_ENTRY_SIZE_BYTES sizeof(unsigned long)
+
 static int add_to_pagemap(unsigned long addr, unsigned long pfn,
 			  struct pagemapread *pm)
 {
 	__put_user(pfn, pm->out);
 	pm->out++;
-	pm->pos += sizeof(unsigned long);
-	pm->count -= sizeof(unsigned long);
-	pm->next = addr + PAGE_SIZE;
+	pm->pos += PAGEMAP_ENTRY_SIZE_BYTES;
+	pm->count -= PAGEMAP_ENTRY_SIZE_BYTES;
 	return 0;
 }
 
@@ -653,13 +654,13 @@ static ssize_t pagemap_read(struct file 
 		goto out;
 
 	ret = -EIO;
-	svpfn = src / sizeof(unsigned long);
+	svpfn = src / PAGEMAP_ENTRY_SIZE_BYTES;
 	addr = PAGE_SIZE * svpfn;
-	if (svpfn * sizeof(unsigned long) != src)
+	if (svpfn * PAGEMAP_ENTRY_SIZE_BYTES != src)
 		goto out;
 	evpfn = min((src + count) / sizeof(unsigned long),
 		    ((~0UL) >> PAGE_SHIFT) + 1);
-	count = (evpfn - svpfn) * sizeof(unsigned long);
+	count = (evpfn - svpfn) * PAGEMAP_ENTRY_SIZE_BYTES;
 	end = PAGE_SIZE * evpfn;
 	//printk("src %ld svpfn %d evpfn %d count %d\n", src, svpfn, evpfn, count);
 
_

[RFC][PATCH 4/5] introduce TASK_SIZE_OF() for all arches

[Dave Hansen]

For the /proc/<pid>/pagemap code[1], we need to able to query how
much virtual address space a particular task has.  The trick is
that we do it through /proc and can't use TASK_SIZE since it
references "current" on some arches.  The process opening the
/proc file might be a 32-bit process opening a 64-bit process's
pagemap file.

x86_64 already has a TASK_SIZE_OF() macro:

#define TASK_SIZE_OF(child)     ((test_tsk_thread_flag(child, TIF_IA32)) ? IA32_PAGE_OFFSET : TASK_SIZE64)

I'd like to have that for other architectures.  So, add it
for all the architectures that actually use "current" in 
their TASK_SIZE.  For the others, just add a quick #define
in sched.h to use plain old TASK_SIZE.

1. http://www.linuxworld.com/news/2007/042407-kernel.html

Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
---

 lxc-dave/include/asm-ia64/processor.h    |    3 ++-
 lxc-dave/include/asm-parisc/processor.h  |    3 ++-
 lxc-dave/include/asm-powerpc/processor.h |    4 +++-
 lxc-dave/include/asm-s390/processor.h    |    2 ++
 lxc-dave/include/linux/sched.h           |    4 ++++
 5 files changed, 13 insertions(+), 3 deletions(-)

diff -puN include/asm-ia64/processor.h~task_size_of include/asm-ia64/processor.h
--- lxc/include/asm-ia64/processor.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
+++ lxc-dave/include/asm-ia64/processor.h	2007-08-07 15:30:54.000000000 -0700
@@ -31,7 +31,8 @@
  * each (assuming 8KB page size), for a total of 8TB of user virtual
  * address space.
  */
-#define TASK_SIZE		(current->thread.task_size)
+#define TASK_SIZE_OF(tsk)	((tsk)->thread.task_size)
+#define TASK_SIZE       	TASK_SIZE_OF(current)
 
 /*
  * This decides where the kernel will search for a free chunk of vm
diff -puN include/asm-parisc/processor.h~task_size_of include/asm-parisc/processor.h
--- lxc/include/asm-parisc/processor.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
+++ lxc-dave/include/asm-parisc/processor.h	2007-08-07 15:30:54.000000000 -0700
@@ -32,7 +32,8 @@
 #endif
 #define current_text_addr() ({ void *pc; current_ia(pc); pc; })
 
-#define TASK_SIZE               (current->thread.task_size)
+#define TASK_SIZE_OF(tsk)       ((tsk)->thread.task_size)
+#define TASK_SIZE	         (current->thread.task_size)
 #define TASK_UNMAPPED_BASE      (current->thread.map_base)
 
 #define DEFAULT_TASK_SIZE32	(0xFFF00000UL)
diff -puN include/asm-powerpc/processor.h~task_size_of include/asm-powerpc/processor.h
--- lxc/include/asm-powerpc/processor.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
+++ lxc-dave/include/asm-powerpc/processor.h	2007-08-07 15:30:54.000000000 -0700
@@ -107,7 +107,9 @@ extern struct task_struct *last_task_use
  */
 #define TASK_SIZE_USER32 (0x0000000100000000UL - (1*PAGE_SIZE))
 
-#define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \
+#define TASK_SIZE	  (test_thread_flag(TIF_32BIT) ? \
+		TASK_SIZE_USER32 : TASK_SIZE_USER64)
+#define TASK_SIZE_OF(tsk) (test_tsk_thread_flag(tsk, TIF_32BIT) ? \
 		TASK_SIZE_USER32 : TASK_SIZE_USER64)
 
 /* This decides where the kernel will search for a free chunk of vm
diff -puN include/asm-s390/processor.h~task_size_of include/asm-s390/processor.h
--- lxc/include/asm-s390/processor.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
+++ lxc-dave/include/asm-s390/processor.h	2007-08-07 15:30:54.000000000 -0700
@@ -75,6 +75,8 @@ extern struct task_struct *last_task_use
 
 # define TASK_SIZE		(test_thread_flag(TIF_31BIT) ? \
 					(0x80000000UL) : (0x40000000000UL))
+# define TASK_SIZE_OF(tsk)	(test_tsk_thread_flag(tsk, TIF_31BIT) ? \
+					(0x80000000UL) : (0x40000000000UL))
 # define TASK_UNMAPPED_BASE	(TASK_SIZE / 2)
 # define DEFAULT_TASK_SIZE	(0x40000000000UL)
 
diff -puN include/linux/sched.h~task_size_of include/linux/sched.h
--- lxc/include/linux/sched.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
+++ lxc-dave/include/linux/sched.h	2007-08-07 15:30:54.000000000 -0700
@@ -1712,6 +1712,10 @@ static inline void inc_syscw(struct task
 }
 #endif
 
+#ifndef TASK_SIZE_OF
+#define TASK_SIZE_OF(tsk)	TASK_SIZE
+#endif
+
 #endif /* __KERNEL__ */
 
 #endif
_

[RFC][PATCH 5/5] pagemap: add walker for empty areas

[Dave Hansen]

There's a pretty good deal of complexity surrounding dealing
with a sparse address space.  We have the pm->next pointer to
help indicate how far we've walked in the pagetables.  We also
attempt to fill empty areas without vmas manually.

This code adds an extension to the mm_walk code: a new handler
for "empty" pte ranges.  Those are areas where there is no
pte page present.  This allows us to get rid of the code that
inspects VMAs or that trys to keep track of how much of the
pagemap we have filled.

I should have broken these out, but there are a few more things
here as well:
- replaced -1's with a #define: PAGEMAP_NO_PAGE_PRESENT
- Rather than calculate the number of pagemap entries we
  expect to be able to make due to count, simply start making
  them, then return an error (PAGEMAP_END_OF_BUFFER) when the
  buffer is full in add_to_pagemap().  This gets rid of the
  vend/evaddr calculations.
- move around the code in the read function, and make some
  variables local to an else{} block.
- remove pagemapread->next variable.  It is no longer needed.

Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
---

 lxc-dave/fs/proc/task_mmu.c |  135 ++++++++++++++++++--------------------------
 lxc-dave/include/linux/mm.h |    1 
 lxc-dave/lib/pagewalk.c     |   67 +++++++++------------
 3 files changed, 88 insertions(+), 115 deletions(-)

diff -puN fs/proc/task_mmu.c~walk-empty-ranges fs/proc/task_mmu.c
--- lxc/fs/proc/task_mmu.c~walk-empty-ranges	2007-08-07 15:30:54.000000000 -0700
+++ lxc-dave/fs/proc/task_mmu.c	2007-08-07 15:30:54.000000000 -0700
@@ -553,7 +553,6 @@ const struct file_operations proc_numa_m
 
 #ifdef CONFIG_PROC_PAGEMAP
 struct pagemapread {
-	unsigned long next;
 	unsigned long pos;
 	size_t count;
 	int index;
@@ -561,57 +560,66 @@ struct pagemapread {
 };
 
 #define PAGEMAP_ENTRY_SIZE_BYTES sizeof(unsigned long)
+#define PAGEMAP_END_OF_BUFFER 1
+#define PAGEMAP_NO_PAGE_PRESENT	((unsigned long)-1)
 
 static int add_to_pagemap(unsigned long addr, unsigned long pfn,
 			  struct pagemapread *pm)
 {
-	__put_user(pfn, pm->out);
+	int out_len = PAGEMAP_ENTRY_SIZE_BYTES;
+	if (pm->count < PAGEMAP_ENTRY_SIZE_BYTES)
+		out_len = pm->count;
+	copy_to_user(pm->out, &pfn, out_len);
 	pm->out++;
-	pm->pos += PAGEMAP_ENTRY_SIZE_BYTES;
-	pm->count -= PAGEMAP_ENTRY_SIZE_BYTES;
+	pm->pos += out_len;
+	pm->count -= out_len;
+	if (pm->count <= 0)
+		return PAGEMAP_END_OF_BUFFER;
 	return 0;
 }
 
+static int pagemap_pte_hole(unsigned long start, unsigned long end,
+			    void *private)
+{
+	struct pagemapread *pm = private;
+	unsigned long addr;
+	int err = 0;
+
+	for (addr = start; addr < end; addr += PAGE_SIZE) {
+		err = add_to_pagemap(addr, PAGEMAP_NO_PAGE_PRESENT, pm);
+		if (err)
+			break;
+	}
+	return err;
+}
+
 static int pagemap_pte_range(pmd_t *pmd, unsigned long addr, unsigned long end,
 			     void *private)
 {
 	struct pagemapread *pm = private;
 	pte_t *pte;
-	int err;
+	int err = 0;
 
 	pte = pte_offset_map(pmd, addr);
 	for (; addr != end; pte++, addr += PAGE_SIZE) {
-		if (addr < pm->next)
-			continue;
-		if (!pte_present(*pte))
-			err = add_to_pagemap(addr, -1, pm);
-		else
-			err = add_to_pagemap(addr, pte_pfn(*pte), pm);
+		unsigned long pfn = PAGEMAP_NO_PAGE_PRESENT;
+		if (pte_present(*pte))
+			pfn = pte_pfn(*pte);
+		err = add_to_pagemap(addr, pte_pfn(*pte), pm);
 		if (err)
-			return err;
-		if (pm->count == 0)
 			break;
 	}
 	pte_unmap(pte - 1);
 
 	cond_resched();
 
-	return 0;
+	return err;
 }
 
-static int pagemap_fill(struct pagemapread *pm, unsigned long end)
-{
-	int ret;
-
-	while (pm->next != end && pm->count > 0) {
-		ret = add_to_pagemap(pm->next, -1UL, pm);
-		if (ret)
-			return ret;
-	}
-	return 0;
-}
-
-static struct mm_walk pagemap_walk = { .pmd_entry = pagemap_pte_range };
+static struct mm_walk pagemap_walk = {
+	.pmd_entry = pagemap_pte_range,
+	.pte_hole = pagemap_pte_hole
+};
 
 /*
  * /proc/pid/pagemap - an array mapping virtual pages to pfns
@@ -625,23 +633,14 @@ static struct mm_walk pagemap_walk = { .
  * Efficient users of this interface will use /proc/pid/maps to
  * determine which areas of memory are actually mapped and llseek to
  * skip over unmapped regions.
- *
- * The first 4 bytes of this file form a simple header:
- *
- * first byte:   0 for big endian, 1 for little
- * second byte:  page shift (eg 12 for 4096 byte pages)
- * third byte:   entry size in bytes (currently either 4 or 8)
- * fourth byte:  header size
  */
 static ssize_t pagemap_read(struct file *file, char __user *buf,
 			    size_t count, loff_t *ppos)
 {
 	struct task_struct *task = get_proc_task(file->f_path.dentry->d_inode);
-	unsigned long src = *ppos;
 	struct page **pages, *page;
-	unsigned long addr, end, vend, svpfn, evpfn, uaddr, uend;
+	unsigned long uaddr, uend;
 	struct mm_struct *mm;
-	struct vm_area_struct *vma;
 	struct pagemapread pm;
 	int pagecount;
 	int ret = -ESRCH;
@@ -653,17 +652,6 @@ static ssize_t pagemap_read(struct file 
 	if (!ptrace_may_attach(task))
 		goto out;
 
-	ret = -EIO;
-	svpfn = src / PAGEMAP_ENTRY_SIZE_BYTES;
-	addr = PAGE_SIZE * svpfn;
-	if (svpfn * PAGEMAP_ENTRY_SIZE_BYTES != src)
-		goto out;
-	evpfn = min((src + count) / sizeof(unsigned long),
-		    ((~0UL) >> PAGE_SHIFT) + 1);
-	count = (evpfn - svpfn) * PAGEMAP_ENTRY_SIZE_BYTES;
-	end = PAGE_SIZE * evpfn;
-	//printk("src %ld svpfn %d evpfn %d count %d\n", src, svpfn, evpfn, count);
-
 	ret = 0;
 	mm = get_task_mm(task);
 	if (!mm)
@@ -672,7 +660,7 @@ static ssize_t pagemap_read(struct file 
 	ret = -ENOMEM;
 	uaddr = (unsigned long)buf & PAGE_MASK;
 	uend = (unsigned long)(buf + count);
-	pagecount = (uend - PAGE_ALIGN(uaddr)) / PAGE_SIZE;
+	pagecount = (PAGE_ALIGN(uend) - uaddr) / PAGE_SIZE;
 	pages = kmalloc(pagecount * sizeof(struct page *), GFP_KERNEL);
 	if (!pages)
 		goto out_task;
@@ -682,44 +670,38 @@ static ssize_t pagemap_read(struct file 
 			     1, 0, pages, NULL);
 	up_read(&current->mm->mmap_sem);
 
-	//printk("%x(%x):%x %d@%ld (%d pages) -> %d\n", uaddr, buf, uend, count, src, pagecount, ret);
 	if (ret < 0)
 		goto out_free;
 
-	pm.next = addr;
-	pm.pos = src;
+	pm.pos = *ppos;
 	pm.count = count;
 	pm.out = (unsigned long __user *)buf;
 
 	down_read(&mm->mmap_sem);
-	vma = find_vma(mm, pm.next);
-	while (pm.count > 0 && vma) {
-		if (!ptrace_may_attach(task)) {
-			ret = -EIO;
-			up_read(&mm->mmap_sem);
-			goto out_release;
-		}
-		vend = min(vma->vm_start - 1, end - 1) + 1;
-		ret = pagemap_fill(&pm, vend);
-		if (ret || !pm.count)
-			break;
-		vend = min(vma->vm_end - 1, end - 1) + 1;
-		ret = walk_page_range(mm, vma->vm_start, vend,
+	if (!ptrace_may_attach(task)) {
+		ret = -EIO;
+	} else {
+		unsigned long src = *ppos;
+		unsigned long svpfn = src / PAGEMAP_ENTRY_SIZE_BYTES;
+		unsigned long start_vaddr = svpfn << PAGE_SHIFT;
+		unsigned long end_vaddr = TASK_SIZE_OF(task);
+		/*
+		 * The odds are that this will stop walking way
+		 * before end_vaddr, because the length of the
+		 * user buffer is tracked in "pm", and the walk
+		 * will stop when we hit the end of the buffer.
+		 */
+		ret = walk_page_range(mm, start_vaddr, end_vaddr,
 				      &pagemap_walk, &pm);
-		vma = vma->vm_next;
+		if (ret == PAGEMAP_END_OF_BUFFER)
+			ret = 0;
+		/* don't need mmap_sem for these, but this looks cleaner */
+		*ppos = pm.pos;
+		if (!ret)
+			ret = pm.pos - src;
 	}
 	up_read(&mm->mmap_sem);
 
-	//printk("before fill at %ld\n", pm.pos);
-	ret = pagemap_fill(&pm, end);
-
-	printk("after fill at %ld\n", pm.pos);
-	*ppos = pm.pos;
-	if (!ret)
-		ret = pm.pos - src;
-
-out_release:
-	printk("releasing pages\n");
 	for (; pagecount; pagecount--) {
 		page = pages[pagecount-1];
 		if (!PageReserved(page))
@@ -732,7 +714,6 @@ out_free:
 out_task:
 	put_task_struct(task);
 out:
-	printk("returning\n");
 	return ret;
 }
 
diff -puN include/linux/mm.h~walk-empty-ranges include/linux/mm.h
--- lxc/include/linux/mm.h~walk-empty-ranges	2007-08-07 15:30:54.000000000 -0700
+++ lxc-dave/include/linux/mm.h	2007-08-07 15:30:54.000000000 -0700
@@ -750,6 +750,7 @@ struct mm_walk {
 	int (*pud_entry)(pud_t *, unsigned long, unsigned long, void *);
 	int (*pmd_entry)(pmd_t *, unsigned long, unsigned long, void *);
 	int (*pte_entry)(pte_t *, unsigned long, unsigned long, void *);
+	int (*pte_hole) (unsigned long, unsigned long, void *);
 };
 
 int walk_page_range(struct mm_struct *, unsigned long addr, unsigned long end,
diff -puN lib/pagewalk.c~walk-empty-ranges lib/pagewalk.c
--- lxc/lib/pagewalk.c~walk-empty-ranges	2007-08-07 15:30:54.000000000 -0700
+++ lxc-dave/lib/pagewalk.c	2007-08-07 15:30:54.000000000 -0700
@@ -6,17 +6,13 @@ static int walk_pte_range(pmd_t *pmd, un
 			  struct mm_walk *walk, void *private)
 {
 	pte_t *pte;
-	int err;
+	int err = 0;
 
 	for (pte = pte_offset_map(pmd, addr); addr != end;
 	     addr += PAGE_SIZE, pte++) {
-		if (pte_none(*pte))
-			continue;
 		err = walk->pte_entry(pte, addr, addr, private);
-		if (err) {
-			pte_unmap(pte);
-			return err;
-		}
+		if (err)
+		       break;
 	}
 	pte_unmap(pte);
 	return 0;
@@ -27,25 +23,23 @@ static int walk_pmd_range(pud_t *pud, un
 {
 	pmd_t *pmd;
 	unsigned long next;
-	int err;
+	int err = 0;
 
 	for (pmd = pmd_offset(pud, addr); addr != end;
 	     pmd++, addr = next) {
 		next = pmd_addr_end(addr, end);
-		if (pmd_none_or_clear_bad(pmd))
+		if (pmd_none(*pmd)) {
+			err = walk->pte_hole(addr, next, private);
+		} else if (pmd_none_or_clear_bad(pmd))
 			continue;
-		if (walk->pmd_entry) {
+		if (!err && walk->pmd_entry)
 			err = walk->pmd_entry(pmd, addr, next, private);
-			if (err)
-				return err;
-		}
-		if (walk->pte_entry) {
+		if (!err && walk->pte_entry)
 			err = walk_pte_range(pmd, addr, next, walk, private);
-			if (err)
-				return err;
-		}
+		if (err)
+			break;
 	}
-	return 0;
+	return err;
 }
 
 static int walk_pud_range(pgd_t *pgd, unsigned long addr, unsigned long end,
@@ -53,23 +47,21 @@ static int walk_pud_range(pgd_t *pgd, un
 {
 	pud_t *pud;
 	unsigned long next;
-	int err;
+	int err = 0;
 
 	for (pud = pud_offset(pgd, addr); addr != end;
 	     pud++, addr = next) {
 		next = pud_addr_end(addr, end);
-		if (pud_none_or_clear_bad(pud))
+		if (pud_none(*pud)) {
+			err = walk->pte_hole(addr, next, private);
+		} else if (pud_none_or_clear_bad(pud))
 			continue;
-		if (walk->pud_entry) {
+		if (!err && walk->pud_entry)
 			err = walk->pud_entry(pud, addr, next, private);
-			if (err)
-				return err;
-		}
-		if (walk->pmd_entry || walk->pte_entry) {
+		if (!err && (walk->pmd_entry || walk->pte_entry))
 			err = walk_pmd_range(pud, addr, next, walk, private);
-			if (err)
-				return err;
-		}
+		if (err)
+			return err;
 	}
 	return 0;
 }
@@ -91,23 +83,22 @@ int walk_page_range(struct mm_struct *mm
 {
 	pgd_t *pgd;
 	unsigned long next;
-	int err;
+	int err = 0;
 
 	for (pgd = pgd_offset(mm, addr); addr != end;
 	     pgd++, addr = next) {
 		next = pgd_addr_end(addr, end);
-		if (pgd_none_or_clear_bad(pgd))
+		if (pgd_none(*pgd)) {
+			err = walk->pte_hole(addr, next, private);
+		} else if (pgd_none_or_clear_bad(pgd))
 			continue;
-		if (walk->pgd_entry) {
+		if (!err && walk->pgd_entry)
 			err = walk->pgd_entry(pgd, addr, next, private);
-			if (err)
-				return err;
-		}
-		if (walk->pud_entry || walk->pmd_entry || walk->pte_entry) {
+		if (!err &&
+		    (walk->pud_entry || walk->pmd_entry || walk->pte_entry))
 			err = walk_pud_range(pgd, addr, next, walk, private);
-			if (err)
-				return err;
-		}
+		if (err)
+			return err;
 	}
 	return 0;
 }
_

Re: [RFC][PATCH 3/5] pagemap: remove open-coded sizeof(unsigned long)

[Matt Mackall]

On Tue, Aug 07, 2007 at 03:33:02PM -0700, Dave Hansen wrote:
> +#define PAGEMAP_ENTRY_SIZE_BYTES sizeof(unsigned long)
> +
>  static int add_to_pagemap(unsigned long addr, unsigned long pfn,
>  			  struct pagemapread *pm)
>  {
>  	__put_user(pfn, pm->out);
>  	pm->out++;
> -	pm->pos += sizeof(unsigned long);
> -	pm->count -= sizeof(unsigned long);
> -	pm->next = addr + PAGE_SIZE;
> +	pm->pos += PAGEMAP_ENTRY_SIZE_BYTES;
> +	pm->count -= PAGEMAP_ENTRY_SIZE_BYTES;

I think deleting ->next is a little premature here?

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH 3/5] pagemap: remove open-coded sizeof(unsigned long)

[Dave Hansen]

On Tue, 2007-08-07 at 18:40 -0500, Matt Mackall wrote:
> On Tue, Aug 07, 2007 at 03:33:02PM -0700, Dave Hansen wrote:
> > +#define PAGEMAP_ENTRY_SIZE_BYTES sizeof(unsigned long)
> > +
> >  static int add_to_pagemap(unsigned long addr, unsigned long pfn,
> >  			  struct pagemapread *pm)
> >  {
> >  	__put_user(pfn, pm->out);
> >  	pm->out++;
> > -	pm->pos += sizeof(unsigned long);
> > -	pm->count -= sizeof(unsigned long);
> > -	pm->next = addr + PAGE_SIZE;
> > +	pm->pos += PAGEMAP_ENTRY_SIZE_BYTES;
> > +	pm->count -= PAGEMAP_ENTRY_SIZE_BYTES;
> 
> I think deleting ->next is a little premature here?

It is.  I'll fix that up.

-- Dave

Re: [RFC][PATCH 1/5] pagemap: remove file header

[Matt Mackall]

On Tue, Aug 07, 2007 at 03:33:00PM -0700, Dave Hansen wrote:
> 
> The /proc/<pid>/pagemap file has a header containing:
>  * first byte:   0 for big endian, 1 for little
>  * second byte:  page shift (eg 12 for 4096 byte pages)
>  * third byte:   entry size in bytes (currently either 4 or 8)
>  * fourth byte:  header size
> 
> The endianness is only useful when examining a raw dump of
> pagemap from a different machine when you don't know the
> source of the file.  This is pretty rare, and the programs
> or scripts doing the copying off-machine can certainly be
> made to hold this information.
> 
> The page size is available in userspace at least with libc's
> getpagesize().  This will also never vary across processes,
> so putting it in a per-process file doesn't make any difference.
> If we need a "kernel's page size" exported to userspace,
> perhaps we can put it in /proc/meminfo.
> 
> The entry size is the really tricky one.  This can't just
> be sizeof(unsigned long) from userspace because we can have
> 32-bit processes on 64-bit kernels.  But, userspace can
> certainly derive this value if it lseek()s to the end of
> the file, and divides the file position by the size of its
> virtual address space.

I'd really strongly prefer to have no header. It was added to
futureproof the thing.

One downside to this is that dumps are no longer portable. But
that's not a showstopper, I suppose.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH 1/5] pagemap: remove file header

[Alan Cox]

O> > The endianness is only useful when examining a raw dump of
> > pagemap from a different machine when you don't know the
> > source of the file.  This is pretty rare, and the programs
> > or scripts doing the copying off-machine can certainly be
> > made to hold this information.

Nobody fancies doing bi-endian MIPS ?

> > The page size is available in userspace at least with libc's
> > getpagesize().  This will also never vary across processes,

For now. Its a logical direction however thant we end up with bigger page
sizes either by hardware or by software merging and end up having
different page sizes for legacy 32bit binaries.

> I'd really strongly prefer to have no header. It was added to
> futureproof the thing.

The information needed to parse /proc/pid/pagemap can be stuck
in /proc/pid/somewherelese. If we ever get page size variations and the
like then /proc/pid/ is going to end up with that information anyway for
ps and friends to use.

Re: [RFC][PATCH 2/5] pagemap: use PAGE_MASK/PAGE_ALIGN()

[Matt Mackall]

On Tue, Aug 07, 2007 at 03:33:01PM -0700, Dave Hansen wrote:
> 
> Use existing macros (PAGE_MASK/PAGE_ALIGN()) instead of
> open-coding them.

Absolutely.

Acked-by: Matt Mackall <mpm@selenic.com>

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH 3/5] pagemap: remove open-coded sizeof(unsigned long)

[Matt Mackall]

On Tue, Aug 07, 2007 at 04:55:02PM -0700, Dave Hansen wrote:
> On Tue, 2007-08-07 at 18:40 -0500, Matt Mackall wrote:
> > On Tue, Aug 07, 2007 at 03:33:02PM -0700, Dave Hansen wrote:
> > > +#define PAGEMAP_ENTRY_SIZE_BYTES sizeof(unsigned long)
> > > +
> > >  static int add_to_pagemap(unsigned long addr, unsigned long pfn,
> > >  			  struct pagemapread *pm)
> > >  {
> > >  	__put_user(pfn, pm->out);
> > >  	pm->out++;
> > > -	pm->pos += sizeof(unsigned long);
> > > -	pm->count -= sizeof(unsigned long);
> > > -	pm->next = addr + PAGE_SIZE;
> > > +	pm->pos += PAGEMAP_ENTRY_SIZE_BYTES;
> > > +	pm->count -= PAGEMAP_ENTRY_SIZE_BYTES;
> > 
> > I think deleting ->next is a little premature here?
> 
> It is.  I'll fix that up.

While we're at it, it seems a shame to replace the unwieldy typedef
with an even longer define. My poor shift key finger aches just
thinking about typing that. PM_ENTRY_SIZE, perhaps?

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH 4/5] introduce TASK_SIZE_OF() for all arches

[Matt Mackall]

On Tue, Aug 07, 2007 at 03:33:03PM -0700, Dave Hansen wrote:
> 
> For the /proc/<pid>/pagemap code[1], we need to able to query how
> much virtual address space a particular task has.  The trick is
> that we do it through /proc and can't use TASK_SIZE since it
> references "current" on some arches.  The process opening the
> /proc file might be a 32-bit process opening a 64-bit process's
> pagemap file.

That seems perfectly sensible to me, but I think you'll need to bounce
it off Andrew at least. (cc:ed)

> x86_64 already has a TASK_SIZE_OF() macro:
> 
> #define TASK_SIZE_OF(child)     ((test_tsk_thread_flag(child, TIF_IA32)) ? IA32_PAGE_OFFSET : TASK_SIZE64)
> 
> I'd like to have that for other architectures.  So, add it
> for all the architectures that actually use "current" in 
> their TASK_SIZE.  For the others, just add a quick #define
> in sched.h to use plain old TASK_SIZE.
> 
> 1. http://www.linuxworld.com/news/2007/042407-kernel.html
> 
> Signed-off-by: Dave Hansen <haveblue@us.ibm.com>
> ---
> 
>  lxc-dave/include/asm-ia64/processor.h    |    3 ++-
>  lxc-dave/include/asm-parisc/processor.h  |    3 ++-
>  lxc-dave/include/asm-powerpc/processor.h |    4 +++-
>  lxc-dave/include/asm-s390/processor.h    |    2 ++
>  lxc-dave/include/linux/sched.h           |    4 ++++
>  5 files changed, 13 insertions(+), 3 deletions(-)
> 
> diff -puN include/asm-ia64/processor.h~task_size_of include/asm-ia64/processor.h
> --- lxc/include/asm-ia64/processor.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
> +++ lxc-dave/include/asm-ia64/processor.h	2007-08-07 15:30:54.000000000 -0700
> @@ -31,7 +31,8 @@
>   * each (assuming 8KB page size), for a total of 8TB of user virtual
>   * address space.
>   */
> -#define TASK_SIZE		(current->thread.task_size)
> +#define TASK_SIZE_OF(tsk)	((tsk)->thread.task_size)
> +#define TASK_SIZE       	TASK_SIZE_OF(current)
>  
>  /*
>   * This decides where the kernel will search for a free chunk of vm
> diff -puN include/asm-parisc/processor.h~task_size_of include/asm-parisc/processor.h
> --- lxc/include/asm-parisc/processor.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
> +++ lxc-dave/include/asm-parisc/processor.h	2007-08-07 15:30:54.000000000 -0700
> @@ -32,7 +32,8 @@
>  #endif
>  #define current_text_addr() ({ void *pc; current_ia(pc); pc; })
>  
> -#define TASK_SIZE               (current->thread.task_size)
> +#define TASK_SIZE_OF(tsk)       ((tsk)->thread.task_size)
> +#define TASK_SIZE	         (current->thread.task_size)
>  #define TASK_UNMAPPED_BASE      (current->thread.map_base)
>  
>  #define DEFAULT_TASK_SIZE32	(0xFFF00000UL)
> diff -puN include/asm-powerpc/processor.h~task_size_of include/asm-powerpc/processor.h
> --- lxc/include/asm-powerpc/processor.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
> +++ lxc-dave/include/asm-powerpc/processor.h	2007-08-07 15:30:54.000000000 -0700
> @@ -107,7 +107,9 @@ extern struct task_struct *last_task_use
>   */
>  #define TASK_SIZE_USER32 (0x0000000100000000UL - (1*PAGE_SIZE))
>  
> -#define TASK_SIZE (test_thread_flag(TIF_32BIT) ? \
> +#define TASK_SIZE	  (test_thread_flag(TIF_32BIT) ? \
> +		TASK_SIZE_USER32 : TASK_SIZE_USER64)
> +#define TASK_SIZE_OF(tsk) (test_tsk_thread_flag(tsk, TIF_32BIT) ? \
>  		TASK_SIZE_USER32 : TASK_SIZE_USER64)
>  
>  /* This decides where the kernel will search for a free chunk of vm
> diff -puN include/asm-s390/processor.h~task_size_of include/asm-s390/processor.h
> --- lxc/include/asm-s390/processor.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
> +++ lxc-dave/include/asm-s390/processor.h	2007-08-07 15:30:54.000000000 -0700
> @@ -75,6 +75,8 @@ extern struct task_struct *last_task_use
>  
>  # define TASK_SIZE		(test_thread_flag(TIF_31BIT) ? \
>  					(0x80000000UL) : (0x40000000000UL))
> +# define TASK_SIZE_OF(tsk)	(test_tsk_thread_flag(tsk, TIF_31BIT) ? \
> +					(0x80000000UL) : (0x40000000000UL))
>  # define TASK_UNMAPPED_BASE	(TASK_SIZE / 2)
>  # define DEFAULT_TASK_SIZE	(0x40000000000UL)
>  
> diff -puN include/linux/sched.h~task_size_of include/linux/sched.h
> --- lxc/include/linux/sched.h~task_size_of	2007-08-07 15:30:54.000000000 -0700
> +++ lxc-dave/include/linux/sched.h	2007-08-07 15:30:54.000000000 -0700
> @@ -1712,6 +1712,10 @@ static inline void inc_syscw(struct task
>  }
>  #endif
>  
> +#ifndef TASK_SIZE_OF
> +#define TASK_SIZE_OF(tsk)	TASK_SIZE
> +#endif
> +
>  #endif /* __KERNEL__ */
>  
>  #endif
> _

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH 1/5] pagemap: remove file header

[Matt Mackall]

On Wed, Aug 08, 2007 at 02:31:00AM +0100, Alan Cox wrote:
> O> > The endianness is only useful when examining a raw dump of
> > > pagemap from a different machine when you don't know the
> > > source of the file.  This is pretty rare, and the programs
> > > or scripts doing the copying off-machine can certainly be
> > > made to hold this information.
> 
> Nobody fancies doing bi-endian MIPS ?

Indeed not.
 
> > > The page size is available in userspace at least with libc's
> > > getpagesize().  This will also never vary across processes,
> 
> For now. Its a logical direction however thant we end up with bigger page
> sizes either by hardware or by software merging and end up having
> different page sizes for legacy 32bit binaries.

Blerch.

> > I'd really strongly prefer to have no header. It was added to
> > futureproof the thing.
> 
> The information needed to parse /proc/pid/pagemap can be stuck
> in /proc/pid/somewherelese. If we ever get page size variations and the
> like then /proc/pid/ is going to end up with that information anyway for
> ps and friends to use.

Well if somewhereelse doesn't exist today, programs written today will
break on tomorrow's kernels.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [RFC][PATCH 1/5] pagemap: remove file header

[Dave Hansen]

On Wed, 2007-08-08 at 02:31 +0100, Alan Cox wrote:
> The information needed to parse /proc/pid/pagemap can be stuck
> in /proc/pid/somewherelese. If we ever get page size variations and the
> like then /proc/pid/ is going to end up with that information anyway for
> ps and friends to use.

We do at least have some of this with hugetlbfs.  In fact, the current
pagemap code bugs out when it hits a huge page area:

/home/dave/work/linux/2.6/22/lxc/mm/memory.c:117: bad pgd 1d0000e7.

I'll look at fixing that.

-- Dave

Re: [RFC][PATCH 1/5] pagemap: remove file header

[Dave Hansen]

On Wed, 2007-08-08 at 09:34 -0700, Dave Hansen wrote:
> On Wed, 2007-08-08 at 02:31 +0100, Alan Cox wrote:
> > The information needed to parse /proc/pid/pagemap can be stuck
> > in /proc/pid/somewherelese. If we ever get page size variations and the
> > like then /proc/pid/ is going to end up with that information anyway for
> > ps and friends to use.
> 
> We do at least have some of this with hugetlbfs.  In fact, the current
> pagemap code bugs out when it hits a huge page area:
> 
> /home/dave/work/linux/2.6/22/lxc/mm/memory.c:117: bad pgd 1d0000e7.
> 
> I'll look at fixing that.

I figure we have three options:

1. ignore huge pages completely
2. teach the ->pte_entry handler that ptes can be different sizes,
   and pass the huge ptes into there
3. let the ->pmd_entry handler (or whatever level we've stuck the huge
   page entry in) handle it
4. create new handlers for large pages, like ->huge_pte_entry.  This
   works for now, but what would we do if we have multiple/variable
   large page sizes?

Any other thoughts?

-- Dave

Re: [RFC][PATCH 4/5] introduce TASK_SIZE_OF() for all arches

[Dave Hansen]

On Tue, 2007-08-07 at 21:03 -0500, Matt Mackall wrote:
> On Tue, Aug 07, 2007 at 03:33:03PM -0700, Dave Hansen wrote:
> > 
> > For the /proc/<pid>/pagemap code[1], we need to able to query how
> > much virtual address space a particular task has.  The trick is
> > that we do it through /proc and can't use TASK_SIZE since it
> > references "current" on some arches.  The process opening the
> > /proc file might be a 32-bit process opening a 64-bit process's
> > pagemap file.
> 
> That seems perfectly sensible to me, but I think you'll need to bounce
> it off Andrew at least. (cc:ed)

It definitely needs a trip to linux-arch.  

-- Dave