Re: [PATCH v2 7/9] mm/memory: factor out common code from vm_normal_page_*()

David Hildenbrand
2025-07-17T20:12:44Z

>>
>> -/*
>> - * vm_normal_page -- This function gets the "struct page" associated with a pte.
>> +/**
>> + * vm_normal_page_pfn() - Get the "struct page" associated with a PFN in a
>> + * non-special page table entry.
>
> This is a bit nebulous/confusing, I mean you'll get PTE entries with PTE special
> bit that'll have a PFN but just no struct page/folio to look at, or should not
> be touched.
>
> So the _pfn() bit doesn't really properly describe what it does.
>
> I wonder if it'd be better to just separate out the special handler, have
> that return a boolean indicating special of either form, and then separate
> other shared code separately from that?

Let me think about that; I played with various approaches and this was
the best I was come up with before running in circles.

>
>> + * @vma: The VMA mapping the @pfn.
>> + * @addr: The address where the @pfn is mapped.
>> + * @pfn: The PFN.
>> + * @entry: The page table entry value for error reporting purposes.
>> *
>> * "Special" mappings do not wish to be associated with a "struct page" (either
>> * it doesn't exist, or it exists but they don't want to touch it). In this
>> @@ -603,10 +608,10 @@ static void print_bad_page_map(struct vm_area_struct *vma,
>> * (such as GUP) can still identify these mappings and work with the
>> * underlying "struct page".
>> *
>> - * There are 2 broad cases. Firstly, an architecture may define a pte_special()
>> - * pte bit, in which case this function is trivial. Secondly, an architecture
>> - * may not have a spare pte bit, which requires a more complicated scheme,
>> - * described below.
>> + * There are 2 broad cases. Firstly, an architecture may define a "special"
>> + * page table entry bit (e.g., pte_special()), in which case this function is
>> + * trivial. Secondly, an architecture may not have a spare page table
>> + * entry bit, which requires a more complicated scheme, described below.
>
> Strikes me this bit of the comment should be with vm_normal_page(). As this
> implies the 2 broad cases are handled here and this isn't the case.

Well, pragmatism. Splitting up the doc doesn't make sense. Having it at
vm_normal_page() doesn't make sense.

I'm sure the educated reader will be able to make sense of it :P

But I'm happy to hear suggestions on how to do it differently :)

>
>> *
>> * A raw VM_PFNMAP mapping (ie. one that is not COWed) is always considered a
>> * special mapping (even if there are underlying and valid "struct pages").
>> @@ -639,15 +644,72 @@ static void print_bad_page_map(struct vm_area_struct *vma,
>> * don't have to follow the strict linearity rule of PFNMAP mappings in
>> * order to support COWable mappings.
>> *
>> + * This function is not expected to be called for obviously special mappings:
>> + * when the page table entry has the "special" bit set.
>
> Hmm this is is a bit weird though, saying "obviously" special, because you're
> handling "special" mappings here, but only for architectures that don't specify
> the PTE special bit.
>
> So it makes it quite nebulous what constitutes 'obviously' here, really you mean
> pte_special().

Yes, I can clarify that.

>
>> + *
>> + * Return: Returns the "struct page" if this is a "normal" mapping. Returns
>> + * NULL if this is a "special" mapping.
>> + */
>> +static inline struct page *vm_normal_page_pfn(struct vm_area_struct *vma,
>> + unsigned long addr, unsigned long pfn, unsigned long long entry)
>> +{
>> + /*
>> + * With CONFIG_ARCH_HAS_PTE_SPECIAL, any special page table mappings
>> + * (incl. shared zero folios) are marked accordingly and are handled
>> + * by the caller.
>> + */
>> + if (!IS_ENABLED(CONFIG_ARCH_HAS_PTE_SPECIAL)) {
>> + if (unlikely(vma->vm_flags & (VM_PFNMAP | VM_MIXEDMAP))) {
>> + if (vma->vm_flags & VM_MIXEDMAP) {
>> + /* If it has a "struct page", it's "normal". */
>> + if (!pfn_valid(pfn))
>> + return NULL;
>> + } else {
>> + unsigned long off = (addr - vma->vm_start) >> PAGE_SHIFT;
>> +
>> + /* Only CoW'ed anon folios are "normal". */
>> + if (pfn == vma->vm_pgoff + off)
>> + return NULL;
>> + if (!is_cow_mapping(vma->vm_flags))
>> + return NULL;
>> + }
>> + }
>> +
>> + if (is_zero_pfn(pfn) || is_huge_zero_pfn(pfn))
>
> This handles zero/zero huge page handling for non-pte_special() case
> only. I wonder if we even need to bother having these marked special
> generally since you can just check the PFN every time anyway.

Well, that makes (a) pte_special() a bit weird -- not set for some
special pages and (b) requires additional runtime checks for the case we
all really care about -- pte_special().

So I don't think we should change that.

[...]

>>
>> +/**
>> + * vm_normal_folio() - Get the "struct folio" associated with a PTE
>> + * @vma: The VMA mapping the @pte.
>> + * @addr: The address where the @pte is mapped.
>> + * @pte: The PTE.
>> + *
>> + * Get the "struct folio" associated with a PTE. See vm_normal_page_pfn()
>> + * for details.
>> + *
>> + * Return: Returns the "struct folio" if this is a "normal" mapping. Returns
>> + * NULL if this is a "special" mapping.
>> + */
>
> Nice to add a comment, but again feels weird to have the whole explanation in
> vm_normal_page_pfn() but then to invoke vm_normal_page()..

You want people to do pointer chasing to find what they are looking for? :)

--
Cheers,

David / dhildenb