| Message ID | 20191103211813.213227-3-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm/gup: track dma-pinned pages: FOLL_PIN, FOLL_LONGTERM | expand |
On Sun, Nov 03, 2019 at 01:17:57PM -0800, John Hubbard wrote: > There are four locations in gup.c that have a fair amount of code > duplication. This means that changing one requires making the same > changes in four places, not to mention reading the same code four > times, and wondering if there are subtle differences. > > Factor out the common code into static functions, thus reducing the > overall line count and the code's complexity. > > Also, take the opportunity to slightly improve the efficiency of the > error cases, by doing a mass subtraction of the refcount, surrounded > by get_page()/put_page(). > > Also, further simplify (slightly), by waiting until the the successful > end of each routine, to increment *nr. > > Cc: Ira Weiny <ira.weiny@intel.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> > Signed-off-by: John Hubbard <jhubbard@nvidia.com> Good cleanup. Reviewed-by: Jérôme Glisse <jglisse@redhat.com> > --- > mm/gup.c | 104 ++++++++++++++++++++++++------------------------------- > 1 file changed, 45 insertions(+), 59 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 85caf76b3012..199da99e8ffc 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1969,6 +1969,34 @@ static int __gup_device_huge_pud(pud_t pud, pud_t *pudp, unsigned long addr, > } > #endif > > +static int __record_subpages(struct page *page, unsigned long addr, > + unsigned long end, struct page **pages, int nr) > +{ > + int nr_recorded_pages = 0; > + > + do { > + pages[nr] = page; > + nr++; > + page++; > + nr_recorded_pages++; > + } while (addr += PAGE_SIZE, addr != end); > + return nr_recorded_pages; > +} > + > +static void put_compound_head(struct page *page, int refs) > +{ > + /* Do a get_page() first, in case refs == page->_refcount */ > + get_page(page); > + page_ref_sub(page, refs); > + put_page(page); > +} > + > +static void __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr) > +{ > + *nr += nr_recorded_pages; > + SetPageReferenced(head); > +} > + > #ifdef CONFIG_ARCH_HAS_HUGEPD > static unsigned long hugepte_addr_end(unsigned long addr, unsigned long end, > unsigned long sz) > @@ -1998,33 +2026,20 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, > /* hugepages are never "special" */ > VM_BUG_ON(!pfn_valid(pte_pfn(pte))); > > - refs = 0; > head = pte_page(pte); > - > page = head + ((addr & (sz-1)) >> PAGE_SHIFT); > - do { > - VM_BUG_ON(compound_head(page) != head); > - pages[*nr] = page; > - (*nr)++; > - page++; > - refs++; > - } while (addr += PAGE_SIZE, addr != end); > + refs = __record_subpages(page, addr, end, pages, *nr); > > head = try_get_compound_head(head, refs); > - if (!head) { > - *nr -= refs; > + if (!head) > return 0; > - } > > if (unlikely(pte_val(pte) != pte_val(*ptep))) { > - /* Could be optimized better */ > - *nr -= refs; > - while (refs--) > - put_page(head); > + put_compound_head(head, refs); > return 0; > } > > - SetPageReferenced(head); > + __huge_pt_done(head, refs, nr); > return 1; > } > > @@ -2071,29 +2086,19 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr, > pages, nr); > } > > - refs = 0; > page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT); > - do { > - pages[*nr] = page; > - (*nr)++; > - page++; > - refs++; > - } while (addr += PAGE_SIZE, addr != end); > + refs = __record_subpages(page, addr, end, pages, *nr); > > head = try_get_compound_head(pmd_page(orig), refs); > - if (!head) { > - *nr -= refs; > + if (!head) > return 0; > - } > > if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) { > - *nr -= refs; > - while (refs--) > - put_page(head); > + put_compound_head(head, refs); > return 0; > } > > - SetPageReferenced(head); > + __huge_pt_done(head, refs, nr); > return 1; > } > > @@ -2114,29 +2119,19 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr, > pages, nr); > } > > - refs = 0; > page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT); > - do { > - pages[*nr] = page; > - (*nr)++; > - page++; > - refs++; > - } while (addr += PAGE_SIZE, addr != end); > + refs = __record_subpages(page, addr, end, pages, *nr); > > head = try_get_compound_head(pud_page(orig), refs); > - if (!head) { > - *nr -= refs; > + if (!head) > return 0; > - } > > if (unlikely(pud_val(orig) != pud_val(*pudp))) { > - *nr -= refs; > - while (refs--) > - put_page(head); > + put_compound_head(head, refs); > return 0; > } > > - SetPageReferenced(head); > + __huge_pt_done(head, refs, nr); > return 1; > } > > @@ -2151,29 +2146,20 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr, > return 0; > > BUILD_BUG_ON(pgd_devmap(orig)); > - refs = 0; > + > page = pgd_page(orig) + ((addr & ~PGDIR_MASK) >> PAGE_SHIFT); > - do { > - pages[*nr] = page; > - (*nr)++; > - page++; > - refs++; > - } while (addr += PAGE_SIZE, addr != end); > + refs = __record_subpages(page, addr, end, pages, *nr); > > head = try_get_compound_head(pgd_page(orig), refs); > - if (!head) { > - *nr -= refs; > + if (!head) > return 0; > - } > > if (unlikely(pgd_val(orig) != pgd_val(*pgdp))) { > - *nr -= refs; > - while (refs--) > - put_page(head); > + put_compound_head(head, refs); > return 0; > } > > - SetPageReferenced(head); > + __huge_pt_done(head, refs, nr); > return 1; > } > > -- > 2.23.0 >
diff --git a/mm/gup.c b/mm/gup.c index 85caf76b3012..199da99e8ffc 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1969,6 +1969,34 @@ static int __gup_device_huge_pud(pud_t pud, pud_t *pudp, unsigned long addr, } #endif +static int __record_subpages(struct page *page, unsigned long addr, + unsigned long end, struct page **pages, int nr) +{ + int nr_recorded_pages = 0; + + do { + pages[nr] = page; + nr++; + page++; + nr_recorded_pages++; + } while (addr += PAGE_SIZE, addr != end); + return nr_recorded_pages; +} + +static void put_compound_head(struct page *page, int refs) +{ + /* Do a get_page() first, in case refs == page->_refcount */ + get_page(page); + page_ref_sub(page, refs); + put_page(page); +} + +static void __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr) +{ + *nr += nr_recorded_pages; + SetPageReferenced(head); +} + #ifdef CONFIG_ARCH_HAS_HUGEPD static unsigned long hugepte_addr_end(unsigned long addr, unsigned long end, unsigned long sz) @@ -1998,33 +2026,20 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, /* hugepages are never "special" */ VM_BUG_ON(!pfn_valid(pte_pfn(pte))); - refs = 0; head = pte_page(pte); - page = head + ((addr & (sz-1)) >> PAGE_SHIFT); - do { - VM_BUG_ON(compound_head(page) != head); - pages[*nr] = page; - (*nr)++; - page++; - refs++; - } while (addr += PAGE_SIZE, addr != end); + refs = __record_subpages(page, addr, end, pages, *nr); head = try_get_compound_head(head, refs); - if (!head) { - *nr -= refs; + if (!head) return 0; - } if (unlikely(pte_val(pte) != pte_val(*ptep))) { - /* Could be optimized better */ - *nr -= refs; - while (refs--) - put_page(head); + put_compound_head(head, refs); return 0; } - SetPageReferenced(head); + __huge_pt_done(head, refs, nr); return 1; } @@ -2071,29 +2086,19 @@ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr, pages, nr); } - refs = 0; page = pmd_page(orig) + ((addr & ~PMD_MASK) >> PAGE_SHIFT); - do { - pages[*nr] = page; - (*nr)++; - page++; - refs++; - } while (addr += PAGE_SIZE, addr != end); + refs = __record_subpages(page, addr, end, pages, *nr); head = try_get_compound_head(pmd_page(orig), refs); - if (!head) { - *nr -= refs; + if (!head) return 0; - } if (unlikely(pmd_val(orig) != pmd_val(*pmdp))) { - *nr -= refs; - while (refs--) - put_page(head); + put_compound_head(head, refs); return 0; } - SetPageReferenced(head); + __huge_pt_done(head, refs, nr); return 1; } @@ -2114,29 +2119,19 @@ static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr, pages, nr); } - refs = 0; page = pud_page(orig) + ((addr & ~PUD_MASK) >> PAGE_SHIFT); - do { - pages[*nr] = page; - (*nr)++; - page++; - refs++; - } while (addr += PAGE_SIZE, addr != end); + refs = __record_subpages(page, addr, end, pages, *nr); head = try_get_compound_head(pud_page(orig), refs); - if (!head) { - *nr -= refs; + if (!head) return 0; - } if (unlikely(pud_val(orig) != pud_val(*pudp))) { - *nr -= refs; - while (refs--) - put_page(head); + put_compound_head(head, refs); return 0; } - SetPageReferenced(head); + __huge_pt_done(head, refs, nr); return 1; } @@ -2151,29 +2146,20 @@ static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr, return 0; BUILD_BUG_ON(pgd_devmap(orig)); - refs = 0; + page = pgd_page(orig) + ((addr & ~PGDIR_MASK) >> PAGE_SHIFT); - do { - pages[*nr] = page; - (*nr)++; - page++; - refs++; - } while (addr += PAGE_SIZE, addr != end); + refs = __record_subpages(page, addr, end, pages, *nr); head = try_get_compound_head(pgd_page(orig), refs); - if (!head) { - *nr -= refs; + if (!head) return 0; - } if (unlikely(pgd_val(orig) != pgd_val(*pgdp))) { - *nr -= refs; - while (refs--) - put_page(head); + put_compound_head(head, refs); return 0; } - SetPageReferenced(head); + __huge_pt_done(head, refs, nr); return 1; }
There are four locations in gup.c that have a fair amount of code duplication. This means that changing one requires making the same changes in four places, not to mention reading the same code four times, and wondering if there are subtle differences. Factor out the common code into static functions, thus reducing the overall line count and the code's complexity. Also, take the opportunity to slightly improve the efficiency of the error cases, by doing a mass subtraction of the refcount, surrounded by get_page()/put_page(). Also, further simplify (slightly), by waiting until the the successful end of each routine, to increment *nr. Cc: Ira Weiny <ira.weiny@intel.com> Cc: Christoph Hellwig <hch@lst.de> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- mm/gup.c | 104 ++++++++++++++++++++++++------------------------------- 1 file changed, 45 insertions(+), 59 deletions(-)