Infiniband use of get_user_pages()

Infiniband use of get_user_pages()

[Jan Kara]

  Hello,

  when checking users of get_user_pages() (I'm doing some cleanups in that
area to fix filesystem's issues with mmap_sem locking) I've noticed that
infiniband drivers add number of pages obtained from get_user_pages() to
mm->pinned_vm counter. Although this makes some sence, it doesn't match
with any other user of get_user_pages() (e.g. direct IO) so has infiniband
some special reason why it does so?

  Also that seems to be the only real reason why mmap_sem has to be grabbed
in exclusive mode, am I right?

  Another suspicious thing (at least in drivers/infiniband/core/umem.c:
ib_umem_get()) is that arguments of get_user_pages() are like:
                ret = get_user_pages(current, current->mm, cur_base,
                                     min_t(unsigned long, npages,
                                           PAGE_SIZE / sizeof (struct page *)),
                                     1, !umem->writable, page_list, vma_list);
So we always have write argument set to 1 and force argument is set to
!umem->writable. Is that really intentional? My naive guess would be that
arguments should be switched... Although even in that case I fail to see
why 'force' argument should be set. Can someone please explain?

  Finally (and here I may show my ignorance ;), I'd like to ask whether
there's any reason why ib_umem_get() checks for is_vm_hugetlb_page() and
not just whether a page is a huge page?


								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Infiniband use of get_user_pages()

[Christoph Lameter]

On Wed, 24 Apr 2013, Jan Kara wrote:

>   Hello,
>
>   when checking users of get_user_pages() (I'm doing some cleanups in that
> area to fix filesystem's issues with mmap_sem locking) I've noticed that
> infiniband drivers add number of pages obtained from get_user_pages() to
> mm->pinned_vm counter. Although this makes some sence, it doesn't match
> with any other user of get_user_pages() (e.g. direct IO) so has infiniband
> some special reason why it does so?

get_user_pages typically is used to temporarily increase the refcount. The
Infiniband layer needs to permanently pin the pages for memory
registration.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Infiniband use of get_user_pages()

[Roland Dreier]

On Wed, Apr 24, 2013 at 8:38 AM, Jan Kara <jack@suse.cz> wrote:
>   when checking users of get_user_pages() (I'm doing some cleanups in that
> area to fix filesystem's issues with mmap_sem locking) I've noticed that
> infiniband drivers add number of pages obtained from get_user_pages() to
> mm->pinned_vm counter. Although this makes some sence, it doesn't match
> with any other user of get_user_pages() (e.g. direct IO) so has infiniband
> some special reason why it does so?

Direct IO mappings are in some sense ephemeral -- they only need to
last while the IO is in flight.  In contrast the IB memory pinning is
controlled by (possibly unprivileged) userspace and might last the
whole lifetime of a long-lived application.  So we want some
accounting and resource control.

>   Also that seems to be the only real reason why mmap_sem has to be grabbed
> in exclusive mode, am I right?

Most likely that is true.

>   Another suspicious thing (at least in drivers/infiniband/core/umem.c:
> ib_umem_get()) is that arguments of get_user_pages() are like:
>                 ret = get_user_pages(current, current->mm, cur_base,
>                                      min_t(unsigned long, npages,
>                                            PAGE_SIZE / sizeof (struct page *)),
>                                      1, !umem->writable, page_list, vma_list);
> So we always have write argument set to 1 and force argument is set to
> !umem->writable. Is that really intentional? My naive guess would be that
> arguments should be switched... Although even in that case I fail to see
> why 'force' argument should be set. Can someone please explain?

This confused even me recently.  We had a long discussion (read the
whole thread starting here: https://lkml.org/lkml/2012/1/26/7) but in
short the current parameters seem to be needed to trigger COW even
when the kernel/hardware want to read the memory, to avoid problems
where we get stale data if userspace triggers COW.

I think I better add a comment explaining this.

>   Finally (and here I may show my ignorance ;), I'd like to ask whether
> there's any reason why ib_umem_get() checks for is_vm_hugetlb_page() and
> not just whether a page is a huge page?

I'm not sure of the history here.  How would one check directly if a
page is a huge page?  get_user_pages() actually goes to some trouble
to return all small pages, even when it has to split a single huge
page into many entries in the page array.  (Which is actually a bit
unfortunate for our use here)

 - R.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Infiniband use of get_user_pages()

[Jan Kara]

On Wed 24-04-13 15:25:25, Roland Dreier wrote:
> On Wed, Apr 24, 2013 at 8:38 AM, Jan Kara <jack@suse.cz> wrote:
> >   when checking users of get_user_pages() (I'm doing some cleanups in that
> > area to fix filesystem's issues with mmap_sem locking) I've noticed that
> > infiniband drivers add number of pages obtained from get_user_pages() to
> > mm->pinned_vm counter. Although this makes some sence, it doesn't match
> > with any other user of get_user_pages() (e.g. direct IO) so has infiniband
> > some special reason why it does so?
> 
> Direct IO mappings are in some sense ephemeral -- they only need to
> last while the IO is in flight.  In contrast the IB memory pinning is
> controlled by (possibly unprivileged) userspace and might last the
> whole lifetime of a long-lived application.  So we want some
> accounting and resource control.
  I see, thanks for explanation.

> >   Also that seems to be the only real reason why mmap_sem has to be grabbed
> > in exclusive mode, am I right?
> 
> Most likely that is true.
> 
> >   Another suspicious thing (at least in drivers/infiniband/core/umem.c:
> > ib_umem_get()) is that arguments of get_user_pages() are like:
> >                 ret = get_user_pages(current, current->mm, cur_base,
> >                                      min_t(unsigned long, npages,
> >                                            PAGE_SIZE / sizeof (struct page *)),
> >                                      1, !umem->writable, page_list, vma_list);
> > So we always have write argument set to 1 and force argument is set to
> > !umem->writable. Is that really intentional? My naive guess would be that
> > arguments should be switched... Although even in that case I fail to see
> > why 'force' argument should be set. Can someone please explain?
> 
> This confused even me recently.  We had a long discussion (read the
> whole thread starting here: https://lkml.org/lkml/2012/1/26/7) but in
> short the current parameters seem to be needed to trigger COW even
> when the kernel/hardware want to read the memory, to avoid problems
> where we get stale data if userspace triggers COW.
  Thanks for the pointer. That was an interesting read :).

> I think I better add a comment explaining this.
> 
> >   Finally (and here I may show my ignorance ;), I'd like to ask whether
> > there's any reason why ib_umem_get() checks for is_vm_hugetlb_page() and
> > not just whether a page is a huge page?
> 
> I'm not sure of the history here.  How would one check directly if a
> page is a huge page?
  PageHuge(page) should do it (see mm/hugetlb.c).

> get_user_pages() actually goes to some trouble to return all small pages,
> even when it has to split a single huge page into many entries in the
> page array.  (Which is actually a bit unfortunate for our use here)
  Does it? As far as I'm checking get_user_pages() and the fault path I
don't see where it would be happening...

								Honza  
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Infiniband use of get_user_pages()

[KOSAKI Motohiro]

>   when checking users of get_user_pages() (I'm doing some cleanups in that
> area to fix filesystem's issues with mmap_sem locking) I've noticed that
> infiniband drivers add number of pages obtained from get_user_pages() to
> mm->pinned_vm counter. Although this makes some sence, it doesn't match
> with any other user of get_user_pages() (e.g. direct IO) so has infiniband
> some special reason why it does so?

I'm also puzzled because mm->pinned_vm_counter is only used from /proc. Who
and how to use this accounting?


>   Also that seems to be the only real reason why mmap_sem has to be grabbed
> in exclusive mode, am I right?

I think so. get_user_pages() doesn't need write lock.


>   Another suspicious thing (at least in drivers/infiniband/core/umem.c:
> ib_umem_get()) is that arguments of get_user_pages() are like:
>                 ret = get_user_pages(current, current->mm, cur_base,
>                                      min_t(unsigned long, npages,
>                                            PAGE_SIZE / sizeof (struct page *)),
>                                      1, !umem->writable, page_list, vma_list);
> So we always have write argument set to 1 and force argument is set to
> !umem->writable. Is that really intentional? My naive guess would be that
> arguments should be switched... Although even in that case I fail to see
> why 'force' argument should be set. Can someone please explain?

If I understand correctly, IB and DirectIO have different set sequence.

DirectIO
 1. write to buf
 2. write(buf). i.e. get_user_pages_fast(write=0)

Infiniband
 1. reg_mr. i.e. get_user_pages(write=1)
 2. write to buf

I mean, if direct-io is passed zero page, it is user mistake. but user
process which uses infiniband set up mr before writing IO buffer,
IIUC.

In this case, I think user process which uses infiniband need to dereg
mr before fork(). but it is another story.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: Infiniband use of get_user_pages()

[Roland Dreier]

On Thu, Apr 25, 2013 at 6:21 AM, Jan Kara <jack@suse.cz> wrote:
>> get_user_pages() actually goes to some trouble to return all small pages,
>> even when it has to split a single huge page into many entries in the
>> page array.  (Which is actually a bit unfortunate for our use here)

>   Does it? As far as I'm checking get_user_pages() and the fault path I
> don't see where it would be happening...

I'm talking about where __get_user_pages() calls follow_hugetlb_page()
and loops to fill in many entries in the page array from one huge
page.

 - R.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>