mm: gup: cleanup gup_fast call chain
Subsystems (2)
cover mm: gup: cleanup gup_fast call chain
These two patches implement the refactor in gup_fast call chain David Hildenbrand mentioned in
https://lore.kernel.org/all/[email protected]/T/#u
Signature · 1 line
Aristeu
PATCH 1/2 mm: gup: move pmd_protnone() into gup_fast_pmd_leaf()
Make pmd handling match pud handling by calling pmd_protnone() inside gup_fast_pmd_leaf().
--- mm/gup.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
mm/gup.c +4 -4
@@ -2929,6 +2929,10 @@ static int gup_fast_pmd_leaf(pmd_t orig,struct folio *folio;int refs; +/* See gup_fast_pte_range() */ +if (pmd_protnone (orig)) +return 0 ; +if (!pmd_access_permitted (orig, flags &FOLL_WRITE ))return 0 ;
@@ -3024,10 +3028,6 @@ static int gup_fast_pmd_range(pud_t *pudreturn 0 ;if (unlikely (pmd_leaf (pmd))) { -/* See gup_fast_pte_range() */ -if (pmd_protnone (pmd)) -return 0 ; -if (!gup_fast_pmd_leaf (pmd, pmdp, addr, next, flags, pages, nr))return 0 ;
Re: [PATCH 1/2] mm: gup: move pmd_protnone() into gup_fast_pmd_leaf()
Quoted 11 lines · Aristeu Rozanski
Make pmd handling match pud handling by calling
pmd_protnone()insidegup_fast_pmd_leaf().Suggested-by David Hildenbrand <[email protected]>Cc Andrew Morton <[email protected]>Cc David Hildenbrand <[email protected]>Cc Jason Gunthorpe <[email protected]>Cc John Hubbard <[email protected]>Cc Peter Xu <[email protected]>Signed-off-by Aristeu Rozanski <[email protected]>
Signature · 3 lines
Cheers, David
PATCH 2/2 mm: gup: cleanup the gup_fast_*() call chain
In reply to Aristeu RozanskiRefactor gup_fast functions so each step of the way returns the number of pages pinned. Because the previous step of the chain knows what the number it should be, less indicates an error. This way there's no need to pass *nr along.
--- mm/gup.c | 179 +++++++++++++++++++++++++++++++++------------------------------ 1 file changed, 94 insertions(+), 85 deletions(-)
mm/gup.c +94 -85
@@ -2826,11 +2826,11 @@ static bool gup_fast_folio_allowed(struc* also check pmd here to make sure pmd doesn't change (corresponds to * pmdp_collapse_flush() in the THP collapse code path). */ -static int gup_fast_pte_range (pmd_t pmd,pmd_t *pmdp,unsigned long addr, -unsigned long end,unsigned int flags,struct page **pages, -int *nr) +static unsigned long gup_fast_pte_range (pmd_t pmd,pmd_t *pmdp, +unsigned long addr,unsigned long end, +unsigned int flags,struct page **pages) { -int ret =0 ; +unsigned long nr_pages =0 ;pte_t *ptep, *ptem; ptem = ptep =pte_offset_map (&pmd, addr);
@@ -2892,15 +2892,13 @@ static int gup_fast_pte_range(pmd_t pmd, goto pte_unmap; }folio_set_referenced (folio); - pages[*nr] = page; - (*nr)++; + pages[nr_pages] = page; + nr_pages++; }while (ptep++, addr +=PAGE_SIZE , addr != end); - ret =1 ; - pte_unmap:pte_unmap (ptem); -return ret; +return nr_pages; }#else
@@ -2913,21 +2911,21 @@ pte_unmap:* get_user_pages_fast_only implementation that can pin pages. Thus it's still * useful to have gup_fast_pmd_leaf even if we can't operate on ptes. */ -static int gup_fast_pte_range (pmd_t pmd,pmd_t *pmdp,unsigned long addr, -unsigned long end,unsigned int flags,struct page **pages, -int *nr) +static unsigned long gup_fast_pte_range (pmd_t pmd,pmd_t *pmdp, +unsigned long addr,unsigned long end, +unsigned int flags,struct page **pages) {return 0 ; }#endif /* CONFIG_ARCH_HAS_PTE_SPECIAL */ -static int gup_fast_pmd_leaf (pmd_t orig,pmd_t *pmdp,unsigned long addr, -unsigned long end,unsigned int flags,struct page **pages, -int *nr) +static unsigned long gup_fast_pmd_leaf (pmd_t orig,pmd_t *pmdp, +unsigned long addr,unsigned long end, +unsigned int flags,struct page **pages) {struct page *page;struct folio *folio; -int refs; +unsigned long nr_pages, i;/* See gup_fast_pte_range() */ if (pmd_protnone (orig))
@@ -2939,42 +2937,40 @@ static int gup_fast_pmd_leaf(pmd_t orig,if (pmd_special (orig))return 0 ; - refs = (end - addr) >>PAGE_SHIFT ; + nr_pages = (end - addr) >>PAGE_SHIFT ; page =pmd_page (orig) + ((addr & ~PMD_MASK ) >>PAGE_SHIFT ); - folio =try_grab_folio_fast (page, refs, flags); + folio =try_grab_folio_fast (page, nr_pages, flags);if (!folio)return 0 ;if (unlikely (pmd_val (orig) !=pmd_val (*pmdp))) { -gup_put_folio (folio, refs, flags); +gup_put_folio (folio, nr_pages, flags);return 0 ; }if (!gup_fast_folio_allowed (folio, flags)) { -gup_put_folio (folio, refs, flags); +gup_put_folio (folio, nr_pages, flags);return 0 ; }if (!pmd_write (orig) &&gup_must_unshare (NULL , flags, &folio->page)) { -gup_put_folio (folio, refs, flags); +gup_put_folio (folio, nr_pages, flags);return 0 ; } - pages += *nr; - *nr += refs; -for (; refs; refs--) +for (i =0 ; i < nr_pages; i++) *(pages++) = page++;folio_set_referenced (folio); -return 1 ; +return nr_pages; } -static int gup_fast_pud_leaf (pud_t orig,pud_t *pudp,unsigned long addr, -unsigned long end,unsigned int flags,struct page **pages, -int *nr) +static unsigned long gup_fast_pud_leaf (pud_t orig,pud_t *pudp, +unsigned long addr,unsigned long end, +unsigned int flags,struct page **pages) {struct page *page;struct folio *folio; -int refs; +unsigned long nr_pages =0 , i;if (!pud_access_permitted (orig, flags &FOLL_WRITE ))return 0 ;
@@ -2982,41 +2978,39 @@ static int gup_fast_pud_leaf(pud_t orig,if (pud_special (orig))return 0 ; - refs = (end - addr) >>PAGE_SHIFT ; + nr_pages = (end - addr) >>PAGE_SHIFT ; page =pud_page (orig) + ((addr & ~PUD_MASK ) >>PAGE_SHIFT ); - folio =try_grab_folio_fast (page, refs, flags); + folio =try_grab_folio_fast (page, nr_pages, flags);if (!folio)return 0 ;if (unlikely (pud_val (orig) !=pud_val (*pudp))) { -gup_put_folio (folio, refs, flags); +gup_put_folio (folio, nr_pages, flags);return 0 ; }if (!gup_fast_folio_allowed (folio, flags)) { -gup_put_folio (folio, refs, flags); +gup_put_folio (folio, nr_pages, flags);return 0 ; }if (!pud_write (orig) &&gup_must_unshare (NULL , flags, &folio->page)) { -gup_put_folio (folio, refs, flags); +gup_put_folio (folio, nr_pages, flags);return 0 ; } - pages += *nr; - *nr += refs; -for (; refs; refs--) +for (i =0 ; i < nr_pages; i++) *(pages++) = page++;folio_set_referenced (folio); -return 1 ; +return nr_pages; } -static int gup_fast_pmd_range (pud_t *pudp,pud_t pud,unsigned long addr, -unsigned long end,unsigned int flags,struct page **pages, -int *nr) +static unsigned long gup_fast_pmd_range (pud_t *pudp,pud_t pud, +unsigned long addr,unsigned long end, +unsigned int flags,struct page **pages) { -unsigned long next; +unsigned long next, nr_pages =0 , chunk_nr_pages;pmd_t *pmdp; pmdp =pmd_offset_lockless (pudp, pud, addr);
@@ -3025,26 +3019,30 @@ static int gup_fast_pmd_range(pud_t *pud next =pmd_addr_end (addr, end);if (!pmd_present (pmd)) -return 0 ; +break ;if (unlikely (pmd_leaf (pmd))) { -if (!gup_fast_pmd_leaf (pmd, pmdp, addr, next, flags, - pages, nr)) -return 0 ; - - }else if (!gup_fast_pte_range (pmd, pmdp, addr, next, flags, - pages, nr)) -return 0 ; + chunk_nr_pages =gup_fast_pmd_leaf (pmd, pmdp, addr, + next, flags, + &pages[nr_pages]); + + }else + chunk_nr_pages =gup_fast_pte_range (pmd, pmdp, addr, + next, flags, + &pages[nr_pages]); + nr_pages += chunk_nr_pages; +if (chunk_nr_pages != (next - addr) >>PAGE_SHIFT ) +break ; }while (pmdp++, addr = next, addr != end); -return 1 ; +return nr_pages; } -static int gup_fast_pud_range (p4d_t *p4dp,p4d_t p4d,unsigned long addr, -unsigned long end,unsigned int flags,struct page **pages, -int *nr) +static unsigned long gup_fast_pud_range (p4d_t *p4dp,p4d_t p4d, +unsigned long addr,unsigned long end, +unsigned int flags,struct page **pages) { -unsigned long next; +unsigned long next, nr_pages =0 , chunk_nr_pages;pud_t *pudp; pudp =pud_offset_lockless (p4dp, p4d, addr);
@@ -3053,24 +3051,27 @@ static int gup_fast_pud_range(p4d_t *p4d next =pud_addr_end (addr, end);if (unlikely (!pud_present (pud))) -return 0 ; -if (unlikely (pud_leaf (pud))) { -if (!gup_fast_pud_leaf (pud, pudp, addr, next, flags, - pages, nr)) -return 0 ; - }else if (!gup_fast_pmd_range (pudp, pud, addr, next, flags, - pages, nr)) -return 0 ; +break ; +if (unlikely (pud_leaf (pud))) + chunk_nr_pages =gup_fast_pud_leaf (pud, pudp, addr, + next, flags, + &pages[nr_pages]); +else + chunk_nr_pages =gup_fast_pmd_range (pudp, pud, addr, + next, flags, + &pages[nr_pages]); + nr_pages += chunk_nr_pages; +if (chunk_nr_pages != (next - addr) >>PAGE_SHIFT ) +break ; }while (pudp++, addr = next, addr != end); -return 1 ; +return nr_pages; } -static int gup_fast_p4d_range (pgd_t *pgdp,pgd_t pgd,unsigned long addr, -unsigned long end,unsigned int flags,struct page **pages, -int *nr) +static unsigned long gup_fast_p4d_range (pgd_t *pgdp,pgd_t pgd,unsigned long addr, +unsigned long end,unsigned int flags,struct page **pages) { -unsigned long next; +unsigned long next, nr_pages =0 , chunk_nr_pages;p4d_t *p4dp; p4dp =p4d_offset_lockless (pgdp, pgd, addr);
@@ -3079,20 +3080,23 @@ static int gup_fast_p4d_range(pgd_t *pgd next =p4d_addr_end (addr, end);if (!p4d_present (p4d)) -return 0 ; +break ;BUILD_BUG_ON (p4d_leaf (p4d)); -if (!gup_fast_pud_range (p4dp, p4d, addr, next, flags, - pages, nr)) -return 0 ; + chunk_nr_pages =gup_fast_pud_range (p4dp, p4d, addr, next, + flags, &pages[nr_pages]); + nr_pages += chunk_nr_pages; +if (chunk_nr_pages != (next - addr) >>PAGE_SHIFT ) +break ; }while (p4dp++, addr = next, addr != end); -return 1 ; +return nr_pages; } -static void gup_fast_pgd_range (unsigned long addr,unsigned long end, -unsigned int flags,struct page **pages,int *nr) +static unsigned long gup_fast_pgd_range (unsigned long addr, +unsigned long end,unsigned int flags, +struct page **pages) { -unsigned long next; +unsigned long next, nr_pages =0 , chunk_nr_pages;pgd_t *pgdp; pgdp =pgd_offset (current->mm, addr);
@@ -3101,17 +3105,23 @@ static void gup_fast_pgd_range(unsigned next =pgd_addr_end (addr, end);if (pgd_none (pgd)) -return ; +break ;BUILD_BUG_ON (pgd_leaf (pgd)); -if (!gup_fast_p4d_range (pgdp, pgd, addr, next, flags, - pages, nr)) - return; + chunk_nr_pages =gup_fast_p4d_range (pgdp, pgd, addr, next, + flags, &pages[nr_pages]); + nr_pages += chunk_nr_pages; +if (chunk_nr_pages != (next - addr) >>PAGE_SHIFT ) + break; }while (pgdp++, addr = next, addr != end); + +return nr_pages; }#else -static inline void gup_fast_pgd_range (unsigned long addr,unsigned long end, -unsigned int flags,struct page **pages,int *nr) +static inline unsigned long gup_fast_pgd_range (unsigned long addr, +unsigned long end,unsigned int flags, +struct page **pages) { +return 0 ; }#endif /* CONFIG_HAVE_GUP_FAST */
@@ -3129,8 +3139,7 @@ static bool gup_fast_permitted(unsignedstatic unsigned long gup_fast (unsigned long start,unsigned long end,unsigned int gup_flags,struct page **pages) { -unsigned long flags; -int nr_pinned =0 ; +unsigned long flags, nr_pinned =0 ;unsigned seq; if (!IS_ENABLED(CONFIG_HAVE_GUP_FAST) ||
@@ -3154,7 +3163,7 @@ static unsigned long gup_fast(unsigned l* that come from callers of tlb_remove_table_sync_one(). */ local_irq_save (flags); -gup_fast_pgd_range (start, end, gup_flags, pages, &nr_pinned); + nr_pinned =gup_fast_pgd_range (start, end, gup_flags, pages);local_irq_restore (flags);/*
Re: [PATCH 2/2] mm: gup: cleanup the gup_fast_*() call chain
Quoted 13 lines · Aristeu Rozanski
Refactor
gup_fastfunctions so each step of the way returns the number of pages pinned. Because the previous step of the chain knows what the number it should be, less indicates an error. This way there's no need to pass *nr along.Suggested-by David Hildenbrand <[email protected]>Cc Andrew Morton <[email protected]>Cc David Hildenbrand <[email protected]>Cc Jason Gunthorpe <[email protected]>Cc John Hubbard <[email protected]>Cc Peter Xu <[email protected]>Signed-off-by Aristeu Rozanski <[email protected]>
[...]
mm/gup.c static int gup_fast_pmd_leaf(pmd_t orig, · line 2973
Quoted 11 lines
mm/gup.c +4
-4-static int gup_fast_pud_leaf (pud_t orig,pud_t *pudp,unsigned long addr, -unsigned long end,unsigned int flags,struct page **pages, -int *nr) +static unsigned long gup_fast_pud_leaf (pud_t orig,pud_t *pudp, +unsigned long addr,unsigned long end, +unsigned int flags,struct page **pages) {struct page *page;struct folio *folio; -int refs; +unsigned long nr_pages =0 , i;
I don't think there is a need to initialize nr_pages to 0 here.
[...]
mm/gup.c static bool gup_fast_permitted(unsigned · line 3142
Quoted 8 lines
mm/gup.c +1
-2@@ -3129,8 +3139,7 @@ static bool gup_fast_permitted(unsignedstatic unsigned long gup_fast (unsigned long start,unsigned long end,unsigned int gup_flags,struct page **pages) { -unsigned long flags; -int nr_pinned =0 ; +unsigned long flags, nr_pinned =0 ;
I don't think we have to initialize nr_pinned to 0 anymore.
mm/gup.c static unsigned long gup_fast(unsigned l · line 3169
Quoted 13 lines
mm/gup.c +1
-1unsigned seq; if (!IS_ENABLED(CONFIG_HAVE_GUP_FAST) ||@@ -3154,7 +3163,7 @@ static unsigned long gup_fast(unsigned l* that come from callers of tlb_remove_table_sync_one(). */ local_irq_save (flags); -gup_fast_pgd_range (start, end, gup_flags, pages, &nr_pinned); + nr_pinned =gup_fast_pgd_range (start, end, gup_flags, pages);local_irq_restore (flags);/*
In general, LGTM, and our gup tests seem to pass.
Signature · 3 lines
Cheers, David
David Hildenbrand (Arm)
maintainerQuoted 5 lines · Aristeu Rozanski
These two patches implement the refactor in
gup_fastcall chain David Hildenbrand mentioned inhttps://lore.kernel.org/all/[email protected]/T/#u
Quoted 9 lines
Suggested-by David Hildenbrand <[email protected]>Cc Andrew Morton <[email protected]>Cc David Hildenbrand <[email protected]>Cc Jason Gunthorpe <[email protected]>Cc John Hubbard <[email protected]>Cc Peter Xu <[email protected]>Signed-off-by Aristeu Rozanski <[email protected]>
Aristeu, I think there were only minor things on the second patch. Can you fix these up and respin?
We might only queue this after the next merge window, but I don't expect a lot of conflicts.
Signature · 3 lines
Cheers, David
Hi David,
Quoted 5 lines · David Hildenbrand (Arm)
Aristeu, I think there were only minor things on the second patch. Can you fix these up and respin?
We might only queue this after the next merge window, but I don't expect a lot of conflicts.
will do
Signature · 1 line
Aristeu