| Message ID | 20240424191914.361554-6-willy@infradead.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | More folio compat code removal | expand |
On 24.04.24 21:19, Matthew Wilcox (Oracle) wrote: Nit: s/gup_devmap/gup_fast_devmap/ > Use try_grab_folio() instead of try_grab_page() so we get the folio > back that we calculated, and then use folio_set_referenced() instead > of SetPageReferenced(). Correspondingly, use gup_put_folio() to > put any unneeded references. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> > --- > mm/gup.c | 17 ++++++++--------- > 1 file changed, 8 insertions(+), 9 deletions(-) > > diff --git a/mm/gup.c b/mm/gup.c > index 49376f756936..e4cc12b8e985 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -2877,13 +2877,10 @@ static void __maybe_unused gup_fast_undo_dev_pagemap(int *nr, int nr_start, > unsigned int flags, struct page **pages) > { > while ((*nr) - nr_start) { > - struct page *page = pages[--(*nr)]; > + struct folio *folio = page_folio(pages[--(*nr)]); > > - ClearPageReferenced(page); I stumbled over that likely unwarranted ClearPageReferenced() recently as well: what if the page was already referenced before we called SetPageReferenced? > - if (flags & FOLL_PIN) > - unpin_user_page(page); > - else > - put_page(page); > + folio_clear_referenced(folio); > + gup_put_folio(folio, 1, flags); For !FOLL_PIN, we wouldn't have done the if (!put_devmap_managed_page_refs(&folio->page, refs)) folio_put_refs(folio, refs); Magic in gup_put_folio() ... was that a BUG? This devmap crap is so confusing.
On Thu, Apr 25, 2024 at 11:49:01AM +0200, David Hildenbrand wrote: > > while ((*nr) - nr_start) { > > - struct page *page = pages[--(*nr)]; > > + struct folio *folio = page_folio(pages[--(*nr)]); > > - ClearPageReferenced(page); > > I stumbled over that likely unwarranted ClearPageReferenced() recently as > well: what if the page was already referenced before we called > SetPageReferenced? Yes, it seems bizarre. I imagine the person who wrote this was trying to undo everything, but no other cleanup path calls folio_clear_referenced(). I don't object to taking it out. > > - if (flags & FOLL_PIN) > > - unpin_user_page(page); > > - else > > - put_page(page); > > + folio_clear_referenced(folio); > > + gup_put_folio(folio, 1, flags); > > For !FOLL_PIN, we wouldn't have done the > if (!put_devmap_managed_page_refs(&folio->page, refs)) > folio_put_refs(folio, refs); > > Magic in gup_put_folio() > > ... was that a BUG? I think so. But probably nobody noticed because it's an error path. Also nobody noticed that we shouldn't have called SetPageReferenced() before getting the refcount on the page. I didn't bother mentioning it in the changelog because I think it's harmless, just conceptually wrong.
diff --git a/mm/gup.c b/mm/gup.c index 49376f756936..e4cc12b8e985 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -2877,13 +2877,10 @@ static void __maybe_unused gup_fast_undo_dev_pagemap(int *nr, int nr_start, unsigned int flags, struct page **pages) { while ((*nr) - nr_start) { - struct page *page = pages[--(*nr)]; + struct folio *folio = page_folio(pages[--(*nr)]); - ClearPageReferenced(page); - if (flags & FOLL_PIN) - unpin_user_page(page); - else - put_page(page); + folio_clear_referenced(folio); + gup_put_folio(folio, 1, flags); } } @@ -3024,6 +3021,7 @@ static int gup_fast_devmap_leaf(unsigned long pfn, unsigned long addr, struct dev_pagemap *pgmap = NULL; do { + struct folio *folio; struct page *page = pfn_to_page(pfn); pgmap = get_dev_pagemap(pfn, pgmap); @@ -3037,12 +3035,13 @@ static int gup_fast_devmap_leaf(unsigned long pfn, unsigned long addr, break; } - SetPageReferenced(page); - pages[*nr] = page; - if (unlikely(try_grab_page(page, flags))) { + folio = try_grab_folio(page, 1, flags); + if (!folio) { gup_fast_undo_dev_pagemap(nr, nr_start, flags, pages); break; } + folio_set_referenced(folio); + pages[*nr] = page; (*nr)++; pfn++; } while (addr += PAGE_SIZE, addr != end);
Use try_grab_folio() instead of try_grab_page() so we get the folio back that we calculated, and then use folio_set_referenced() instead of SetPageReferenced(). Correspondingly, use gup_put_folio() to put any unneeded references. Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> --- mm/gup.c | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-)