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.