futex: replace get_user_pages() with get_user_pages_fast()
RFC PATCH 1/1 futex: replace get_user_pages() with get_user_pages_fast()
Hello,
Is there any reason to take mmap_sem explicitly here?
Should we change gup_fast() to allow NULL argument (ie for avoid get_page)?
============================
down_read(&mm->mmap_sem);
get_user_pages(current, current->mm)
up_read(&mm->mmap_sem);
and
get_user_pages_fast()
make an equivalent result, And latter would be better when mamp_sem highly contended case, because it can avoid to take mmap_sem if the target page doesn't need a page fault.
--- kernel/futex.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)
kernel/futex.c +3 -5
@@ -351,13 +351,11 @@ static inline void put_futex_key(union futex_key *key)*/ static int fault_in_user_writeable (u32 __user *uaddr) { -struct mm_struct *mm = current->mm;int ret; +struct page *page; -down_read (&mm->mmap_sem); - ret =get_user_pages (current, mm, (unsigned long )uaddr, -1 ,1 ,0 ,NULL ,NULL ); -up_read (&mm->mmap_sem); + ret =get_user_pages_fast ((unsigned long )uaddr,1 ,1 , &page); +put_page (page);return ret <0 ? ret :0 ; }
Signature · 1 line
1.7.3.1
Quoted 10 lines · KOSAKI Motohiro
Hello,
Is there any reason to take
mmap_semexplicitly here?
Should we changegup_fast()to allow NULL argument (ie for avoidget_page)?============================
down_read(&mm->mmap_sem);
get_user_pages(current, current->mm)
up_read(&mm->mmap_sem);
Quoted 7 lines
and
get_user_pages_fast()make an equivalent result, And latter would be better when
mamp_semhighly contended case, because it can avoid to takemmap_semif the target page doesn't need a page fault.
I can't speak authoritatively here, but it seems to me that get_user_pages_fast falls back to get_user_pages with mmap_sem anyway, so this seems like a reasonable optimization for the best case, with a minor overhead for the slow case.
Peter, am I missing something? Is there a reason you left this as is during your fast gup futex update?
Quoted 27 lines
Signed-off-by KOSAKI Motohiro <[email protected]>--- kernel/futex.c | 8 +++----- 1 files changed, 3 insertions(+), 5 deletions(-)kernel/futex.c +3
-5@@ -351,13 +351,11 @@ static inline void put_futex_key(union futex_key *key)*/ static int fault_in_user_writeable (u32 __user *uaddr) { -struct mm_struct *mm = current->mm;int ret; +struct page *page; -down_read (&mm->mmap_sem); - ret =get_user_pages (current, mm, (unsigned long )uaddr, -1 ,1 ,0 ,NULL ,NULL ); -up_read (&mm->mmap_sem); + ret =get_user_pages_fast ((unsigned long )uaddr,1 ,1 , &page); +put_page (page);return ret <0 ? ret :0 ; }
Signature · 3 lines
Darren Hart Intel Open Source Technology Center Yocto Project - Linux Kernel
Quoted 21 lines · Darren Hart
Quoted 10 lines · KOSAKI Motohiro
Hello,
Is there any reason to take
mmap_semexplicitly here?
Should we changegup_fast()to allow NULL argument (ie for avoidget_page)?============================
down_read(&mm->mmap_sem);
get_user_pages(current, current->mm)
up_read(&mm->mmap_sem);Quoted 7 lines
and
get_user_pages_fast()make an equivalent result, And latter would be better when
mamp_semhighly contended case, because it can avoid to takemmap_semif the target page doesn't need a page fault.
Quoted 6 lines
I can't speak authoritatively here, but it seems to me that
get_user_pages_fastfalls back toget_user_pageswithmmap_semanyway, so this seems like a reasonable optimization for the best case, with a minor overhead for the slow case.Peter, am I missing something?
Right, its much cheaper on the fast-path where the page is found present and writable, if we have to take the fault, the extra cost in the slow path is neglectable.
Quoted 2 lines
Is there a reason you left this as is
during your fast gup futex update?
I probably completely missed this gup() user.
The proposed change looks good to me.