| Message ID | 20190807233729.3899352-6-songliubraving@fb.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | THP aware uprobe | expand |
On 08/07, Song Liu wrote: > > +void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr) > +{ > + unsigned long haddr = addr & HPAGE_PMD_MASK; > + struct vm_area_struct *vma = find_vma(mm, haddr); > + struct page *hpage = NULL; > + pmd_t *pmd, _pmd; > + spinlock_t *ptl; > + int count = 0; > + int i; > + > + if (!vma || !vma->vm_file || > + vma->vm_start > haddr || vma->vm_end < haddr + HPAGE_PMD_SIZE) > + return; > + > + /* > + * This vm_flags may not have VM_HUGEPAGE if the page was not > + * collapsed by this mm. But we can still collapse if the page is > + * the valid THP. Add extra VM_HUGEPAGE so hugepage_vma_check() > + * will not fail the vma for missing VM_HUGEPAGE > + */ > + if (!hugepage_vma_check(vma, vma->vm_flags | VM_HUGEPAGE)) > + return; > + > + pmd = mm_find_pmd(mm, haddr); OK, I do not see anything really wrong... a couple of questions below. > + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { > + pte_t *pte = pte_offset_map(pmd, addr); > + struct page *page; > + > + if (pte_none(*pte)) > + continue; > + > + page = vm_normal_page(vma, addr, *pte); > + > + if (!page || !PageCompound(page)) > + return; > + > + if (!hpage) { > + hpage = compound_head(page); OK, > + if (hpage->mapping != vma->vm_file->f_mapping) > + return; is it really possible? May be WARN_ON(hpage->mapping != vm_file->f_mapping) makes more sense ? > + if (hpage + i != page) > + return; ditto. Oleg.
> On Aug 8, 2019, at 9:33 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 08/07, Song Liu wrote: >> >> +void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr) >> +{ >> + unsigned long haddr = addr & HPAGE_PMD_MASK; >> + struct vm_area_struct *vma = find_vma(mm, haddr); >> + struct page *hpage = NULL; >> + pmd_t *pmd, _pmd; >> + spinlock_t *ptl; >> + int count = 0; >> + int i; >> + >> + if (!vma || !vma->vm_file || >> + vma->vm_start > haddr || vma->vm_end < haddr + HPAGE_PMD_SIZE) >> + return; >> + >> + /* >> + * This vm_flags may not have VM_HUGEPAGE if the page was not >> + * collapsed by this mm. But we can still collapse if the page is >> + * the valid THP. Add extra VM_HUGEPAGE so hugepage_vma_check() >> + * will not fail the vma for missing VM_HUGEPAGE >> + */ >> + if (!hugepage_vma_check(vma, vma->vm_flags | VM_HUGEPAGE)) >> + return; >> + >> + pmd = mm_find_pmd(mm, haddr); > > OK, I do not see anything really wrong... > > a couple of questions below. > >> + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { >> + pte_t *pte = pte_offset_map(pmd, addr); >> + struct page *page; >> + >> + if (pte_none(*pte)) >> + continue; >> + >> + page = vm_normal_page(vma, addr, *pte); >> + >> + if (!page || !PageCompound(page)) >> + return; >> + >> + if (!hpage) { >> + hpage = compound_head(page); > > OK, > >> + if (hpage->mapping != vma->vm_file->f_mapping) >> + return; > > is it really possible? May be WARN_ON(hpage->mapping != vm_file->f_mapping) > makes more sense ? I haven't found code paths lead to this, but this is technically possible. This pmd could contain subpages from different THPs. The __replace_page() function in uprobes.c creates similar pmd. Current uprobe code won't really create this problem, because !PageCompound() check above is sufficient. But it won't be difficult to modify uprobe code to break this. For this code to be accurate and safe, I think both this check and the one below are necessary. Also, this code is not on any critical path, so the overhead should be negligible. Does this make sense? Thanks, Song
On 08/08, Song Liu wrote: > > > On Aug 8, 2019, at 9:33 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > > >> + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { > >> + pte_t *pte = pte_offset_map(pmd, addr); > >> + struct page *page; > >> + > >> + if (pte_none(*pte)) > >> + continue; > >> + > >> + page = vm_normal_page(vma, addr, *pte); just noticed... shouldn't you also check pte_present() before vm_normal_page() ? > >> + if (!page || !PageCompound(page)) > >> + return; > >> + > >> + if (!hpage) { > >> + hpage = compound_head(page); > > > > OK, > > > >> + if (hpage->mapping != vma->vm_file->f_mapping) > >> + return; > > > > is it really possible? May be WARN_ON(hpage->mapping != vm_file->f_mapping) > > makes more sense ? > > I haven't found code paths lead to this, Neither me, that is why I asked. I think this should not be possible, but again this is not my area. > but this is technically possible. > This pmd could contain subpages from different THPs. Then please explain how this can happen ? > The __replace_page() > function in uprobes.c creates similar pmd. No it doesn't, > Current uprobe code won't really create this problem, because > !PageCompound() check above is sufficient. But it won't be difficult to > modify uprobe code to break this. I bet it will be a) difficult and b) the very idea to do this would be wrong. > For this code to be accurate and safe, > I think both this check and the one below are necessary. I didn't suggest to remove these checks. > Also, this code > is not on any critical path, so the overhead should be negligible. I do not care about overhead. But I do care about a poor reader like me who will try to understand this code. If you too do not understand how a THP page can have a different mapping then use VM_WARN or at least add a comment to explain that this is not supposed to happen! > Does this make sense? Not to me :/ Oleg.
> On Aug 9, 2019, at 8:24 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 08/08, Song Liu wrote: >> >>> On Aug 8, 2019, at 9:33 AM, Oleg Nesterov <oleg@redhat.com> wrote: >>> >>>> + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { >>>> + pte_t *pte = pte_offset_map(pmd, addr); >>>> + struct page *page; >>>> + >>>> + if (pte_none(*pte)) >>>> + continue; >>>> + >>>> + page = vm_normal_page(vma, addr, *pte); > > just noticed... shouldn't you also check pte_present() before > vm_normal_page() ? Good catch! Let me fix this. > >>>> + if (!page || !PageCompound(page)) >>>> + return; >>>> + >>>> + if (!hpage) { >>>> + hpage = compound_head(page); >>> >>> OK, >>> >>>> + if (hpage->mapping != vma->vm_file->f_mapping) >>>> + return; >>> >>> is it really possible? May be WARN_ON(hpage->mapping != vm_file->f_mapping) >>> makes more sense ? >> >> I haven't found code paths lead to this, > > Neither me, that is why I asked. I think this should not be possible, > but again this is not my area. > >> but this is technically possible. >> This pmd could contain subpages from different THPs. > > Then please explain how this can happen ? > >> The __replace_page() >> function in uprobes.c creates similar pmd. > > No it doesn't, > >> Current uprobe code won't really create this problem, because >> !PageCompound() check above is sufficient. But it won't be difficult to >> modify uprobe code to break this. > > I bet it will be a) difficult and b) the very idea to do this would be wrong. > >> For this code to be accurate and safe, >> I think both this check and the one below are necessary. > > I didn't suggest to remove these checks. > >> Also, this code >> is not on any critical path, so the overhead should be negligible. > > I do not care about overhead. But I do care about a poor reader like me > who will try to understand this code. > > If you too do not understand how a THP page can have a different mapping > then use VM_WARN or at least add a comment to explain that this is not > supposed to happen! Fair enough. I will add WARN and more comments. Thanks, Song
> On Aug 9, 2019, at 9:30 AM, Song Liu <songliubraving@fb.com> wrote: > > > >> On Aug 9, 2019, at 8:24 AM, Oleg Nesterov <oleg@redhat.com> wrote: >> >> On 08/08, Song Liu wrote: >>> >>>> On Aug 8, 2019, at 9:33 AM, Oleg Nesterov <oleg@redhat.com> wrote: >>>> >>>>> + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { >>>>> + pte_t *pte = pte_offset_map(pmd, addr); >>>>> + struct page *page; >>>>> + >>>>> + if (pte_none(*pte)) >>>>> + continue; >>>>> + >>>>> + page = vm_normal_page(vma, addr, *pte); >> >> just noticed... shouldn't you also check pte_present() before >> vm_normal_page() ? > > Good catch! Let me fix this. > >> >>>>> + if (!page || !PageCompound(page)) >>>>> + return; >>>>> + >>>>> + if (!hpage) { >>>>> + hpage = compound_head(page); >>>> >>>> OK, >>>> >>>>> + if (hpage->mapping != vma->vm_file->f_mapping) >>>>> + return; >>>> >>>> is it really possible? May be WARN_ON(hpage->mapping != vm_file->f_mapping) >>>> makes more sense ? >>> >>> I haven't found code paths lead to this, >> >> Neither me, that is why I asked. I think this should not be possible, >> but again this is not my area. >> >>> but this is technically possible. >>> This pmd could contain subpages from different THPs. >> >> Then please explain how this can happen ? >> >>> The __replace_page() >>> function in uprobes.c creates similar pmd. >> >> No it doesn't, >> >>> Current uprobe code won't really create this problem, because >>> !PageCompound() check above is sufficient. But it won't be difficult to >>> modify uprobe code to break this. >> >> I bet it will be a) difficult and b) the very idea to do this would be wrong. >> >>> For this code to be accurate and safe, >>> I think both this check and the one below are necessary. >> >> I didn't suggest to remove these checks. >> >>> Also, this code >>> is not on any critical path, so the overhead should be negligible. >> >> I do not care about overhead. But I do care about a poor reader like me >> who will try to understand this code. >> >> If you too do not understand how a THP page can have a different mapping >> then use VM_WARN or at least add a comment to explain that this is not >> supposed to happen! > > Fair enough. I will add WARN and more comments. > > Thanks, > Song To reduce spamming, I attached updated 5/6 here. Thanks, Song ====================== 8< ============================= From 3fb735e03b149bf8a90918dd383a3a31b3f9008a Mon Sep 17 00:00:00 2001 From: Song Liu <songliubraving@fb.com> Date: Sun, 28 Jul 2019 03:43:48 -0700 Subject: [PATCH v13 5/6] khugepaged: enable collapse pmd for pte-mapped THP khugepaged needs exclusive mmap_sem to access page table. When it fails to lock mmap_sem, the page will fault in as pte-mapped THP. As the page is already a THP, khugepaged will not handle this pmd again. This patch enables the khugepaged to retry collapse the page table. struct mm_slot (in khugepaged.c) is extended with an array, containing addresses of pte-mapped THPs. We use array here for simplicity. We can easily replace it with more advanced data structures when needed. In khugepaged_scan_mm_slot(), if the mm contains pte-mapped THP, we try to collapse the page table. Since collapse may happen at an later time, some pages may already fault in. collapse_pte_mapped_thp() is added to properly handle these pages. collapse_pte_mapped_thp() also double checks whether all ptes in this pmd are mapping to the same THP. This is necessary because some subpage of the THP may be replaced, for example by uprobe. In such cases, it is not possible to collapse the pmd. Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Cc: Oleg Nesterov <oleg@redhat.com> Signed-off-by: Song Liu <songliubraving@fb.com> --- include/linux/khugepaged.h | 12 +++ mm/khugepaged.c | 154 ++++++++++++++++++++++++++++++++++++- 2 files changed, 165 insertions(+), 1 deletion(-) diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h index 082d1d2a5216..bc45ea1efbf7 100644 --- a/include/linux/khugepaged.h +++ b/include/linux/khugepaged.h @@ -15,6 +15,14 @@ extern int __khugepaged_enter(struct mm_struct *mm); extern void __khugepaged_exit(struct mm_struct *mm); extern int khugepaged_enter_vma_merge(struct vm_area_struct *vma, unsigned long vm_flags); +#ifdef CONFIG_SHMEM +extern void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr); +#else +static inline void collapse_pte_mapped_thp(struct mm_struct *mm, + unsigned long addr) +{ +} +#endif #define khugepaged_enabled() \ (transparent_hugepage_flags & \ @@ -73,6 +81,10 @@ static inline int khugepaged_enter_vma_merge(struct vm_area_struct *vma, { return 0; } +static inline void collapse_pte_mapped_thp(struct mm_struct *mm, + unsigned long addr) +{ +} #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ #endif /* _LINUX_KHUGEPAGED_H */ diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 40c25ddf29e4..3e722065e909 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -77,6 +77,8 @@ static __read_mostly DEFINE_HASHTABLE(mm_slots_hash, MM_SLOTS_HASH_BITS); static struct kmem_cache *mm_slot_cache __read_mostly; +#define MAX_PTE_MAPPED_THP 8 + /** * struct mm_slot - hash lookup from mm to mm_slot * @hash: hash collision list @@ -87,6 +89,10 @@ struct mm_slot { struct hlist_node hash; struct list_head mm_node; struct mm_struct *mm; + + /* pte-mapped THP in this mm */ + int nr_pte_mapped_thp; + unsigned long pte_mapped_thp[MAX_PTE_MAPPED_THP]; }; /** @@ -1254,6 +1260,145 @@ static void collect_mm_slot(struct mm_slot *mm_slot) } #if defined(CONFIG_SHMEM) && defined(CONFIG_TRANSPARENT_HUGE_PAGECACHE) +/* + * Notify khugepaged that given addr of the mm is pte-mapped THP. Then + * khugepaged should try to collapse the page table. + */ +static int khugepaged_add_pte_mapped_thp(struct mm_struct *mm, + unsigned long addr) +{ + struct mm_slot *mm_slot; + + VM_BUG_ON(addr & ~HPAGE_PMD_MASK); + + spin_lock(&khugepaged_mm_lock); + mm_slot = get_mm_slot(mm); + if (likely(mm_slot && mm_slot->nr_pte_mapped_thp < MAX_PTE_MAPPED_THP)) + mm_slot->pte_mapped_thp[mm_slot->nr_pte_mapped_thp++] = addr; + spin_unlock(&khugepaged_mm_lock); + return 0; +} + +/** + * Try to collapse a pte-mapped THP for mm at address haddr. + * + * This function checks whether all the PTEs in the PMD are pointing to the + * right THP. If so, retract the page table so the THP can refault in with + * as pmd-mapped. + */ +void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr) +{ + unsigned long haddr = addr & HPAGE_PMD_MASK; + struct vm_area_struct *vma = find_vma(mm, haddr); + struct page *hpage = NULL; + pmd_t *pmd, _pmd; + spinlock_t *ptl; + int count = 0; + int i; + + if (!vma || !vma->vm_file || + vma->vm_start > haddr || vma->vm_end < haddr + HPAGE_PMD_SIZE) + return; + + /* + * This vm_flags may not have VM_HUGEPAGE if the page was not + * collapsed by this mm. But we can still collapse if the page is + * the valid THP. Add extra VM_HUGEPAGE so hugepage_vma_check() + * will not fail the vma for missing VM_HUGEPAGE + */ + if (!hugepage_vma_check(vma, vma->vm_flags | VM_HUGEPAGE)) + return; + + pmd = mm_find_pmd(mm, haddr); + if (!pmd) + return; + + /* step 1: check all mapped PTEs are to the right huge page */ + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { + pte_t *pte = pte_offset_map(pmd, addr); + struct page *page; + + if (pte_none(*pte) || !pte_present(*pte)) + continue; + + page = vm_normal_page(vma, addr, *pte); + + if (!page || !PageCompound(page)) + return; + + if (!hpage) { + hpage = compound_head(page); + /* + * The mapping of the THP should not change. + * + * Note that uprobe may change the page table, but + * the new page installed by uprobe will not pass + * PageCompound() check. + */ + if (VM_WARN_ON(hpage->mapping != vma->vm_file->f_mapping)) + return; + } + + /* + * Confirm the page maps to the correct subpage. + * + * Note that uprobe may change the page table, but the new + * page installed by uprobe will not pass PageCompound() + * check. + */ + if (VM_WARN_ON(hpage + i != page)) + return; + count++; + } + + /* step 2: adjust rmap */ + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { + pte_t *pte = pte_offset_map(pmd, addr); + struct page *page; + + if (pte_none(*pte)) + continue; + page = vm_normal_page(vma, addr, *pte); + page_remove_rmap(page, false); + } + + /* step 3: set proper refcount and mm_counters. */ + if (hpage) { + page_ref_sub(hpage, count); + add_mm_counter(vma->vm_mm, mm_counter_file(hpage), -count); + } + + /* step 4: collapse pmd */ + ptl = pmd_lock(vma->vm_mm, pmd); + _pmd = pmdp_collapse_flush(vma, addr, pmd); + spin_unlock(ptl); + mm_dec_nr_ptes(mm); + pte_free(mm, pmd_pgtable(_pmd)); +} + +static int khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot) +{ + struct mm_struct *mm = mm_slot->mm; + int i; + + if (likely(mm_slot->nr_pte_mapped_thp == 0)) + return 0; + + if (!down_write_trylock(&mm->mmap_sem)) + return -EBUSY; + + if (unlikely(khugepaged_test_exit(mm))) + goto out; + + for (i = 0; i < mm_slot->nr_pte_mapped_thp; i++) + collapse_pte_mapped_thp(mm, mm_slot->pte_mapped_thp[i]); + +out: + mm_slot->nr_pte_mapped_thp = 0; + up_write(&mm->mmap_sem); + return 0; +} + static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) { struct vm_area_struct *vma; @@ -1287,7 +1432,8 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) up_write(&vma->vm_mm->mmap_sem); mm_dec_nr_ptes(vma->vm_mm); pte_free(vma->vm_mm, pmd_pgtable(_pmd)); - } + } else + khugepaged_add_pte_mapped_thp(vma->vm_mm, addr); } i_mmap_unlock_write(mapping); } @@ -1709,6 +1855,11 @@ static void khugepaged_scan_file(struct mm_struct *mm, { BUILD_BUG(); } + +static int khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot) +{ + return 0; +} #endif static unsigned int khugepaged_scan_mm_slot(unsigned int pages, @@ -1733,6 +1884,7 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, khugepaged_scan.mm_slot = mm_slot; } spin_unlock(&khugepaged_mm_lock); + khugepaged_collapse_pte_mapped_thps(mm_slot); mm = mm_slot->mm; /*
On Fri, Aug 09, 2019 at 06:01:18PM +0000, Song Liu wrote: > + if (pte_none(*pte) || !pte_present(*pte)) > + continue; You don't need to check both. Present is never none.
On 08/09, Song Liu wrote: > > +void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr) > +{ > + unsigned long haddr = addr & HPAGE_PMD_MASK; > + struct vm_area_struct *vma = find_vma(mm, haddr); > + struct page *hpage = NULL; > + pmd_t *pmd, _pmd; > + spinlock_t *ptl; > + int count = 0; > + int i; > + > + if (!vma || !vma->vm_file || > + vma->vm_start > haddr || vma->vm_end < haddr + HPAGE_PMD_SIZE) > + return; > + > + /* > + * This vm_flags may not have VM_HUGEPAGE if the page was not > + * collapsed by this mm. But we can still collapse if the page is > + * the valid THP. Add extra VM_HUGEPAGE so hugepage_vma_check() > + * will not fail the vma for missing VM_HUGEPAGE > + */ > + if (!hugepage_vma_check(vma, vma->vm_flags | VM_HUGEPAGE)) > + return; > + > + pmd = mm_find_pmd(mm, haddr); > + if (!pmd) > + return; > + > + /* step 1: check all mapped PTEs are to the right huge page */ > + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { > + pte_t *pte = pte_offset_map(pmd, addr); > + struct page *page; > + > + if (pte_none(*pte) || !pte_present(*pte)) > + continue; if (!pte_present(*pte)) return; you can't simply flush pmd if this page is swapped out. > + > + page = vm_normal_page(vma, addr, *pte); > + > + if (!page || !PageCompound(page)) > + return; > + > + if (!hpage) { > + hpage = compound_head(page); > + /* > + * The mapping of the THP should not change. > + * > + * Note that uprobe may change the page table, Not only uprobe can cow the page. Debugger can do. Or mmap(PROT_WRITE, MAP_PRIVATE). uprobe() is "special" because it a) it works with a foreign mm and b) it can't stop the process which uses this mm. Otherwise it could simply update the page returned by get_user_pages_remote(FOLL_FORCE), just we would need to add FOLL_WRITE and if we do this we do not even need SPLIT, that is why, say, __access_remote_vm() works without SPLIT. Oleg.
On 08/12, Kirill A. Shutemov wrote: > > On Fri, Aug 09, 2019 at 06:01:18PM +0000, Song Liu wrote: > > + if (pte_none(*pte) || !pte_present(*pte)) > > + continue; > > You don't need to check both. Present is never none. Agreed. Kirill, while you are here, shouldn't retract_page_tables() check vma->anon_vma (and probably do mm_find_pmd) under vm_mm->mmap_sem? Can't it race with, say, do_cow_fault? Oleg.
> On Aug 12, 2019, at 6:06 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > On 08/09, Song Liu wrote: >> >> +void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr) >> +{ >> + unsigned long haddr = addr & HPAGE_PMD_MASK; >> + struct vm_area_struct *vma = find_vma(mm, haddr); >> + struct page *hpage = NULL; >> + pmd_t *pmd, _pmd; >> + spinlock_t *ptl; >> + int count = 0; >> + int i; >> + >> + if (!vma || !vma->vm_file || >> + vma->vm_start > haddr || vma->vm_end < haddr + HPAGE_PMD_SIZE) >> + return; >> + >> + /* >> + * This vm_flags may not have VM_HUGEPAGE if the page was not >> + * collapsed by this mm. But we can still collapse if the page is >> + * the valid THP. Add extra VM_HUGEPAGE so hugepage_vma_check() >> + * will not fail the vma for missing VM_HUGEPAGE >> + */ >> + if (!hugepage_vma_check(vma, vma->vm_flags | VM_HUGEPAGE)) >> + return; >> + >> + pmd = mm_find_pmd(mm, haddr); >> + if (!pmd) >> + return; >> + >> + /* step 1: check all mapped PTEs are to the right huge page */ >> + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { >> + pte_t *pte = pte_offset_map(pmd, addr); >> + struct page *page; >> + >> + if (pte_none(*pte) || !pte_present(*pte)) >> + continue; > > if (!pte_present(*pte)) > return; > > you can't simply flush pmd if this page is swapped out. hmm... how about if (pte_none(*pte)) continue; if (!pte_present(*pte)) return; If the page hasn't faulted in for this mm, i.e. pte_none(), we can flush the pmd. > >> + >> + page = vm_normal_page(vma, addr, *pte); >> + >> + if (!page || !PageCompound(page)) >> + return; >> + >> + if (!hpage) { >> + hpage = compound_head(page); >> + /* >> + * The mapping of the THP should not change. >> + * >> + * Note that uprobe may change the page table, > > Not only uprobe can cow the page. Debugger can do. Or mmap(PROT_WRITE, MAP_PRIVATE). > > uprobe() is "special" because it a) it works with a foreign mm and b) > it can't stop the process which uses this mm. Otherwise it could simply > update the page returned by get_user_pages_remote(FOLL_FORCE), just we > would need to add FOLL_WRITE and if we do this we do not even need SPLIT, > that is why, say, __access_remote_vm() works without SPLIT. Will update the comment in next version. Thanks! Song
On Mon, Aug 12, 2019 at 03:22:58PM +0200, Oleg Nesterov wrote: > On 08/12, Kirill A. Shutemov wrote: > > > > On Fri, Aug 09, 2019 at 06:01:18PM +0000, Song Liu wrote: > > > + if (pte_none(*pte) || !pte_present(*pte)) > > > + continue; > > > > You don't need to check both. Present is never none. > > Agreed. > > Kirill, while you are here, shouldn't retract_page_tables() check > vma->anon_vma (and probably do mm_find_pmd) under vm_mm->mmap_sem? > > Can't it race with, say, do_cow_fault? vma->anon_vma can race, but it doesn't matter. False-negative is fine. It's attempt to avoid taking mmap_sem where it can be not productive. mm_find_pmd() cannot race with do_cow_fault() since the page is locked. __do_fault() has to return locked page before we touch page tables. It is somewhat subtle, but I wanted to avoid taking mmap_sem where it is possible.
> On Aug 12, 2019, at 7:40 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > > On Mon, Aug 12, 2019 at 03:22:58PM +0200, Oleg Nesterov wrote: >> On 08/12, Kirill A. Shutemov wrote: >>> >>> On Fri, Aug 09, 2019 at 06:01:18PM +0000, Song Liu wrote: >>>> + if (pte_none(*pte) || !pte_present(*pte)) >>>> + continue; >>> >>> You don't need to check both. Present is never none. >> >> Agreed. >> >> Kirill, while you are here, shouldn't retract_page_tables() check >> vma->anon_vma (and probably do mm_find_pmd) under vm_mm->mmap_sem? >> >> Can't it race with, say, do_cow_fault? > > vma->anon_vma can race, but it doesn't matter. False-negative is fine. > It's attempt to avoid taking mmap_sem where it can be not productive. > > mm_find_pmd() cannot race with do_cow_fault() since the page is locked. > __do_fault() has to return locked page before we touch page tables. > It is somewhat subtle, but I wanted to avoid taking mmap_sem where it is > possible. > > -- > Kirill A. Shutemov Updated version attached. Besides feedbacks from Oleg and Kirill, I also revise the locking in collapse_pte_mapped_thp(): use pte_offset_map_lock() for the two loops to cover highmem. zap_pte_range() has similar use of the lock. This change is suggested by Johannes. Thanks, Song ================ 8< ====================== From 3d931bc4780abb6109fe478a4b1a0004ce81efe1 Mon Sep 17 00:00:00 2001 From: Song Liu <songliubraving@fb.com> Date: Sun, 28 Jul 2019 03:43:48 -0700 Subject: [PATCH 5/6] khugepaged: enable collapse pmd for pte-mapped THP khugepaged needs exclusive mmap_sem to access page table. When it fails to lock mmap_sem, the page will fault in as pte-mapped THP. As the page is already a THP, khugepaged will not handle this pmd again. This patch enables the khugepaged to retry collapse the page table. struct mm_slot (in khugepaged.c) is extended with an array, containing addresses of pte-mapped THPs. We use array here for simplicity. We can easily replace it with more advanced data structures when needed. In khugepaged_scan_mm_slot(), if the mm contains pte-mapped THP, we try to collapse the page table. Since collapse may happen at an later time, some pages may already fault in. collapse_pte_mapped_thp() is added to properly handle these pages. collapse_pte_mapped_thp() also double checks whether all ptes in this pmd are mapping to the same THP. This is necessary because some subpage of the THP may be replaced, for example by uprobe. In such cases, it is not possible to collapse the pmd. Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> Suggested-by: Johannes Weiner <hannes@cmpxchg.org> Cc: Oleg Nesterov <oleg@redhat.com> Signed-off-by: Song Liu <songliubraving@fb.com> --- include/linux/khugepaged.h | 12 +++ mm/khugepaged.c | 168 ++++++++++++++++++++++++++++++++++++- 2 files changed, 179 insertions(+), 1 deletion(-) diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h index 082d1d2a5216..bc45ea1efbf7 100644 --- a/include/linux/khugepaged.h +++ b/include/linux/khugepaged.h @@ -15,6 +15,14 @@ extern int __khugepaged_enter(struct mm_struct *mm); extern void __khugepaged_exit(struct mm_struct *mm); extern int khugepaged_enter_vma_merge(struct vm_area_struct *vma, unsigned long vm_flags); +#ifdef CONFIG_SHMEM +extern void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr); +#else +static inline void collapse_pte_mapped_thp(struct mm_struct *mm, + unsigned long addr) +{ +} +#endif #define khugepaged_enabled() \ (transparent_hugepage_flags & \ @@ -73,6 +81,10 @@ static inline int khugepaged_enter_vma_merge(struct vm_area_struct *vma, { return 0; } +static inline void collapse_pte_mapped_thp(struct mm_struct *mm, + unsigned long addr) +{ +} #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ #endif /* _LINUX_KHUGEPAGED_H */ diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 40c25ddf29e4..cea0fbf2d7b9 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -77,6 +77,8 @@ static __read_mostly DEFINE_HASHTABLE(mm_slots_hash, MM_SLOTS_HASH_BITS); static struct kmem_cache *mm_slot_cache __read_mostly; +#define MAX_PTE_MAPPED_THP 8 + /** * struct mm_slot - hash lookup from mm to mm_slot * @hash: hash collision list @@ -87,6 +89,10 @@ struct mm_slot { struct hlist_node hash; struct list_head mm_node; struct mm_struct *mm; + + /* pte-mapped THP in this mm */ + int nr_pte_mapped_thp; + unsigned long pte_mapped_thp[MAX_PTE_MAPPED_THP]; }; /** @@ -1254,6 +1260,159 @@ static void collect_mm_slot(struct mm_slot *mm_slot) } #if defined(CONFIG_SHMEM) && defined(CONFIG_TRANSPARENT_HUGE_PAGECACHE) +/* + * Notify khugepaged that given addr of the mm is pte-mapped THP. Then + * khugepaged should try to collapse the page table. + */ +static int khugepaged_add_pte_mapped_thp(struct mm_struct *mm, + unsigned long addr) +{ + struct mm_slot *mm_slot; + + VM_BUG_ON(addr & ~HPAGE_PMD_MASK); + + spin_lock(&khugepaged_mm_lock); + mm_slot = get_mm_slot(mm); + if (likely(mm_slot && mm_slot->nr_pte_mapped_thp < MAX_PTE_MAPPED_THP)) + mm_slot->pte_mapped_thp[mm_slot->nr_pte_mapped_thp++] = addr; + spin_unlock(&khugepaged_mm_lock); + return 0; +} + +/** + * Try to collapse a pte-mapped THP for mm at address haddr. + * + * This function checks whether all the PTEs in the PMD are pointing to the + * right THP. If so, retract the page table so the THP can refault in with + * as pmd-mapped. + */ +void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr) +{ + unsigned long haddr = addr & HPAGE_PMD_MASK; + struct vm_area_struct *vma = find_vma(mm, haddr); + struct page *hpage = NULL; + pte_t *start_pte, *pte; + pmd_t *pmd, _pmd; + spinlock_t *ptl; + int count = 0; + int i; + + if (!vma || !vma->vm_file || + vma->vm_start > haddr || vma->vm_end < haddr + HPAGE_PMD_SIZE) + return; + + /* + * This vm_flags may not have VM_HUGEPAGE if the page was not + * collapsed by this mm. But we can still collapse if the page is + * the valid THP. Add extra VM_HUGEPAGE so hugepage_vma_check() + * will not fail the vma for missing VM_HUGEPAGE + */ + if (!hugepage_vma_check(vma, vma->vm_flags | VM_HUGEPAGE)) + return; + + pmd = mm_find_pmd(mm, haddr); + if (!pmd) + return; + + start_pte = pte_offset_map_lock(mm, pmd, haddr, &ptl); + + /* step 1: check all mapped PTEs are to the right huge page */ + for (i = 0, addr = haddr, pte = start_pte; + i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE, pte++) { + struct page *page; + + /* empty pte, skip */ + if (pte_none(*pte)) + continue; + + /* page swapped out, abort */ + if (!pte_present(*pte)) + goto abort; + + page = vm_normal_page(vma, addr, *pte); + + if (!page || !PageCompound(page)) + goto abort; + + if (!hpage) { + hpage = compound_head(page); + /* + * The mapping of the THP should not change. + * + * Note that uprobe, debugger, or MAP_PRIVATE may + * change the page table, but the new page will + * not pass PageCompound() check. + */ + if (WARN_ON(hpage->mapping != vma->vm_file->f_mapping)) + goto abort; + } + + /* + * Confirm the page maps to the correct subpage. + * + * Note that uprobe, debugger, or MAP_PRIVATE may change + * the page table, but the new page will not pass + * PageCompound() check. + */ + if (WARN_ON(hpage + i != page)) + goto abort; + count++; + } + + /* step 2: adjust rmap */ + for (i = 0, addr = haddr, pte = start_pte; + i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE, pte++) { + struct page *page; + + if (pte_none(*pte)) + continue; + page = vm_normal_page(vma, addr, *pte); + page_remove_rmap(page, false); + } + + pte_unmap_unlock(start_pte, ptl); + + /* step 3: set proper refcount and mm_counters. */ + if (hpage) { + page_ref_sub(hpage, count); + add_mm_counter(vma->vm_mm, mm_counter_file(hpage), -count); + } + + /* step 4: collapse pmd */ + ptl = pmd_lock(vma->vm_mm, pmd); + _pmd = pmdp_collapse_flush(vma, addr, pmd); + spin_unlock(ptl); + mm_dec_nr_ptes(mm); + pte_free(mm, pmd_pgtable(_pmd)); + return; + +abort: + pte_unmap_unlock(start_pte, ptl); +} + +static int khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot) +{ + struct mm_struct *mm = mm_slot->mm; + int i; + + if (likely(mm_slot->nr_pte_mapped_thp == 0)) + return 0; + + if (!down_write_trylock(&mm->mmap_sem)) + return -EBUSY; + + if (unlikely(khugepaged_test_exit(mm))) + goto out; + + for (i = 0; i < mm_slot->nr_pte_mapped_thp; i++) + collapse_pte_mapped_thp(mm, mm_slot->pte_mapped_thp[i]); + +out: + mm_slot->nr_pte_mapped_thp = 0; + up_write(&mm->mmap_sem); + return 0; +} + static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) { struct vm_area_struct *vma; @@ -1287,7 +1446,8 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) up_write(&vma->vm_mm->mmap_sem); mm_dec_nr_ptes(vma->vm_mm); pte_free(vma->vm_mm, pmd_pgtable(_pmd)); - } + } else + khugepaged_add_pte_mapped_thp(vma->vm_mm, addr); } i_mmap_unlock_write(mapping); } @@ -1709,6 +1869,11 @@ static void khugepaged_scan_file(struct mm_struct *mm, { BUILD_BUG(); } + +static int khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot) +{ + return 0; +} #endif static unsigned int khugepaged_scan_mm_slot(unsigned int pages, @@ -1733,6 +1898,7 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, khugepaged_scan.mm_slot = mm_slot; } spin_unlock(&khugepaged_mm_lock); + khugepaged_collapse_pte_mapped_thps(mm_slot); mm = mm_slot->mm; /* -- 2.17.1
On 08/12, Kirill A. Shutemov wrote: > > On Mon, Aug 12, 2019 at 03:22:58PM +0200, Oleg Nesterov wrote: > > On 08/12, Kirill A. Shutemov wrote: > > > > > > On Fri, Aug 09, 2019 at 06:01:18PM +0000, Song Liu wrote: > > > > + if (pte_none(*pte) || !pte_present(*pte)) > > > > + continue; > > > > > > You don't need to check both. Present is never none. > > > > Agreed. > > > > Kirill, while you are here, shouldn't retract_page_tables() check > > vma->anon_vma (and probably do mm_find_pmd) under vm_mm->mmap_sem? > > > > Can't it race with, say, do_cow_fault? > > vma->anon_vma can race, but it doesn't matter. False-negative is fine. > It's attempt to avoid taking mmap_sem where it can be not productive. I guess I misunderstood the purpose of this check or your answer... Let me reword my question. Why can retract_page_tables() safely do pmdp_collapse_flush(vma) without additional checks similar to what collapse_pte_mapped_thp() does? I thought that retract_page_tables() checks vma->anon_vma to ensure that this vma doesn't have a cow'ed PageAnon() page. And I still can't understand why can't it race with __handle_mm_fault() paths. Suppose that shmem_file was mmaped with PROT_READ|WRITE, MAP_PRIVATE. To simplify, suppose that a non-THP page was already faulted in, pte_present() == T. Userspace writes to this page. Why __handle_mm_fault()->handle_pte_fault()->do_wp_page()->wp_page_copy() can not cow this page and update pte after the vma->anon_vma chech and before down_write_trylock(mmap_sem) ? Oleg.
On 08/13, Oleg Nesterov wrote: > > On 08/12, Kirill A. Shutemov wrote: > > > > On Mon, Aug 12, 2019 at 03:22:58PM +0200, Oleg Nesterov wrote: > > > On 08/12, Kirill A. Shutemov wrote: > > > > > > > > On Fri, Aug 09, 2019 at 06:01:18PM +0000, Song Liu wrote: > > > > > + if (pte_none(*pte) || !pte_present(*pte)) > > > > > + continue; > > > > > > > > You don't need to check both. Present is never none. > > > > > > Agreed. > > > > > > Kirill, while you are here, shouldn't retract_page_tables() check > > > vma->anon_vma (and probably do mm_find_pmd) under vm_mm->mmap_sem? > > > > > > Can't it race with, say, do_cow_fault? > > > > vma->anon_vma can race, but it doesn't matter. False-negative is fine. > > It's attempt to avoid taking mmap_sem where it can be not productive. > > I guess I misunderstood the purpose of this check or your answer... > > Let me reword my question. Why can retract_page_tables() safely do > pmdp_collapse_flush(vma) without additional checks similar to what > collapse_pte_mapped_thp() does? > > I thought that retract_page_tables() checks vma->anon_vma to ensure that > this vma doesn't have a cow'ed PageAnon() page. And I still can't understand > why can't it race with __handle_mm_fault() paths. > > Suppose that shmem_file was mmaped with PROT_READ|WRITE, MAP_PRIVATE. > To simplify, suppose that a non-THP page was already faulted in, > pte_present() == T. > > Userspace writes to this page. > > Why __handle_mm_fault()->handle_pte_fault()->do_wp_page()->wp_page_copy() > can not cow this page and update pte after the vma->anon_vma chech and > before down_write_trylock(mmap_sem) ? OK, probably this is impossible, collapse_shmem() does unmap_mapping_pages(), so handle_pte_fault() will call shmem_fault() which iiuc should block in find_lock_entry() because new_page is locked, and thus down_write_trylock() can't succeed. Nevermind, I am sure I missed something. Perhaps you can update the comments to make this more clear. Oleg.
Hi Oleg, > On Aug 12, 2019, at 2:04 PM, Song Liu <songliubraving@fb.com> wrote: > > > >> On Aug 12, 2019, at 7:40 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: >> >> On Mon, Aug 12, 2019 at 03:22:58PM +0200, Oleg Nesterov wrote: >>> On 08/12, Kirill A. Shutemov wrote: >>>> >>>> On Fri, Aug 09, 2019 at 06:01:18PM +0000, Song Liu wrote: >>>>> + if (pte_none(*pte) || !pte_present(*pte)) >>>>> + continue; >>>> >>>> You don't need to check both. Present is never none. >>> >>> Agreed. >>> >>> Kirill, while you are here, shouldn't retract_page_tables() check >>> vma->anon_vma (and probably do mm_find_pmd) under vm_mm->mmap_sem? >>> >>> Can't it race with, say, do_cow_fault? >> >> vma->anon_vma can race, but it doesn't matter. False-negative is fine. >> It's attempt to avoid taking mmap_sem where it can be not productive. >> >> mm_find_pmd() cannot race with do_cow_fault() since the page is locked. >> __do_fault() has to return locked page before we touch page tables. >> It is somewhat subtle, but I wanted to avoid taking mmap_sem where it is >> possible. >> >> -- >> Kirill A. Shutemov > > Updated version attached. > > > Besides feedbacks from Oleg and Kirill, I also revise the locking in > collapse_pte_mapped_thp(): use pte_offset_map_lock() for the two loops > to cover highmem. zap_pte_range() has similar use of the lock. > > This change is suggested by Johannes. > Do you have further comments for the version below? If not, could you please reply with your Acked-by or Reviewed-by? Thanks, Song > > ================ 8< ====================== > From 3d931bc4780abb6109fe478a4b1a0004ce81efe1 Mon Sep 17 00:00:00 2001 > From: Song Liu <songliubraving@fb.com> > Date: Sun, 28 Jul 2019 03:43:48 -0700 > Subject: [PATCH 5/6] khugepaged: enable collapse pmd for pte-mapped THP > [...]
On Tue, Aug 13, 2019 at 04:05:53PM +0200, Oleg Nesterov wrote: > On 08/13, Oleg Nesterov wrote: > > > > On 08/12, Kirill A. Shutemov wrote: > > > > > > On Mon, Aug 12, 2019 at 03:22:58PM +0200, Oleg Nesterov wrote: > > > > On 08/12, Kirill A. Shutemov wrote: > > > > > > > > > > On Fri, Aug 09, 2019 at 06:01:18PM +0000, Song Liu wrote: > > > > > > + if (pte_none(*pte) || !pte_present(*pte)) > > > > > > + continue; > > > > > > > > > > You don't need to check both. Present is never none. > > > > > > > > Agreed. > > > > > > > > Kirill, while you are here, shouldn't retract_page_tables() check > > > > vma->anon_vma (and probably do mm_find_pmd) under vm_mm->mmap_sem? > > > > > > > > Can't it race with, say, do_cow_fault? > > > > > > vma->anon_vma can race, but it doesn't matter. False-negative is fine. > > > It's attempt to avoid taking mmap_sem where it can be not productive. > > > > I guess I misunderstood the purpose of this check or your answer... > > > > Let me reword my question. Why can retract_page_tables() safely do > > pmdp_collapse_flush(vma) without additional checks similar to what > > collapse_pte_mapped_thp() does? > > > > I thought that retract_page_tables() checks vma->anon_vma to ensure that > > this vma doesn't have a cow'ed PageAnon() page. And I still can't understand > > why can't it race with __handle_mm_fault() paths. vma->anon_vma check is a cheap way to exclude MAP_PRIVATE mappings that got written from userspace. My thinking was that these VMAs are not worth investing down_write(mmap_sem) as PMD-mapping is likely to be split later. (It's totally made up reasoning, I don't have numbers to back it up). vma->anon_vma can be set up after the check but before taking mmap_sem. But page lock would prevent establishing any new ptes of the page, so we are safe. An alternative would be drop the check, but check that page table is clear before calling pmdp_collapse_flush() under ptl. It has higher chance to recover THP for the VMA, but has higher cost too. I don't know which way is better, so I've chosen which is easier to implement. > > > > Suppose that shmem_file was mmaped with PROT_READ|WRITE, MAP_PRIVATE. > > To simplify, suppose that a non-THP page was already faulted in, > > pte_present() == T. > > > > Userspace writes to this page. > > > > Why __handle_mm_fault()->handle_pte_fault()->do_wp_page()->wp_page_copy() > > can not cow this page and update pte after the vma->anon_vma chech and > > before down_write_trylock(mmap_sem) ? > > OK, probably this is impossible, collapse_shmem() does unmap_mapping_pages(), > so handle_pte_fault() will call shmem_fault() which iiuc should block in > find_lock_entry() because new_page is locked, and thus down_write_trylock() > can't succeed. You've got it right. > Nevermind, I am sure I missed something. Perhaps you can update the comments > to make this more clear. Let me see first that my explanation makes sense :P
On 08/13, Kirill A. Shutemov wrote: > > On Tue, Aug 13, 2019 at 04:05:53PM +0200, Oleg Nesterov wrote: > > > > > > I thought that retract_page_tables() checks vma->anon_vma to ensure that > > > this vma doesn't have a cow'ed PageAnon() page. And I still can't understand > > > why can't it race with __handle_mm_fault() paths. > > vma->anon_vma check is a cheap way to exclude MAP_PRIVATE mappings that > got written from userspace. Yes, and this is how I understood it from the very beginning, but then I was confused. > vma->anon_vma can be set up after the check but before taking mmap_sem. > But page lock would prevent establishing any new ptes of the page, so we > are safe. And this is what was not clear to me until I noticed unmap_mapping_pages() in collapse_shmem(). Plus I was confused by the comment above down_write_trylock(mmap_sem). To me it looks as if we _could_ do down_write(), but we do not want to disturb the system. But iiuc we simply can't do down_write(), exactly because handle_mm_fault() can wait for page lock with mmap_sem held. > > > Suppose that shmem_file was mmaped with PROT_READ|WRITE, MAP_PRIVATE. > > > To simplify, suppose that a non-THP page was already faulted in, > > > pte_present() == T. > > > > > > Userspace writes to this page. > > > > > > Why __handle_mm_fault()->handle_pte_fault()->do_wp_page()->wp_page_copy() > > > can not cow this page and update pte after the vma->anon_vma chech and > > > before down_write_trylock(mmap_sem) ? > > > > OK, probably this is impossible, collapse_shmem() does unmap_mapping_pages(), > > so handle_pte_fault() will call shmem_fault() which iiuc should block in > > find_lock_entry() because new_page is locked, and thus down_write_trylock() > > can't succeed. > > You've got it right. Great, thanks. > > Nevermind, I am sure I missed something. Perhaps you can update the comments > > to make this more clear. > > Let me see first that my explanation makes sense :P It does ;) Oleg.
Hi Song, sorry, I forgot to reply to this email, On 08/13, Song Liu wrote: > > Do you have further comments for the version below? If not, could you > please reply with your Acked-by or Reviewed-by? I see nothing wrong in the last series, no objections from me. I don't think I can't ack the changes in this area, but feel free to add my Reviewed-by. Oleg.
> On Aug 15, 2019, at 3:16 AM, Oleg Nesterov <oleg@redhat.com> wrote: > > Hi Song, > > sorry, I forgot to reply to this email, > > On 08/13, Song Liu wrote: >> >> Do you have further comments for the version below? If not, could you >> please reply with your Acked-by or Reviewed-by? > > I see nothing wrong in the last series, no objections from me. > > I don't think I can't ack the changes in this area, but feel free to > add my Reviewed-by. > > Oleg. > Thanks Oleg! Will resend latest version shortly. Song
On Tue, Aug 13, 2019 at 06:24:51PM +0200, Oleg Nesterov wrote: > > Let me see first that my explanation makes sense :P > > It does ;) Does it look fine to you? It's on top of Song's patchset. From 58834d6c1e63321af742b208558a6b5cb86fc7ec Mon Sep 17 00:00:00 2001 From: "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> Date: Fri, 16 Aug 2019 17:50:41 +0300 Subject: [PATCH] khugepaged: Add comments for retract_page_tables() Oleg Nesterov pointed that logic behind checks in retract_page_tables() are not obvious. Add comments to clarify the reasoning for the checks and why they are safe. Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> --- mm/khugepaged.c | 28 +++++++++++++++++++++++----- 1 file changed, 23 insertions(+), 5 deletions(-) diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 5c0a5f0826b2..00cec6a127aa 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -1421,7 +1421,22 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) i_mmap_lock_write(mapping); vma_interval_tree_foreach(vma, &mapping->i_mmap, pgoff, pgoff) { - /* probably overkill */ + /* + * Check vma->anon_vma to exclude MAP_PRIVATE mappings that + * got written to. These VMAs are likely not worth investing + * down_write(mmap_sem) as PMD-mapping is likely to be split + * later. + * + * Not that vma->anon_vma check is racy: it can be set up after + * the check but before we took mmap_sem by the fault path. + * But page lock would prevent establishing any new ptes of the + * page, so we are safe. + * + * An alternative would be drop the check, but check that page + * table is clear before calling pmdp_collapse_flush() under + * ptl. It has higher chance to recover THP for the VMA, but + * has higher cost too. + */ if (vma->anon_vma) continue; addr = vma->vm_start + ((pgoff - vma->vm_pgoff) << PAGE_SHIFT); @@ -1434,9 +1449,10 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) continue; /* * We need exclusive mmap_sem to retract page table. - * If trylock fails we would end up with pte-mapped THP after - * re-fault. Not ideal, but it's more important to not disturb - * the system too much. + * + * We use trylock due to lock inversion: we need to acquire + * mmap_sem while holding page lock. Fault path does it in + * reverse order. Trylock is a way to avoid deadlock. */ if (down_write_trylock(&vma->vm_mm->mmap_sem)) { spinlock_t *ptl = pmd_lock(vma->vm_mm, pmd); @@ -1446,8 +1462,10 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) up_write(&vma->vm_mm->mmap_sem); mm_dec_nr_ptes(vma->vm_mm); pte_free(vma->vm_mm, pmd_pgtable(_pmd)); - } else + } else { + /* Try again later */ khugepaged_add_pte_mapped_thp(vma->vm_mm, addr); + } } i_mmap_unlock_write(mapping); }
diff --git a/include/linux/khugepaged.h b/include/linux/khugepaged.h index 082d1d2a5216..bc45ea1efbf7 100644 --- a/include/linux/khugepaged.h +++ b/include/linux/khugepaged.h @@ -15,6 +15,14 @@ extern int __khugepaged_enter(struct mm_struct *mm); extern void __khugepaged_exit(struct mm_struct *mm); extern int khugepaged_enter_vma_merge(struct vm_area_struct *vma, unsigned long vm_flags); +#ifdef CONFIG_SHMEM +extern void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr); +#else +static inline void collapse_pte_mapped_thp(struct mm_struct *mm, + unsigned long addr) +{ +} +#endif #define khugepaged_enabled() \ (transparent_hugepage_flags & \ @@ -73,6 +81,10 @@ static inline int khugepaged_enter_vma_merge(struct vm_area_struct *vma, { return 0; } +static inline void collapse_pte_mapped_thp(struct mm_struct *mm, + unsigned long addr) +{ +} #endif /* CONFIG_TRANSPARENT_HUGEPAGE */ #endif /* _LINUX_KHUGEPAGED_H */ diff --git a/mm/khugepaged.c b/mm/khugepaged.c index 40c25ddf29e4..208ea1ce204a 100644 --- a/mm/khugepaged.c +++ b/mm/khugepaged.c @@ -77,6 +77,8 @@ static __read_mostly DEFINE_HASHTABLE(mm_slots_hash, MM_SLOTS_HASH_BITS); static struct kmem_cache *mm_slot_cache __read_mostly; +#define MAX_PTE_MAPPED_THP 8 + /** * struct mm_slot - hash lookup from mm to mm_slot * @hash: hash collision list @@ -87,6 +89,10 @@ struct mm_slot { struct hlist_node hash; struct list_head mm_node; struct mm_struct *mm; + + /* pte-mapped THP in this mm */ + int nr_pte_mapped_thp; + unsigned long pte_mapped_thp[MAX_PTE_MAPPED_THP]; }; /** @@ -1254,6 +1260,131 @@ static void collect_mm_slot(struct mm_slot *mm_slot) } #if defined(CONFIG_SHMEM) && defined(CONFIG_TRANSPARENT_HUGE_PAGECACHE) +/* + * Notify khugepaged that given addr of the mm is pte-mapped THP. Then + * khugepaged should try to collapse the page table. + */ +static int khugepaged_add_pte_mapped_thp(struct mm_struct *mm, + unsigned long addr) +{ + struct mm_slot *mm_slot; + + VM_BUG_ON(addr & ~HPAGE_PMD_MASK); + + spin_lock(&khugepaged_mm_lock); + mm_slot = get_mm_slot(mm); + if (likely(mm_slot && mm_slot->nr_pte_mapped_thp < MAX_PTE_MAPPED_THP)) + mm_slot->pte_mapped_thp[mm_slot->nr_pte_mapped_thp++] = addr; + spin_unlock(&khugepaged_mm_lock); + return 0; +} + +/** + * Try to collapse a pte-mapped THP for mm at address haddr. + * + * This function checks whether all the PTEs in the PMD are pointing to the + * right THP. If so, retract the page table so the THP can refault in with + * as pmd-mapped. + */ +void collapse_pte_mapped_thp(struct mm_struct *mm, unsigned long addr) +{ + unsigned long haddr = addr & HPAGE_PMD_MASK; + struct vm_area_struct *vma = find_vma(mm, haddr); + struct page *hpage = NULL; + pmd_t *pmd, _pmd; + spinlock_t *ptl; + int count = 0; + int i; + + if (!vma || !vma->vm_file || + vma->vm_start > haddr || vma->vm_end < haddr + HPAGE_PMD_SIZE) + return; + + /* + * This vm_flags may not have VM_HUGEPAGE if the page was not + * collapsed by this mm. But we can still collapse if the page is + * the valid THP. Add extra VM_HUGEPAGE so hugepage_vma_check() + * will not fail the vma for missing VM_HUGEPAGE + */ + if (!hugepage_vma_check(vma, vma->vm_flags | VM_HUGEPAGE)) + return; + + pmd = mm_find_pmd(mm, haddr); + if (!pmd) + return; + + /* step 1: check all mapped PTEs are to the right huge page */ + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { + pte_t *pte = pte_offset_map(pmd, addr); + struct page *page; + + if (pte_none(*pte)) + continue; + + page = vm_normal_page(vma, addr, *pte); + + if (!page || !PageCompound(page)) + return; + + if (!hpage) { + hpage = compound_head(page); + if (hpage->mapping != vma->vm_file->f_mapping) + return; + } + + if (hpage + i != page) + return; + count++; + } + + /* step 2: adjust rmap */ + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) { + pte_t *pte = pte_offset_map(pmd, addr); + struct page *page; + + if (pte_none(*pte)) + continue; + page = vm_normal_page(vma, addr, *pte); + page_remove_rmap(page, false); + } + + /* step 3: set proper refcount and mm_counters. */ + if (hpage) { + page_ref_sub(hpage, count); + add_mm_counter(vma->vm_mm, mm_counter_file(hpage), -count); + } + + /* step 4: collapse pmd */ + ptl = pmd_lock(vma->vm_mm, pmd); + _pmd = pmdp_collapse_flush(vma, addr, pmd); + spin_unlock(ptl); + mm_dec_nr_ptes(mm); + pte_free(mm, pmd_pgtable(_pmd)); +} + +static int khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot) +{ + struct mm_struct *mm = mm_slot->mm; + int i; + + if (likely(mm_slot->nr_pte_mapped_thp == 0)) + return 0; + + if (!down_write_trylock(&mm->mmap_sem)) + return -EBUSY; + + if (unlikely(khugepaged_test_exit(mm))) + goto out; + + for (i = 0; i < mm_slot->nr_pte_mapped_thp; i++) + collapse_pte_mapped_thp(mm, mm_slot->pte_mapped_thp[i]); + +out: + mm_slot->nr_pte_mapped_thp = 0; + up_write(&mm->mmap_sem); + return 0; +} + static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) { struct vm_area_struct *vma; @@ -1287,7 +1418,8 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff) up_write(&vma->vm_mm->mmap_sem); mm_dec_nr_ptes(vma->vm_mm); pte_free(vma->vm_mm, pmd_pgtable(_pmd)); - } + } else + khugepaged_add_pte_mapped_thp(vma->vm_mm, addr); } i_mmap_unlock_write(mapping); } @@ -1709,6 +1841,11 @@ static void khugepaged_scan_file(struct mm_struct *mm, { BUILD_BUG(); } + +static int khugepaged_collapse_pte_mapped_thps(struct mm_slot *mm_slot) +{ + return 0; +} #endif static unsigned int khugepaged_scan_mm_slot(unsigned int pages, @@ -1733,6 +1870,7 @@ static unsigned int khugepaged_scan_mm_slot(unsigned int pages, khugepaged_scan.mm_slot = mm_slot; } spin_unlock(&khugepaged_mm_lock); + khugepaged_collapse_pte_mapped_thps(mm_slot); mm = mm_slot->mm; /*