| Message ID | 1563724685-6540-4-git-send-email-linux.bhar@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sgi-gru: get_user_page changes | expand |
On 7/21/19 8:58 AM, Bharath Vedartham wrote: > *pte_lookup functions get the physical address for a given virtual > address by getting a physical page using gup and use page_to_phys to get > the physical address. > > Currently, atomic_pte_lookup manually walks the page tables. If this > function fails to get a physical page, it will fall back too > non_atomic_pte_lookup to get a physical page which uses the slow gup > path to get the physical page. > > Instead of manually walking the page tables use __get_user_pages_fast > which does the same thing and it does not fall back to the slow gup > path. > > This is largely inspired from kvm code. kvm uses __get_user_pages_fast > in hva_to_pfn_fast function which can run in an atomic context. > > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: John Hubbard <jhubbard@nvidia.com> > Cc: Jérôme Glisse <jglisse@redhat.com> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Dimitri Sivanich <sivanich@sgi.com> > Cc: Arnd Bergmann <arnd@arndb.de> > Cc: linux-kernel@vger.kernel.org > Cc: linux-mm@kvack.org > Signed-off-by: Bharath Vedartham <linux.bhar@gmail.com> > --- > drivers/misc/sgi-gru/grufault.c | 39 +++++---------------------------------- > 1 file changed, 5 insertions(+), 34 deletions(-) > > diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c > index 75108d2..121c9a4 100644 > --- a/drivers/misc/sgi-gru/grufault.c > +++ b/drivers/misc/sgi-gru/grufault.c > @@ -202,46 +202,17 @@ static int non_atomic_pte_lookup(struct vm_area_struct *vma, > static int atomic_pte_lookup(struct vm_area_struct *vma, unsigned long vaddr, > int write, unsigned long *paddr, int *pageshift) > { > - pgd_t *pgdp; > - p4d_t *p4dp; > - pud_t *pudp; > - pmd_t *pmdp; > - pte_t pte; > - > - pgdp = pgd_offset(vma->vm_mm, vaddr); > - if (unlikely(pgd_none(*pgdp))) > - goto err; > - > - p4dp = p4d_offset(pgdp, vaddr); > - if (unlikely(p4d_none(*p4dp))) > - goto err; > - > - pudp = pud_offset(p4dp, vaddr); > - if (unlikely(pud_none(*pudp))) > - goto err; > + struct page *page; > > - pmdp = pmd_offset(pudp, vaddr); > - if (unlikely(pmd_none(*pmdp))) > - goto err; > -#ifdef CONFIG_X86_64 > - if (unlikely(pmd_large(*pmdp))) > - pte = *(pte_t *) pmdp; > - else > -#endif > - pte = *pte_offset_kernel(pmdp, vaddr); > + *pageshift = is_vm_hugetlb_page(vma) ? HPAGE_SHIFT : PAGE_SHIFT; > > - if (unlikely(!pte_present(pte) || > - (write && (!pte_write(pte) || !pte_dirty(pte))))) > + if (!__get_user_pages_fast(vaddr, 1, write, &page)) > return 1; Let's please use numeric, not boolean comparison, for the return value of gup. Also, optional: as long as you're there, atomic_pte_lookup() ought to either return a bool (true == success) or an errno, rather than a numeric zero or one. Other than that, this looks like a good cleanup, I wonder how many open-coded gup implementations are floating around like this. thanks,
On Sun, Jul 21, 2019 at 07:32:36PM -0700, John Hubbard wrote: > On 7/21/19 8:58 AM, Bharath Vedartham wrote: > > *pte_lookup functions get the physical address for a given virtual > > address by getting a physical page using gup and use page_to_phys to get > > the physical address. > > > > Currently, atomic_pte_lookup manually walks the page tables. If this > > function fails to get a physical page, it will fall back too > > non_atomic_pte_lookup to get a physical page which uses the slow gup > > path to get the physical page. > > > > Instead of manually walking the page tables use __get_user_pages_fast > > which does the same thing and it does not fall back to the slow gup > > path. > > > > This is largely inspired from kvm code. kvm uses __get_user_pages_fast > > in hva_to_pfn_fast function which can run in an atomic context. > > > > Cc: Ira Weiny <ira.weiny@intel.com> > > Cc: John Hubbard <jhubbard@nvidia.com> > > Cc: Jérôme Glisse <jglisse@redhat.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Dimitri Sivanich <sivanich@sgi.com> > > Cc: Arnd Bergmann <arnd@arndb.de> > > Cc: linux-kernel@vger.kernel.org > > Cc: linux-mm@kvack.org > > Signed-off-by: Bharath Vedartham <linux.bhar@gmail.com> > > --- > > drivers/misc/sgi-gru/grufault.c | 39 +++++---------------------------------- > > 1 file changed, 5 insertions(+), 34 deletions(-) > > > > diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c > > index 75108d2..121c9a4 100644 > > --- a/drivers/misc/sgi-gru/grufault.c > > +++ b/drivers/misc/sgi-gru/grufault.c > > @@ -202,46 +202,17 @@ static int non_atomic_pte_lookup(struct vm_area_struct *vma, > > static int atomic_pte_lookup(struct vm_area_struct *vma, unsigned long vaddr, > > int write, unsigned long *paddr, int *pageshift) > > { > > - pgd_t *pgdp; > > - p4d_t *p4dp; > > - pud_t *pudp; > > - pmd_t *pmdp; > > - pte_t pte; > > - > > - pgdp = pgd_offset(vma->vm_mm, vaddr); > > - if (unlikely(pgd_none(*pgdp))) > > - goto err; > > - > > - p4dp = p4d_offset(pgdp, vaddr); > > - if (unlikely(p4d_none(*p4dp))) > > - goto err; > > - > > - pudp = pud_offset(p4dp, vaddr); > > - if (unlikely(pud_none(*pudp))) > > - goto err; > > + struct page *page; > > > > - pmdp = pmd_offset(pudp, vaddr); > > - if (unlikely(pmd_none(*pmdp))) > > - goto err; > > -#ifdef CONFIG_X86_64 > > - if (unlikely(pmd_large(*pmdp))) > > - pte = *(pte_t *) pmdp; > > - else > > -#endif > > - pte = *pte_offset_kernel(pmdp, vaddr); > > + *pageshift = is_vm_hugetlb_page(vma) ? HPAGE_SHIFT : PAGE_SHIFT; > > > > - if (unlikely(!pte_present(pte) || > > - (write && (!pte_write(pte) || !pte_dirty(pte))))) > > + if (!__get_user_pages_fast(vaddr, 1, write, &page)) > > return 1; > > Let's please use numeric, not boolean comparison, for the return value of > gup. Alright then! I ll resubmit it! > Also, optional: as long as you're there, atomic_pte_lookup() ought to > either return a bool (true == success) or an errno, rather than a > numeric zero or one. That makes sense. But the code which uses atomic_pte_lookup uses the return value of 1 for success and failure value of 0 in gru_vtop. That's why I did not mess with the return values in this code. It would require some change in the driver functionality which I am not ready to do :( > Other than that, this looks like a good cleanup, I wonder how many > open-coded gup implementations are floating around like this. I ll be on the lookout! > thanks, > -- > John Hubbard > NVIDIA > > > > > - *paddr = pte_pfn(pte) << PAGE_SHIFT; > > - > > - *pageshift = is_vm_hugetlb_page(vma) ? HPAGE_SHIFT : PAGE_SHIFT; > > + *paddr = page_to_phys(page); > > + put_user_page(page); > > > > return 0; > > - > > -err: > > - return 1; > > } > > > > static int gru_vtop(struct gru_thread_state *gts, unsigned long vaddr, > >
On 7/22/19 10:53 AM, Bharath Vedartham wrote: > On Sun, Jul 21, 2019 at 07:32:36PM -0700, John Hubbard wrote: >> On 7/21/19 8:58 AM, Bharath Vedartham wrote: ... >> Also, optional: as long as you're there, atomic_pte_lookup() ought to >> either return a bool (true == success) or an errno, rather than a >> numeric zero or one. > That makes sense. But the code which uses atomic_pte_lookup uses the > return value of 1 for success and failure value of 0 in gru_vtop. That's > why I did not mess with the return values in this code. It would require > some change in the driver functionality which I am not ready to do :( It's a static function with only one caller. You could just merge in something like this, on top of what you have: diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c index 121c9a4ccb94..2f768fc06432 100644 --- a/drivers/misc/sgi-gru/grufault.c +++ b/drivers/misc/sgi-gru/grufault.c @@ -189,10 +189,11 @@ static int non_atomic_pte_lookup(struct vm_area_struct *vma, return 0; } -/* - * atomic_pte_lookup +/** + * atomic_pte_lookup() - Convert a user virtual address to a physical address + * @Return: true for success, false for failure. Failure means that the page + * could not be pinned via gup fast. * - * Convert a user virtual address to a physical address * Only supports Intel large pages (2MB only) on x86_64. * ZZZ - hugepage support is incomplete * @@ -207,12 +208,12 @@ static int atomic_pte_lookup(struct vm_area_struct *vma, unsigned long vaddr, *pageshift = is_vm_hugetlb_page(vma) ? HPAGE_SHIFT : PAGE_SHIFT; if (!__get_user_pages_fast(vaddr, 1, write, &page)) - return 1; + return false; *paddr = page_to_phys(page); put_user_page(page); - return 0; + return true; } static int gru_vtop(struct gru_thread_state *gts, unsigned long vaddr, @@ -221,7 +222,8 @@ static int gru_vtop(struct gru_thread_state *gts, unsigned long vaddr, struct mm_struct *mm = gts->ts_mm; struct vm_area_struct *vma; unsigned long paddr; - int ret, ps; + int ps; + bool success; vma = find_vma(mm, vaddr); if (!vma) @@ -232,8 +234,8 @@ static int gru_vtop(struct gru_thread_state *gts, unsigned long vaddr, * context. */ rmb(); /* Must/check ms_range_active before loading PTEs */ - ret = atomic_pte_lookup(vma, vaddr, write, &paddr, &ps); - if (ret) { + success = atomic_pte_lookup(vma, vaddr, write, &paddr, &ps); + if (!success) { if (atomic) goto upm; if (non_atomic_pte_lookup(vma, vaddr, write, &paddr, &ps)) thanks,
diff --git a/drivers/misc/sgi-gru/grufault.c b/drivers/misc/sgi-gru/grufault.c index 75108d2..121c9a4 100644 --- a/drivers/misc/sgi-gru/grufault.c +++ b/drivers/misc/sgi-gru/grufault.c @@ -202,46 +202,17 @@ static int non_atomic_pte_lookup(struct vm_area_struct *vma, static int atomic_pte_lookup(struct vm_area_struct *vma, unsigned long vaddr, int write, unsigned long *paddr, int *pageshift) { - pgd_t *pgdp; - p4d_t *p4dp; - pud_t *pudp; - pmd_t *pmdp; - pte_t pte; - - pgdp = pgd_offset(vma->vm_mm, vaddr); - if (unlikely(pgd_none(*pgdp))) - goto err; - - p4dp = p4d_offset(pgdp, vaddr); - if (unlikely(p4d_none(*p4dp))) - goto err; - - pudp = pud_offset(p4dp, vaddr); - if (unlikely(pud_none(*pudp))) - goto err; + struct page *page; - pmdp = pmd_offset(pudp, vaddr); - if (unlikely(pmd_none(*pmdp))) - goto err; -#ifdef CONFIG_X86_64 - if (unlikely(pmd_large(*pmdp))) - pte = *(pte_t *) pmdp; - else -#endif - pte = *pte_offset_kernel(pmdp, vaddr); + *pageshift = is_vm_hugetlb_page(vma) ? HPAGE_SHIFT : PAGE_SHIFT; - if (unlikely(!pte_present(pte) || - (write && (!pte_write(pte) || !pte_dirty(pte))))) + if (!__get_user_pages_fast(vaddr, 1, write, &page)) return 1; - *paddr = pte_pfn(pte) << PAGE_SHIFT; - - *pageshift = is_vm_hugetlb_page(vma) ? HPAGE_SHIFT : PAGE_SHIFT; + *paddr = page_to_phys(page); + put_user_page(page); return 0; - -err: - return 1; } static int gru_vtop(struct gru_thread_state *gts, unsigned long vaddr,
*pte_lookup functions get the physical address for a given virtual address by getting a physical page using gup and use page_to_phys to get the physical address. Currently, atomic_pte_lookup manually walks the page tables. If this function fails to get a physical page, it will fall back too non_atomic_pte_lookup to get a physical page which uses the slow gup path to get the physical page. Instead of manually walking the page tables use __get_user_pages_fast which does the same thing and it does not fall back to the slow gup path. This is largely inspired from kvm code. kvm uses __get_user_pages_fast in hva_to_pfn_fast function which can run in an atomic context. Cc: Ira Weiny <ira.weiny@intel.com> Cc: John Hubbard <jhubbard@nvidia.com> Cc: Jérôme Glisse <jglisse@redhat.com> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Dimitri Sivanich <sivanich@sgi.com> Cc: Arnd Bergmann <arnd@arndb.de> Cc: linux-kernel@vger.kernel.org Cc: linux-mm@kvack.org Signed-off-by: Bharath Vedartham <linux.bhar@gmail.com> --- drivers/misc/sgi-gru/grufault.c | 39 +++++---------------------------------- 1 file changed, 5 insertions(+), 34 deletions(-)