diff mbox series

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

Message ID 20220128045412.18695-4-peterx@redhat.com (mailing list archive)
State New
Headers show
Series mm: Rework zap ptes on swap entries | expand

Commit Message

Peter Xu Jan. 28, 2022, 4:54 a.m. UTC
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@google.com/

Quotting partly from Hugh:

  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@google.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 mm/memory.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

Comments

David Hildenbrand Jan. 28, 2022, 9:03 a.m. UTC | #1
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@google.com/
> 
> Quotting partly from Hugh:

s/Quotting/Quoting/

> 
>   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@google.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  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,
};
Peter Xu Jan. 28, 2022, 9:17 a.m. UTC | #2
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@google.com/
> > 
> > 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@google.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  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?

Thanks,
David Hildenbrand Jan. 28, 2022, 9:18 a.m. UTC | #3
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@google.com/
>>>
>>> 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@google.com>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>  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.
diff mbox series

Patch

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;
 	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;