> On Jun 25, 2019, at 7:12 AM, Kirill A. Shutemov <kir...@shutemov.name> wrote:
>
> On Tue, Jun 25, 2019 at 12:33:03PM +0000, Song Liu wrote:
>>
>>
>>> On Jun 25, 2019, at 3:34 AM, Kirill A. Shutemov <kir...@shutemov.name>
>>> wrote:
>>>
>>> On Mon, Jun 24, 2019 at 02:25:42PM +0000, Song Liu wrote:
>>>>
>>>>
>>>>> On Jun 24, 2019, at 6:19 AM, Kirill A. Shutemov <kir...@shutemov.name>
>>>>> 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 <songliubrav...@fb.com>
>>>>>> ---
>>>>>> 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.
>>
>> Yeah, you are right. Current logic only covers multiple THPs within single
>> call of khugepaged_scan_file(). I missed the case you just described.
>>
>> What do you think about the first 4 patches of set and the other set? If
>> these patches look good, how about we get them in first? I will work on
>> proper fix (or re-design) for 5/6 and 6/6 in the meanwhile.
>
> You can use
>
> Acked-by: Kirill A. Shutemov <kirill.shute...@linux.intel.com>
>
> for the first 4 patches.
Thanks Kirill!
Song
