Messages in this thread | | | Date | Thu, 21 Dec 2023 10:29:09 +0100 | Subject | Re: [PATCH v2 04/40] mm/rmap: introduce and use hugetlb_try_dup_anon_rmap() | From | David Hildenbrand <> |
| |
On 21.12.23 05:40, Matthew Wilcox wrote: > On Wed, Dec 20, 2023 at 11:44:28PM +0100, David Hildenbrand wrote: >> hugetlb rmap handling differs quite a lot from "ordinary" rmap code. >> For example, hugetlb currently only supports entire mappings, and treats >> any mapping as mapped using a single "logical PTE". Let's move it out >> of the way so we can overhaul our "ordinary" rmap. >> implementation/interface. >> >> So let's introduce and use hugetlb_try_dup_anon_rmap() to make all >> hugetlb handling use dedicated hugetlb_* rmap functions. >> >> Add sanity checks that we end up with the right folios in the right >> functions. >> >> Note that is_device_private_page() does not apply to hugetlb. >> >> Reviewed-by: Yin Fengwei <fengwei.yin@intel.com> >> Reviewed-by: Ryan Roberts <ryan.roberts@arm.com> >> Signed-off-by: David Hildenbrand <david@redhat.com> > > Reviewed-by: Matthew Wilcox (Oracle) <willy@infradead.org> >
Thanks!
>> +static inline bool folio_needs_cow_for_dma(struct vm_area_struct *vma, >> + struct folio *folio) > > I particularly like it that you introduced this.
And a later patch even removes page_needs_cow_for_dma() :)
A note that we have one remaining user of page_maybe_dma_pinned(). Instead of converting that code to folios, we should probably just remove that pte_is_pinned() handling completely: it's inconsistent (only checks PTEs) and cannot handle concurrent GUP-fast. It's a leftover from the COW issues we had before PageAnonExclusive. [I've had patch lying around to do that for a long time, but never sent it]
> >> +static inline int hugetlb_try_dup_anon_rmap(struct folio *folio, >> + struct vm_area_struct *vma) >> +{ >> + VM_WARN_ON_FOLIO(!folio_test_hugetlb(folio), folio); >> + VM_WARN_ON_FOLIO(!folio_test_anon(folio), folio); >> + >> + if (PageAnonExclusive(&folio->page)) { > > I wonder if we need a folio_test_hugetlb_anon_exclusive() to make this > a little more ergonomic? > >> + if (unlikely(folio_needs_cow_for_dma(vma, folio))) >> + return -EBUSY; >> + ClearPageAnonExclusive(&folio->page); > > ... and set/clear variants. >
I thought about that as well, and even going a step further and instead of having PageAnonExclusive checks outside rmap code, have something like the following instead:
hugetlb_test_anon_rmap_exclusive() folio_test_anon_rmap_exclusive_[pte|pmd]()
I added that to my TODO list, because it results again in a bigger patchset (especially also in GUP).
-- Cheers,
David / dhildenb
| |