* [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() @ 2025-08-15 8:49 Wei Yang 2025-08-15 8:49 ` [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED Wei Yang ` (3 more replies) 0 siblings, 4 replies; 18+ messages in thread From: Wei Yang @ 2025-08-15 8:49 UTC (permalink / raw) To: akpm, david, lorenzo.stoakes, riel, Liam.Howlett, vbabka, harry.yoo Cc: linux-mm, Wei Yang During code reading, I found some small places where we can improve based on the knowledge we already have. For example, since base folio is handled in another branch, we case use folio_large_nr_pages() to get the number directly. Hope I don't mess it up. Wei Yang (3): mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED mm/rmap: could be partially_mapped only after no entire map mm/rmap: use folio_large_nr_pages() when we are sure it is a large folio mm/rmap.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED 2025-08-15 8:49 [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() Wei Yang @ 2025-08-15 8:49 ` Wei Yang 2025-08-15 9:38 ` Lorenzo Stoakes 2025-08-16 6:29 ` David Hildenbrand 2025-08-15 8:49 ` [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map Wei Yang ` (2 subsequent siblings) 3 siblings, 2 replies; 18+ messages in thread From: Wei Yang @ 2025-08-15 8:49 UTC (permalink / raw) To: akpm, david, lorenzo.stoakes, riel, Liam.Howlett, vbabka, harry.yoo Cc: linux-mm, Wei Yang At this point, we are sure (nr < ENTIRELY_MAPPED). This means the upper bits are already cleared. Not necessary to mask off it. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Cc: David Hildenbrand <david@redhat.com> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Cc: Rik van Riel <riel@surriel.com> Cc: Liam R. Howlett <Liam.Howlett@oracle.com> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Harry Yoo <harry.yoo@oracle.com> --- mm/rmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/rmap.c b/mm/rmap.c index 1c5988dbd1e7..a927437a56c2 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1749,7 +1749,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, nr_pages = folio_large_nr_pages(folio); if (level == PGTABLE_LEVEL_PMD) nr_pmdmapped = nr_pages; - nr = nr_pages - (nr & FOLIO_PAGES_MAPPED); + nr = nr_pages - nr; /* Raced ahead of another remove and an add? */ if (unlikely(nr < 0)) nr = 0; -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED 2025-08-15 8:49 ` [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED Wei Yang @ 2025-08-15 9:38 ` Lorenzo Stoakes 2025-08-16 0:33 ` Wei Yang 2025-08-16 6:29 ` David Hildenbrand 1 sibling, 1 reply; 18+ messages in thread From: Lorenzo Stoakes @ 2025-08-15 9:38 UTC (permalink / raw) To: Wei Yang; +Cc: akpm, david, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On Fri, Aug 15, 2025 at 08:49:41AM +0000, Wei Yang wrote: > At this point, we are sure (nr < ENTIRELY_MAPPED). This means the upper > bits are already cleared. > > Not necessary to mask off it. Worth saying it's because we are in an if branch conditional on nr < ENTIRELY_MAPPED, and FOLIO_PAGES_MAPPED is equal to ENTIRELY_MAPPED - 1. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> I suspect that this is copy/pasta from __folio_add_rmap(), but it may also be some way of asserting we don't underflow nr. Anyway I think this is fine because the branch implies this is ok so: Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Cc: Rik van Riel <riel@surriel.com> > Cc: Liam R. Howlett <Liam.Howlett@oracle.com> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Harry Yoo <harry.yoo@oracle.com> > --- > mm/rmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/rmap.c b/mm/rmap.c > index 1c5988dbd1e7..a927437a56c2 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1749,7 +1749,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, > nr_pages = folio_large_nr_pages(folio); > if (level == PGTABLE_LEVEL_PMD) > nr_pmdmapped = nr_pages; > - nr = nr_pages - (nr & FOLIO_PAGES_MAPPED); > + nr = nr_pages - nr; > /* Raced ahead of another remove and an add? */ > if (unlikely(nr < 0)) > nr = 0; > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED 2025-08-15 9:38 ` Lorenzo Stoakes @ 2025-08-16 0:33 ` Wei Yang 0 siblings, 0 replies; 18+ messages in thread From: Wei Yang @ 2025-08-16 0:33 UTC (permalink / raw) To: Lorenzo Stoakes Cc: Wei Yang, akpm, david, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On Fri, Aug 15, 2025 at 10:38:32AM +0100, Lorenzo Stoakes wrote: >On Fri, Aug 15, 2025 at 08:49:41AM +0000, Wei Yang wrote: >> At this point, we are sure (nr < ENTIRELY_MAPPED). This means the upper >> bits are already cleared. >> >> Not necessary to mask off it. > >Worth saying it's because we are in an if branch conditional on nr < >ENTIRELY_MAPPED, and FOLIO_PAGES_MAPPED is equal to ENTIRELY_MAPPED - 1. > Will adjust it. >> >> Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > >I suspect that this is copy/pasta from __folio_add_rmap(), but it may also >be some way of asserting we don't underflow nr. > >Anyway I think this is fine because the branch implies this is ok so: > >Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Thanks -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED 2025-08-15 8:49 ` [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED Wei Yang 2025-08-15 9:38 ` Lorenzo Stoakes @ 2025-08-16 6:29 ` David Hildenbrand 1 sibling, 0 replies; 18+ messages in thread From: David Hildenbrand @ 2025-08-16 6:29 UTC (permalink / raw) To: Wei Yang, akpm, lorenzo.stoakes, riel, Liam.Howlett, vbabka, harry.yoo Cc: linux-mm On 15.08.25 10:49, Wei Yang wrote: > At this point, we are sure (nr < ENTIRELY_MAPPED). This means the upper > bits are already cleared. > > Not necessary to mask off it. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Cc: Rik van Riel <riel@surriel.com> > Cc: Liam R. Howlett <Liam.Howlett@oracle.com> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Harry Yoo <harry.yoo@oracle.com> > --- > mm/rmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/rmap.c b/mm/rmap.c > index 1c5988dbd1e7..a927437a56c2 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1749,7 +1749,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, > nr_pages = folio_large_nr_pages(folio); > if (level == PGTABLE_LEVEL_PMD) > nr_pmdmapped = nr_pages; > - nr = nr_pages - (nr & FOLIO_PAGES_MAPPED); > + nr = nr_pages - nr; > /* Raced ahead of another remove and an add? */ > if (unlikely(nr < 0)) > nr = 0; Acked-by: David Hildenbrand <david@redhat.com> -- Cheers David / dhildenb ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map 2025-08-15 8:49 [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() Wei Yang 2025-08-15 8:49 ` [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED Wei Yang @ 2025-08-15 8:49 ` Wei Yang 2025-08-15 10:08 ` Lorenzo Stoakes 2025-08-15 8:49 ` [PATCH 3/3] mm/rmap: use folio_large_nr_pages() when we are sure it is a large folio Wei Yang 2025-08-15 10:11 ` [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() Lorenzo Stoakes 3 siblings, 1 reply; 18+ messages in thread From: Wei Yang @ 2025-08-15 8:49 UTC (permalink / raw) To: akpm, david, lorenzo.stoakes, riel, Liam.Howlett, vbabka, harry.yoo Cc: linux-mm, Wei Yang If it is not the last entire map, we are sure the folio is not partially mapped. Move the check when there is no entire map. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Cc: David Hildenbrand <david@redhat.com> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Cc: Rik van Riel <riel@surriel.com> Cc: Liam R. Howlett <Liam.Howlett@oracle.com> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Harry Yoo <harry.yoo@oracle.com> --- mm/rmap.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/mm/rmap.c b/mm/rmap.c index a927437a56c2..645d924bfc7d 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1757,9 +1757,9 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, /* An add of ENTIRELY_MAPPED raced ahead */ nr = 0; } - } - partially_mapped = nr && nr < nr_pmdmapped; + partially_mapped = nr && nr < nr_pmdmapped; + } break; default: BUILD_BUG(); -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map 2025-08-15 8:49 ` [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map Wei Yang @ 2025-08-15 10:08 ` Lorenzo Stoakes 2025-08-16 6:31 ` David Hildenbrand 0 siblings, 1 reply; 18+ messages in thread From: Lorenzo Stoakes @ 2025-08-15 10:08 UTC (permalink / raw) To: Wei Yang; +Cc: akpm, david, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On Fri, Aug 15, 2025 at 08:49:42AM +0000, Wei Yang wrote: > If it is not the last entire map, we are sure the folio is not partially > mapped. > > Move the check when there is no entire map. > This one I don't like, you're having to sit and think about why it is that this would be the case, vs. just unconditionally doing it. Again, as mentioned on previous series, just because we could do something doesn't mean we should, unless there's statistically reliable perf data on something real-world indicating we _must_, code clarity absolutely beats everything else on importance. So yeah, sorry but no to this patch, please resend with just the two reviewed (unless David radically disagrees with me :) > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Cc: Rik van Riel <riel@surriel.com> > Cc: Liam R. Howlett <Liam.Howlett@oracle.com> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Harry Yoo <harry.yoo@oracle.com> > --- > mm/rmap.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/mm/rmap.c b/mm/rmap.c > index a927437a56c2..645d924bfc7d 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1757,9 +1757,9 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, > /* An add of ENTIRELY_MAPPED raced ahead */ > nr = 0; > } > - } > > - partially_mapped = nr && nr < nr_pmdmapped; > + partially_mapped = nr && nr < nr_pmdmapped; > + } > break; > default: > BUILD_BUG(); > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map 2025-08-15 10:08 ` Lorenzo Stoakes @ 2025-08-16 6:31 ` David Hildenbrand 2025-08-16 9:06 ` Wei Yang 0 siblings, 1 reply; 18+ messages in thread From: David Hildenbrand @ 2025-08-16 6:31 UTC (permalink / raw) To: Lorenzo Stoakes, Wei Yang Cc: akpm, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On 15.08.25 12:08, Lorenzo Stoakes wrote: > On Fri, Aug 15, 2025 at 08:49:42AM +0000, Wei Yang wrote: >> If it is not the last entire map, we are sure the folio is not partially >> mapped. >> >> Move the check when there is no entire map. >> > > This one I don't like, you're having to sit and think about why it is that > this would be the case, vs. just unconditionally doing it. > > Again, as mentioned on previous series, just because we could do something > doesn't mean we should, unless there's statistically reliable perf data on > something real-world indicating we _must_, code clarity absolutely beats > everything else on importance. > > So yeah, sorry but no to this patch, please resend with just the two > reviewed (unless David radically disagrees with me :) The compiler can figure out that "nr == 0" if the "if (last)" branch is not taken. So I tend to prefer keeping it as is. -- Cheers David / dhildenb ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map 2025-08-16 6:31 ` David Hildenbrand @ 2025-08-16 9:06 ` Wei Yang 2025-08-16 9:16 ` David Hildenbrand 0 siblings, 1 reply; 18+ messages in thread From: Wei Yang @ 2025-08-16 9:06 UTC (permalink / raw) To: David Hildenbrand Cc: Lorenzo Stoakes, Wei Yang, akpm, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On Sat, Aug 16, 2025 at 08:31:59AM +0200, David Hildenbrand wrote: >On 15.08.25 12:08, Lorenzo Stoakes wrote: >> On Fri, Aug 15, 2025 at 08:49:42AM +0000, Wei Yang wrote: >> > If it is not the last entire map, we are sure the folio is not partially >> > mapped. >> > >> > Move the check when there is no entire map. >> > >> >> This one I don't like, you're having to sit and think about why it is that >> this would be the case, vs. just unconditionally doing it. >> >> Again, as mentioned on previous series, just because we could do something >> doesn't mean we should, unless there's statistically reliable perf data on >> something real-world indicating we _must_, code clarity absolutely beats >> everything else on importance. >> >> So yeah, sorry but no to this patch, please resend with just the two >> reviewed (unless David radically disagrees with me :) > >The compiler can figure out that "nr == 0" if the "if (last)" branch is not >taken. > Per my understanding, last is a run time value. I don't figure out how compiler could help here. I may miss something. Would you mind giving more hint? >So I tend to prefer keeping it as is. >-- >Cheers > >David / dhildenb -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map 2025-08-16 9:06 ` Wei Yang @ 2025-08-16 9:16 ` David Hildenbrand 2025-08-16 10:05 ` Giorgi Tchankvetadze 2025-08-16 14:03 ` Wei Yang 0 siblings, 2 replies; 18+ messages in thread From: David Hildenbrand @ 2025-08-16 9:16 UTC (permalink / raw) To: Wei Yang Cc: Lorenzo Stoakes, akpm, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On 16.08.25 11:06, Wei Yang wrote: > On Sat, Aug 16, 2025 at 08:31:59AM +0200, David Hildenbrand wrote: >> On 15.08.25 12:08, Lorenzo Stoakes wrote: >>> On Fri, Aug 15, 2025 at 08:49:42AM +0000, Wei Yang wrote: >>>> If it is not the last entire map, we are sure the folio is not partially >>>> mapped. >>>> >>>> Move the check when there is no entire map. >>>> >>> >>> This one I don't like, you're having to sit and think about why it is that >>> this would be the case, vs. just unconditionally doing it. >>> >>> Again, as mentioned on previous series, just because we could do something >>> doesn't mean we should, unless there's statistically reliable perf data on >>> something real-world indicating we _must_, code clarity absolutely beats >>> everything else on importance. >>> >>> So yeah, sorry but no to this patch, please resend with just the two >>> reviewed (unless David radically disagrees with me :) >> >> The compiler can figure out that "nr == 0" if the "if (last)" branch is not >> taken. >> > > Per my understanding, last is a run time value. I don't figure out how > compiler could help here. > > I may miss something. Would you mind giving more hint? The compiler can figure out that it can move the whole statement to the "if (last)" branch because it knows that nr == 0 otherwise and partially_mapped == 0 already. Best to see if there is any actual change in generated code, though. Note that if we're already moving that around, couldn't we move it further in? diff --git a/mm/rmap.c b/mm/rmap.c index 0e9c4041f8687..fb83db88cd1fd 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1753,13 +1753,13 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, /* Raced ahead of another remove and an add? */ if (unlikely(nr < 0)) nr = 0; + partially_mapped = nr && nr < nr_pmdmapped; } else { /* An add of ENTIRELY_MAPPED raced ahead */ nr = 0; } } - partially_mapped = nr && nr < nr_pmdmapped; break; default: BUILD_BUG(); -- Cheers David / dhildenb ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map 2025-08-16 9:16 ` David Hildenbrand @ 2025-08-16 10:05 ` Giorgi Tchankvetadze 2025-08-16 10:32 ` David Hildenbrand 2025-08-16 14:03 ` Wei Yang 1 sibling, 1 reply; 18+ messages in thread From: Giorgi Tchankvetadze @ 2025-08-16 10:05 UTC (permalink / raw) To: David Hildenbrand, Wei Yang Cc: Lorenzo Stoakes, akpm, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm Since the compiler can prove the expression will always be false in one branch, there's no need to execute that computation at all in that branch, right? On 8/16/2025 1:16 PM, David Hildenbrand wrote: > The compiler can figure out that it can move the whole statement to the > "if (last)" branch because it knows that nr == 0 otherwise and > partially_mapped == 0 already. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map 2025-08-16 10:05 ` Giorgi Tchankvetadze @ 2025-08-16 10:32 ` David Hildenbrand 0 siblings, 0 replies; 18+ messages in thread From: David Hildenbrand @ 2025-08-16 10:32 UTC (permalink / raw) To: Giorgi Tchankvetadze, Wei Yang Cc: Lorenzo Stoakes, akpm, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm Hi, please don't top-post. On 16.08.25 12:05, Giorgi Tchankvetadze wrote: > Since the compiler can prove the expression will always be false in one > branch, there's no need to execute that computation at all in that > branch, right? It's mostly a matter of making the code easier to read. The way we have it right now, we calculate partially_mapped in all cases just before the end of the case. For example, partially_mapped is irrelevant for !folio_test_anon() and !folio_test_large(), but we rely on the compiler to figure that out and optimize the code accordingly. Having something like if (folio_test_large(folio) && folio_test_anon(folio) && !folio_test_partially_mapped(folio)) { switch (level) { case PGTABLE_LEVEL_PTE: if (IS_ENABLED(CONFIG_NO_PAGE_MAPCOUNT)) { ... break; } partially_mapped = ... case PGTABLE_LEVEL_PMD: ... } if (partially_mapped) deferred_split_folio(folio, true); } Could be done, but it's arguably harder to read. > > On 8/16/2025 1:16 PM, David Hildenbrand wrote: >> The compiler can figure out that it can move the whole statement to the >> "if (last)" branch because it knows that nr == 0 otherwise and >> partially_mapped == 0 already. > -- Cheers David / dhildenb ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map 2025-08-16 9:16 ` David Hildenbrand 2025-08-16 10:05 ` Giorgi Tchankvetadze @ 2025-08-16 14:03 ` Wei Yang 1 sibling, 0 replies; 18+ messages in thread From: Wei Yang @ 2025-08-16 14:03 UTC (permalink / raw) To: David Hildenbrand Cc: Wei Yang, Lorenzo Stoakes, akpm, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On Sat, Aug 16, 2025 at 11:16:37AM +0200, David Hildenbrand wrote: >On 16.08.25 11:06, Wei Yang wrote: >> On Sat, Aug 16, 2025 at 08:31:59AM +0200, David Hildenbrand wrote: >> > On 15.08.25 12:08, Lorenzo Stoakes wrote: >> > > On Fri, Aug 15, 2025 at 08:49:42AM +0000, Wei Yang wrote: >> > > > If it is not the last entire map, we are sure the folio is not partially >> > > > mapped. >> > > > >> > > > Move the check when there is no entire map. >> > > > >> > > >> > > This one I don't like, you're having to sit and think about why it is that >> > > this would be the case, vs. just unconditionally doing it. >> > > >> > > Again, as mentioned on previous series, just because we could do something >> > > doesn't mean we should, unless there's statistically reliable perf data on >> > > something real-world indicating we _must_, code clarity absolutely beats >> > > everything else on importance. >> > > >> > > So yeah, sorry but no to this patch, please resend with just the two >> > > reviewed (unless David radically disagrees with me :) >> > >> > The compiler can figure out that "nr == 0" if the "if (last)" branch is not >> > taken. >> > >> >> Per my understanding, last is a run time value. I don't figure out how >> compiler could help here. >> >> I may miss something. Would you mind giving more hint? > >The compiler can figure out that it can move the whole statement to the "if (last)" branch because it knows that nr == 0 otherwise and partially_mapped == 0 already. Wow, this is interesting. Compiler is smarter than I thought. Thanks > >Best to see if there is any actual change in generated code, though. > Take a look into the generated code, there is no difference. >Note that if we're already moving that around, couldn't we move it further in? > Yes, I have thought about this. But I was afraid it was too aggressive. While after a second look, this may make the logic similar with CONFIG_NO_PAGE_MAPCOUNT. I personally like this one, but it seems not a big deal. >diff --git a/mm/rmap.c b/mm/rmap.c >index 0e9c4041f8687..fb83db88cd1fd 100644 >--- a/mm/rmap.c >+++ b/mm/rmap.c >@@ -1753,13 +1753,13 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, > /* Raced ahead of another remove and an add? */ > if (unlikely(nr < 0)) > nr = 0; >+ partially_mapped = nr && nr < nr_pmdmapped; > } else { > /* An add of ENTIRELY_MAPPED raced ahead */ > nr = 0; > } > } >- partially_mapped = nr && nr < nr_pmdmapped; > break; > default: > BUILD_BUG(); > > >-- >Cheers > >David / dhildenb -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/3] mm/rmap: use folio_large_nr_pages() when we are sure it is a large folio 2025-08-15 8:49 [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() Wei Yang 2025-08-15 8:49 ` [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED Wei Yang 2025-08-15 8:49 ` [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map Wei Yang @ 2025-08-15 8:49 ` Wei Yang 2025-08-15 10:04 ` Lorenzo Stoakes 2025-08-16 6:32 ` David Hildenbrand 2025-08-15 10:11 ` [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() Lorenzo Stoakes 3 siblings, 2 replies; 18+ messages in thread From: Wei Yang @ 2025-08-15 8:49 UTC (permalink / raw) To: akpm, david, lorenzo.stoakes, riel, Liam.Howlett, vbabka, harry.yoo Cc: linux-mm, Wei Yang Non-large folio is handled at the beginning, so it is a large folio for sure. Use folio_large_nr_pages() here like elsewhere. Signed-off-by: Wei Yang <richard.weiyang@gmail.com> Cc: David Hildenbrand <david@redhat.com> Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> Cc: Rik van Riel <riel@surriel.com> Cc: Liam R. Howlett <Liam.Howlett@oracle.com> Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Harry Yoo <harry.yoo@oracle.com> --- mm/rmap.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/mm/rmap.c b/mm/rmap.c index 645d924bfc7d..1b7011afc663 100644 --- a/mm/rmap.c +++ b/mm/rmap.c @@ -1703,7 +1703,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, nr = folio_sub_return_large_mapcount(folio, nr_pages, vma); if (!nr) { /* Now completely unmapped. */ - nr = folio_nr_pages(folio); + nr = folio_large_nr_pages(folio); } else { partially_mapped = nr < folio_large_nr_pages(folio) && !folio_entire_mapcount(folio); -- 2.34.1 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] mm/rmap: use folio_large_nr_pages() when we are sure it is a large folio 2025-08-15 8:49 ` [PATCH 3/3] mm/rmap: use folio_large_nr_pages() when we are sure it is a large folio Wei Yang @ 2025-08-15 10:04 ` Lorenzo Stoakes 2025-08-16 6:32 ` David Hildenbrand 1 sibling, 0 replies; 18+ messages in thread From: Lorenzo Stoakes @ 2025-08-15 10:04 UTC (permalink / raw) To: Wei Yang; +Cc: akpm, david, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On Fri, Aug 15, 2025 at 08:49:43AM +0000, Wei Yang wrote: > Non-large folio is handled at the beginning, so it is a large folio for > sure. > > Use folio_large_nr_pages() here like elsewhere. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> This looks fine to me, so: Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Cc: Rik van Riel <riel@surriel.com> > Cc: Liam R. Howlett <Liam.Howlett@oracle.com> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Harry Yoo <harry.yoo@oracle.com> > --- > mm/rmap.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/mm/rmap.c b/mm/rmap.c > index 645d924bfc7d..1b7011afc663 100644 > --- a/mm/rmap.c > +++ b/mm/rmap.c > @@ -1703,7 +1703,7 @@ static __always_inline void __folio_remove_rmap(struct folio *folio, > nr = folio_sub_return_large_mapcount(folio, nr_pages, vma); > if (!nr) { > /* Now completely unmapped. */ > - nr = folio_nr_pages(folio); > + nr = folio_large_nr_pages(folio); > } else { > partially_mapped = nr < folio_large_nr_pages(folio) && > !folio_entire_mapcount(folio); > -- > 2.34.1 > > ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 3/3] mm/rmap: use folio_large_nr_pages() when we are sure it is a large folio 2025-08-15 8:49 ` [PATCH 3/3] mm/rmap: use folio_large_nr_pages() when we are sure it is a large folio Wei Yang 2025-08-15 10:04 ` Lorenzo Stoakes @ 2025-08-16 6:32 ` David Hildenbrand 1 sibling, 0 replies; 18+ messages in thread From: David Hildenbrand @ 2025-08-16 6:32 UTC (permalink / raw) To: Wei Yang, akpm, lorenzo.stoakes, riel, Liam.Howlett, vbabka, harry.yoo Cc: linux-mm On 15.08.25 10:49, Wei Yang wrote: > Non-large folio is handled at the beginning, so it is a large folio for > sure. > > Use folio_large_nr_pages() here like elsewhere. > > Signed-off-by: Wei Yang <richard.weiyang@gmail.com> > Cc: David Hildenbrand <david@redhat.com> > Cc: Lorenzo Stoakes <lorenzo.stoakes@oracle.com> > Cc: Rik van Riel <riel@surriel.com> > Cc: Liam R. Howlett <Liam.Howlett@oracle.com> > Cc: Vlastimil Babka <vbabka@suse.cz> > Cc: Harry Yoo <harry.yoo@oracle.com> > --- Acked-by: David Hildenbrand <david@redhat.com> -- Cheers David / dhildenb ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() 2025-08-15 8:49 [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() Wei Yang ` (2 preceding siblings ...) 2025-08-15 8:49 ` [PATCH 3/3] mm/rmap: use folio_large_nr_pages() when we are sure it is a large folio Wei Yang @ 2025-08-15 10:11 ` Lorenzo Stoakes 2025-08-16 0:36 ` Wei Yang 3 siblings, 1 reply; 18+ messages in thread From: Lorenzo Stoakes @ 2025-08-15 10:11 UTC (permalink / raw) To: Wei Yang; +Cc: akpm, david, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On Fri, Aug 15, 2025 at 08:49:40AM +0000, Wei Yang wrote: > During code reading, I found some small places where we can improve based on > the knowledge we already have. > > For example, since base folio is handled in another branch, we case use > folio_large_nr_pages() to get the number directly. I think this cover letter is pretty weak, please describe what you are doing clearly and why. > > Hope I don't mess it up. Please don't put this kind of thing in the cover letter, it gets reproduced into the actual commit messages and this isn't useful for people reading through commit messages. > > Wei Yang (3): > mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED > mm/rmap: could be partially_mapped only after no entire map > mm/rmap: use folio_large_nr_pages() when we are sure it is a large > folio I find 1/3 and 3/3 ok, 2/3 I don't like, so please resend series as v2 with only 1 and 2, propagating tags + improved cover letter please. FWIW, I build-tested each commit and ran mm self tests + VMA tests against series and all passed. > > mm/rmap.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > -- > 2.34.1 > > Cheers, Lorenzo ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() 2025-08-15 10:11 ` [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() Lorenzo Stoakes @ 2025-08-16 0:36 ` Wei Yang 0 siblings, 0 replies; 18+ messages in thread From: Wei Yang @ 2025-08-16 0:36 UTC (permalink / raw) To: Lorenzo Stoakes Cc: Wei Yang, akpm, david, riel, Liam.Howlett, vbabka, harry.yoo, linux-mm On Fri, Aug 15, 2025 at 11:11:24AM +0100, Lorenzo Stoakes wrote: >On Fri, Aug 15, 2025 at 08:49:40AM +0000, Wei Yang wrote: >> During code reading, I found some small places where we can improve based on >> the knowledge we already have. >> >> For example, since base folio is handled in another branch, we case use >> folio_large_nr_pages() to get the number directly. > >I think this cover letter is pretty weak, please describe what you are >doing clearly and why. > Will adjust it. >> >> Hope I don't mess it up. > >Please don't put this kind of thing in the cover letter, it gets reproduced >into the actual commit messages and this isn't useful for people reading >through commit messages. > Sure. -- Wei Yang Help you, Help me ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2025-08-16 14:03 UTC | newest] Thread overview: 18+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2025-08-15 8:49 [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() Wei Yang 2025-08-15 8:49 ` [PATCH 1/3] mm/rmap: not necessary to mask off FOLIO_PAGES_MAPPED Wei Yang 2025-08-15 9:38 ` Lorenzo Stoakes 2025-08-16 0:33 ` Wei Yang 2025-08-16 6:29 ` David Hildenbrand 2025-08-15 8:49 ` [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map Wei Yang 2025-08-15 10:08 ` Lorenzo Stoakes 2025-08-16 6:31 ` David Hildenbrand 2025-08-16 9:06 ` Wei Yang 2025-08-16 9:16 ` David Hildenbrand 2025-08-16 10:05 ` Giorgi Tchankvetadze 2025-08-16 10:32 ` David Hildenbrand 2025-08-16 14:03 ` Wei Yang 2025-08-15 8:49 ` [PATCH 3/3] mm/rmap: use folio_large_nr_pages() when we are sure it is a large folio Wei Yang 2025-08-15 10:04 ` Lorenzo Stoakes 2025-08-16 6:32 ` David Hildenbrand 2025-08-15 10:11 ` [PATCH 0/3] mm/rmap: small cleanup for __folio_remove_rmap() Lorenzo Stoakes 2025-08-16 0:36 ` Wei Yang
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 NNTP newsgroup(s).