Re: [PATCH v1 00/11] mm: replace follow_page() by folio_walk

[David Hildenbrand <david@redhat.com>]

On 07.08.24 11:15, Claudio Imbrenda wrote:
> On Fri,  2 Aug 2024 17:55:13 +0200
> David Hildenbrand <david@redhat.com> wrote:
> 
>> Looking into a way of moving the last folio_likely_mapped_shared() call
>> in add_folio_for_migration() under the PTL, I found myself removing
>> follow_page(). This paves the way for cleaning up all the FOLL_, follow_*
>> terminology to just be called "GUP" nowadays.
>>
>> The new page table walker will lookup a mapped folio and return to the
>> caller with the PTL held, such that the folio cannot get unmapped
>> concurrently. Callers can then conditionally decide whether they really
>> want to take a short-term folio reference or whether the can simply
>> unlock the PTL and be done with it.
>>
>> folio_walk is similar to page_vma_mapped_walk(), except that we don't know
>> the folio we want to walk to and that we are only walking to exactly one
>> PTE/PMD/PUD.
>>
>> folio_walk provides access to the pte/pmd/pud (and the referenced folio
>> page because things like KSM need that), however, as part of this series
>> no page table modifications are performed by users.
>>
>> We might be able to convert some other walk_page_range() users that really
>> only walk to one address, such as DAMON with
>> damon_mkold_ops/damon_young_ops. It might make sense to extend folio_walk
>> in the future to optionally fault in a folio (if applicable), such that we
>> can replace some get_user_pages() users that really only want to lookup
>> a single page/folio under PTL without unconditionally grabbing a folio
>> reference.
>>
>> I have plans to extend the approach to a range walker that will try
>> batching various page table entries (not just folio pages) to be a better
>> replace for walk_page_range() -- and users will be able to opt in which
>> type of page table entries they want to process -- but that will require
>> more work and more thoughts.
>>
>> KSM seems to work just fine (ksm_functional_tests selftests) and
>> move_pages seems to work (migration selftest). I tested the leaf
>> implementation excessively using various hugetlb sizes (64K, 2M, 32M, 1G)
>> on arm64 using move_pages and did some more testing on x86-64. Cross
>> compiled on a bunch of architectures.
>>
>> I am not able to test the s390x Secure Execution changes, unfortunately.
> 
> The whole series looks good to me, but I do not feel confident enough
> about all the folio details to actually r-b any of the non-s390
> patches. (I do have a few questions, though)
> 
> As for the s390 patches: they look fine. I have tested the series on
> s390 and nothing caught fire.
> 
> We will be able to get more CI coverage once this lands in -next.

Thanks for the review! Note that it's already in -next.

-- 
Cheers,

David / dhildenb