Re: [PATCH v1 07/11] mm/huge_memory: convert split_huge_pages_pid() from follow_page() to folio_walk

[David Hildenbrand]

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