On 15.08.24 12:04, Pankaj Raghav wrote:
Hi David,
On Fri, Aug 02, 2024 at 05:55:20PM +0200, David Hildenbrand wrote:
continue;
}
- /* FOLL_DUMP to ignore special (like zero) pages */
- page = follow_page(vma, addr, FOLL_GET | FOLL_DUMP);
-
- if (IS_ERR_OR_NULL(page))
+ folio = folio_walk_start(&fw, vma, addr, 0);
+ if (!folio)
continue;
- folio = page_folio(page);
if (!is_transparent_hugepage(folio))
goto next;
@@ -3544,13 +3542,19 @@ static int split_huge_pages_pid(int pid, unsigned long vaddr_start,
if (!folio_trylock(folio))
goto next;
+ folio_get(folio);
Shouldn't we lock the folio after we increase the refcount on the folio?
i.e we do folio_get() first and then folio_trylock()?
That is how it was done before (through follow_page) and this patch changes
that. Maybe it doesn't matter? To me increasing the refcount and then
locking sounds more logical but I do see this ordering getting mixed all
over the kernel.
There is no need to grab a folio reference if we hold an implicit
reference through the mapping that cannot go away (not that we hold the
page table lock). Locking the folio is not special in that regard: we
just have to make sure that the folio cannot get freed concurrently,
which is the case here.
So here, we really only grab a reference if we have to -- when we are
about to drop the page table lock and will continue using the folio
afterwards.
--
Cheers,
David / dhildenb
