| Message ID | 20191030224930.3990755-3-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm/gup: track dma-pinned pages: FOLL_PIN, FOLL_LONGTERM | expand |
On Wed, Oct 30, 2019 at 03:49:13PM -0700, 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. Overall it seems like a pretty good clean up. It did take a bit of review but I _think_ it is correct. A couple of comments below. > > 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 | 113 ++++++++++++++++++++++--------------------------------- > 1 file changed, 46 insertions(+), 67 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 85caf76b3012..8fb0d9cdfaf5 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -1969,6 +1969,35 @@ 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 __remove_refs_from_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); > +} I wonder if this is better implemented as "put_compound_head()"? To match the try_get_compound_head() call below? > + > +static int __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr) > +{ > + *nr += nr_recorded_pages; > + SetPageReferenced(head); > + return 1; When will this return anything but 1? Ira > +} > + > #ifdef CONFIG_ARCH_HAS_HUGEPD > static unsigned long hugepte_addr_end(unsigned long addr, unsigned long end, > unsigned long sz) > @@ -1998,34 +2027,19 @@ 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); > + __remove_refs_from_head(head, refs); > return 0; > } > - > - SetPageReferenced(head); > - return 1; > + return __huge_pt_done(head, refs, nr); > } > > static int gup_huge_pd(hugepd_t hugepd, unsigned long addr, > @@ -2071,30 +2085,18 @@ 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); > + __remove_refs_from_head(head, refs); > return 0; > } > - > - SetPageReferenced(head); > - return 1; > + return __huge_pt_done(head, refs, nr); > } > > static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr, > @@ -2114,30 +2116,18 @@ 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); > + __remove_refs_from_head(head, refs); > return 0; > } > - > - SetPageReferenced(head); > - return 1; > + return __huge_pt_done(head, refs, nr); > } > > static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr, > @@ -2151,30 +2141,19 @@ 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); > + __remove_refs_from_head(head, refs); > return 0; > } > - > - SetPageReferenced(head); > - return 1; > + return __huge_pt_done(head, refs, nr); > } > > static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end, > -- > 2.23.0 > >
On 10/31/19 11:35 AM, Ira Weiny wrote: > On Wed, Oct 30, 2019 at 03:49:13PM -0700, John Hubbard wrote: ... >> + >> +static void __remove_refs_from_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); >> +} > > I wonder if this is better implemented as "put_compound_head()"? To match the > try_get_compound_head() call below? Hi Ira, Good idea, I'll rename it to that. > >> + >> +static int __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr) >> +{ >> + *nr += nr_recorded_pages; >> + SetPageReferenced(head); >> + return 1; > > When will this return anything but 1? > Never, but it saves a line at all four call sites, by having it return like that. I could see how maybe people would prefer to just have it be a void function, and return 1 directly at the call sites. Since this was a lower line count I thought maybe it would be slightly better, but it's hard to say really. thanks, John Hubbard NVIDIA
On Thu, Oct 31, 2019 at 11:43:37AM -0700, John Hubbard wrote: > On 10/31/19 11:35 AM, Ira Weiny wrote: > > On Wed, Oct 30, 2019 at 03:49:13PM -0700, John Hubbard wrote: > ... > >> + > >> +static void __remove_refs_from_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); > >> +} > > > > I wonder if this is better implemented as "put_compound_head()"? To match the > > try_get_compound_head() call below? > > Hi Ira, > > Good idea, I'll rename it to that. > > > > >> + > >> +static int __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr) > >> +{ > >> + *nr += nr_recorded_pages; > >> + SetPageReferenced(head); > >> + return 1; > > > > When will this return anything but 1? > > > > Never, but it saves a line at all four call sites, by having it return like that. > > I could see how maybe people would prefer to just have it be a void function, > and return 1 directly at the call sites. Since this was a lower line count I > thought maybe it would be slightly better, but it's hard to say really. It is a NIT perhaps but I feel like the signature of a function should stand on it's own. What this does is mix the meaning of this function with those calling it. Which IMO is not good style. We can see what others say. Ira > > thanks, > > John Hubbard > NVIDIA >
On 10/31/19 2:09 PM, Ira Weiny wrote: > On Thu, Oct 31, 2019 at 11:43:37AM -0700, John Hubbard wrote: >> On 10/31/19 11:35 AM, Ira Weiny wrote: >>> On Wed, Oct 30, 2019 at 03:49:13PM -0700, John Hubbard wrote: >> ... >>>> + >>>> +static int __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr) >>>> +{ >>>> + *nr += nr_recorded_pages; >>>> + SetPageReferenced(head); >>>> + return 1; >>> >>> When will this return anything but 1? >>> >> >> Never, but it saves a line at all four call sites, by having it return like that. >> >> I could see how maybe people would prefer to just have it be a void function, >> and return 1 directly at the call sites. Since this was a lower line count I >> thought maybe it would be slightly better, but it's hard to say really. > > It is a NIT perhaps but I feel like the signature of a function should stand on > it's own. What this does is mix the meaning of this function with those > calling it. Which IMO is not good style. > > We can see what others say. > Sure. I'll plan on changing it to a void return type, then, unless someone else pipes up. thanks, John Hubbard NVIDIA
diff --git a/mm/gup.c b/mm/gup.c index 85caf76b3012..8fb0d9cdfaf5 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1969,6 +1969,35 @@ 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 __remove_refs_from_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 int __huge_pt_done(struct page *head, int nr_recorded_pages, int *nr) +{ + *nr += nr_recorded_pages; + SetPageReferenced(head); + return 1; +} + #ifdef CONFIG_ARCH_HAS_HUGEPD static unsigned long hugepte_addr_end(unsigned long addr, unsigned long end, unsigned long sz) @@ -1998,34 +2027,19 @@ 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); + __remove_refs_from_head(head, refs); return 0; } - - SetPageReferenced(head); - return 1; + return __huge_pt_done(head, refs, nr); } static int gup_huge_pd(hugepd_t hugepd, unsigned long addr, @@ -2071,30 +2085,18 @@ 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); + __remove_refs_from_head(head, refs); return 0; } - - SetPageReferenced(head); - return 1; + return __huge_pt_done(head, refs, nr); } static int gup_huge_pud(pud_t orig, pud_t *pudp, unsigned long addr, @@ -2114,30 +2116,18 @@ 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); + __remove_refs_from_head(head, refs); return 0; } - - SetPageReferenced(head); - return 1; + return __huge_pt_done(head, refs, nr); } static int gup_huge_pgd(pgd_t orig, pgd_t *pgdp, unsigned long addr, @@ -2151,30 +2141,19 @@ 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); + __remove_refs_from_head(head, refs); return 0; } - - SetPageReferenced(head); - return 1; + return __huge_pt_done(head, refs, nr); } static int gup_pmd_range(pud_t pud, unsigned long addr, unsigned long end,
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: 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 | 113 ++++++++++++++++++++++--------------------------------- 1 file changed, 46 insertions(+), 67 deletions(-)