Linux-mm Archive mirror
 help / color / mirror / Atom feed
* [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats
@ 2024-05-06 21:13 Yosry Ahmed
  2024-05-07  8:52 ` David Hildenbrand
  0 siblings, 1 reply; 5+ messages in thread
From: Yosry Ahmed @ 2024-05-06 21:13 UTC (permalink / raw
  To: Andrew Morton; +Cc: David Hildenbrand, linux-mm, linux-kernel, Yosry Ahmed

A lot of intricacies go into updating the stats when adding or removing
mappings: which stat index to use and which function. Abstract this away
into a new static helper in rmap.c, __folio_mod_stat().

This adds an unnecessary call to folio_test_anon() in
__folio_add_anon_rmap() and __folio_add_file_rmap(). However, the folio
struct should already be in the cache at this point, so it shouldn't
cause any noticeable overhead.

No functional change intended.

Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
---

This applies on top of "mm: do not update memcg stats for
NR_{FILE/SHMEM}_PMDMAPPED":
https://lore.kernel.org/lkml/20240506192924.271999-1-yosryahmed@google.com/

David, I was on the fence about adding a Suggested-by here. You did
suggest adding a helper, but the one with the extra folio_test_anon()
was my idea and I didn't want to blame it on you. So I'll leave this up
to you :)

---
 mm/rmap.c | 56 ++++++++++++++++++++++++++++---------------------------
 1 file changed, 29 insertions(+), 27 deletions(-)

diff --git a/mm/rmap.c b/mm/rmap.c
index ed7f820369864..9ed995da47099 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1269,6 +1269,28 @@ static void __page_check_anon_rmap(struct folio *folio, struct page *page,
 		       page);
 }
 
+static void __folio_mod_stat(struct folio *folio, int nr, int nr_pmdmapped)
+{
+	int idx;
+
+	if (nr) {
+		idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_MAPPED;
+		__lruvec_stat_mod_folio(folio, idx, nr);
+	}
+	if (nr_pmdmapped) {
+		if (folio_test_anon(folio)) {
+			idx = NR_ANON_THPS;
+			__lruvec_stat_mod_folio(folio, idx, nr_pmdmapped);
+		} else {
+			/* NR_*_PMDMAPPED are not maintained per-memcg */
+			idx = folio_test_swapbacked(folio) ?
+				NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED;
+			__mod_node_page_state(folio_pgdat(folio), idx,
+					      nr_pmdmapped);
+		}
+	}
+}
+
 static __always_inline void __folio_add_anon_rmap(struct folio *folio,
 		struct page *page, int nr_pages, struct vm_area_struct *vma,
 		unsigned long address, rmap_t flags, enum rmap_level level)
@@ -1276,10 +1298,6 @@ static __always_inline void __folio_add_anon_rmap(struct folio *folio,
 	int i, nr, nr_pmdmapped = 0;
 
 	nr = __folio_add_rmap(folio, page, nr_pages, level, &nr_pmdmapped);
-	if (nr_pmdmapped)
-		__lruvec_stat_mod_folio(folio, NR_ANON_THPS, nr_pmdmapped);
-	if (nr)
-		__lruvec_stat_mod_folio(folio, NR_ANON_MAPPED, nr);
 
 	if (unlikely(!folio_test_anon(folio))) {
 		VM_WARN_ON_FOLIO(!folio_test_locked(folio), folio);
@@ -1297,6 +1315,8 @@ static __always_inline void __folio_add_anon_rmap(struct folio *folio,
 		__page_check_anon_rmap(folio, page, vma, address);
 	}
 
+	__folio_mod_stat(folio, nr, nr_pmdmapped);
+
 	if (flags & RMAP_EXCLUSIVE) {
 		switch (level) {
 		case RMAP_LEVEL_PTE:
@@ -1393,6 +1413,7 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
 		unsigned long address)
 {
 	int nr = folio_nr_pages(folio);
+	int nr_pmdmapped = 0;
 
 	VM_WARN_ON_FOLIO(folio_test_hugetlb(folio), folio);
 	VM_BUG_ON_VMA(address < vma->vm_start ||
@@ -1425,27 +1446,22 @@ void folio_add_new_anon_rmap(struct folio *folio, struct vm_area_struct *vma,
 		atomic_set(&folio->_large_mapcount, 0);
 		atomic_set(&folio->_nr_pages_mapped, ENTIRELY_MAPPED);
 		SetPageAnonExclusive(&folio->page);
-		__lruvec_stat_mod_folio(folio, NR_ANON_THPS, nr);
+		nr_pmdmapped = nr;
 	}
 
-	__lruvec_stat_mod_folio(folio, NR_ANON_MAPPED, nr);
+	__folio_mod_stat(folio, nr, nr_pmdmapped);
 }
 
 static __always_inline void __folio_add_file_rmap(struct folio *folio,
 		struct page *page, int nr_pages, struct vm_area_struct *vma,
 		enum rmap_level level)
 {
-	pg_data_t *pgdat = folio_pgdat(folio);
 	int nr, nr_pmdmapped = 0;
 
 	VM_WARN_ON_FOLIO(folio_test_anon(folio), folio);
 
 	nr = __folio_add_rmap(folio, page, nr_pages, level, &nr_pmdmapped);
-	if (nr_pmdmapped)
-		__mod_node_page_state(pgdat, folio_test_swapbacked(folio) ?
-			NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED, nr_pmdmapped);
-	if (nr)
-		__lruvec_stat_mod_folio(folio, NR_FILE_MAPPED, nr);
+	__folio_mod_stat(folio, nr, nr_pmdmapped);
 
 	/* See comments in folio_add_anon_rmap_*() */
 	if (!folio_test_large(folio))
@@ -1494,10 +1510,8 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
 		enum rmap_level level)
 {
 	atomic_t *mapped = &folio->_nr_pages_mapped;
-	pg_data_t *pgdat = folio_pgdat(folio);
 	int last, nr = 0, nr_pmdmapped = 0;
 	bool partially_mapped = false;
-	enum node_stat_item idx;
 
 	__folio_rmap_sanity_checks(folio, page, nr_pages, level);
 
@@ -1541,20 +1555,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
 		break;
 	}
 
-	if (nr_pmdmapped) {
-		/* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-memcg */
-		if (folio_test_anon(folio))
-			__lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_pmdmapped);
-		else
-			__mod_node_page_state(pgdat,
-					folio_test_swapbacked(folio) ?
-					NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED,
-					-nr_pmdmapped);
-	}
 	if (nr) {
-		idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_MAPPED;
-		__lruvec_stat_mod_folio(folio, idx, -nr);
-
 		/*
 		 * Queue anon large folio for deferred split if at least one
 		 * page of the folio is unmapped and at least one page
@@ -1566,6 +1567,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
 		    list_empty(&folio->_deferred_list))
 			deferred_split_folio(folio);
 	}
+	__folio_mod_stat(folio, nr, nr_pmdmapped);
 
 	/*
 	 * It would be tidy to reset folio_test_anon mapping when fully
-- 
2.45.0.rc1.225.g2a3ae87e7f-goog



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats
  2024-05-06 21:13 [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats Yosry Ahmed
@ 2024-05-07  8:52 ` David Hildenbrand
  2024-05-07 15:54   ` Yosry Ahmed
  0 siblings, 1 reply; 5+ messages in thread
From: David Hildenbrand @ 2024-05-07  8:52 UTC (permalink / raw
  To: Yosry Ahmed, Andrew Morton; +Cc: linux-mm, linux-kernel

On 06.05.24 23:13, Yosry Ahmed wrote:
> A lot of intricacies go into updating the stats when adding or removing
> mappings: which stat index to use and which function. Abstract this away
> into a new static helper in rmap.c, __folio_mod_stat().
> 
> This adds an unnecessary call to folio_test_anon() in
> __folio_add_anon_rmap() and __folio_add_file_rmap(). However, the folio
> struct should already be in the cache at this point, so it shouldn't
> cause any noticeable overhead.

Depending on the inlining, we might have more branches that could be avoided
(especially in folio_add_new_anon_rmap()).

[the rmap code is more performance-sensitive and relevant than you might think]

> 
> No functional change intended.
> 
> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> ---
> 
> This applies on top of "mm: do not update memcg stats for
> NR_{FILE/SHMEM}_PMDMAPPED":
> https://lore.kernel.org/lkml/20240506192924.271999-1-yosryahmed@google.com/
> 
> David, I was on the fence about adding a Suggested-by here. You did
> suggest adding a helper, but the one with the extra folio_test_anon()
> was my idea and I didn't want to blame it on you. So I'll leave this up
> to you :)

:) fair enough! It's a clear improvement to readability.

[...]
>   
> -	if (nr_pmdmapped) {
> -		/* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-memcg */
> -		if (folio_test_anon(folio))
> -			__lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_pmdmapped);
> -		else
> -			__mod_node_page_state(pgdat,
> -					folio_test_swapbacked(folio) ?
> -					NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED,
> -					-nr_pmdmapped);
> -	}
>   	if (nr) {
> -		idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_MAPPED;
> -		__lruvec_stat_mod_folio(folio, idx, -nr);
> -


We can now even do:

diff --git a/mm/rmap.c b/mm/rmap.c
index 9ed995da4709..7a147195e512 100644
--- a/mm/rmap.c
+++ b/mm/rmap.c
@@ -1555,18 +1555,17 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
                 break;
         }
  
-       if (nr) {
-               /*
-                * Queue anon large folio for deferred split if at least one
-                * page of the folio is unmapped and at least one page
-                * is still mapped.
-                *
-                * Check partially_mapped first to ensure it is a large folio.
-                */
-               if (folio_test_anon(folio) && partially_mapped &&
-                   list_empty(&folio->_deferred_list))
-                       deferred_split_folio(folio);
-       }
+       /*
+        * Queue anon large folio for deferred split if at least one
+        * page of the folio is unmapped and at least one page
+        * is still mapped.
+        *
+        * Check partially_mapped first to ensure it is a large folio.
+        */
+       if (folio_test_anon(folio) && partially_mapped &&
+           list_empty(&folio->_deferred_list))
+               deferred_split_folio(folio);
+
         __folio_mod_stat(folio, nr, nr_pmdmapped);
  
         /*


Which will help some of my upcoming patches.

Feel free to include that in a v2, otherwise I'll include it in an upcoming
patch series.


Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Cheers,

David / dhildenb



^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats
  2024-05-07  8:52 ` David Hildenbrand
@ 2024-05-07 15:54   ` Yosry Ahmed
  2024-05-07 18:37     ` David Hildenbrand
  0 siblings, 1 reply; 5+ messages in thread
From: Yosry Ahmed @ 2024-05-07 15:54 UTC (permalink / raw
  To: David Hildenbrand; +Cc: Andrew Morton, linux-mm, linux-kernel

On Tue, May 7, 2024 at 1:52 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 06.05.24 23:13, Yosry Ahmed wrote:
> > A lot of intricacies go into updating the stats when adding or removing
> > mappings: which stat index to use and which function. Abstract this away
> > into a new static helper in rmap.c, __folio_mod_stat().
> >
> > This adds an unnecessary call to folio_test_anon() in
> > __folio_add_anon_rmap() and __folio_add_file_rmap(). However, the folio
> > struct should already be in the cache at this point, so it shouldn't
> > cause any noticeable overhead.
>
> Depending on the inlining, we might have more branches that could be avoided
> (especially in folio_add_new_anon_rmap()).
>
> [the rmap code is more performance-sensitive and relevant than you might think]

I thought about making the helper __always_inline. Would that be better?

>
> >
> > No functional change intended.
> >
> > Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
> > ---
> >
> > This applies on top of "mm: do not update memcg stats for
> > NR_{FILE/SHMEM}_PMDMAPPED":
> > https://lore.kernel.org/lkml/20240506192924.271999-1-yosryahmed@google.com/
> >
> > David, I was on the fence about adding a Suggested-by here. You did
> > suggest adding a helper, but the one with the extra folio_test_anon()
> > was my idea and I didn't want to blame it on you. So I'll leave this up
> > to you :)
>
> :) fair enough! It's a clear improvement to readability.
>
> [...]
> >
> > -     if (nr_pmdmapped) {
> > -             /* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-memcg */
> > -             if (folio_test_anon(folio))
> > -                     __lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_pmdmapped);
> > -             else
> > -                     __mod_node_page_state(pgdat,
> > -                                     folio_test_swapbacked(folio) ?
> > -                                     NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED,
> > -                                     -nr_pmdmapped);
> > -     }
> >       if (nr) {
> > -             idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_MAPPED;
> > -             __lruvec_stat_mod_folio(folio, idx, -nr);
> > -
>
>
> We can now even do:
>
> diff --git a/mm/rmap.c b/mm/rmap.c
> index 9ed995da4709..7a147195e512 100644
> --- a/mm/rmap.c
> +++ b/mm/rmap.c
> @@ -1555,18 +1555,17 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
>                  break;
>          }
>
> -       if (nr) {
> -               /*
> -                * Queue anon large folio for deferred split if at least one
> -                * page of the folio is unmapped and at least one page
> -                * is still mapped.
> -                *
> -                * Check partially_mapped first to ensure it is a large folio.
> -                */
> -               if (folio_test_anon(folio) && partially_mapped &&
> -                   list_empty(&folio->_deferred_list))
> -                       deferred_split_folio(folio);
> -       }
> +       /*
> +        * Queue anon large folio for deferred split if at least one
> +        * page of the folio is unmapped and at least one page
> +        * is still mapped.
> +        *
> +        * Check partially_mapped first to ensure it is a large folio.
> +        */
> +       if (folio_test_anon(folio) && partially_mapped &&
> +           list_empty(&folio->_deferred_list))
> +               deferred_split_folio(folio);
> +

Dumb question: why is it okay to remove the 'if (nr)' condition here?
It seems to me by looking at the code in case RMAP_LEVEL_PMD that it
is possible for partially_mapped to be true while nr == 0.

Is this practically impossible for some reason, or is adding the folio
to the deferred split queue okay either way?

>          __folio_mod_stat(folio, nr, nr_pmdmapped);
>
>          /*
>
>
> Which will help some of my upcoming patches.
>
> Feel free to include that in a v2, otherwise I'll include it in an upcoming
> patch series.
>
>
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats
  2024-05-07 15:54   ` Yosry Ahmed
@ 2024-05-07 18:37     ` David Hildenbrand
  2024-05-07 18:42       ` Yosry Ahmed
  0 siblings, 1 reply; 5+ messages in thread
From: David Hildenbrand @ 2024-05-07 18:37 UTC (permalink / raw
  To: Yosry Ahmed; +Cc: Andrew Morton, linux-mm, linux-kernel

On 07.05.24 17:54, Yosry Ahmed wrote:
> On Tue, May 7, 2024 at 1:52 AM David Hildenbrand <david@redhat.com> wrote:
>>
>> On 06.05.24 23:13, Yosry Ahmed wrote:
>>> A lot of intricacies go into updating the stats when adding or removing
>>> mappings: which stat index to use and which function. Abstract this away
>>> into a new static helper in rmap.c, __folio_mod_stat().
>>>
>>> This adds an unnecessary call to folio_test_anon() in
>>> __folio_add_anon_rmap() and __folio_add_file_rmap(). However, the folio
>>> struct should already be in the cache at this point, so it shouldn't
>>> cause any noticeable overhead.
>>
>> Depending on the inlining, we might have more branches that could be avoided
>> (especially in folio_add_new_anon_rmap()).
>>
>> [the rmap code is more performance-sensitive and relevant than you might think]
> 
> I thought about making the helper __always_inline. Would that be better?

Let's leave it like that. I might do some actual measurements to see if 
it makes a difference at all.


>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Yosry Ahmed <yosryahmed@google.com>
>>> ---
>>>
>>> This applies on top of "mm: do not update memcg stats for
>>> NR_{FILE/SHMEM}_PMDMAPPED":
>>> https://lore.kernel.org/lkml/20240506192924.271999-1-yosryahmed@google.com/
>>>
>>> David, I was on the fence about adding a Suggested-by here. You did
>>> suggest adding a helper, but the one with the extra folio_test_anon()
>>> was my idea and I didn't want to blame it on you. So I'll leave this up
>>> to you :)
>>
>> :) fair enough! It's a clear improvement to readability.
>>
>> [...]
>>>
>>> -     if (nr_pmdmapped) {
>>> -             /* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-memcg */
>>> -             if (folio_test_anon(folio))
>>> -                     __lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_pmdmapped);
>>> -             else
>>> -                     __mod_node_page_state(pgdat,
>>> -                                     folio_test_swapbacked(folio) ?
>>> -                                     NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED,
>>> -                                     -nr_pmdmapped);
>>> -     }
>>>        if (nr) {
>>> -             idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_MAPPED;
>>> -             __lruvec_stat_mod_folio(folio, idx, -nr);
>>> -
>>
>>
>> We can now even do:
>>
>> diff --git a/mm/rmap.c b/mm/rmap.c
>> index 9ed995da4709..7a147195e512 100644
>> --- a/mm/rmap.c
>> +++ b/mm/rmap.c
>> @@ -1555,18 +1555,17 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
>>                   break;
>>           }
>>
>> -       if (nr) {
>> -               /*
>> -                * Queue anon large folio for deferred split if at least one
>> -                * page of the folio is unmapped and at least one page
>> -                * is still mapped.
>> -                *
>> -                * Check partially_mapped first to ensure it is a large folio.
>> -                */
>> -               if (folio_test_anon(folio) && partially_mapped &&
>> -                   list_empty(&folio->_deferred_list))
>> -                       deferred_split_folio(folio);
>> -       }
>> +       /*
>> +        * Queue anon large folio for deferred split if at least one
>> +        * page of the folio is unmapped and at least one page
>> +        * is still mapped.
>> +        *
>> +        * Check partially_mapped first to ensure it is a large folio.
>> +        */
>> +       if (folio_test_anon(folio) && partially_mapped &&
>> +           list_empty(&folio->_deferred_list))
>> +               deferred_split_folio(folio);
>> +
> 
> Dumb question: why is it okay to remove the 'if (nr)' condition here?
> It seems to me by looking at the code in case RMAP_LEVEL_PMD that it
> is possible for partially_mapped to be true while nr == 0.

Not a dumb question at all, and I cannot immediately tell if we might 
have to move the "nr" check to the RMAP_LEVEL_PMD case (I feel like 
we're good, but will have to double check). So let's keep it as is for 
now and I'll perform that change separately.

Thanks!

-- 
Cheers,

David / dhildenb



^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats
  2024-05-07 18:37     ` David Hildenbrand
@ 2024-05-07 18:42       ` Yosry Ahmed
  0 siblings, 0 replies; 5+ messages in thread
From: Yosry Ahmed @ 2024-05-07 18:42 UTC (permalink / raw
  To: David Hildenbrand; +Cc: Andrew Morton, linux-mm, linux-kernel

On Tue, May 7, 2024 at 11:38 AM David Hildenbrand <david@redhat.com> wrote:
>
> On 07.05.24 17:54, Yosry Ahmed wrote:
> > On Tue, May 7, 2024 at 1:52 AM David Hildenbrand <david@redhat.com> wrote:
> >>
> >> On 06.05.24 23:13, Yosry Ahmed wrote:
> >>> A lot of intricacies go into updating the stats when adding or removing
> >>> mappings: which stat index to use and which function. Abstract this away
> >>> into a new static helper in rmap.c, __folio_mod_stat().
> >>>
> >>> This adds an unnecessary call to folio_test_anon() in
> >>> __folio_add_anon_rmap() and __folio_add_file_rmap(). However, the folio
> >>> struct should already be in the cache at this point, so it shouldn't
> >>> cause any noticeable overhead.
> >>
> >> Depending on the inlining, we might have more branches that could be avoided
> >> (especially in folio_add_new_anon_rmap()).
> >>
> >> [the rmap code is more performance-sensitive and relevant than you might think]
> >
> > I thought about making the helper __always_inline. Would that be better?
>
> Let's leave it like that. I might do some actual measurements to see if
> it makes a difference at all.

That would be interesting to find out for sure.

[..]
> >>>
> >>> -     if (nr_pmdmapped) {
> >>> -             /* NR_{FILE/SHMEM}_PMDMAPPED are not maintained per-memcg */
> >>> -             if (folio_test_anon(folio))
> >>> -                     __lruvec_stat_mod_folio(folio, NR_ANON_THPS, -nr_pmdmapped);
> >>> -             else
> >>> -                     __mod_node_page_state(pgdat,
> >>> -                                     folio_test_swapbacked(folio) ?
> >>> -                                     NR_SHMEM_PMDMAPPED : NR_FILE_PMDMAPPED,
> >>> -                                     -nr_pmdmapped);
> >>> -     }
> >>>        if (nr) {
> >>> -             idx = folio_test_anon(folio) ? NR_ANON_MAPPED : NR_FILE_MAPPED;
> >>> -             __lruvec_stat_mod_folio(folio, idx, -nr);
> >>> -
> >>
> >>
> >> We can now even do:
> >>
> >> diff --git a/mm/rmap.c b/mm/rmap.c
> >> index 9ed995da4709..7a147195e512 100644
> >> --- a/mm/rmap.c
> >> +++ b/mm/rmap.c
> >> @@ -1555,18 +1555,17 @@ static __always_inline void __folio_remove_rmap(struct folio *folio,
> >>                   break;
> >>           }
> >>
> >> -       if (nr) {
> >> -               /*
> >> -                * Queue anon large folio for deferred split if at least one
> >> -                * page of the folio is unmapped and at least one page
> >> -                * is still mapped.
> >> -                *
> >> -                * Check partially_mapped first to ensure it is a large folio.
> >> -                */
> >> -               if (folio_test_anon(folio) && partially_mapped &&
> >> -                   list_empty(&folio->_deferred_list))
> >> -                       deferred_split_folio(folio);
> >> -       }
> >> +       /*
> >> +        * Queue anon large folio for deferred split if at least one
> >> +        * page of the folio is unmapped and at least one page
> >> +        * is still mapped.
> >> +        *
> >> +        * Check partially_mapped first to ensure it is a large folio.
> >> +        */
> >> +       if (folio_test_anon(folio) && partially_mapped &&
> >> +           list_empty(&folio->_deferred_list))
> >> +               deferred_split_folio(folio);
> >> +
> >
> > Dumb question: why is it okay to remove the 'if (nr)' condition here?
> > It seems to me by looking at the code in case RMAP_LEVEL_PMD that it
> > is possible for partially_mapped to be true while nr == 0.
>
> Not a dumb question at all, and I cannot immediately tell if we might
> have to move the "nr" check to the RMAP_LEVEL_PMD case (I feel like
> we're good, but will have to double check). So let's keep it as is for
> now and I'll perform that change separately.

SGTM, thanks for checking and for the review.

It appears to me that no changes are required here then :)


^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2024-05-07 18:42 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-05-06 21:13 [PATCH mm-unstable] mm: rmap: abstract updating per-node and per-memcg stats Yosry Ahmed
2024-05-07  8:52 ` David Hildenbrand
2024-05-07 15:54   ` Yosry Ahmed
2024-05-07 18:37     ` David Hildenbrand
2024-05-07 18:42       ` Yosry Ahmed

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).