| Message ID | 11b92502b3df0e0bba6a1dc71476d79cab6c79ba.1651216964.git.baolin.wang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | Fix CONT-PTE/PMD size hugetlb issue when unmapping or migrating | expand |
On 4/29/22 01:14, Baolin Wang wrote: > On some architectures (like ARM64), it can support CONT-PTE/PMD size > hugetlb, which means it can support not only PMD/PUD size hugetlb: > 2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page > size specified. <snip> > diff --git a/mm/rmap.c b/mm/rmap.c > index 6fdd198..7cf2408 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, > break; > } > } > + > + /* Nuke the hugetlb page table entry */ > + pteval = huge_ptep_clear_flush(vma, address, pvmw.pte); > } else { > flush_cache_page(vma, address, pte_pfn(*pvmw.pte)); > + /* Nuke the page table entry. */ > + pteval = ptep_clear_flush(vma, address, pvmw.pte); > } > On arm64 with CONT-PTE/PMD the returned pteval will have dirty or young set if ANY of the PTE/PMDs had dirty or young set. > - /* Nuke the page table entry. */ > - pteval = ptep_clear_flush(vma, address, pvmw.pte); > - > /* Set the dirty flag on the folio now the pte is gone. */ > if (pte_dirty(pteval)) > folio_mark_dirty(folio); > @@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, > pte_t swp_pte; > > if (arch_unmap_one(mm, vma, address, pteval) < 0) { > - set_pte_at(mm, address, pvmw.pte, pteval); > + if (folio_test_hugetlb(folio)) > + set_huge_pte_at(mm, address, pvmw.pte, pteval); And, we will use that pteval for ALL the PTE/PMDs here. So, we would set the dirty or young bit in ALL PTE/PMDs. Could that cause any issues? May be more of a question for the arm64 people.
On 5/6/2022 7:53 AM, Mike Kravetz wrote: > On 4/29/22 01:14, Baolin Wang wrote: >> On some architectures (like ARM64), it can support CONT-PTE/PMD size >> hugetlb, which means it can support not only PMD/PUD size hugetlb: >> 2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page >> size specified. > <snip> >> diff --git a/mm/rmap.c b/mm/rmap.c >> index 6fdd198..7cf2408 100644 >> --- a/mm/rmap.c >> +++ b/mm/rmap.c >> @@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, >> break; >> } >> } >> + >> + /* Nuke the hugetlb page table entry */ >> + pteval = huge_ptep_clear_flush(vma, address, pvmw.pte); >> } else { >> flush_cache_page(vma, address, pte_pfn(*pvmw.pte)); >> + /* Nuke the page table entry. */ >> + pteval = ptep_clear_flush(vma, address, pvmw.pte); >> } >> > > On arm64 with CONT-PTE/PMD the returned pteval will have dirty or young set > if ANY of the PTE/PMDs had dirty or young set. Right. > >> - /* Nuke the page table entry. */ >> - pteval = ptep_clear_flush(vma, address, pvmw.pte); >> - >> /* Set the dirty flag on the folio now the pte is gone. */ >> if (pte_dirty(pteval)) >> folio_mark_dirty(folio); >> @@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, >> pte_t swp_pte; >> >> if (arch_unmap_one(mm, vma, address, pteval) < 0) { >> - set_pte_at(mm, address, pvmw.pte, pteval); >> + if (folio_test_hugetlb(folio)) >> + set_huge_pte_at(mm, address, pvmw.pte, pteval); > > And, we will use that pteval for ALL the PTE/PMDs here. So, we would set > the dirty or young bit in ALL PTE/PMDs. > > Could that cause any issues? May be more of a question for the arm64 people. I don't think this will cause any issues. Since the hugetlb can not be split, and we should not lose the the dirty or young state if any subpages were set. Meanwhile we already did like this in hugetlb.c: pte = huge_ptep_get_and_clear(mm, address, ptep); tlb_remove_huge_tlb_entry(h, tlb, ptep, address); if (huge_pte_dirty(pte)) set_page_dirty(page);
On 5/5/22 20:39, Baolin Wang wrote: > > On 5/6/2022 7:53 AM, Mike Kravetz wrote: >> On 4/29/22 01:14, Baolin Wang wrote: >>> On some architectures (like ARM64), it can support CONT-PTE/PMD size >>> hugetlb, which means it can support not only PMD/PUD size hugetlb: >>> 2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page >>> size specified. >> <snip> >>> diff --git a/mm/rmap.c b/mm/rmap.c >>> index 6fdd198..7cf2408 100644 >>> --- a/mm/rmap.c >>> +++ b/mm/rmap.c >>> @@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, >>> break; >>> } >>> } >>> + >>> + /* Nuke the hugetlb page table entry */ >>> + pteval = huge_ptep_clear_flush(vma, address, pvmw.pte); >>> } else { >>> flush_cache_page(vma, address, pte_pfn(*pvmw.pte)); >>> + /* Nuke the page table entry. */ >>> + pteval = ptep_clear_flush(vma, address, pvmw.pte); >>> } >>> >> >> On arm64 with CONT-PTE/PMD the returned pteval will have dirty or young set >> if ANY of the PTE/PMDs had dirty or young set. > > Right. > >> >>> - /* Nuke the page table entry. */ >>> - pteval = ptep_clear_flush(vma, address, pvmw.pte); >>> - >>> /* Set the dirty flag on the folio now the pte is gone. */ >>> if (pte_dirty(pteval)) >>> folio_mark_dirty(folio); >>> @@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, >>> pte_t swp_pte; >>> if (arch_unmap_one(mm, vma, address, pteval) < 0) { >>> - set_pte_at(mm, address, pvmw.pte, pteval); >>> + if (folio_test_hugetlb(folio)) >>> + set_huge_pte_at(mm, address, pvmw.pte, pteval); >> >> And, we will use that pteval for ALL the PTE/PMDs here. So, we would set >> the dirty or young bit in ALL PTE/PMDs. >> >> Could that cause any issues? May be more of a question for the arm64 people. > > I don't think this will cause any issues. Since the hugetlb can not be split, and we should not lose the the dirty or young state if any subpages were set. Meanwhile we already did like this in hugetlb.c: > > pte = huge_ptep_get_and_clear(mm, address, ptep); > tlb_remove_huge_tlb_entry(h, tlb, ptep, address); > if (huge_pte_dirty(pte)) > set_page_dirty(page); > Agree that it 'should not' cause issues. It just seems inconsistent. This is not a problem specifically with your patch, just the handling of CONT-PTE/PMD entries. There does not appear to be an arm64 specific version of huge_ptep_get() that takes CONT-PTE/PMD into account. So, huge_ptep_get() would only return the one specific value. It would not take into account the dirty or young bits of CONT-PTE/PMDs like your new version of huge_ptep_get_and_clear. Is that correct? Or, am I missing something. If I am correct, then code like the following may not work: static int gather_hugetlb_stats(pte_t *pte, unsigned long hmask, unsigned long addr, unsigned long end, struct mm_walk *walk) { pte_t huge_pte = huge_ptep_get(pte); struct numa_maps *md; struct page *page; if (!pte_present(huge_pte)) return 0; page = pte_page(huge_pte); md = walk->private; gather_stats(page, md, pte_dirty(huge_pte), 1); return 0; }
On 5/7/2022 1:56 AM, Mike Kravetz wrote: > On 5/5/22 20:39, Baolin Wang wrote: >> >> On 5/6/2022 7:53 AM, Mike Kravetz wrote: >>> On 4/29/22 01:14, Baolin Wang wrote: >>>> On some architectures (like ARM64), it can support CONT-PTE/PMD size >>>> hugetlb, which means it can support not only PMD/PUD size hugetlb: >>>> 2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page >>>> size specified. >>> <snip> >>>> diff --git a/mm/rmap.c b/mm/rmap.c >>>> index 6fdd198..7cf2408 100644 >>>> --- a/mm/rmap.c >>>> +++ b/mm/rmap.c >>>> @@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, >>>> break; >>>> } >>>> } >>>> + >>>> + /* Nuke the hugetlb page table entry */ >>>> + pteval = huge_ptep_clear_flush(vma, address, pvmw.pte); >>>> } else { >>>> flush_cache_page(vma, address, pte_pfn(*pvmw.pte)); >>>> + /* Nuke the page table entry. */ >>>> + pteval = ptep_clear_flush(vma, address, pvmw.pte); >>>> } >>>> >>> >>> On arm64 with CONT-PTE/PMD the returned pteval will have dirty or young set >>> if ANY of the PTE/PMDs had dirty or young set. >> >> Right. >> >>> >>>> - /* Nuke the page table entry. */ >>>> - pteval = ptep_clear_flush(vma, address, pvmw.pte); >>>> - >>>> /* Set the dirty flag on the folio now the pte is gone. */ >>>> if (pte_dirty(pteval)) >>>> folio_mark_dirty(folio); >>>> @@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, >>>> pte_t swp_pte; >>>> if (arch_unmap_one(mm, vma, address, pteval) < 0) { >>>> - set_pte_at(mm, address, pvmw.pte, pteval); >>>> + if (folio_test_hugetlb(folio)) >>>> + set_huge_pte_at(mm, address, pvmw.pte, pteval); >>> >>> And, we will use that pteval for ALL the PTE/PMDs here. So, we would set >>> the dirty or young bit in ALL PTE/PMDs. >>> >>> Could that cause any issues? May be more of a question for the arm64 people. >> >> I don't think this will cause any issues. Since the hugetlb can not be split, and we should not lose the the dirty or young state if any subpages were set. Meanwhile we already did like this in hugetlb.c: >> >> pte = huge_ptep_get_and_clear(mm, address, ptep); >> tlb_remove_huge_tlb_entry(h, tlb, ptep, address); >> if (huge_pte_dirty(pte)) >> set_page_dirty(page); >> > > Agree that it 'should not' cause issues. It just seems inconsistent. > This is not a problem specifically with your patch, just the handling of > CONT-PTE/PMD entries. > > There does not appear to be an arm64 specific version of huge_ptep_get() > that takes CONT-PTE/PMD into account. So, huge_ptep_get() would only > return the one specific value. It would not take into account the dirty > or young bits of CONT-PTE/PMDs like your new version of > huge_ptep_get_and_clear. Is that correct? Or, am I missing something. Yes, you are right. > > If I am correct, then code like the following may not work: > > static int gather_hugetlb_stats(pte_t *pte, unsigned long hmask, > unsigned long addr, unsigned long end, struct mm_walk *walk) > { > pte_t huge_pte = huge_ptep_get(pte); > struct numa_maps *md; > struct page *page; > > if (!pte_present(huge_pte)) > return 0; > > page = pte_page(huge_pte); > > md = walk->private; > gather_stats(page, md, pte_dirty(huge_pte), 1); > return 0; > } Right, this is inconsistent with current huge_ptep_get() interface like you said. So I think we can define an ARCH-specific huge_ptep_get() interface for arm64, and some sample code like below. How do you think? +pte_t huge_ptep_get(pte_t *ptep, unsigned long size) +{ + int ncontig; + pte_t orig_pte = ptep_get(ptep); + + if (!pte_cont(orig_pte)) + return orig_pte; + + switch (size) { + case CONT_PMD_SIZE: + ncontig = CONT_PMDS; + break; + case CONT_PTE_SIZE: + ncontig = CONT_PTES; + break; + default: + WARN_ON_ONCE(1); + return orig_pte; + } + + for (i = 0; i < ncontig; i++, ptep++) { + pte_t pte = ptep_get(ptep); + + if (pte_dirty(pte)) + orig_pte = pte_mkdirty(orig_pte); + + if (pte_young(pte)) + orig_pte = pte_mkyong(orig_pte); + } + + return orig_pte; +}
On 5/7/2022 10:33 AM, Baolin Wang wrote: > > > On 5/7/2022 1:56 AM, Mike Kravetz wrote: >> On 5/5/22 20:39, Baolin Wang wrote: >>> >>> On 5/6/2022 7:53 AM, Mike Kravetz wrote: >>>> On 4/29/22 01:14, Baolin Wang wrote: >>>>> On some architectures (like ARM64), it can support CONT-PTE/PMD size >>>>> hugetlb, which means it can support not only PMD/PUD size hugetlb: >>>>> 2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page >>>>> size specified. >>>> <snip> >>>>> diff --git a/mm/rmap.c b/mm/rmap.c >>>>> index 6fdd198..7cf2408 100644 >>>>> --- a/mm/rmap.c >>>>> +++ b/mm/rmap.c >>>>> @@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio >>>>> *folio, struct vm_area_struct *vma, >>>>> break; >>>>> } >>>>> } >>>>> + >>>>> + /* Nuke the hugetlb page table entry */ >>>>> + pteval = huge_ptep_clear_flush(vma, address, pvmw.pte); >>>>> } else { >>>>> flush_cache_page(vma, address, pte_pfn(*pvmw.pte)); >>>>> + /* Nuke the page table entry. */ >>>>> + pteval = ptep_clear_flush(vma, address, pvmw.pte); >>>>> } >>>> >>>> On arm64 with CONT-PTE/PMD the returned pteval will have dirty or >>>> young set >>>> if ANY of the PTE/PMDs had dirty or young set. >>> >>> Right. >>> >>>> >>>>> - /* Nuke the page table entry. */ >>>>> - pteval = ptep_clear_flush(vma, address, pvmw.pte); >>>>> - >>>>> /* Set the dirty flag on the folio now the pte is gone. */ >>>>> if (pte_dirty(pteval)) >>>>> folio_mark_dirty(folio); >>>>> @@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio >>>>> *folio, struct vm_area_struct *vma, >>>>> pte_t swp_pte; >>>>> if (arch_unmap_one(mm, vma, address, pteval) < 0) { >>>>> - set_pte_at(mm, address, pvmw.pte, pteval); >>>>> + if (folio_test_hugetlb(folio)) >>>>> + set_huge_pte_at(mm, address, pvmw.pte, pteval); >>>> >>>> And, we will use that pteval for ALL the PTE/PMDs here. So, we >>>> would set >>>> the dirty or young bit in ALL PTE/PMDs. >>>> >>>> Could that cause any issues? May be more of a question for the >>>> arm64 people. >>> >>> I don't think this will cause any issues. Since the hugetlb can not >>> be split, and we should not lose the the dirty or young state if any >>> subpages were set. Meanwhile we already did like this in hugetlb.c: >>> >>> pte = huge_ptep_get_and_clear(mm, address, ptep); >>> tlb_remove_huge_tlb_entry(h, tlb, ptep, address); >>> if (huge_pte_dirty(pte)) >>> set_page_dirty(page); >>> >> >> Agree that it 'should not' cause issues. It just seems inconsistent. >> This is not a problem specifically with your patch, just the handling of >> CONT-PTE/PMD entries. >> >> There does not appear to be an arm64 specific version of huge_ptep_get() >> that takes CONT-PTE/PMD into account. So, huge_ptep_get() would only >> return the one specific value. It would not take into account the dirty >> or young bits of CONT-PTE/PMDs like your new version of >> huge_ptep_get_and_clear. Is that correct? Or, am I missing something. > > Yes, you are right. > >> >> If I am correct, then code like the following may not work: >> >> static int gather_hugetlb_stats(pte_t *pte, unsigned long hmask, >> unsigned long addr, unsigned long end, struct mm_walk >> *walk) >> { >> pte_t huge_pte = huge_ptep_get(pte); >> struct numa_maps *md; >> struct page *page; >> >> if (!pte_present(huge_pte)) >> return 0; >> >> page = pte_page(huge_pte); >> >> md = walk->private; >> gather_stats(page, md, pte_dirty(huge_pte), 1); >> return 0; >> } > > Right, this is inconsistent with current huge_ptep_get() interface like > you said. So I think we can define an ARCH-specific huge_ptep_get() > interface for arm64, and some sample code like below. How do you think? After some investigation, I send out a RFC patch set[1] to address this issue. We can talk about this issue in that thread. Thanks. [1] https://lore.kernel.org/all/cover.1651998586.git.baolin.wang@linux.alibaba.com/
diff --git a/mm/rmap.c b/mm/rmap.c index 6fdd198..7cf2408 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1924,13 +1924,15 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, break; } } + + /* Nuke the hugetlb page table entry */ + pteval = huge_ptep_clear_flush(vma, address, pvmw.pte); } else { flush_cache_page(vma, address, pte_pfn(*pvmw.pte)); + /* Nuke the page table entry. */ + pteval = ptep_clear_flush(vma, address, pvmw.pte); } - /* Nuke the page table entry. */ - pteval = ptep_clear_flush(vma, address, pvmw.pte); - /* Set the dirty flag on the folio now the pte is gone. */ if (pte_dirty(pteval)) folio_mark_dirty(folio); @@ -2015,7 +2017,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, pte_t swp_pte; if (arch_unmap_one(mm, vma, address, pteval) < 0) { - set_pte_at(mm, address, pvmw.pte, pteval); + if (folio_test_hugetlb(folio)) + set_huge_pte_at(mm, address, pvmw.pte, pteval); + else + set_pte_at(mm, address, pvmw.pte, pteval); ret = false; page_vma_mapped_walk_done(&pvmw); break; @@ -2024,7 +2029,10 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, !anon_exclusive, subpage); if (anon_exclusive && page_try_share_anon_rmap(subpage)) { - set_pte_at(mm, address, pvmw.pte, pteval); + if (folio_test_hugetlb(folio)) + set_huge_pte_at(mm, address, pvmw.pte, pteval); + else + set_pte_at(mm, address, pvmw.pte, pteval); ret = false; page_vma_mapped_walk_done(&pvmw); break; @@ -2050,7 +2058,11 @@ static bool try_to_migrate_one(struct folio *folio, struct vm_area_struct *vma, swp_pte = pte_swp_mksoft_dirty(swp_pte); if (pte_uffd_wp(pteval)) swp_pte = pte_swp_mkuffd_wp(swp_pte); - set_pte_at(mm, address, pvmw.pte, swp_pte); + if (folio_test_hugetlb(folio)) + set_huge_swap_pte_at(mm, address, pvmw.pte, + swp_pte, vma_mmu_pagesize(vma)); + else + set_pte_at(mm, address, pvmw.pte, swp_pte); trace_set_migration_pte(address, pte_val(swp_pte), compound_order(&folio->page)); /*
On some architectures (like ARM64), it can support CONT-PTE/PMD size hugetlb, which means it can support not only PMD/PUD size hugetlb: 2M and 1G, but also CONT-PTE/PMD size: 64K and 32M if a 4K page size specified. When migrating a hugetlb page, we will get the relevant page table entry by huge_pte_offset() only once to nuke it and remap it with a migration pte entry. This is correct for PMD or PUD size hugetlb, since they always contain only one pmd entry or pud entry in the page table. However this is incorrect for CONT-PTE and CONT-PMD size hugetlb, since they can contain several continuous pte or pmd entry with same page table attributes. So we will nuke or remap only one pte or pmd entry for this CONT-PTE/PMD size hugetlb page, which is not expected for hugetlb migration. The problem is we can still continue to modify the subpages' data of a hugetlb page during migrating a hugetlb page, which can cause a serious data consistent issue, since we did not nuke the page table entry and set a migration pte for the subpages of a hugetlb page. To fix this issue, we should change to use huge_ptep_clear_flush() to nuke a hugetlb page table, and remap it with set_huge_pte_at() and set_huge_swap_pte_at() when migrating a hugetlb page, which already considered the CONT-PTE or CONT-PMD size hugetlb. Signed-off-by: Baolin Wang <baolin.wang@linux.alibaba.com> --- mm/rmap.c | 24 ++++++++++++++++++------ 1 file changed, 18 insertions(+), 6 deletions(-)