Dan Williams <dan.j.williams@xxxxxxxxx> writes:
> 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 dax_insert_pfn_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 would be useful to mention the rationale for the locking changes and
> your understanding of the new "pgtable deposit" handling, because those
> things make this not a trivial conversion.
My intent was not to change the locking for the existing
vmf_insert_pfn_pmd() but just to move it up a level in the stack so
dax_insert_pfn_pmd() could do the metadata manipulation while holding
the lock. Looks like I didn't get that quite right though, so I will
review it for the next version.
>>
>> Signed-off-by: Alistair Popple <apopple@xxxxxxxxxx>
>> ---
>> include/linux/huge_mm.h | 1 +-
>> mm/huge_memory.c | 57 ++++++++++++++++++++++++++++++++++--------
>> 2 files changed, 48 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
>> index d3a1872..eaf3f78 100644
>> --- a/include/linux/huge_mm.h
>> +++ b/include/linux/huge_mm.h
>> @@ -40,6 +40,7 @@ int change_huge_pmd(struct mmu_gather *tlb, struct vm_area_struct *vma,
>>
>> vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write);
>> vm_fault_t vmf_insert_pfn_pud(struct vm_fault *vmf, pfn_t pfn, bool write);
>> +vm_fault_t dax_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write);
>> vm_fault_t dax_insert_pfn_pud(struct vm_fault *vmf, pfn_t pfn, bool write);
>>
>> enum transparent_hugepage_flag {
>> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
>> index e8985a4..790041e 100644
>> --- a/mm/huge_memory.c
>> +++ b/mm/huge_memory.c
>> @@ -1237,14 +1237,12 @@ static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
>> {
>> struct mm_struct *mm = vma->vm_mm;
>> pmd_t entry;
>> - spinlock_t *ptl;
>>
>> - ptl = pmd_lock(mm, pmd);
>> if (!pmd_none(*pmd)) {
>> if (write) {
>> if (pmd_pfn(*pmd) != pfn_t_to_pfn(pfn)) {
>> WARN_ON_ONCE(!is_huge_zero_pmd(*pmd));
>> - goto out_unlock;
>> + return;
>> }
>> entry = pmd_mkyoung(*pmd);
>> entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
>> @@ -1252,7 +1250,7 @@ static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
>> update_mmu_cache_pmd(vma, addr, pmd);
>> }
>>
>> - goto out_unlock;
>> + return;
>> }
>>
>> entry = pmd_mkhuge(pfn_t_pmd(pfn, prot));
>> @@ -1271,11 +1269,6 @@ static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
>>
>> set_pmd_at(mm, addr, pmd, entry);
>> update_mmu_cache_pmd(vma, addr, pmd);
>> -
>> -out_unlock:
>> - spin_unlock(ptl);
>> - if (pgtable)
>> - pte_free(mm, pgtable);
>> }
>>
>> /**
>> @@ -1294,6 +1287,7 @@ vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write)
>> struct vm_area_struct *vma = vmf->vma;
>> pgprot_t pgprot = vma->vm_page_prot;
>> pgtable_t pgtable = NULL;
>> + spinlock_t *ptl;
>>
>> /*
>> * If we had pmd_special, we could avoid all these restrictions,
>> @@ -1316,12 +1310,55 @@ vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write)
>> }
>>
>> track_pfn_insert(vma, &pgprot, pfn);
>> -
>> + ptl = pmd_lock(vma->vm_mm, vmf->pmd);
>> insert_pfn_pmd(vma, addr, vmf->pmd, pfn, pgprot, write, pgtable);
>> + spin_unlock(ptl);
>> + if (pgtable)
>> + pte_free(vma->vm_mm, pgtable);
>> +
>> return VM_FAULT_NOPAGE;
>> }
>> EXPORT_SYMBOL_GPL(vmf_insert_pfn_pmd);
>>
>> +vm_fault_t dax_insert_pfn_pmd(struct vm_fault *vmf, pfn_t pfn, bool write)
>> +{
>> + struct vm_area_struct *vma = vmf->vma;
>> + unsigned long addr = vmf->address & PMD_MASK;
>> + struct mm_struct *mm = vma->vm_mm;
>> + spinlock_t *ptl;
>> + pgtable_t pgtable = NULL;
>> + struct folio *folio;
>> + struct page *page;
>> +
>> + if (addr < vma->vm_start || addr >= vma->vm_end)
>> + return VM_FAULT_SIGBUS;
>> +
>> + if (arch_needs_pgtable_deposit()) {
>> + pgtable = pte_alloc_one(vma->vm_mm);
>> + if (!pgtable)
>> + return VM_FAULT_OOM;
>> + }
>> +
>> + track_pfn_insert(vma, &vma->vm_page_prot, pfn);
>> +
>> + ptl = pmd_lock(mm, vmf->pmd);
>> + if (pmd_none(*vmf->pmd)) {
>> + page = pfn_t_to_page(pfn);
>> + folio = page_folio(page);
>> + folio_get(folio);
>> + folio_add_file_rmap_pmd(folio, page, vma);
>> + add_mm_counter(mm, mm_counter_file(folio), HPAGE_PMD_NR);
>> + }
>> + insert_pfn_pmd(vma, addr, vmf->pmd, pfn, vma->vm_page_prot,
>> + write, pgtable);
>> + spin_unlock(ptl);
>> + if (pgtable)
>> + pte_free(mm, pgtable);
>
> Are not the deposit rules that the extra page table stick around for the
> lifetime of the inserted pte? So would that not require this incremental
> change?
Yeah, thanks for catching this.
> ---
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index ea65c2db2bb1..5ef1e5d21a96 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -1232,7 +1232,7 @@ vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
>
> static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
> pmd_t *pmd, unsigned long pfn, pgprot_t prot,
> - bool write, pgtable_t pgtable)
> + bool write, pgtable_t *pgtable)
> {
> struct mm_struct *mm = vma->vm_mm;
> pmd_t entry;
> @@ -1258,10 +1258,10 @@ static void insert_pfn_pmd(struct vm_area_struct *vma, unsigned long addr,
> entry = maybe_pmd_mkwrite(entry, vma);
> }
>
> - if (pgtable) {
> - pgtable_trans_huge_deposit(mm, pmd, pgtable);
> + if (*pgtable) {
> + pgtable_trans_huge_deposit(mm, pmd, *pgtable);
> mm_inc_nr_ptes(mm);
> - pgtable = NULL;
> + *pgtable = NULL;
> }
>
> set_pmd_at(mm, addr, pmd, entry);
> @@ -1306,7 +1306,7 @@ vm_fault_t vmf_insert_pfn_pmd(struct vm_fault *vmf, unsigned long pfn, bool writ
>
> track_pfn_insert(vma, &pgprot, pfn);
> ptl = pmd_lock(vma->vm_mm, vmf->pmd);
> - insert_pfn_pmd(vma, addr, vmf->pmd, pfn, pgprot, write, pgtable);
> + insert_pfn_pmd(vma, addr, vmf->pmd, pfn, pgprot, write, &pgtable);
> spin_unlock(ptl);
> if (pgtable)
> pte_free(vma->vm_mm, pgtable);
> @@ -1344,8 +1344,8 @@ vm_fault_t dax_insert_pfn_pmd(struct vm_fault *vmf, unsigned long pfn, bool writ
> folio_add_file_rmap_pmd(folio, page, vma);
> add_mm_counter(mm, mm_counter_file(folio), HPAGE_PMD_NR);
> }
> - insert_pfn_pmd(vma, addr, vmf->pmd, pfn, vma->vm_page_prot,
> - write, pgtable);
> + insert_pfn_pmd(vma, addr, vmf->pmd, pfn, vma->vm_page_prot, write,
> + &pgtable);
> spin_unlock(ptl);
> if (pgtable)
> pte_free(mm, pgtable);
> ---
>
> Along these lines it would be lovely if someone from the PowerPC side
> could test these changes, or if someone has a canned qemu command line
> to test radix vs hash with pmem+dax that they can share?
Michael, Nick, do you know of a qemu command or anyone who might?
>> +
>> + return VM_FAULT_NOPAGE;
>> +}
>> +EXPORT_SYMBOL_GPL(dax_insert_pfn_pmd);
>
> Like I mentioned before, lets make the exported function
> vmf_insert_folio() and move the pte, pmd, pud internal private / static
> details of the implementation. The "dax_" specific aspect of this was
> removed at the conversion of a dax_pfn to a folio.
Ok, let me try that. Note that vmf_insert_pfn{_pmd|_pud} will have to
stick around though.
