Re: [PATCH v3 3/4] mm: Change zap_details.zap_mapping into even_cows

[David Hildenbrand]

On 28.01.22 10:17, Peter Xu wrote:
> On Fri, Jan 28, 2022 at 10:03:20AM +0100, David Hildenbrand wrote:
>> On 28.01.22 05:54, Peter Xu wrote:
>>> Currently we have a zap_mapping pointer maintained in zap_details, when it is
>>> specified we only want to zap the pages that has the same mapping with what the
>>> caller has specified.
>>>
>>> But what we want to do is actually simpler: we want to skip zapping
>>> private (COW-ed) pages in some cases.  We can refer to unmap_mapping_pages()
>>> callers where we could have passed in different even_cows values.  The other
>>> user is unmap_mapping_folio() where we always want to skip private pages.
>>>
>>> According to Hugh, we used a mapping pointer for historical reason, as
>>> explained here:
>>>
>>>   https://lore.kernel.org/lkml/391aa58d-ce84-9d4-d68d-d98a9c533255@xxxxxxxxxx/
>>>
>>> Quotting partly from Hugh:
>>
>> s/Quotting/Quoting/
> 
> Will fix.
> 
>>
>>>
>>>   Which raises the question again of why I did not just use a boolean flag
>>>   there originally: aah, I think I've found why.  In those days there was a
>>>   horrible "optimization", for better performance on some benchmark I guess,
>>>   which when you read from /dev/zero into a private mapping, would map the zero
>>>   page there (look up read_zero_pagealigned() and zeromap_page_range() if you
>>>   dare).  So there was another category of page to be skipped along with the
>>>   anon COWs, and I didn't want multiple tests in the zap loop, so checking
>>>   check_mapping against page->mapping did both.  I think nowadays you could do
>>>   it by checking for PageAnon page (or genuine swap entry) instead.
>>>
>>> This patch replaced the zap_details.zap_mapping pointer into the even_cows
>>> boolean, then we check it against PageAnon.
>>>
>>> Suggested-by: Hugh Dickins <hughd@xxxxxxxxxx>
>>> Signed-off-by: Peter Xu <peterx@xxxxxxxxxx>
>>> ---
>>>  mm/memory.c | 16 +++++++---------
>>>  1 file changed, 7 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index 14d8428ff4db..ffa8c7dfe9ad 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -1309,8 +1309,8 @@ copy_page_range(struct vm_area_struct *dst_vma, struct vm_area_struct *src_vma)
>>>   * Parameter block passed down to zap_pte_range in exceptional cases.
>>>   */
>>>  struct zap_details {
>>> -	struct address_space *zap_mapping;	/* Check page->mapping if set */
>>>  	struct folio *single_folio;	/* Locked folio to be unmapped */
>>> +	bool even_cows;			/* Zap COWed private pages too? */
>>>  };
>>>  
>>>  /* Whether we should zap all COWed (private) pages too */
>>> @@ -1321,13 +1321,10 @@ static inline bool should_zap_cows(struct zap_details *details)
>>>  		return true;
>>>  
>>>  	/* Or, we zap COWed pages only if the caller wants to */
>>> -	return !details->zap_mapping;
>>> +	return details->even_cows;
>>>  }
>>>  
>>> -/*
>>> - * We set details->zap_mapping when we want to unmap shared but keep private
>>> - * pages. Return true if we should zap this page, false otherwise.
>>> - */
>>> +/* Decides whether we should zap this page with the page pointer specified */
>>>  static inline bool should_zap_page(struct zap_details *details, struct page *page)
>>>  {
>>>  	/* If we can make a decision without *page.. */
>>> @@ -1338,7 +1335,8 @@ static inline bool should_zap_page(struct zap_details *details, struct page *pag
>>>  	if (!page)
>>>  		return true;
>>>  
>>> -	return details->zap_mapping == page_rmapping(page);
>>> +	/* Otherwise we should only zap non-anon pages */
>>> +	return !PageAnon(page);
>>>  }
>>>  
>>>  static unsigned long zap_pte_range(struct mmu_gather *tlb,
>>> @@ -3403,7 +3401,7 @@ void unmap_mapping_folio(struct folio *folio)
>>>  	first_index = folio->index;
>>>  	last_index = folio->index + folio_nr_pages(folio) - 1;
>>>  
>>> -	details.zap_mapping = mapping;
>>> +	details.even_cows = false;
>>
>> Already initialized to 0 via struct zap_details details = { };
>>
>> We could think about
>>
>> struct zap_details details = {
>> 	.single_folio = folio,
>> };
>>
>>>  	details.single_folio = folio;
>>>  
>>>  	i_mmap_lock_write(mapping);
>>> @@ -3432,7 +3430,7 @@ void unmap_mapping_pages(struct address_space *mapping, pgoff_t start,
>>>  	pgoff_t	first_index = start;
>>>  	pgoff_t	last_index = start + nr - 1;
>>>  
>>> -	details.zap_mapping = even_cows ? NULL : mapping;
>>> +	details.even_cows = even_cows;
>>>  	if (last_index < first_index)
>>>  		last_index = ULONG_MAX;
>>>  
>>
>> Eventually
>>
>> struct zap_details details = {
>> 	.even_cows = even_cows,
>> };
> 
> I think in the very initial version I have had that C99 init format but I
> dropped it for some reason, perhaps when rebasing to the single_page work to
> avoid touching the existing code.
> 
> Since as you mentioned single_folio is another.. let's do the cleanup on top?

Sure, why not.


-- 
Thanks,

David / dhildenb