Re: [PATCH v12 5/6] khugepaged: enable collapse pmd for pte-mapped THP

[Song Liu <songliubraving@fb.com>]

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
> 

[...]