[PATCH v11 05/16] mm/gup: drop local variable in gup_fast_folio_allowed

Kalyazin, Nikita posted 16 patches 2 weeks, 1 day ago
[PATCH v11 05/16] mm/gup: drop local variable in gup_fast_folio_allowed
Posted by Kalyazin, Nikita 2 weeks, 1 day ago
From: Nikita Kalyazin <kalyazin@amazon.com>

Move the check for pinning closer to where the result is used.
No functional changes.

Signed-off-by: Nikita Kalyazin <kalyazin@amazon.com>
---
 mm/gup.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/mm/gup.c b/mm/gup.c
index 5856d35be385..869d79c8daa4 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -2737,18 +2737,9 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
  */
 static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
 {
-	bool reject_file_backed = false;
 	struct address_space *mapping;
 	unsigned long mapping_flags;
 
-	/*
-	 * If we aren't pinning then no problematic write can occur. A long term
-	 * pin is the most egregious case so this is the one we disallow.
-	 */
-	if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) ==
-	    (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))
-		reject_file_backed = true;
-
 	/* We hold a folio reference, so we can safely access folio fields. */
 	if (WARN_ON_ONCE(folio_test_slab(folio)))
 		return false;
@@ -2793,8 +2784,18 @@ static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
 	 */
 	if (secretmem_mapping(mapping))
 		return false;
-	/* The only remaining allowed file system is shmem. */
-	return !reject_file_backed || shmem_mapping(mapping);
+
+	/*
+	 * If we aren't pinning then no problematic write can occur. A writable
+	 * long term pin is the most egregious case, so this is the one we
+	 * allow only for ...
+	 */
+	if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) !=
+	    (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))
+		return true;
+
+	/* ... hugetlb (which we allowed above already) and shared memory. */
+	return shmem_mapping(mapping);
 }
 
 #ifdef CONFIG_ARCH_HAS_PTE_SPECIAL
-- 
2.50.1

Re: [PATCH v11 05/16] mm/gup: drop local variable in gup_fast_folio_allowed
Posted by David Hildenbrand (Arm) 1 week, 2 days ago
On 3/17/26 15:11, Kalyazin, Nikita wrote:
> From: Nikita Kalyazin <kalyazin@amazon.com>
> 
> Move the check for pinning closer to where the result is used.
> No functional changes.
> 
> Signed-off-by: Nikita Kalyazin <kalyazin@amazon.com>
> ---
>  mm/gup.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
> 
> diff --git a/mm/gup.c b/mm/gup.c
> index 5856d35be385..869d79c8daa4 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -2737,18 +2737,9 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
>   */
>  static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
>  {
> -	bool reject_file_backed = false;
>  	struct address_space *mapping;
>  	unsigned long mapping_flags;
>  
> -	/*
> -	 * If we aren't pinning then no problematic write can occur. A long term
> -	 * pin is the most egregious case so this is the one we disallow.
> -	 */
> -	if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) ==
> -	    (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))
> -		reject_file_backed = true;
> -
>  	/* We hold a folio reference, so we can safely access folio fields. */
>  	if (WARN_ON_ONCE(folio_test_slab(folio)))
>  		return false;
> @@ -2793,8 +2784,18 @@ static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
>  	 */
>  	if (secretmem_mapping(mapping))
>  		return false;
> -	/* The only remaining allowed file system is shmem. */
> -	return !reject_file_backed || shmem_mapping(mapping);
> +
> +	/*
> +	 * If we aren't pinning then no problematic write can occur. A writable
> +	 * long term pin is the most egregious case, so this is the one we
> +	 * allow only for ...
> +	 */
> +	if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) !=
> +	    (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))
> +		return true;
> +
> +	/* ... hugetlb (which we allowed above already) and shared memory. */
> +	return shmem_mapping(mapping);

Acked-by: David Hildenbrand (Arm) <david@kernel.org>

I'm wondering if it would be a good idea to check for a hugetlb mapping
here instead of having the folio_test_hugetlb() check above.

Something to ponder about :)

-- 
Cheers,

David
Re: [PATCH v11 05/16] mm/gup: drop local variable in gup_fast_folio_allowed
Posted by Ackerley Tng 1 week, 2 days ago
"David Hildenbrand (Arm)" <david@kernel.org> writes:

> On 3/17/26 15:11, Kalyazin, Nikita wrote:
>> From: Nikita Kalyazin <kalyazin@amazon.com>
>>
>> Move the check for pinning closer to where the result is used.
>> No functional changes.
>>
>> Signed-off-by: Nikita Kalyazin <kalyazin@amazon.com>
>> ---
>>  mm/gup.c | 23 ++++++++++++-----------
>>  1 file changed, 12 insertions(+), 11 deletions(-)
>>
>> diff --git a/mm/gup.c b/mm/gup.c
>> index 5856d35be385..869d79c8daa4 100644
>> --- a/mm/gup.c
>> +++ b/mm/gup.c
>> @@ -2737,18 +2737,9 @@ EXPORT_SYMBOL(get_user_pages_unlocked);
>>   */
>>  static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
>>  {
>> -	bool reject_file_backed = false;
>>  	struct address_space *mapping;
>>  	unsigned long mapping_flags;
>>
>> -	/*
>> -	 * If we aren't pinning then no problematic write can occur. A long term
>> -	 * pin is the most egregious case so this is the one we disallow.
>> -	 */
>> -	if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) ==
>> -	    (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))
>> -		reject_file_backed = true;
>> -
>>  	/* We hold a folio reference, so we can safely access folio fields. */
>>  	if (WARN_ON_ONCE(folio_test_slab(folio)))
>>  		return false;
>> @@ -2793,8 +2784,18 @@ static bool gup_fast_folio_allowed(struct folio *folio, unsigned int flags)
>>  	 */
>>  	if (secretmem_mapping(mapping))
>>  		return false;
>> -	/* The only remaining allowed file system is shmem. */
>> -	return !reject_file_backed || shmem_mapping(mapping);
>> +
>> +	/*
>> +	 * If we aren't pinning then no problematic write can occur. A writable
>> +	 * long term pin is the most egregious case, so this is the one we
>> +	 * allow only for ...
>> +	 */
>> +	if ((flags & (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE)) !=
>> +	    (FOLL_PIN | FOLL_LONGTERM | FOLL_WRITE))
>> +		return true;
>> +
>> +	/* ... hugetlb (which we allowed above already) and shared memory. */
>> +	return shmem_mapping(mapping);
>
> Acked-by: David Hildenbrand (Arm) <david@kernel.org>
>
> I'm wondering if it would be a good idea to check for a hugetlb mapping
> here instead of having the folio_test_hugetlb() check above.
>

I think it's nice that hugetlb folios are determined immediately to be
eligible for GUP-fast regardless of whether the folio is file-backed or
not.

> Something to ponder about :)
>
> --
> Cheers,
>
> David