| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <77DBF8A2-DF3D-4635-A5E6-66D80A8BFA50@fb.com>
Date: Tue, 25 Jun 2019 12:33:03 +0000
From: Song Liu <songliubraving@...com>
To: "Kirill A. Shutemov" <kirill@...temov.name>
CC: LKML <linux-kernel@...r.kernel.org>, Linux-MM <linux-mm@...ck.org>,
"matthew.wilcox@...cle.com" <matthew.wilcox@...cle.com>,
"kirill.shutemov@...ux.intel.com" <kirill.shutemov@...ux.intel.com>,
"peterz@...radead.org" <peterz@...radead.org>,
"oleg@...hat.com" <oleg@...hat.com>,
"rostedt@...dmis.org" <rostedt@...dmis.org>,
Kernel Team <Kernel-team@...com>,
"william.kucharski@...cle.com" <william.kucharski@...cle.com>
Subject: Re: [PATCH v6 5/6] khugepaged: enable collapse pmd for pte-mapped THP
> On Jun 25, 2019, at 3:34 AM, Kirill A. Shutemov <kirill@...temov.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 <kirill@...temov.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 <songliubraving@...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.
Thanks,
Song
Powered by blists - more mailing lists