On Mon, Jun 24, 2019 at 02:25:42PM +0000, Song Liu wrote:
>
>
> > On Jun 24, 2019, at 6:19 AM, Kirill A. Shutemov <kirill@xxxxxxxxxxxxx> wrote:
> >
> > On Sat, Jun 22, 2019 at 10:48:28PM -0700, Song Liu wrote:
> >> 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 retract_page_tables().
> >>
> >> A new flag AS_COLLAPSE_PMD is introduced to show the address_space may
> >> contain pte-mapped THPs. When khugepaged fails to trylock the mmap_sem,
> >> it sets AS_COLLAPSE_PMD. Then, at a later time, khugepaged will retry
> >> compound pages in this address_space.
> >>
> >> Since collapse may happen at an later time, some pages may already fault
> >> in. To handle these pages properly, it is necessary to prepare the pmd
> >> before collapsing. prepare_pmd_for_collapse() is introduced to prepare
> >> the pmd by removing rmap, adjusting refcount and mm_counter.
> >>
> >> prepare_pmd_for_collapse() 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, so we fall back.
> >>
> >> Signed-off-by: Song Liu <songliubraving@xxxxxx>
> >> ---
> >> include/linux/pagemap.h | 1 +
> >> mm/khugepaged.c | 69 +++++++++++++++++++++++++++++++++++------
> >> 2 files changed, 60 insertions(+), 10 deletions(-)
> >>
> >> diff --git a/include/linux/pagemap.h b/include/linux/pagemap.h
> >> index 9ec3544baee2..eac881de2a46 100644
> >> --- a/include/linux/pagemap.h
> >> +++ b/include/linux/pagemap.h
> >> @@ -29,6 +29,7 @@ enum mapping_flags {
> >> AS_EXITING = 4, /* final truncate in progress */
> >> /* writeback related tags are not used */
> >> AS_NO_WRITEBACK_TAGS = 5,
> >> + AS_COLLAPSE_PMD = 6, /* try collapse pmd for THP */
> >> };
> >>
> >> /**
> >> diff --git a/mm/khugepaged.c b/mm/khugepaged.c
> >> index a4f90a1b06f5..9b980327fd9b 100644
> >> --- a/mm/khugepaged.c
> >> +++ b/mm/khugepaged.c
> >> @@ -1254,7 +1254,47 @@ static void collect_mm_slot(struct mm_slot *mm_slot)
> >> }
> >>
> >> #if defined(CONFIG_SHMEM) && defined(CONFIG_TRANSPARENT_HUGE_PAGECACHE)
> >> -static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
> >> +
> >> +/* return whether the pmd is ready for collapse */
> >> +bool prepare_pmd_for_collapse(struct vm_area_struct *vma, pgoff_t pgoff,
> >> + struct page *hpage, pmd_t *pmd)
> >> +{
> >> + unsigned long haddr = page_address_in_vma(hpage, vma);
> >> + unsigned long addr;
> >> + int i, count = 0;
> >> +
> >> + /* step 1: check all mapped PTEs are to this huge page */
> >> + for (i = 0, addr = haddr; i < HPAGE_PMD_NR; i++, addr += PAGE_SIZE) {
> >> + pte_t *pte = pte_offset_map(pmd, addr);
> >> +
> >> + if (pte_none(*pte))
> >> + continue;
> >> +
> >> + if (hpage + i != vm_normal_page(vma, addr, *pte))
> >> + return false;
> >> + 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. */
> >> + page_ref_sub(hpage, count);
> >> + add_mm_counter(vma->vm_mm, mm_counter_file(hpage), -count);
> >> + return true;
> >> +}
> >> +
> >> +extern pid_t sysctl_dump_pt_pid;
> >> +static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff,
> >> + struct page *hpage)
> >> {
> >> struct vm_area_struct *vma;
> >> unsigned long addr;
> >> @@ -1273,21 +1313,21 @@ static void retract_page_tables(struct address_space *mapping, pgoff_t pgoff)
> >> pmd = mm_find_pmd(vma->vm_mm, addr);
> >> if (!pmd)
> >> 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.
> >> - */
> >> if (down_write_trylock(&vma->vm_mm->mmap_sem)) {
> >> spinlock_t *ptl = pmd_lock(vma->vm_mm, pmd);
> >> - /* assume page table is clear */
> >> +
> >> + if (!prepare_pmd_for_collapse(vma, pgoff, hpage, pmd)) {
> >> + spin_unlock(ptl);
> >> + up_write(&vma->vm_mm->mmap_sem);
> >> + continue;
> >> + }
> >> _pmd = pmdp_collapse_flush(vma, addr, pmd);
> >> spin_unlock(ptl);
> >> up_write(&vma->vm_mm->mmap_sem);
> >> mm_dec_nr_ptes(vma->vm_mm);
> >> pte_free(vma->vm_mm, pmd_pgtable(_pmd));
> >> - }
> >> + } else
> >> + set_bit(AS_COLLAPSE_PMD, &mapping->flags);
> >> }
> >> i_mmap_unlock_write(mapping);
> >> }
> >> @@ -1561,7 +1601,7 @@ static void collapse_file(struct mm_struct *mm,
> >> /*
> >> * Remove pte page tables, so we can re-fault the page as huge.
> >> */
> >> - retract_page_tables(mapping, start);
> >> + retract_page_tables(mapping, start, new_page);
> >> *hpage = NULL;
> >>
> >> khugepaged_pages_collapsed++;
> >> @@ -1622,6 +1662,7 @@ static void khugepaged_scan_file(struct mm_struct *mm,
> >> int present, swap;
> >> int node = NUMA_NO_NODE;
> >> int result = SCAN_SUCCEED;
> >> + bool collapse_pmd = false;
> >>
> >> present = 0;
> >> swap = 0;
> >> @@ -1640,6 +1681,14 @@ static void khugepaged_scan_file(struct mm_struct *mm,
> >> }
> >>
> >> if (PageTransCompound(page)) {
> >> + if (collapse_pmd ||
> >> + test_and_clear_bit(AS_COLLAPSE_PMD,
> >> + &mapping->flags)) {
> >
> > Who said it's the only PMD range that's subject to collapse? The bit has
> > to be per-PMD, not per-mapping.
>
> I didn't assume this is the only PMD range that subject to collapse.
> So once we found AS_COLLAPSE_PMD, it will continue scan the whole mapping:
> retract_page_tables(), then continue.
I still don't get it.
Assume we have two ranges that subject to collapse. khugepaged_scan_file()
sees and clears AS_COLLAPSE_PMD. Tries to collapse the first range, fails
and set the bit again. khugepaged_scan_file() sees the second range,
clears the bit, but this time collapse is successful: the bit is still not
set, but it should be.
> > I beleive we can store the bit in struct page of PTE page table, clearing
> > it if we've mapped anyting that doesn't belong to there from fault path.
> >
> > And in general this calls for more substantial re-design for khugepaged:
> > we might want to split if into two different kernel threads. One works on
> > collapsing small pages into compound and the other changes virtual address
> > space to map the page as PMD.
>
> I had almost same idea of splitting into two threads. Or ask one thread to
> scan two lists: one for pages to collapse, the other for page tables to
> map as PMD. However, that does need substantial re-design.
>
> On the other hand, this patch is much simpler and does the work fine. It is
> not optimal, as it scans the whole mapping for opportunity in just one PMD
> range. But the overhead is not in any hot path, so it just works. The extra
> double check in prepare_pmd_for_collapse() makes sure it will not collapse
> wrong pages into pmd mapped.
>
> Overall, I believe this is an accurate solution for the problem. We can
> further improve it. But I really hope to do the improvements after these
> two patchsets get in.
I don't think AS_COLLAPSE_PMD adds much value. I don't see it relieably
serving the purpose.
--
Kirill A. Shutemov
