On Thu, Aug 15, 2024 at 12:20:04PM +0200, David Hildenbrand wrote: > 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. Got it. Thanks! > > -- > Cheers, > > David / dhildenb >
