On 9/11/24 18:44, David Hildenbrand wrote:

On 11.09.24 15:05, Dev Jain wrote:

quoted

On 9/11/24 18:30, David Hildenbrand wrote:

quoted

On 11.09.24 14:53, Dev Jain wrote:

quoted

On 9/11/24 14:57, David Hildenbrand wrote:

quoted

On 11.09.24 08:55, Dev Jain wrote:

quoted

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.

[...]

quoted

+
+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.

quoted

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!