linux-kernel - Re: [PATCH v7 16/20] huge_memory: Add vmf_insert_folio

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6mmjoe27y63cfe5cycqje63gehgumod3bp7zzgvpz7qehgfuv4@uomvqgizba2m>
Date: Mon, 17 Feb 2025 15:29:40 +1100
From: Alistair Popple <apopple@...dia.com>
To: David Hildenbrand <david@...hat.com>
Cc: akpm@...ux-foundation.org, dan.j.williams@...el.com, 
	linux-mm@...ck.org, Alison Schofield <alison.schofield@...el.com>, 
	lina@...hilina.net, zhang.lyra@...il.com, gerald.schaefer@...ux.ibm.com, 
	vishal.l.verma@...el.com, dave.jiang@...el.com, logang@...tatee.com, bhelgaas@...gle.com, 
	jack@...e.cz, jgg@...pe.ca, catalin.marinas@....com, will@...nel.org, 
	mpe@...erman.id.au, npiggin@...il.com, dave.hansen@...ux.intel.com, 
	ira.weiny@...el.com, willy@...radead.org, djwong@...nel.org, tytso@....edu, 
	linmiaohe@...wei.com, peterx@...hat.com, linux-doc@...r.kernel.org, 
	linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linuxppc-dev@...ts.ozlabs.org, nvdimm@...ts.linux.dev, linux-cxl@...r.kernel.org, 
	linux-fsdevel@...r.kernel.org, linux-ext4@...r.kernel.org, linux-xfs@...r.kernel.org, 
	jhubbard@...dia.com, hch@....de, david@...morbit.com, chenhuacai@...nel.org, 
	kernel@...0n.name, loongarch@...ts.linux.dev
Subject: Re: [PATCH v7 16/20] huge_memory: Add vmf_insert_folio_pmd()

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@...dia.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
>