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

[Kirill Tkhai <ktkhai@virtuozzo.com>]

On 30.12.2018 00:40, Andrew Morton wrote:
> On Thu, 13 Dec 2018 18:29:08 +0300 Kirill Tkhai <ktkhai@virtuozzo.com> 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.
>>
>> ...
>>
>> +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);
>> +	page->index = linear_page_index(vma, address);
>> +	page_ref_unfreeze(page, 1);
>> +
>> +	return true;
>> +}
> 
> Can we avoid those BUG_ON()s?
> 
> Something like this:
> 
> --- a/mm/ksm.c~mm-reuse-only-pte-mapped-ksm-page-in-do_wp_page-fix
> +++ a/mm/ksm.c
> @@ -2649,9 +2649,14 @@ 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);
> +#ifdef CONFIG_DEBUG_VM
> +	if (WARN_ON(is_zero_pfn(page_to_pfn(page))) ||
> +			WARN_ON(!page_mapped(page)) ||
> +			WARN_ON(!PageLocked(page))) {
> +		dump_page(page, "reuse_ksm_page");
> +		return false;
> +	}
> +#endif

Looks good!
  
>  	if (PageSwapCache(page) || !page_stable_node(page))
>  		return false;
> 
> We don't have a VM_WARN_ON_PAGE() and we can't provide one because the
> VM_foo() macros don't return a value.  It's irritating and I keep
> forgetting why we ended up doing them this way.
Thanks!