Export get_user_pages_fast

Using a simple page table thrashing program I measure a slight improvement. The program creates five processes. Each touches 1000 pages then schedules the next process. We repeat this 1000 times. As lguest only caches 4 cr3 values, this rebuilds a lot of shadow page tables requiring virt->phys mappings.

	Before: 5.93 seconds
	After: 5.40 seconds

(Counts of slow vs fastpath in this usage are 6092 and 2852462 respectively.)

And more importantly for lguest, the code is simpler.

Signed-off-by Rusty Russell <[email protected]>

---
 drivers/lguest/page_tables.c |   25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff -r cb465381f6d5 drivers/lguest/page_tables.c

drivers/lguest/page_tables.c +0 -1

@@ -108,9 +108,8 @@ static unsigned long gpte_addr(pgd_t gpg
 }
 /*:*/
 
-/*M:014 get_pfn is slow; it takes the mmap sem and calls get_user_pages. 

We - * could probably try to grab batches of pages here as an optimization
- * (ie. pre-faulting). :*/
+/*M:014 get_pfn is slow: we could probably try to grab batches of pages here as + * an optimization (ie. pre-faulting). :*/

/*H:350 This routine takes a page number given by the Guest and converts
it to * an actual, physical page number. It can fail for several reasons:
the @@ -123,19 +122,13 @@ static unsigned long get_pfn(unsigned lo
static unsigned long get_pfn(unsigned long virtpfn, int write) {

drivers/lguest/page_tables.c +6 -12

 	struct page *page;
+
+	/* gup me one page at this address please! */
+	if (get_user_pages_fast(virtpfn << PAGE_SHIFT, 1, write, &page) == 1)
+		return page_to_pfn(page);
+
 	/* This value indicates failure. */
-	unsigned long ret = -1UL;
-
-	/* get_user_pages() is a complex interface: it gets the "struct
-	 * vm_area_struct" and "struct page" assocated with a range of pages.
-	 * It also needs the task's mmap_sem held, and is not very quick.
-	 * It returns the number of pages it got. */
-	down_read(&current->mm->mmap_sem);
-	if (get_user_pages(current, current->mm, virtpfn << PAGE_SHIFT,
-			   1, write, 1, &page, NULL) == 1)
-		ret = page_to_pfn(page);
-	up_read(&current->mm->mmap_sem);
-	return ret;
+	return -1UL;
 }
 
 /*H:340 Converting a Guest page table entry to a shadow (ie. real) page

table @@ -174,7 +167,7 @@ static pte_t gpte_to_spte(struct lg_cpu
/*H:460 And to complete the chain, release_pte() looks like this: */ static void release_pte(pte_t pte)
{

drivers/lguest/page_tables.c +1 -1

-	/* Remember that get_user_pages() took a reference to the page, in
+	/* Remember that get_user_pages_fast() took a reference to the page, in
 	 * get_pfn()?  We have to put it back now. */
 	if (pte_flags(pte) & _PAGE_PRESENT)
 		put_page(pfn_to_page(pte_pfn(pte)));

Quoted 3 lines · Rusty Russell

	Before: 5.93 seconds
	After: 5.40 seconds

Quoted 4 lines

(Counts of slow vs fastpath in this usage are 6092 and 2852462 respectively.)

And more importantly for lguest, the code is simpler.

Cool. That's actually a fairly significant straight-line performance improvement, given that the lguest processes are running one-at-a-time right? (and not contending on the locks or bouncing cachelines too much)

After all, it was made for lguest wasn't it?

Still think it should be
called get_current_pages() though.

Signed-off-by Rusty Russell <[email protected]>

---
 arch/x86/mm/gup.c |    2 ++
 1 file changed, 2 insertions(+)

diff -r cb465381f6d5 arch/x86/mm/gup.c

arch/x86/mm/gup.c +2 -0

@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
+#include <linux/module.h>
 
 #include <asm/pgtable.h>
 

@@ -296,3 +303,4 @@ slow_irqon:
 		return ret;
 	}
 }
+EXPORT_SYMBOL_GPL(get_user_pages_fast);

After all, it was made for lguest wasn't it? Still think it should be called get_current_pages() though.

Signed-off-by Rusty Russell <[email protected]>

---
 arch/x86/mm/gup.c |    2 ++
 1 file changed, 2 insertions(+)

diff -r cb465381f6d5 arch/x86/mm/gup.c

arch/x86/mm/gup.c +1 -0

@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
+#include <linux/module.h>
 
 #include <asm/pgtable.h>
 

Quoted 20 lines · On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <[email protected]> wrote:

After all, it was made for lguest wasn't it? Still think it should be called get_current_pages() though.

Signed-off-by Rusty Russell <[email protected]>

---
 arch/x86/mm/gup.c |    2 ++
 1 file changed, 2 insertions(+)

diff -r cb465381f6d5 arch/x86/mm/gup.c

arch/x86/mm/gup.c +1 -0

@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
+#include <linux/module.h>
 
 #include <asm/pgtable.h>
 

arch/x86/mm/gup.c slow_irqon: · line 306

Quoted 4 lines

arch/x86/mm/gup.c +1 -0

@@ -296,3 +303,4 @@ slow_irqon:
 		return ret;
 	}
 }
+EXPORT_SYMBOL_GPL(get_user_pages_fast);

A regularly-occurring problem with an export like this is that someone writes a driver and tests it on x86, then the driver explodes on an architecture which didn't export the same symbol.

So a better implementation might be to put

#ifdef CONFIG_HAVE_GET_USER_PAGES_FAST
EXPORT_SYMBOL_GPL(get_user_pages_fast);
#endif

into generic code somewhere.

Quoted 33 lines · Andrew Morton

Quoted 20 lines · On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <[email protected]> wrote:

After all, it was made for lguest wasn't it? Still think it should be called get_current_pages() though.

Signed-off-by Rusty Russell <[email protected]>

---
 arch/x86/mm/gup.c |    2 ++
 1 file changed, 2 insertions(+)

diff -r cb465381f6d5 arch/x86/mm/gup.c

arch/x86/mm/gup.c +1 -0

@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
+#include <linux/module.h>
 
 #include <asm/pgtable.h>
 

arch/x86/mm/gup.c slow_irqon: · line 306

Quoted 4 lines

arch/x86/mm/gup.c +1 -0

@@ -296,3 +303,4 @@ slow_irqon:
 		return ret;
 	}
 }
+EXPORT_SYMBOL_GPL(get_user_pages_fast);

A regularly-occurring problem with an export like this is that someone writes a driver and tests it on x86, then the driver explodes on an architecture which didn't export the same symbol.

So a better implementation might be to put

Quoted 5 lines

#ifdef CONFIG_HAVE_GET_USER_PAGES_FAST
EXPORT_SYMBOL_GPL(get_user_pages_fast);
#endif

into generic code somewhere.

How would that help?

Quoted 38 lines · On Wed, 30 Jul 2008 18:47:36 +0300 Adrian Bunk <[email protected]> wrote:

Quoted 37 lines · Andrew Morton

Quoted 20 lines · On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <[email protected]> wrote:

After all, it was made for lguest wasn't it? Still think it should be called get_current_pages() though.

Signed-off-by Rusty Russell <[email protected]>

---
 arch/x86/mm/gup.c |    2 ++
 1 file changed, 2 insertions(+)

diff -r cb465381f6d5 arch/x86/mm/gup.c

arch/x86/mm/gup.c +1 -0

@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
+#include <linux/module.h>
 
 #include <asm/pgtable.h>
 

arch/x86/mm/gup.c slow_irqon: · line 306

Quoted 4 lines

arch/x86/mm/gup.c +1 -0

@@ -296,3 +303,4 @@ slow_irqon:
 		return ret;
 	}
 }
+EXPORT_SYMBOL_GPL(get_user_pages_fast);

A regularly-occurring problem with an export like this is that someone writes a driver and tests it on x86, then the driver explodes on an architecture which didn't export the same symbol.

So a better implementation might be to put

#ifdef CONFIG_HAVE_GET_USER_PAGES_FAST
EXPORT_SYMBOL_GPL(get_user_pages_fast);
#endif

Quoted 3 lines

Quoted 1 line

into generic code somewhere.

How would that help?

It would fix the problem which I described.

Quoted 40 lines · Andrew Morton

Quoted 38 lines · On Wed, 30 Jul 2008 18:47:36 +0300 Adrian Bunk <[email protected]> wrote:

Quoted 37 lines · Andrew Morton

Quoted 20 lines · On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <[email protected]> wrote:

After all, it was made for lguest wasn't it? Still think it should be called get_current_pages() though.

Signed-off-by Rusty Russell <[email protected]>

---
 arch/x86/mm/gup.c |    2 ++
 1 file changed, 2 insertions(+)

diff -r cb465381f6d5 arch/x86/mm/gup.c

arch/x86/mm/gup.c +1 -0

@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
+#include <linux/module.h>
 
 #include <asm/pgtable.h>
 

arch/x86/mm/gup.c slow_irqon: · line 306

Quoted 4 lines

arch/x86/mm/gup.c +1 -0

@@ -296,3 +303,4 @@ slow_irqon:
 		return ret;
 	}
 }
+EXPORT_SYMBOL_GPL(get_user_pages_fast);

A regularly-occurring problem with an export like this is that someone writes a driver and tests it on x86, then the driver explodes on an architecture which didn't export the same symbol.

So a better implementation might be to put

#ifdef CONFIG_HAVE_GET_USER_PAGES_FAST
EXPORT_SYMBOL_GPL(get_user_pages_fast);
#endif

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

into generic code somewhere.

How would that help?

It would fix the problem which I described.

The export would still only be available on x86 since no other arch would define CONFIG_HAVE_GET_USER_PAGES_FAST.

Quoted 45 lines · On Wed, 30 Jul 2008 20:09:39 +0300 Adrian Bunk <[email protected]> wrote:

Quoted 44 lines · Andrew Morton

Quoted 41 lines · On Wed, 30 Jul 2008 18:47:36 +0300 Adrian Bunk <[email protected]> wrote:

Quoted 31 lines · Andrew Morton

Quoted 20 lines · On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <[email protected]> wrote:

After all, it was made for lguest wasn't it? Still think it should be called get_current_pages() though.

Signed-off-by Rusty Russell <[email protected]>

---
 arch/x86/mm/gup.c |    2 ++
 1 file changed, 2 insertions(+)

diff -r cb465381f6d5 arch/x86/mm/gup.c

arch/x86/mm/gup.c +1 -0

@@ -8,6 +8,7 @@
 #include <linux/mm.h>
 #include <linux/vmstat.h>
 #include <linux/highmem.h>
+#include <linux/module.h>
 
 #include <asm/pgtable.h>
 

arch/x86/mm/gup.c slow_irqon: · line 306

Quoted 4 lines

arch/x86/mm/gup.c +1 -0

@@ -296,3 +303,4 @@ slow_irqon:
 		return ret;
 	}
 }
+EXPORT_SYMBOL_GPL(get_user_pages_fast);

A regularly-occurring problem with an export like this is that someone writes a driver and tests it on x86, then the driver explodes on an architecture which didn't export the same symbol.

Quoted 7 lines

So a better implementation might be to put

#ifdef CONFIG_HAVE_GET_USER_PAGES_FAST
EXPORT_SYMBOL_GPL(get_user_pages_fast);
#endif

into generic code somewhere.

How would that help?

Quoted 5 lines

Quoted 1 line

It would fix the problem which I described.

The export would still only be available on x86 since no other arch would define CONFIG_HAVE_GET_USER_PAGES_FAST.

That's presently true in Linus mainline. The powerpc implementation is in -mm, but apprarently needs a bit more work.

Quoted 6 lines

Quoted 3 lines

The export would still only be available on x86 since no other arch would define CONFIG_HAVE_GET_USER_PAGES_FAST.

That's presently true in Linus mainline. The powerpc implementation is in -mm, but apprarently needs a bit more work.

I still don't get the advantage of your suggestion.

With the powerpc implementation included your suggestion doesn't change the fact that the export is available only on x86 and powerpc, and if a driver author mistakenly uses it and only tests it's driver on x86 it will still break on architectures other than x86 or powerpc.

Quoted 7 lines · On Wed, 30 Jul 2008 20:18:45 +0300 Adrian Bunk <[email protected]> wrote:

Quoted 6 lines

Quoted 3 lines

The export would still only be available on x86 since no other arch would define CONFIG_HAVE_GET_USER_PAGES_FAST.

That's presently true in Linus mainline. The powerpc implementation is in -mm, but apprarently needs a bit more work.

Quoted 6 lines

I still don't get the advantage of your suggestion.

With the powerpc implementation included your suggestion doesn't change the fact that the export is available only on x86 and powerpc, and if a driver author mistakenly uses it and only tests it's driver on x86 it will still break on architectures other than x86 or powerpc.

On non-CONFIG_HAVE_GET_USER_PAGES_FAST architectures a get_user_pages_fast() call will in fact call get_user_pages().

On CONFIG_HAVE_GET_USER_PAGES_FAST architectures, get_user_pages_fast() is a real function. All architectures which implement that function should export it to modules. Hence it should be exported from within generic code.

Quoted 7 lines

On non-CONFIG_HAVE_GET_USER_PAGES_FAST architectures a get_user_pages_fast() call will in fact call get_user_pages().

On CONFIG_HAVE_GET_USER_PAGES_FAST architectures, get_user_pages_fast() is a real function. All architectures which implement that function should export it to modules. Hence it should be exported from within generic code.

Are we talking at cross purposes?

I thought you were talking an error due to a missing function.

If you think about an error where the function is present but only the EXPORT_SYMBOL is missing

then I get your point (although I do not
consider it a huge problem).

Quoted 3 lines

o other arch would define CONFIG_HAVE_GET_USER_PAGES_FAST.

That's presently true in Linus mainline. The powerpc implementation is in -mm, but apprarently needs a bit more work.

Quoted 5 lines · Andrew Morton

Quoted 4 lines

o other arch
would define CONFIG_HAVE_GET_USER_PAGES_FAST.

Quoted 2 lines

That's presently true in Linus mainline. The powerpc implementation is in -mm, but apprarently needs a bit more work.

Wouldn't a generic get_user_pages_fast() implementation which calls get_user_pages_slow() if the arch hadn't implemented the former reduce a lot of #ifdefs in call sites?

That is how it is presently implemented.

#ifdef CONFIG_HAVE_GET_USER_PAGES_FAST
/*
* get_user_pages_fast provides equivalent functionality to get_user_pages,
* operating on current and current->mm (force=0 and doesn't return any vmas). *
* get_user_pages_fast may take mmap_sem and page tables, so no assumptions
* can be made about locking. get_user_pages_fast is to be implemented in a
* way that is advantageous (vs get_user_pages()) when the user memory area is
* already faulted in and present in ptes. However if the pages have to be
* faulted in, it may turn out to be slightly slower).
*/
int get_user_pages_fast(unsigned long start, int nr_pages, int write,

			struct page **pages);

#else
/*
* Should probably be moved to asm-generic, and architectures can include it if
* they don't implement their own get_user_pages_fast.
*/
#define get_user_pages_fast(start, nr_pages, write, pages) \

({								\
	struct mm_struct *mm = current->mm;			\
	int ret;						\
								\
	down_read(&mm->mmap_sem);				\
	ret = get_user_pages(current, mm, start, nr_pages,	\
					write, 0, pages, NULL);	\
	up_read(&mm->mmap_sem);					\
								\
	ret;							\
})

#endif

Quoted 31 lines

#ifdef CONFIG_HAVE_GET_USER_PAGES_FAST
/*
* get_user_pages_fast provides equivalent functionality to get_user_pages,
* operating on current and current->mm (force=0 and doesn't return any vmas). *
* get_user_pages_fast may take mmap_sem and page tables, so no assumptions
* can be made about locking. get_user_pages_fast is to be implemented in a
* way that is advantageous (vs get_user_pages()) when the user memory area is
* already faulted in and present in ptes. However if the pages have to be
* faulted in, it may turn out to be slightly slower).
*/
int get_user_pages_fast(unsigned long start, int nr_pages, int write,

			struct page **pages);

#else
/*
* Should probably be moved to asm-generic, and architectures can include it if
* they don't implement their own get_user_pages_fast.
*/
#define get_user_pages_fast(start, nr_pages, write, pages) \

({								\
	struct mm_struct *mm = current->mm;			\
	int ret;						\
								\
	down_read(&mm->mmap_sem);				\
	ret = get_user_pages(current, mm, start, nr_pages,	\
					write, 0, pages, NULL);	\
	up_read(&mm->mmap_sem);					\
								\
	ret;							\
})

Quoted 3 lines

#endif

Surprised. Why not an out-of-line function? Would eliminate the present discussion as well.

Quoted 2 lines · On Thu, 31 Jul 2008 09:43:59 +0300 Avi Kivity <[email protected]> wrote:

Surprised. Why not an out-of-line function? Would eliminate the present discussion as well.

Excellent question!

Quoted 6 lines · Andrew Morton

Quoted 2 lines · On Thu, 31 Jul 2008 09:43:59 +0300 Avi Kivity <[email protected]> wrote:

Surprised. Why not an out-of-line function? Would eliminate the present discussion as well.

Excellent question!

Like this?

Index: linux-2.6/include/linux/mm.h
===================================================================

include/linux/mm.h +0 -20

@@ -834,7 +834,6 @@ extern int mprotect_fixup(struct vm_area
 			  struct vm_area_struct **pprev, unsigned long start,
 			  unsigned long end, unsigned long newflags);
 
-#ifdef CONFIG_HAVE_GET_USER_PAGES_FAST
 /*
  * get_user_pages_fast provides equivalent functionality to get_user_pages,
  * operating on current and current->mm (force=0 and doesn't return any vmas).

@@ -848,25 +847,6 @@ extern int mprotect_fixup(struct vm_area
 int get_user_pages_fast(unsigned long start, int nr_pages, int write,
 			struct page **pages);
 
-#else
-/*
- * Should probably be moved to asm-generic, and architectures can include it if
- * they don't implement their own get_user_pages_fast.
- */
-#define get_user_pages_fast(start, nr_pages, write, pages)	\
-({								\
-	struct mm_struct *mm = current->mm;			\
-	int ret;						\
-								\
-	down_read(&mm->mmap_sem);				\
-	ret = get_user_pages(current, mm, start, nr_pages,	\
-					write, 0, pages, NULL);	\
-	up_read(&mm->mmap_sem);					\
-								\
-	ret;							\
-})
-#endif
-
 /*
  * A callback you can register to apply pressure to ageable caches.
  *

Index: linux-2.6/mm/memory.c
===================================================================

mm/memory.c +4 -0

@@ -1270,6 +1270,21 @@ int get_user_pages(struct task_struct *t
 }
 EXPORT_SYMBOL(get_user_pages);
 
+int __attribute__((weak)) get_user_pages_fast(unsigned long start,
+				int nr_pages, int write, struct page **pages)
+{
+	struct mm_struct *mm = current->mm;

mm/memory.c +0 -0

 pte_t *get_locked_pte(struct mm_struct *mm, unsigned long addr,
 			spinlock_t **ptl)
 {

Index: linux-2.6/arch/x86/Kconfig
===================================================================

arch/x86/Kconfig +0 -1

@@ -22,7 +22,6 @@ config X86
 	select HAVE_IDE
 	select HAVE_OPROFILE
 	select HAVE_IOREMAP_PROT
-	select HAVE_GET_USER_PAGES_FAST
 	select HAVE_KPROBES
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select HAVE_KRETPROBES

Index: linux-2.6/arch/x86/mm/Makefile
===================================================================

If we do this then corresponding changes should be made to the powerpc patch upon which Ben presently sits.

On Thu, 31 Jul 2008 18:10:57 +1000 Nick Piggin <[email protected]>

Quoted 8 lines · Andrew Morton

Quoted 5 lines · On Thu, 31 Jul 2008 09:43:59 +0300 Avi Kivity <[email protected]> wrote:

Quoted 2 lines

Surprised. Why not an out-of-line function? Would eliminate the present discussion as well.

Excellent question!

Like this?

Boy, that cleaned out a lot of crud, didn't it?

Quoted 9 lines

mm/memory.c +9 -0

+int __attribute__((weak)) get_user_pages_fast(unsigned long start,
+				int nr_pages, int write, struct page **pages)
+{
+	struct mm_struct *mm = current->mm;
+	int ret;
+
+	down_read(&mm->mmap_sem);
+	ret = get_user_pages(current, mm, start, nr_pages,
+					write, 0, pages, NULL);

mm/memory.c int get_user_pages(struct task_struct *t · line 1286

Quoted 5 lines

mm/memory.c +5 -0

+	up_read(&mm->mmap_sem);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(get_user_pages_fast);

hm, OK, I guess there's no reason why gcc should muck up the export of a weak symbol.

Quoted 24 lines

pte_t *get_locked_pte(struct mm_struct *mm, unsigned long addr,
spinlock_t **ptl)
{
Index: linux-2.6/arch/x86/Kconfig
===================================================================

arch/x86/Kconfig +0 -1

@@ -22,7 +22,6 @@ config X86
 	select HAVE_IDE
 	select HAVE_OPROFILE
 	select HAVE_IOREMAP_PROT
-	select HAVE_GET_USER_PAGES_FAST
 	select HAVE_KPROBES
 	select ARCH_WANT_OPTIONAL_GPIOLIB
 	select HAVE_KRETPROBES

Index: linux-2.6/arch/x86/mm/Makefile
===================================================================
--- linux-2.6.orig/arch/x86/mm/Makefile 2008-07-31 18:08:58.000000000
+1000 +++ linux-2.6/arch/x86/mm/Makefile 2008-07-31 18:09:07.000000000
+1000 @@ -1,7 +1,6 @@
obj-y := init_$(BITS).o fault.o ioremap.o extable.o pageattr.o mmap.o \

arch/x86/Kconfig +1 -1

-	    pat.o pgtable.o
+	    pat.o pgtable.o gup.o

Quoted 4 lines

-obj-$(CONFIG_HAVE_GET_USER_PAGES_FAST) += gup.o
obj-$(CONFIG_X86_32) += pgtable_32.o

obj-$(CONFIG_HUGETLB_PAGE) += hugetlbpage.o

If we do this then corresponding changes should be made to the powerpc patch upon which Ben presently sits.

Quoted 2 lines

If we do this then corresponding changes should be made to the powerpc patch upon which Ben presently sits.

Powerpc will still use the CONFIG symbol for the moment because 32-bit TLB flushing has not been verified to work, so we don't want to pull in the 64-bit gup. There is some discussion going on, but they probably need at least to implement pte_special, and then also do RCU freeing of page tables.

Quoted 8 lines · Nick Piggin

Quoted 2 lines

If we do this then corresponding changes should be made to the powerpc patch upon which Ben presently sits.

Powerpc will still use the CONFIG symbol for the moment because 32-bit TLB flushing has not been verified to work, so we don't want to pull in the 64-bit gup. There is some discussion going on, but they probably need at least to implement pte_special, and then also do RCU freeing of page tables.

It's just a matter of building gup.c or not .. as long as it's in the makefile as obj-$(CONFIG_PPC64) (as I put it in the merged version of the patch), the weak symbol should work.

I see. Here we are, then.