[PATCH 1/2] Export get_user_pages_fast

[PATCH 1/2] Export get_user_pages_fast

[Rusty Russell]

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 <rusty@rustcorp.com.au>
---
 arch/x86/mm/gup.c |    2 ++
 1 file changed, 2 insertions(+)

diff -r cb465381f6d5 arch/x86/mm/gup.c
--- a/arch/x86/mm/gup.c	Wed Jul 30 10:18:44 2008 +1000
+++ b/arch/x86/mm/gup.c	Wed Jul 30 14:22:53 2008 +1000
@@ -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);

[PATCH 2/2] lguest: use get_user_pages_fast() instead of get_user_pages()

[Rusty Russell]

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 <rusty@rustcorp.com.au>
---
 drivers/lguest/page_tables.c |   25 +++++++++----------------
 1 file changed, 9 insertions(+), 16 deletions(-)

diff -r cb465381f6d5 drivers/lguest/page_tables.c
--- a/drivers/lguest/page_tables.c	Wed Jul 30 10:18:44 2008 +1000
+++ b/drivers/lguest/page_tables.c	Wed Jul 30 12:11:42 2008 +1000
@@ -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)
 {
 	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)
 {
-	/* 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)));

Re: [PATCH 1/2] Export get_user_pages_fast

[Nick Piggin]

On Wednesday 30 July 2008 14:38, Rusty Russell wrote:
> After all, it was made for lguest wasn't it?

Right, I'd been using direct-IO to iron out all the bugs before
I consider it fit for use by lguest. Given that it doesn't
appear to have corrupted any disks yet, and no longer oopses...

Acked-by: Nick Piggin <npiggin@suse.de>


> Still think it should be 
> called get_current_pages() though.

We have to have some comical way to solve these disagreements,
preferably with the loser being publically humiliated. Hmm...


> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  arch/x86/mm/gup.c |    2 ++
>  1 file changed, 2 insertions(+)
>
> diff -r cb465381f6d5 arch/x86/mm/gup.c
> --- a/arch/x86/mm/gup.c	Wed Jul 30 10:18:44 2008 +1000
> +++ b/arch/x86/mm/gup.c	Wed Jul 30 14:22:53 2008 +1000
> @@ -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);

Re: [PATCH 2/2] lguest: use get_user_pages_fast() instead of get_user_pages()

[Nick Piggin]

On Wednesday 30 July 2008 14:39, Rusty Russell wrote:
> 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.

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)

Nice.

>
> Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
> ---
>  drivers/lguest/page_tables.c |   25 +++++++++----------------
>  1 file changed, 9 insertions(+), 16 deletions(-)
>
> diff -r cb465381f6d5 drivers/lguest/page_tables.c
> --- a/drivers/lguest/page_tables.c	Wed Jul 30 10:18:44 2008 +1000
> +++ b/drivers/lguest/page_tables.c	Wed Jul 30 12:11:42 2008 +1000
> @@ -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)
>  {
>  	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)
>  {
> -	/* 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)));

Re: [PATCH 2/2] lguest: use get_user_pages_fast() instead of get_user_pages()

[Rusty Russell]

On Wednesday 30 July 2008 15:30:10 Nick Piggin wrote:
> On Wednesday 30 July 2008 14:39, Rusty Russell wrote:
> > 	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.
>
> 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)

Yep.  I mean, the performance is abysmal either way (native for this is about 
10x faster), but it would be interesting to see kvm numbers as they have a 
more sophisticated shadow page table setup.  Not as trivial as lguest since 
they seem to use the mm sem to protect other things...

Cheers,
Rusty.

Re: [PATCH 1/2] Export get_user_pages_fast

[Andrew Morton]

On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <rusty@rustcorp.com.au> 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 <rusty@rustcorp.com.au>
> ---
>  arch/x86/mm/gup.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff -r cb465381f6d5 arch/x86/mm/gup.c
> --- a/arch/x86/mm/gup.c	Wed Jul 30 10:18:44 2008 +1000
> +++ b/arch/x86/mm/gup.c	Wed Jul 30 14:22:53 2008 +1000
> @@ -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);

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.

Re: [PATCH 1/2] Export get_user_pages_fast

[Adrian Bunk]

On Wed, Jul 30, 2008 at 03:35:23AM -0700, Andrew Morton wrote:
> On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <rusty@rustcorp.com.au> 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 <rusty@rustcorp.com.au>
> > ---
> >  arch/x86/mm/gup.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff -r cb465381f6d5 arch/x86/mm/gup.c
> > --- a/arch/x86/mm/gup.c	Wed Jul 30 10:18:44 2008 +1000
> > +++ b/arch/x86/mm/gup.c	Wed Jul 30 14:22:53 2008 +1000
> > @@ -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);
> 
> 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.

How would that help?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1/2] Export get_user_pages_fast

[Andrew Morton]

On Wed, 30 Jul 2008 18:47:36 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> On Wed, Jul 30, 2008 at 03:35:23AM -0700, Andrew Morton wrote:
> > On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <rusty@rustcorp.com.au> 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 <rusty@rustcorp.com.au>
> > > ---
> > >  arch/x86/mm/gup.c |    2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff -r cb465381f6d5 arch/x86/mm/gup.c
> > > --- a/arch/x86/mm/gup.c	Wed Jul 30 10:18:44 2008 +1000
> > > +++ b/arch/x86/mm/gup.c	Wed Jul 30 14:22:53 2008 +1000
> > > @@ -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);
> > 
> > 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.
> 
> How would that help?
> 

It would fix the problem which I described.

Re: [PATCH 1/2] Export get_user_pages_fast

[Adrian Bunk]

On Wed, Jul 30, 2008 at 10:03:55AM -0700, Andrew Morton wrote:
> On Wed, 30 Jul 2008 18:47:36 +0300 Adrian Bunk <bunk@kernel.org> wrote:
> 
> > On Wed, Jul 30, 2008 at 03:35:23AM -0700, Andrew Morton wrote:
> > > On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <rusty@rustcorp.com.au> 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 <rusty@rustcorp.com.au>
> > > > ---
> > > >  arch/x86/mm/gup.c |    2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff -r cb465381f6d5 arch/x86/mm/gup.c
> > > > --- a/arch/x86/mm/gup.c	Wed Jul 30 10:18:44 2008 +1000
> > > > +++ b/arch/x86/mm/gup.c	Wed Jul 30 14:22:53 2008 +1000
> > > > @@ -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);
> > > 
> > > 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.
> > 
> > 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.

Or what do I miss?

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1/2] Export get_user_pages_fast

[Andrew Morton]

On Wed, 30 Jul 2008 20:09:39 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> On Wed, Jul 30, 2008 at 10:03:55AM -0700, Andrew Morton wrote:
> > On Wed, 30 Jul 2008 18:47:36 +0300 Adrian Bunk <bunk@kernel.org> wrote:
> > 
> > > On Wed, Jul 30, 2008 at 03:35:23AM -0700, Andrew Morton wrote:
> > > > On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <rusty@rustcorp.com.au> 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 <rusty@rustcorp.com.au>
> > > > > ---
> > > > >  arch/x86/mm/gup.c |    2 ++
> > > > >  1 file changed, 2 insertions(+)
> > > > > 
> > > > > diff -r cb465381f6d5 arch/x86/mm/gup.c
> > > > > --- a/arch/x86/mm/gup.c	Wed Jul 30 10:18:44 2008 +1000
> > > > > +++ b/arch/x86/mm/gup.c	Wed Jul 30 14:22:53 2008 +1000
> > > > > @@ -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);
> > > > 
> > > > 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.
> > > 
> > > 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.
> 

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

Re: [PATCH 1/2] Export get_user_pages_fast

[Adrian Bunk]

On Wed, Jul 30, 2008 at 10:14:38AM -0700, Andrew Morton wrote:
> On Wed, 30 Jul 2008 20:09:39 +0300 Adrian Bunk <bunk@kernel.org> wrote:
> 
> > On Wed, Jul 30, 2008 at 10:03:55AM -0700, Andrew Morton wrote:
> > > On Wed, 30 Jul 2008 18:47:36 +0300 Adrian Bunk <bunk@kernel.org> wrote:
> > > 
> > > > On Wed, Jul 30, 2008 at 03:35:23AM -0700, Andrew Morton wrote:
> > > > > On Wed, 30 Jul 2008 14:38:55 +1000 Rusty Russell <rusty@rustcorp.com.au> 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 <rusty@rustcorp.com.au>
> > > > > > ---
> > > > > >  arch/x86/mm/gup.c |    2 ++
> > > > > >  1 file changed, 2 insertions(+)
> > > > > > 
> > > > > > diff -r cb465381f6d5 arch/x86/mm/gup.c
> > > > > > --- a/arch/x86/mm/gup.c	Wed Jul 30 10:18:44 2008 +1000
> > > > > > +++ b/arch/x86/mm/gup.c	Wed Jul 30 14:22:53 2008 +1000
> > > > > > @@ -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);
> > > > > 
> > > > > 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.
> > > > 
> > > > 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.
> > 
> 
> 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.

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1/2] Export get_user_pages_fast

[Avi Kivity]

Andrew Morton wrote:
>> 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.

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?

-- 
error compiling committee.c: too many arguments to function

Re: [PATCH 1/2] Export get_user_pages_fast

[Andrew Morton]

On Wed, 30 Jul 2008 20:18:45 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> > > 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.

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.

Re: [PATCH 1/2] Export get_user_pages_fast

[Andrew Morton]

On Wed, 30 Jul 2008 20:23:01 +0300 Avi Kivity <avi@qumranet.com> wrote:

> Andrew Morton wrote:
> >> 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.
> 
> 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

Re: [PATCH 1/2] Export get_user_pages_fast

[Adrian Bunk]

On Wed, Jul 30, 2008 at 10:28:17AM -0700, Andrew Morton wrote:
> On Wed, 30 Jul 2008 20:18:45 +0300 Adrian Bunk <bunk@kernel.org> wrote:
> 
> > > > 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.
> 
> 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).

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

Re: [PATCH 1/2] Export get_user_pages_fast

[Andrew Morton]

On Wed, 30 Jul 2008 20:39:44 +0300 Adrian Bunk <bunk@kernel.org> wrote:

> > 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?

Apparently.

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

Nope.

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

yup.

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

Me either.

Re: [PATCH 1/2] Export get_user_pages_fast

[Avi Kivity]

Andrew Morton wrote:
> That is how it is presently implemented.
>
>   

Cool.

> #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
>
>   

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

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [PATCH 1/2] Export get_user_pages_fast

[Andrew Morton]

On Thu, 31 Jul 2008 09:43:59 +0300 Avi Kivity <avi@qumranet.com> wrote:

> > #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
> >
> >   
> 
> Surprised.  Why not an out-of-line function?  Would eliminate the 
> present discussion as well.

Excellent question!

Re: [PATCH 1/2] Export get_user_pages_fast

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 266 bytes --]

On Thursday 31 July 2008 17:02, Andrew Morton wrote:
> On Thu, 31 Jul 2008 09:43:59 +0300 Avi Kivity <avi@qumranet.com> wrote:

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

Like this?

[-- Attachment #2: out-of-line-gup.patch --]
[-- Type: text/x-diff, Size: 3122 bytes --]

Index: linux-2.6/include/linux/mm.h
===================================================================
--- linux-2.6.orig/include/linux/mm.h	2008-07-31 17:59:04.000000000 +1000
+++ linux-2.6/include/linux/mm.h	2008-07-31 18:01:51.000000000 +1000
@@ -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
===================================================================
--- linux-2.6.orig/mm/memory.c	2008-07-31 17:59:26.000000000 +1000
+++ linux-2.6/mm/memory.c	2008-07-31 18:01:25.000000000 +1000
@@ -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;
+	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);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(get_user_pages_fast);
+
 pte_t *get_locked_pte(struct mm_struct *mm, unsigned long addr,
 			spinlock_t **ptl)
 {
Index: linux-2.6/arch/x86/Kconfig
===================================================================
--- linux-2.6.orig/arch/x86/Kconfig	2008-07-31 18:08:35.000000000 +1000
+++ linux-2.6/arch/x86/Kconfig	2008-07-31 18:08:53.000000000 +1000
@@ -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 \
-	    pat.o pgtable.o
+	    pat.o pgtable.o gup.o
 
-obj-$(CONFIG_HAVE_GET_USER_PAGES_FAST) += gup.o
 obj-$(CONFIG_X86_32)		+= pgtable_32.o
 
 obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o

Re: [PATCH 1/2] Export get_user_pages_fast

[Andrew Morton]

On Thu, 31 Jul 2008 18:10:57 +1000 Nick Piggin <nickpiggin@yahoo.com.au> wrote:

> On Thursday 31 July 2008 17:02, Andrew Morton wrote:
> > On Thu, 31 Jul 2008 09:43:59 +0300 Avi Kivity <avi@qumranet.com> wrote:
> 
> > > 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?

> Index: linux-2.6/include/linux/mm.h
> ===================================================================
> --- linux-2.6.orig/include/linux/mm.h	2008-07-31 17:59:04.000000000 +1000
> +++ linux-2.6/include/linux/mm.h	2008-07-31 18:01:51.000000000 +1000
> @@ -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
> ===================================================================
> --- linux-2.6.orig/mm/memory.c	2008-07-31 17:59:26.000000000 +1000
> +++ linux-2.6/mm/memory.c	2008-07-31 18:01:25.000000000 +1000
> @@ -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;
> +	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);
> +
> +	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.


>  pte_t *get_locked_pte(struct mm_struct *mm, unsigned long addr,
>  			spinlock_t **ptl)
>  {
> Index: linux-2.6/arch/x86/Kconfig
> ===================================================================
> --- linux-2.6.orig/arch/x86/Kconfig	2008-07-31 18:08:35.000000000 +1000
> +++ linux-2.6/arch/x86/Kconfig	2008-07-31 18:08:53.000000000 +1000
> @@ -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 \
> -	    pat.o pgtable.o
> +	    pat.o pgtable.o gup.o
>  
> -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.

Re: [PATCH 1/2] Export get_user_pages_fast

[Benjamin Herrenschmidt]

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

It's upstream :-) I had a couple of fixes to do to it, but it's
working well now and merged in the last batch.

Nick, we should look into cleaning up the fallback path too,
it's a bit ugly as it is.

Cheers,
Ben.

Re: [PATCH 1/2] Export get_user_pages_fast

[Nick Piggin]

On Thursday 31 July 2008 18:21, Andrew Morton wrote:
> On Thu, 31 Jul 2008 18:10:57 +1000 Nick Piggin <nickpiggin@yahoo.com.au> 
wrote:
> > On Thursday 31 July 2008 17:02, Andrew Morton wrote:
> > > On Thu, 31 Jul 2008 09:43:59 +0300 Avi Kivity <avi@qumranet.com> wrote:
> > > > 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?

Yes.


> > +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);
> > +	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.

Well it seemed to compile both with and without the arch override, and
I added a get_user_pages_fast call to a module and no warnings of any
kind.


> >  pte_t *get_locked_pte(struct mm_struct *mm, unsigned long addr,
> >  			spinlock_t **ptl)
> >  {
> > Index: linux-2.6/arch/x86/Kconfig
> > ===================================================================
> > --- linux-2.6.orig/arch/x86/Kconfig	2008-07-31 18:08:35.000000000 +1000
> > +++ linux-2.6/arch/x86/Kconfig	2008-07-31 18:08:53.000000000 +1000
> > @@ -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 \
> > -	    pat.o pgtable.o
> > +	    pat.o pgtable.o gup.o
> >
> > -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.

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.

Re: [PATCH 1/2] Export get_user_pages_fast

[Benjamin Herrenschmidt]

On Thu, 2008-07-31 at 18:48 +1000, Nick Piggin wrote:
> > 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.

Ben.

Re: [PATCH 1/2] Export get_user_pages_fast

[Nick Piggin]

[-- Attachment #1: Type: text/plain, Size: 775 bytes --]

On Thursday 31 July 2008 18:52, Benjamin Herrenschmidt wrote:
> On Thu, 2008-07-31 at 18:48 +1000, Nick Piggin wrote:
> > > 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.

[-- Attachment #2: out-of-line-gup.patch --]
[-- Type: text/x-diff, Size: 4248 bytes --]

Out of line get_user_pages_fast fallback implementation, make it a weak
symbol, get rid of CONFIG_HAVE_GET_USER_PAGES_FAST.

Export the symbol to modules so lguest can use it.

Signed-off-by: Nick Piggin <npiggin@suse.de>
---

Index: linux-2.6/include/linux/mm.h
===================================================================
--- linux-2.6.orig/include/linux/mm.h	2008-07-31 18:45:19.000000000 +1000
+++ linux-2.6/include/linux/mm.h	2008-07-31 18:53:43.000000000 +1000
@@ -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
===================================================================
--- linux-2.6.orig/mm/memory.c	2008-07-31 18:45:19.000000000 +1000
+++ linux-2.6/mm/memory.c	2008-07-31 18:53:43.000000000 +1000
@@ -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;
+	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);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(get_user_pages_fast);
+
 pte_t *get_locked_pte(struct mm_struct *mm, unsigned long addr,
 			spinlock_t **ptl)
 {
Index: linux-2.6/arch/x86/Kconfig
===================================================================
--- linux-2.6.orig/arch/x86/Kconfig	2008-07-31 18:45:20.000000000 +1000
+++ linux-2.6/arch/x86/Kconfig	2008-07-31 18:53:43.000000000 +1000
@@ -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:45:20.000000000 +1000
+++ linux-2.6/arch/x86/mm/Makefile	2008-07-31 18:53:43.000000000 +1000
@@ -1,7 +1,6 @@
 obj-y	:=  init_$(BITS).o fault.o ioremap.o extable.o pageattr.o mmap.o \
-	    pat.o pgtable.o
+	    pat.o pgtable.o gup.o
 
-obj-$(CONFIG_HAVE_GET_USER_PAGES_FAST) += gup.o
 obj-$(CONFIG_X86_32)		+= pgtable_32.o
 
 obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
Index: linux-2.6/arch/powerpc/Kconfig
===================================================================
--- linux-2.6.orig/arch/powerpc/Kconfig	2008-07-31 18:53:45.000000000 +1000
+++ linux-2.6/arch/powerpc/Kconfig	2008-07-31 18:53:51.000000000 +1000
@@ -42,9 +42,6 @@ config GENERIC_HARDIRQS
 	bool
 	default y
 
-config HAVE_GET_USER_PAGES_FAST
-	def_bool PPC64
-
 config HAVE_SETUP_PER_CPU_AREA
 	def_bool PPC64
 
Index: linux-2.6/mm/Kconfig
===================================================================
--- linux-2.6.orig/mm/Kconfig	2008-07-31 18:55:16.000000000 +1000
+++ linux-2.6/mm/Kconfig	2008-07-31 18:55:20.000000000 +1000
@@ -77,9 +77,6 @@ config FLAT_NODE_MEM_MAP
 	def_bool y
 	depends on !SPARSEMEM
 
-config HAVE_GET_USER_PAGES_FAST
-	bool
-
 #
 # Both the NUMA code and DISCONTIGMEM use arrays of pg_data_t's
 # to represent different areas of memory.  This variable allows

Re: [PATCH 1/2] Export get_user_pages_fast

[Rusty Russell]

On Thursday 31 July 2008 18:56:39 Nick Piggin wrote:
> I see. Here we are, then.

I can't see this in linux-next?  I put it in mm/util.c instead of
mm/memory.c: your version will barf on CONFIG_MMU=n AFAICT.

Since I'm the one who wants the export, I guess I'll fwd:

mm: Make generic weak get_user_pages_fast and EXPORT_GPL it

Out of line get_user_pages_fast fallback implementation, make it a weak
symbol, get rid of CONFIG_HAVE_GET_USER_PAGES_FAST.

Export the symbol to modules so lguest can use it.

Signed-off-by: Nick Piggin <npiggin@suse.de>
Signed-off-by: Rusty Russell <rusty@rustcorp.com.au>
---
 arch/powerpc/Kconfig |    3 ---
 arch/x86/Kconfig     |    1 -
 arch/x86/mm/Makefile |    3 +--
 include/linux/mm.h   |   20 --------------------
 mm/Kconfig           |    3 ---
 mm/util.c            |   15 +++++++++++++++
 6 files changed, 16 insertions(+), 29 deletions(-)

diff -r 607fc39e88f5 arch/powerpc/Kconfig
--- a/arch/powerpc/Kconfig	Tue Aug 05 15:14:02 2008 +1000
+++ b/arch/powerpc/Kconfig	Tue Aug 05 15:46:04 2008 +1000
@@ -41,9 +41,6 @@ config GENERIC_HARDIRQS
 config GENERIC_HARDIRQS
 	bool
 	default y
-
-config HAVE_GET_USER_PAGES_FAST
-	def_bool PPC64
 
 config HAVE_SETUP_PER_CPU_AREA
 	def_bool PPC64
diff -r 607fc39e88f5 arch/x86/Kconfig
--- a/arch/x86/Kconfig	Tue Aug 05 15:14:02 2008 +1000
+++ b/arch/x86/Kconfig	Tue Aug 05 15:46:04 2008 +1000
@@ -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
diff -r 607fc39e88f5 arch/x86/mm/Makefile
--- a/arch/x86/mm/Makefile	Tue Aug 05 15:14:02 2008 +1000
+++ b/arch/x86/mm/Makefile	Tue Aug 05 15:46:04 2008 +1000
@@ -1,7 +1,6 @@ obj-y	:=  init_$(BITS).o fault.o ioremap
 obj-y	:=  init_$(BITS).o fault.o ioremap.o extable.o pageattr.o mmap.o \
-	    pat.o pgtable.o
+	    pat.o pgtable.o gup.o
 
-obj-$(CONFIG_HAVE_GET_USER_PAGES_FAST) += gup.o
 obj-$(CONFIG_X86_32)		+= pgtable_32.o
 
 obj-$(CONFIG_HUGETLB_PAGE)	+= hugetlbpage.o
diff -r 607fc39e88f5 include/linux/mm.h
--- a/include/linux/mm.h	Tue Aug 05 15:14:02 2008 +1000
+++ b/include/linux/mm.h	Tue Aug 05 15:46:04 2008 +1000
@@ -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).
@@ -847,25 +846,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.
diff -r 607fc39e88f5 mm/Kconfig
--- a/mm/Kconfig	Tue Aug 05 15:14:02 2008 +1000
+++ b/mm/Kconfig	Tue Aug 05 15:46:04 2008 +1000
@@ -76,9 +76,6 @@ config FLAT_NODE_MEM_MAP
 config FLAT_NODE_MEM_MAP
 	def_bool y
 	depends on !SPARSEMEM
-
-config HAVE_GET_USER_PAGES_FAST
-	bool
 
 #
 # Both the NUMA code and DISCONTIGMEM use arrays of pg_data_t's
diff -r 607fc39e88f5 mm/util.c
--- a/mm/util.c	Tue Aug 05 15:14:02 2008 +1000
+++ b/mm/util.c	Tue Aug 05 15:46:04 2008 +1000
@@ -171,3 +171,18 @@ void arch_pick_mmap_layout(struct mm_str
 	mm->unmap_area = arch_unmap_area;
 }
 #endif
+
+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);
+	up_read(&mm->mmap_sem);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(get_user_pages_fast);