LinuxLists.cc - [PATCH 1/2] Export get_user_pages

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

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 <[email protected]>

> 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 <[email protected]>
> ---
> 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);

2008-07-30 05:30:37

[permalink] [raw]

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

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 <[email protected]>
> ---
> 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)));

2008-07-30 06:53:24

by Rusty Russell

[permalink] [raw]

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

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.

2008-07-30 10:36:25

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

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

2008-07-30 15:48:37

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Wed, Jul 30, 2008 at 03:35:23AM -0700, Andrew Morton wrote:
> 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
> > --- 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

2008-07-30 17:04:25

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Wed, 30 Jul 2008 18:47:36 +0300 Adrian Bunk <[email protected]> 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 <[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
> > > --- 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.

2008-07-30 17:10:41

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Wed, Jul 30, 2008 at 10:03:55AM -0700, Andrew Morton wrote:
> On Wed, 30 Jul 2008 18:47:36 +0300 Adrian Bunk <[email protected]> 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 <[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
> > > > --- 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

2008-07-30 17:15:07

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Wed, 30 Jul 2008 20:09:39 +0300 Adrian Bunk <[email protected]> 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 <[email protected]> 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 <[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
> > > > > --- 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.

2008-07-30 17:19:47

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Wed, Jul 30, 2008 at 10:14:38AM -0700, Andrew Morton wrote:
> On Wed, 30 Jul 2008 20:09:39 +0300 Adrian Bunk <[email protected]> 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 <[email protected]> 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 <[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
> > > > > > --- 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

2008-07-30 17:23:19

by Avi Kivity

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

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

2008-07-30 17:28:47

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Wed, 30 Jul 2008 20:18:45 +0300 Adrian Bunk <[email protected]> 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.

2008-07-30 17:29:55

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Wed, 30 Jul 2008 20:23:01 +0300 Avi Kivity <[email protected]> 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

2008-07-30 17:40:44

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Wed, Jul 30, 2008 at 10:28:17AM -0700, Andrew Morton wrote:
> On Wed, 30 Jul 2008 20:18:45 +0300 Adrian Bunk <[email protected]> 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

2008-07-30 17:51:48

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Wed, 30 Jul 2008 20:39:44 +0300 Adrian Bunk <[email protected]> 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.

2008-07-31 06:44:19

by Avi Kivity

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

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.

2008-07-31 07:03:45

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Thu, 31 Jul 2008 09:43:59 +0300 Avi Kivity <[email protected]> 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!

2008-07-31 08:11:25

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Thursday 31 July 2008 17:02, Andrew Morton wrote:
> 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?

Attachments:

(No filename) (266.00 B)
out-of-line-gup.patch (3.05 kB)
Download all attachments

2008-07-31 08:23:00

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

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

> On Thursday 31 July 2008 17:02, Andrew Morton wrote:
> > 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?
>

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.

2008-07-31 08:33:02

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

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

2008-07-31 08:48:32

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

On Thursday 31 July 2008 18:21, Andrew Morton wrote:
> On Thu, 31 Jul 2008 18:10:57 +1000 Nick Piggin <[email protected]>
wrote:
> > On Thursday 31 July 2008 17:02, Andrew Morton wrote:
> > > 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?
>
> 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.

2008-07-31 08:52:57

by Benjamin Herrenschmidt

[permalink] [raw]

Subject: Re: [PATCH 1/2] Export get_user_pages_fast

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.

2008-07-31 08:56:59