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

[David Hildenbrand <david@redhat.com>]

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