Re: [PATCH] mm: Reuse only-pte-mapped KSM page in do_wp_page()

[Kirill Tkhai]

On 14.12.2018 21:06, Yang Shi wrote:
> 
> 
> On 12/14/18 1:26 AM, Kirill Tkhai wrote:
>> On 13.12.2018 22:15, Yang Shi wrote:
>>>
>>> On 12/13/18 7:29 AM, Kirill Tkhai wrote:
>>>> This patch adds an optimization for KSM pages almost
>>>> in the same way, that we have for ordinary anonymous
>>>> pages. If there is a write fault in a page, which is
>>>> mapped to an only pte, and it is not related to swap
>>>> cache; the page may be reused without copying its
>>>> content.
>>>>
>>>> [Note, that we do not consider PageSwapCache() pages
>>>> ÂÂ at least for now, since we don't want to complicate
>>>> ÂÂ __get_ksm_page(), which has nice optimization based
>>>> ÂÂ on this (for the migration case). Currenly it is
>>>> ÂÂ spinning on PageSwapCache() pages, waiting for when
>>>> ÂÂ they have unfreezed counters (i.e., for the migration
>>>> ÂÂ finish). But we don't want to make it also spinning
>>>> ÂÂ on swap cache pages, which we try to reuse, since
>>>> ÂÂ there is not a very high probability to reuse them.
>>>> ÂÂ So, for now we do not consider PageSwapCache() pages
>>>> ÂÂ at all.]
>>>>
>>>> So, in reuse_ksm_page() we check for 1)PageSwapCache()
>>>> and 2)page_stable_node(), to skip a page, which KSM
>>>> is currently trying to link to stable tree. Then we
>>>> do page_ref_freeze() to prohibit KSM to merge one more
>>>> page into the page, we are reusing. After that, nobody
>>>> can refer to the reusing page: KSM skips !PageSwapCache()
>>>> pages with zero refcount; and the protection against
>>>> of all other participants is the same as for reused
>>>> ordinary anon pages pte lock, page lock and mmap_sem.
>>>>
>>>> Signed-off-by: Kirill Tkhai <ktkhai@xxxxxxxxxxxxx>
>>>> ---
>>>> ÂÂ include/linux/ksm.h |ÂÂÂ 7 +++++++
>>>> ÂÂ mm/ksm.cÂÂÂÂÂÂÂÂÂÂÂ |ÂÂ 25 +++++++++++++++++++++++--
>>>> ÂÂ mm/memory.cÂÂÂÂÂÂÂÂ |ÂÂ 16 ++++++++++++++--
>>>> ÂÂ 3 files changed, 44 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/include/linux/ksm.h b/include/linux/ksm.h
>>>> index 161e8164abcf..e48b1e453ff5 100644
>>>> --- a/include/linux/ksm.h
>>>> +++ b/include/linux/ksm.h
>>>> @@ -53,6 +53,8 @@ struct page *ksm_might_need_to_copy(struct page *page,
>>>> ÂÂ Â void rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc);
>>>> ÂÂ void ksm_migrate_page(struct page *newpage, struct page *oldpage);
>>>> +bool reuse_ksm_page(struct page *page,
>>>> +ÂÂÂÂÂÂÂÂÂÂÂ struct vm_area_struct *vma, unsigned long address);
>>>>   #else /* !CONFIG_KSM */
>>>> ÂÂ @@ -86,6 +88,11 @@ static inline void rmap_walk_ksm(struct page *page,
>>>> ÂÂ static inline void ksm_migrate_page(struct page *newpage, struct page *oldpage)
>>>> ÂÂ {
>>>> ÂÂ }
>>>> +static inline bool reuse_ksm_page(struct page *page,
>>>> +ÂÂÂÂÂÂÂÂÂÂÂ struct vm_area_struct *vma, unsigned long address)
>>>> +{
>>>> +ÂÂÂ return false;
>>>> +}
>>>> ÂÂ #endif /* CONFIG_MMU */
>>>> ÂÂ #endif /* !CONFIG_KSM */
>>>> ÂÂ diff --git a/mm/ksm.c b/mm/ksm.c
>>>> index 383f961e577a..fbd14264d784 100644
>>>> --- a/mm/ksm.c
>>>> +++ b/mm/ksm.c
>>>> @@ -707,8 +707,9 @@ static struct page *__get_ksm_page(struct stable_node *stable_node,
>>>> ÂÂÂÂÂÂÂ * case this node is no longer referenced, and should be freed;
>>>> ÂÂÂÂÂÂÂ * however, it might mean that the page is under page_ref_freeze().
>>>> ÂÂÂÂÂÂÂ * The __remove_mapping() case is easy, again the node is now stale;
>>>> -ÂÂÂÂ * but if page is swapcache in migrate_page_move_mapping(), it might
>>>> -ÂÂÂÂ * still be our page, in which case it's essential to keep the node.
>>>> +ÂÂÂÂ * the same is in reuse_ksm_page() case; but if page is swapcache
>>>> +ÂÂÂÂ * in migrate_page_move_mapping(), it might still be our page,
>>>> +ÂÂÂÂ * in which case it's essential to keep the node.
>>>> ÂÂÂÂÂÂÂ */
>>>> ÂÂÂÂÂÂ while (!get_page_unless_zero(page)) {
>>>> ÂÂÂÂÂÂÂÂÂÂ /*
>>>> @@ -2666,6 +2667,26 @@ void rmap_walk_ksm(struct page *page, struct rmap_walk_control *rwc)
>>>> ÂÂÂÂÂÂÂÂÂÂ goto again;
>>>> ÂÂ }
>>>> ÂÂ +bool reuse_ksm_page(struct page *page,
>>>> +ÂÂÂÂÂÂÂÂÂÂÂ struct vm_area_struct *vma,
>>>> +ÂÂÂÂÂÂÂÂÂÂÂ unsigned long address)
>>>> +{
>>>> +ÂÂÂ VM_BUG_ON_PAGE(is_zero_pfn(page_to_pfn(page)), page);
>>>> +ÂÂÂ VM_BUG_ON_PAGE(!page_mapped(page), page);
>>>> +ÂÂÂ VM_BUG_ON_PAGE(!PageLocked(page), page);
>>>> +
>>>> +ÂÂÂ if (PageSwapCache(page) || !page_stable_node(page))
>>>> +ÂÂÂÂÂÂÂ return false;
>>>> +ÂÂÂ /* Prohibit parallel get_ksm_page() */
>>>> +ÂÂÂ if (!page_ref_freeze(page, 1))
>>>> +ÂÂÂÂÂÂÂ return false;
>>>> +
>>>> +ÂÂÂ page_move_anon_rmap(page, vma);
>>> Once the mapping is changed, it is not KSM mapping anymore. It looks later get_ksm_page() would always fail on this page. Is this expected?
>> Yes, this is the thing that the patch makes. Let's look at the actions,
>> we have without the patch, when there is a writing to an only-pte-mapped
>> KSM page.
>>
>> We enter to do_wp_page() with page_count() == 1, since KSM page is mapped
>> in only pte (and we do not get extra reference to a page, when we add it
>> to KSM stable tree). Then:
>>
>> ÂÂ do_wp_page()
>> ÂÂÂÂ get_page(vmf->page) <- page_count() is 2
>> ÂÂÂÂ wp_page_copy()
>> ÂÂÂÂÂÂ ..
>> ÂÂÂÂÂÂ cow_user_page() /* Copy user page to a new one */
>> ÂÂÂÂÂÂ ..
>> ÂÂÂÂÂÂ put_page(vmf->page) <- page_count() is 1
>> ÂÂÂÂÂÂ put_page(vmf->page) <- page_count() is 0
>>
>> Second put_page() frees the page (and also zeroes page->mapping),
>> and since that it's not a PageKsm() page anymore. Further
>> __get_ksm_page() calls will fail on this page (since the mapping
>> was zeroed), and its node will be unlinked from ksm stable tree:
>>
>> __get_ksm_page()
>> {
>> ÂÂÂÂ/* page->mapping == NULL, expected_mapping != NULL */
>> ÂÂÂÂif (READ_ONCE(page->mapping) != expected_mapping)
>> ÂÂÂÂÂÂÂ goto stale;
>> ÂÂÂÂ.......
>> stale:
>> ÂÂÂÂremove_node_from_stable_tree(stable_node);
>> }
>>
>>
>> The patch optimizes do_wp_page(), and makes it to avoid the copying
>> (like we have for ordinary anon pages). Since KSM page is freed anyway,
>> after we dropped the last reference to it; we reuse it instead of this.
>> So, the thing will now work in this way:
>>
>> do_wp_page()
>> ÂÂ lock_page(vmf->page)
>> ÂÂ reuse_ksm_page()
>> ÂÂÂÂ check PageSwapCache() and page_stable_node()
>> ÂÂÂÂ page_ref_freeze(page, 1) <- Freeze the page to make parallel
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ __get_ksm_page() (if any) waiting
>> ÂÂÂÂ page_move_anon_rmap()ÂÂÂ <- Write new mapping, so __get_ksm_page()
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ sees this is not a KSM page anymore,
>> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ and it removes stable node.
>>
>> So, the result is the same, but after the patch we achieve it faster :)
>>
>> Also, note, that in the most probably case, do_wp_page() does not cross
>> with __get_ksm_page() (the race window is very small; __get_ksm_page()
>> is spinning, only when reuse_ksm_page() is between page_ref_freeze()
>> and page_move_anon_rmap(), which are on neighboring lines).
>>
>> So, this is the idea. Please, let me know in case of something is unclear
>> for you.
> 
> Thanks for elaborating this. It sounds reasonable. You can add Reviewed-by: Yang Shi <yang.shi@xxxxxxxxxxxxxxxxx>

Thanks!

Kirill