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

[David Hildenbrand <david@xxxxxxxxxx>]

> > -/*
> > - * 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