Re: [PATCH v7 16/20] huge_memory: Add vmf_insert_folio_pmd()

[Alistair Popple <apopple@nvidia.com>]

On Mon, Feb 10, 2025 at 07:45:09PM +0100, David Hildenbrand wrote:
> On 04.02.25 23:48, Alistair Popple wrote:
> > Currently DAX folio/page reference counts are managed differently to normal
> > pages. To allow these to be managed the same as normal pages introduce
> > vmf_insert_folio_pmd. This will map the entire PMD-sized folio and take
> > references as it would for a normally mapped page.
> > 
> > This is distinct from the current mechanism, vmf_insert_pfn_pmd, which
> > simply inserts a special devmap PMD entry into the page table without
> > holding a reference to the page for the mapping.
> > 
> > It is not currently useful to implement a more generic vmf_insert_folio()
> > which selects the correct behaviour based on folio_order(). This is because
> > PTE faults require only a subpage of the folio to be PTE mapped rather than
> > the entire folio. It would be possible to add this context somewhere but
> > callers already need to handle PTE faults and PMD faults separately so a
> > more generic function is not useful.
> > 
> > Signed-off-by: Alistair Popple <apopple@nvidia.com>
> 
> Nit: patch subject ;)
> 
> > 
> > ---
> > 
> > Changes for v7:
> > 
> >   - Fix bad pgtable handling for PPC64 (Thanks Dan and Dave)
> 
> Is it? ;) insert_pfn_pmd() still doesn't consume a "pgtable_t *"
> 
> But maybe I am missing something ...

At a high-level all I'm trying to do (perhaps badly) is pull the ptl locking one
level up the callstack.

As far as I can tell the pgtable is consumed here:

static int insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
		pmd_t *pmd, pfn_t pfn, pgprot_t prot, bool write,
		pgtable_t pgtable)

[...]

	if (pgtable) {
		pgtable_trans_huge_deposit(mm, pmd, pgtable);
		mm_inc_nr_ptes(mm);
		pgtable = NULL;
	}

[...]

	return 0;

Now I can see I failed to clean up the useless pgtable = NULL asignment, which
is confusing because I'm not trying to look at pgtable in the caller (ie.
vmf_insert_pfn_pmd()/vmf_insert_folio_pmd()) to determine if it needs freeing.
So I will remove this assignment.

Instead callers just look at the return code from insert_pfn_pmd() - if there
was an error pgtable_trans_huge_deposit(pgtable) wasn't called and if the caller
passed a pgtable it should be freed. Otherwise if insert_pfn_pmd() succeeded
then callers can assume the pgtable was consumed by pgtable_trans_huge_deposit()
and therefore should not be freed.

Hopefully that all makes sense, but maybe I've missed something obvious too...

 - Alistair

> -- 
> Cheers,
> 
> David / dhildenb
>