Re: [PATCH 2/3] mm/rmap: could be partially_mapped only after no entire map

[Wei Yang <richard.weiyang@gmail.com>]

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