| Message ID | 20191121071354.456618-3-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | mm/gup: track dma-pinned pages: FOLL_PIN | expand |
On Wed, Nov 20, 2019 at 11:13:32PM -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. Any reason for the spurious underscore in the function name? Otherwise this looks fine and might be a worthwhile cleanup to feed Andrew for 5.5 independent of the gut of the changes. Reviewed-by: Christoph Hellwig <hch@lst.de>
On 11/21/19 12:03 AM, Christoph Hellwig wrote: > On Wed, Nov 20, 2019 at 11:13:32PM -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. > > Any reason for the spurious underscore in the function name? argghh, I just fixed that, but applied the fix to the wrong patch! So now patch 17 ("mm/gup: track FOLL_PIN pages") is improperly renaming it, instead of this patch naming it correctly in the first place. Will fix. > > Otherwise this looks fine and might be a worthwhile cleanup to feed > Andrew for 5.5 independent of the gut of the changes. > > Reviewed-by: Christoph Hellwig <hch@lst.de> > Thanks for the reviews! Say, it sounds like your view here is that this series should be targeted at 5.6 (not 5.5), is that what you have in mind? And get the preparatory patches (1-9, and maybe even 10-16) into 5.5? thanks,
On Thu 21-11-19 00:29:59, John Hubbard wrote: > On 11/21/19 12:03 AM, Christoph Hellwig wrote: > > Otherwise this looks fine and might be a worthwhile cleanup to feed > > Andrew for 5.5 independent of the gut of the changes. > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > Thanks for the reviews! Say, it sounds like your view here is that this > series should be targeted at 5.6 (not 5.5), is that what you have in mind? > And get the preparatory patches (1-9, and maybe even 10-16) into 5.5? Yeah, actually I feel the same. The merge window is going to open on Sunday and the series isn't still fully baked and happily sitting in linux-next (and larger changes should really sit in linux-next for at least a week, preferably two, before the merge window opens to get some reasonable test coverage). So I'd take out the independent easy patches that are already reviewed, get them merged into Andrew's (or whatever other appropriate tree) now so that they get at least a week of testing in linux-next before going upstream. And the more involved bits will have to wait for 5.6 - which means let's just continue working on them as we do now because ideally in 4 weeks we should have them ready with all the reviews so that they can be picked up and integrated into linux-next. Honza
On Thu 21-11-19 00:29:59, John Hubbard wrote: > > > > Otherwise this looks fine and might be a worthwhile cleanup to feed > > Andrew for 5.5 independent of the gut of the changes. > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > Thanks for the reviews! Say, it sounds like your view here is that this > series should be targeted at 5.6 (not 5.5), is that what you have in mind? > And get the preparatory patches (1-9, and maybe even 10-16) into 5.5? One more note :) If you are going to push pin_user_pages() interfaces (which I'm fine with), it would probably make sense to push also the put_user_pages() -> unpin_user_pages() renaming so that that inconsistency in naming does not exist in the released upstream kernel. Honza
On 11/21/19 1:49 AM, Jan Kara wrote: > On Thu 21-11-19 00:29:59, John Hubbard wrote: >> On 11/21/19 12:03 AM, Christoph Hellwig wrote: >>> Otherwise this looks fine and might be a worthwhile cleanup to feed >>> Andrew for 5.5 independent of the gut of the changes. >>> >>> Reviewed-by: Christoph Hellwig <hch@lst.de> >>> >> >> Thanks for the reviews! Say, it sounds like your view here is that this >> series should be targeted at 5.6 (not 5.5), is that what you have in mind? >> And get the preparatory patches (1-9, and maybe even 10-16) into 5.5? > > Yeah, actually I feel the same. The merge window is going to open on Sunday > and the series isn't still fully baked and happily sitting in linux-next > (and larger changes should really sit in linux-next for at least a week, > preferably two, before the merge window opens to get some reasonable test > coverage). So I'd take out the independent easy patches that are already > reviewed, get them merged into Andrew's (or whatever other appropriate > tree) now so that they get at least a week of testing in linux-next before > going upstream. And the more involved bits will have to wait for 5.6 - > which means let's just continue working on them as we do now because > ideally in 4 weeks we should have them ready with all the reviews so that > they can be picked up and integrated into linux-next. > > Honza OK, thanks for spelling it out. I'll shift over to getting the easy patches prepared for 5.5, for now. thanks,
On 11/21/19 1:54 AM, Jan Kara wrote: > On Thu 21-11-19 00:29:59, John Hubbard wrote: >>> >>> Otherwise this looks fine and might be a worthwhile cleanup to feed >>> Andrew for 5.5 independent of the gut of the changes. >>> >>> Reviewed-by: Christoph Hellwig <hch@lst.de> >>> >> >> Thanks for the reviews! Say, it sounds like your view here is that this >> series should be targeted at 5.6 (not 5.5), is that what you have in mind? >> And get the preparatory patches (1-9, and maybe even 10-16) into 5.5? > > One more note :) If you are going to push pin_user_pages() interfaces > (which I'm fine with), it would probably make sense to push also the > put_user_pages() -> unpin_user_pages() renaming so that that inconsistency > in naming does not exist in the released upstream kernel. > > Honza Yes, that's what this patch series does. But I'm not sure if "push" here means, "push out: defer to 5.6", "push (now) into 5.5", or "advocate for"? I will note that it's not going to be easy to rename in one step, now that this is being split up. Because various put_user_pages()-based items are going into 5.5 via different maintainer trees now. Probably I'd need to introduce unpin_user_page() alongside put_user_page()...thoughts? thanks,
On Thu 21-11-19 18:54:02, John Hubbard wrote: > On 11/21/19 1:54 AM, Jan Kara wrote: > > On Thu 21-11-19 00:29:59, John Hubbard wrote: > > > > > > > > Otherwise this looks fine and might be a worthwhile cleanup to feed > > > > Andrew for 5.5 independent of the gut of the changes. > > > > > > > > Reviewed-by: Christoph Hellwig <hch@lst.de> > > > > > > > > > > Thanks for the reviews! Say, it sounds like your view here is that this > > > series should be targeted at 5.6 (not 5.5), is that what you have in mind? > > > And get the preparatory patches (1-9, and maybe even 10-16) into 5.5? > > > > One more note :) If you are going to push pin_user_pages() interfaces > > (which I'm fine with), it would probably make sense to push also the > > put_user_pages() -> unpin_user_pages() renaming so that that inconsistency > > in naming does not exist in the released upstream kernel. > > > > Honza > > Yes, that's what this patch series does. But I'm not sure if "push" here > means, "push out: defer to 5.6", "push (now) into 5.5", or "advocate for"? I meant to include the patch in the "for 5.5" batch. > I will note that it's not going to be easy to rename in one step, now > that this is being split up. Because various put_user_pages()-based items > are going into 5.5 via different maintainer trees now. Probably I'd need > to introduce unpin_user_page() alongside put_user_page()...thoughts? Yes, I understand that moving that patch from the end of the series would cause fair amount of conflicts. I was hoping that you could generate the patch with sed/Coccinelle and then rebasing what remains for 5.6 on top of that patch should not be that painful so overall it should not be that much work. But I may be wrong so if it proves to be too tedious, let's just postpone the renaming to 5.6. I don't find having both unpin_user_page() and put_user_page() a better alternative to current state. Thanks! Honza
diff --git a/mm/gup.c b/mm/gup.c index 85caf76b3012..f3c7d6625817 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1969,6 +1969,25 @@ 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; + + for (nr = 0; addr != end; addr += PAGE_SIZE) + pages[nr++] = page++; + + return nr; +} + +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); +} + #ifdef CONFIG_ARCH_HAS_HUGEPD static unsigned long hugepte_addr_end(unsigned long addr, unsigned long end, unsigned long sz) @@ -1998,32 +2017,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; } + *nr += refs; SetPageReferenced(head); return 1; } @@ -2071,28 +2078,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; } + *nr += refs; SetPageReferenced(head); return 1; } @@ -2114,28 +2112,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; } + *nr += refs; SetPageReferenced(head); return 1; } @@ -2151,28 +2140,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; } + *nr += refs; SetPageReferenced(head); return 1; }