Re: [PATCH v3 1/2] mm: Abstract THP allocation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 9/11/24 18:44, David Hildenbrand wrote:
On 11.09.24 15:05, Dev Jain wrote:

On 9/11/24 18:30, David Hildenbrand wrote:
On 11.09.24 14:53, Dev Jain wrote:

On 9/11/24 14:57, David Hildenbrand wrote:
On 11.09.24 08:55, Dev Jain wrote:
In preparation for the second patch, abstract away the THP allocation logic present in the create_huge_pmd() path, which corresponds to the
faulting case when no page is present.

There should be no functional change as a result of applying
this patch.


[...]

+
+static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf,
+            struct vm_area_struct *vma, unsigned long haddr)
+{
+    pmd_t entry;
+
+    entry = mk_huge_pmd(&folio->page, vma->vm_page_prot);
+    entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma);
+    folio_add_new_anon_rmap(folio, vma, haddr, RMAP_EXCLUSIVE);
+    folio_add_lru_vma(folio, vma);
+    set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
+    update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);

It's quite weird to see a mixture of haddr and vmf->address, and
likely this mixture is wrong or not not required.

Looking at arc's update_mmu_cache_pmd() implementation, I cannot see
how passing in the unaligned address would do the right thing. But
maybe arc also doesn't trigger that code path ... who knows :)

If I am reading correctly, arch/arc/mm/tlb.c: update_mmu_cache_pmd()
calls update_mmu_cache_range() which is already expecting an unaligned
address? But...

So update_mmu_cache_pmd() calls

     update_mmu_cache_range(NULL, vma, addr, &pte, HPAGE_PMD_NR);

But update_mmu_cache_range() only aligns it *to page boundary*:

     unsigned long vaddr = vaddr_unaligned & PAGE_MASK;

Ah, totally missed that it was PAGE_MASK. Thanks.

We obtain the correct hugepage-aligned physical address from the PTE

     phys_addr_t paddr = pte_val(*ptep) & PAGE_MASK_PHYS;

Then, we look at the offset in our folio

     unsigned long offset = offset_in_folio(folio, paddr);

And adjust both vaddr and paddr

     paddr -= offset;
     vaddr -= offset;

To then use that combination with

     __inv_icache_pages(paddr, vaddr, nr);

If I am not wrong, getting a non-hugepage aligned vaddr messes up
things here. But only regarding the icache I think.

Looks like it...

As we are adding a fresh page where there previously wasn't anything mapped (no icache invaldiation required?), and because most anon mappings are not executable, maybe that's why nobody notices so far.

Thanks for the observation!




[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]

  Powered by Linux