Re: [PATCH v2 3/3] mm/huge_memory: don't mark refcounted folios special in vmf_insert_folio_pud()

[Lorenzo Stoakes <lorenzo.stoakes@oracle.com>]

On Thu, Jun 12, 2025 at 07:00:01PM +0200, David Hildenbrand wrote:
> On 12.06.25 18:49, Lorenzo Stoakes wrote:
> > On Wed, Jun 11, 2025 at 02:06:54PM +0200, David Hildenbrand wrote:
> > > Marking PUDs that map a "normal" refcounted folios as special is
> > > against our rules documented for vm_normal_page().
> >
> > Might be worth referring to specifically which rule. I'm guessing it's the
> > general one of special == don't touch (from vm_normal_page() comment):
> >
> > /*
> >   * vm_normal_page -- This function gets the "struct page" associated with a pte.
> >   *
> >   * "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
> >   * case, NULL is returned here. "Normal" mappings do have a struct page.
> >   *
> >   * ...
> >   *
> >   */
>
> Well, yes, the one vm_normal_page() is all about ... ? :)

Lol yes to be fair that is pretty obvious...

>
> >
> > But don't we already violate this E.g.:
> >
> > 		if (vma->vm_ops && vma->vm_ops->find_special_page)
> > 			return vma->vm_ops->find_special_page(vma, addr);
> > > I mean this in itself perhaps means we should update this comment to say
> 'except
> > when file-backed and there is a find_special_page() hook'.
>
> I rather hope we severely break this case such that we can remove that hack.
>
> Read as in: I couldn't care less about this XEN hack, in particular, not
> documenting it.
>
> I was already wondering about hiding it behind a XEN config so not each and
> every sane user of this function has to perform this crappy-hack check.

Yeah, I'm not a fan of generalised hooks if they can be avoided, especially ones
where you pass critical data structures like VMAs.

It means you can, in theory, make no assumptions about what the caller does and
yeah.

To do this for such a stupid edge case is ridiculous.

>
> [...]
>
> > >   	}
> > >
> > > -	entry = pud_mkhuge(pfn_t_pud(pfn, prot));
> > > -	if (pfn_t_devmap(pfn))
> > > -		entry = pud_mkdevmap(entry);
> > > -	else
> > > -		entry = pud_mkspecial(entry);
> > > +	if (fop.is_folio) {
> > > +		entry = folio_mk_pud(fop.folio, vma->vm_page_prot);
> > > +
> > > +		folio_get(fop.folio);
> > > +		folio_add_file_rmap_pud(fop.folio, &fop.folio->page, vma);
> > > +		add_mm_counter(mm, mm_counter_file(fop.folio), HPAGE_PUD_NR);
> >
> > Nit, but might be nice to abstract for PMD/PUD.
>
> Which part exactly? Likely a follow-up if it should be abstracted.

Ah on second thoughts it doesn't matter, because you're using pud variants of
everything such that it wouldn't be worth it.

Disregard this ;)

>
> >
> > > +	} else {
> > > +		entry = pud_mkhuge(pfn_t_pud(fop.pfn, prot));
> >
> > Same incredibly pedantic whitespace comment from previous patch :)
>
> ;)
>
>
> --
> Cheers,
>
> David / dhildenb
>