Replace open-coded folio reference count calculations with the
folio_expected_ref_count() helper to improve code maintainability
and reduce duplication.

No functional changes intended.

Signed-off-by: Shivank Garg <shivankg@amd.com>
---
 kernel/events/uprobes.c | 5 +++--
 mm/memfd.c              | 4 ++--
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index 4c965ba77f9f..c978c8c27340 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -434,10 +434,11 @@ static int __uprobe_write_opcode(struct vm_area_struct *vma,
 	/*
 	 * When unregistering, we may only zap a PTE if uffd is disabled and
 	 * there are no unexpected folio references ...
+	 * Expected refs: mappings + swapcache.
+	 * We hold one additional reference (+1).
 	 */
 	if (is_register || userfaultfd_missing(vma) ||
-	    (folio_ref_count(folio) != folio_mapcount(folio) + 1 +
-	     folio_test_swapcache(folio) * folio_nr_pages(folio)))
+	    (folio_ref_count(folio) != folio_expected_ref_count(folio) + 1))
 		goto remap;
 
 	/*
diff --git a/mm/memfd.c b/mm/memfd.c
index ab367e61553d..4ed5506221b7 100644
--- a/mm/memfd.c
+++ b/mm/memfd.c
@@ -32,8 +32,8 @@
 
 static bool memfd_folio_has_extra_refs(struct folio *folio)
 {
-	return folio_ref_count(folio) - folio_mapcount(folio) !=
-	       folio_nr_pages(folio);
+	/* Expected refs: pagecache + mappings */
+	return folio_ref_count(folio) != folio_expected_ref_count(folio);
 }
 
 static void memfd_tag_pins(struct xa_state *xas)
-- 
2.43.0

On 09.06.25 19:08, Shivank Garg wrote:
> Replace open-coded folio reference count calculations with the
> folio_expected_ref_count() helper to improve code maintainability
> and reduce duplication.
> 
> No functional changes intended.
> 
> Signed-off-by: Shivank Garg <shivankg@amd.com>
> ---
>   kernel/events/uprobes.c | 5 +++--
>   mm/memfd.c              | 4 ++--
>   2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4c965ba77f9f..c978c8c27340 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -434,10 +434,11 @@ static int __uprobe_write_opcode(struct vm_area_struct *vma,
>   	/*
>   	 * When unregistering, we may only zap a PTE if uffd is disabled and
>   	 * there are no unexpected folio references ...
> +	 * Expected refs: mappings + swapcache.
> +	 * We hold one additional reference (+1).
>   	 */
>   	if (is_register || userfaultfd_missing(vma) ||
> -	    (folio_ref_count(folio) != folio_mapcount(folio) + 1 +
> -	     folio_test_swapcache(folio) * folio_nr_pages(folio)))
> +	    (folio_ref_count(folio) != folio_expected_ref_count(folio) + 1))
>   		goto remap;
>   

With the comment removed (the caller holds a reference from GUP) or 
simplified ("Caller holds an additional folio reference.")

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

-- 
Cheers,

David / dhildenb

On 6/10/2025 12:38 PM, David Hildenbrand wrote:
> On 09.06.25 19:08, Shivank Garg wrote:
>> Replace open-coded folio reference count calculations with the
>> folio_expected_ref_count() helper to improve code maintainability
>> and reduce duplication.
>>
>> No functional changes intended.
>>
>> Signed-off-by: Shivank Garg <shivankg@amd.com>
>> ---
>>   kernel/events/uprobes.c | 5 +++--
>>   mm/memfd.c              | 4 ++--
>>   2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
>> index 4c965ba77f9f..c978c8c27340 100644
>> --- a/kernel/events/uprobes.c
>> +++ b/kernel/events/uprobes.c
>> @@ -434,10 +434,11 @@ static int __uprobe_write_opcode(struct vm_area_struct *vma,
>>       /*
>>        * When unregistering, we may only zap a PTE if uffd is disabled and
>>        * there are no unexpected folio references ...
>> +     * Expected refs: mappings + swapcache.
>> +     * We hold one additional reference (+1).
>>        */
>>       if (is_register || userfaultfd_missing(vma) ||
>> -        (folio_ref_count(folio) != folio_mapcount(folio) + 1 +
>> -         folio_test_swapcache(folio) * folio_nr_pages(folio)))
>> +        (folio_ref_count(folio) != folio_expected_ref_count(folio) + 1))
>>           goto remap;
>>   
> 
> With the comment removed (the caller holds a reference from GUP) or simplified ("Caller holds an additional folio reference.")
> 
Sure, I'll make this change in revision.

> Acked-by: David Hildenbrand <david@redhat.com>
> 
Thank you!

Best Regards,
Shivank

On Mon, Jun 09, 2025 at 05:08:07PM +0000, Shivank Garg wrote:
> Replace open-coded folio reference count calculations with the
> folio_expected_ref_count() helper to improve code maintainability
> and reduce duplication.

I wonder if there is any opportunity for reducing duplication more broadly?
The migration code has similar helpers (folio_expected_refs) as does
khugepaged (is_refcount_suitable) and vmscan (is_page_cache_freeable).
do_huge_pmd_wp_page() also has an open-coded version of these checks and there
are probably others around the place to.

These could all be converted to a helper that returns all the "extra" references
after taking into account things like mapping, swapcache, etc. depending on folio.

> No functional changes intended.
> 
> Signed-off-by: Shivank Garg <shivankg@amd.com>
> ---
>  kernel/events/uprobes.c | 5 +++--
>  mm/memfd.c              | 4 ++--
>  2 files changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
> index 4c965ba77f9f..c978c8c27340 100644
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -434,10 +434,11 @@ static int __uprobe_write_opcode(struct vm_area_struct *vma,
>  	/*
>  	 * When unregistering, we may only zap a PTE if uffd is disabled and
>  	 * there are no unexpected folio references ...
> +	 * Expected refs: mappings + swapcache.
> +	 * We hold one additional reference (+1).
>  	 */
>  	if (is_register || userfaultfd_missing(vma) ||
> -	    (folio_ref_count(folio) != folio_mapcount(folio) + 1 +
> -	     folio_test_swapcache(folio) * folio_nr_pages(folio)))
> +	    (folio_ref_count(folio) != folio_expected_ref_count(folio) + 1))
>  		goto remap;
>  
>  	/*
> diff --git a/mm/memfd.c b/mm/memfd.c
> index ab367e61553d..4ed5506221b7 100644
> --- a/mm/memfd.c
> +++ b/mm/memfd.c
> @@ -32,8 +32,8 @@
>  
>  static bool memfd_folio_has_extra_refs(struct folio *folio)
>  {
> -	return folio_ref_count(folio) - folio_mapcount(folio) !=
> -	       folio_nr_pages(folio);
> +	/* Expected refs: pagecache + mappings */
> +	return folio_ref_count(folio) != folio_expected_ref_count(folio);
>  }
>  
>  static void memfd_tag_pins(struct xa_state *xas)
> -- 
> 2.43.0
> 
>

On 6/10/2025 7:56 AM, Alistair Popple wrote:
> On Mon, Jun 09, 2025 at 05:08:07PM +0000, Shivank Garg wrote:
>> Replace open-coded folio reference count calculations with the
>> folio_expected_ref_count() helper to improve code maintainability
>> and reduce duplication.
> 
> I wonder if there is any opportunity for reducing duplication more broadly?
> The migration code has similar helpers (folio_expected_refs) as does
> khugepaged (is_refcount_suitable) and vmscan (is_page_cache_freeable).

The folio_expected_refs() and is_refcount_suitable() consolidation was 
recently merged:
- 86ebd50224c0 ("mm: add folio_expected_ref_count() for reference count calculation")
- 0b43b8bc8ef8 ("mm/khugepaged: clean up refcount check using folio_expected_ref_count()")

> do_huge_pmd_wp_page() also has an open-coded version of these checks and there
> are probably others around the place to.
> 
> These could all be converted to a helper that returns all the "extra" references
> after taking into account things like mapping, swapcache, etc. depending on folio.> 
>> No functional changes intended.
>>
>> Signed-off-by: Shivank Garg <shivankg@amd.com>
>> ---
>>  kernel/events/uprobes.c | 5 +++--
>>  mm/memfd.c              | 4 ++--
>>  2 files changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
>> index 4c965ba77f9f..c978c8c27340 100644
>> --- a/kernel/events/uprobes.c
>> +++ b/kernel/events/uprobes.c
>> @@ -434,10 +434,11 @@ static int __uprobe_write_opcode(struct vm_area_struct *vma,
>>  	/*
>>  	 * When unregistering, we may only zap a PTE if uffd is disabled and
>>  	 * there are no unexpected folio references ...
>> +	 * Expected refs: mappings + swapcache.
>> +	 * We hold one additional reference (+1).
>>  	 */
>>  	if (is_register || userfaultfd_missing(vma) ||
>> -	    (folio_ref_count(folio) != folio_mapcount(folio) + 1 +
>> -	     folio_test_swapcache(folio) * folio_nr_pages(folio)))
>> +	    (folio_ref_count(folio) != folio_expected_ref_count(folio) + 1))
>>  		goto remap;
>>  
>>  	/*
>> diff --git a/mm/memfd.c b/mm/memfd.c
>> index ab367e61553d..4ed5506221b7 100644
>> --- a/mm/memfd.c
>> +++ b/mm/memfd.c
>> @@ -32,8 +32,8 @@
>>  
>>  static bool memfd_folio_has_extra_refs(struct folio *folio)
>>  {
>> -	return folio_ref_count(folio) - folio_mapcount(folio) !=
>> -	       folio_nr_pages(folio);
>> +	/* Expected refs: pagecache + mappings */
>> +	return folio_ref_count(folio) != folio_expected_ref_count(folio);
>>  }
>>  
>>  static void memfd_tag_pins(struct xa_state *xas)
>> -- 
>> 2.43.0
>>
>>

On Mon, Jun 09, 2025 at 05:08:07PM +0000, Shivank Garg wrote:
> Replace open-coded folio reference count calculations with the
> folio_expected_ref_count() helper to improve code maintainability
> and reduce duplication.

If it needs this much additional commentary, perhaps it's not actually
clearer?

On June 9, 2025 3:21:20 PM EDT, Matthew Wilcox <willy@infradead.org> wrote:
>On Mon, Jun 09, 2025 at 05:08:07PM +0000, Shivank Garg wrote:
>> Replace open-coded folio reference count calculations with the
>> folio_expected_ref_count() helper to improve code maintainability
>> and reduce duplication.
>
>If it needs this much additional commentary, perhaps it's not actually
>clearer?

I don't know. I tend to over explain as I rather make it totally obvious what is happening. I wouldn't say excessive commentary is necessarily a sign that it's not clearer.

Not knowing the code, the explanation makes sense to me.

-- Steve 

On Mon, Jun 09, 2025 at 03:31:40PM -0400, Steven Rostedt wrote:
> 
> 
> On June 9, 2025 3:21:20 PM EDT, Matthew Wilcox <willy@infradead.org> wrote:
> >On Mon, Jun 09, 2025 at 05:08:07PM +0000, Shivank Garg wrote:
> >> Replace open-coded folio reference count calculations with the
> >> folio_expected_ref_count() helper to improve code maintainability
> >> and reduce duplication.
> >
> >If it needs this much additional commentary, perhaps it's not actually
> >clearer?
> 
> I don't know. I tend to over explain as I rather make it totally obvious what is happening. I wouldn't say excessive commentary is necessarily a sign that it's not clearer.

That was a Socratic question, not for you to answer.

My opinion is that the extra commentary is obfuscatory and should be
removed.

On 09.06.25 21:51, Matthew Wilcox wrote:
> On Mon, Jun 09, 2025 at 03:31:40PM -0400, Steven Rostedt wrote:
>>
>>
>> On June 9, 2025 3:21:20 PM EDT, Matthew Wilcox <willy@infradead.org> wrote:
>>> On Mon, Jun 09, 2025 at 05:08:07PM +0000, Shivank Garg wrote:
>>>> Replace open-coded folio reference count calculations with the
>>>> folio_expected_ref_count() helper to improve code maintainability
>>>> and reduce duplication.
>>>
>>> If it needs this much additional commentary, perhaps it's not actually
>>> clearer?
>>
>> I don't know. I tend to over explain as I rather make it totally obvious what is happening. I wouldn't say excessive commentary is necessarily a sign that it's not clearer.
> 
> That was a Socratic question, not for you to answer.
> 
> My opinion is that the extra commentary is obfuscatory and should be
> removed.

+1

-- 
Cheers,

David / dhildenb

On Mon, 9 Jun 2025 20:51:00 +0100
Matthew Wilcox <willy@infradead.org> wrote:

> On Mon, Jun 09, 2025 at 03:31:40PM -0400, Steven Rostedt wrote:
> > 
> > 
> > On June 9, 2025 3:21:20 PM EDT, Matthew Wilcox <willy@infradead.org> wrote:  
> > >On Mon, Jun 09, 2025 at 05:08:07PM +0000, Shivank Garg wrote:  
> > >> Replace open-coded folio reference count calculations with the
> > >> folio_expected_ref_count() helper to improve code maintainability
> > >> and reduce duplication.  
> > >
> > >If it needs this much additional commentary, perhaps it's not actually
> > >clearer?  
> > 
> > I don't know. I tend to over explain as I rather make it totally
> > obvious what is happening. I wouldn't say excessive commentary is
> > necessarily a sign that it's not clearer.  
> 
> That was a Socratic question, not for you to answer.
> 
> My opinion is that the extra commentary is obfuscatory and should be
> removed.


Ah, sorry, your response wasn't clear ;-)

-- Steve

On 6/10/2025 1:44 AM, Steven Rostedt wrote:
> On Mon, 9 Jun 2025 20:51:00 +0100
> Matthew Wilcox <willy@infradead.org> wrote:
> 
>> On Mon, Jun 09, 2025 at 03:31:40PM -0400, Steven Rostedt wrote:
>>>
>>>
>>> On June 9, 2025 3:21:20 PM EDT, Matthew Wilcox <willy@infradead.org> wrote:  
>>>> On Mon, Jun 09, 2025 at 05:08:07PM +0000, Shivank Garg wrote:  
>>>>> Replace open-coded folio reference count calculations with the
>>>>> folio_expected_ref_count() helper to improve code maintainability
>>>>> and reduce duplication.  
>>>>
>>>> If it needs this much additional commentary, perhaps it's not actually
>>>> clearer?  
>>>
>>> I don't know. I tend to over explain as I rather make it totally
>>> obvious what is happening. I wouldn't say excessive commentary is
>>> necessarily a sign that it's not clearer.  
>>
>> That was a Socratic question, not for you to answer.
>>
>> My opinion is that the extra commentary is obfuscatory and should be
>> removed.
> 
> 
> Ah, sorry, your response wasn't clear ;-)

Thank you Willy, Steve for the discussion.
Fair point about extra comments, I'll remove them.

Thanks,
Shivank