[RFC PATCH 1/3] mm/memory.c: convert __copy_remote_vm_str() to folios

Vishal Moola (Oracle) posted 3 patches 3 months, 2 weeks ago
[RFC PATCH 1/3] mm/memory.c: convert __copy_remote_vm_str() to folios
Posted by Vishal Moola (Oracle) 3 months, 2 weeks ago
Use kmap_local_folio() instead of kmap_local_page().
Replaces 2 calls to compound_head() from unmap_and_put_page() with one.

This prepares us for the removal of unmap_and_put_page(), and helps
prepare for the eventual gup folio conversions since this function
now supports individual subpages from large folios.

Signed-off-by: Vishal Moola (Oracle) <vishal.moola@gmail.com>
---
 mm/memory.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 0f9b32a20e5b..747866060658 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -6820,9 +6820,10 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
 	}
 
 	while (len) {
-		int bytes, offset, retval;
+		int bytes, folio_offset, page_offset retval;
 		void *maddr;
 		struct page *page;
+		struct folio *folio;
 		struct vm_area_struct *vma = NULL;
 
 		page = get_user_page_vma_remote(mm, addr, gup_flags, &vma);
@@ -6837,17 +6838,20 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
 			goto out;
 		}
 
+		folio = page_folio(page);
 		bytes = len;
-		offset = addr & (PAGE_SIZE - 1);
-		if (bytes > PAGE_SIZE - offset)
-			bytes = PAGE_SIZE - offset;
+		folio_offset = offset_in_folio(folio, addr);
+		page_offset = offset_in_page(folio_offset);
+
+		if (bytes > PAGE_SIZE - page_offset)
+			bytes = PAGE_SIZE - page_offset;
 
-		maddr = kmap_local_page(page);
-		retval = strscpy(buf, maddr + offset, bytes);
+		maddr = kmap_local_folio(folio, folio_offset);
+		retval = strscpy(buf, maddr, bytes);
 		if (retval >= 0) {
 			/* Found the end of the string */
 			buf += retval;
-			unmap_and_put_page(page, maddr);
+			folio_release_kmap(folio, maddr);
 			break;
 		}
 
@@ -6859,13 +6863,16 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
 		 */
 		if (bytes != len) {
 			addr += bytes - 1;
-			copy_from_user_page(vma, page, addr, buf, maddr + (PAGE_SIZE - 1), 1);
+			copy_from_user_page(vma,
+				folio_page(folio, folio_offset / PAGE_SIZE),
+				addr, buf,
+				maddr + (PAGE_SIZE - page_offset - 1), 1);
 			buf += 1;
 			addr += 1;
 		}
 		len -= bytes;
 
-		unmap_and_put_page(page, maddr);
+		folio_release_kmap(folio, maddr);
 	}
 
 out:
-- 
2.49.0
Re: [RFC PATCH 1/3] mm/memory.c: convert __copy_remote_vm_str() to folios
Posted by Matthew Wilcox 3 months, 2 weeks ago
On Wed, Jun 25, 2025 at 10:48:39AM -0700, Vishal Moola (Oracle) wrote:
> +++ b/mm/memory.c
> @@ -6820,9 +6820,10 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
>  	}
>  
>  	while (len) {
> -		int bytes, offset, retval;
> +		int bytes, folio_offset, page_offset retval;

offset_in_folio() returns a size_t so that we can support folios larger
than 2GB (which is a real possibility here; hugetlbfs might end up with
a 16GB folio on some architectures).

> @@ -6837,17 +6838,20 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
>  			goto out;
>  		}
>  
> +		folio = page_folio(page);
>  		bytes = len;
> -		offset = addr & (PAGE_SIZE - 1);
> -		if (bytes > PAGE_SIZE - offset)
> -			bytes = PAGE_SIZE - offset;
> +		folio_offset = offset_in_folio(folio, addr);

Umm.  Not sure this is safe.  A folio might be mapped misaligned, so
'addr' might not give you the right offset within the folio.  I think
you might need to use addr - (vma->vm_pgoff << PAGE_SHIFT).  But I'd
defer to others here ... particularly when it comes to anonymous folios.
Re: [RFC PATCH 1/3] mm/memory.c: convert __copy_remote_vm_str() to folios
Posted by Matthew Wilcox 3 months, 2 weeks ago
On Wed, Jun 25, 2025 at 07:00:22PM +0100, Matthew Wilcox wrote:
> > +		folio_offset = offset_in_folio(folio, addr);
> 
> Umm.  Not sure this is safe.  A folio might be mapped misaligned, so
> 'addr' might not give you the right offset within the folio.  I think
> you might need to use addr - (vma->vm_pgoff << PAGE_SHIFT).  But I'd
> defer to others here ... particularly when it comes to anonymous folios.

Sorry, this calculation is obviously wrong.  It should be something like
the calculation in linear_page_index(), only without throwing away the
bottom PAGE_SHIFT bits.  But that's for file VMAs only, and I'm not sure
what should be done for anon vmas.  Possibly there is no way?
Re: [RFC PATCH 1/3] mm/memory.c: convert __copy_remote_vm_str() to folios
Posted by David Hildenbrand 3 months, 2 weeks ago
On 26.06.25 05:02, Matthew Wilcox wrote:
> On Wed, Jun 25, 2025 at 07:00:22PM +0100, Matthew Wilcox wrote:
>>> +		folio_offset = offset_in_folio(folio, addr);
>>
>> Umm.  Not sure this is safe.  A folio might be mapped misaligned, so
>> 'addr' might not give you the right offset within the folio.  I think
>> you might need to use addr - (vma->vm_pgoff << PAGE_SHIFT).  But I'd
>> defer to others here ... particularly when it comes to anonymous folios.
> 
> Sorry, this calculation is obviously wrong.  It should be something like
> the calculation in linear_page_index(), only without throwing away the
> bottom PAGE_SHIFT bits.  But that's for file VMAs only, and I'm not sure
> what should be done for anon vmas.  Possibly there is no way?

In __folio_set_anon(), we set

	folio->index = linear_page_index(vma, address);

and in __page_check_anon_rmap, we check

	VM_BUG_ON_PAGE(page_pgoff(folio, page) != linear_page_index(vma, address),

So given the VMA, addr, folio, we should be able to figure out the offset
into the folio.

... but I didn't have my second coffee yet.

-- 
Cheers,

David / dhildenb
Re: [RFC PATCH 1/3] mm/memory.c: convert __copy_remote_vm_str() to folios
Posted by Vishal Moola (Oracle) 3 months, 2 weeks ago
On Wed, Jun 25, 2025 at 07:00:22PM +0100, Matthew Wilcox wrote:
> On Wed, Jun 25, 2025 at 10:48:39AM -0700, Vishal Moola (Oracle) wrote:
> > +++ b/mm/memory.c
> > @@ -6820,9 +6820,10 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
> >  	}
> >  
> >  	while (len) {
> > -		int bytes, offset, retval;
> > +		int bytes, folio_offset, page_offset retval;
> 
> offset_in_folio() returns a size_t so that we can support folios larger
> than 2GB (which is a real possibility here; hugetlbfs might end up with
> a 16GB folio on some architectures).

Got it, I'll change that.

> > @@ -6837,17 +6838,20 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
> >  			goto out;
> >  		}
> >  
> > +		folio = page_folio(page);
> >  		bytes = len;
> > -		offset = addr & (PAGE_SIZE - 1);
> > -		if (bytes > PAGE_SIZE - offset)
> > -			bytes = PAGE_SIZE - offset;
> > +		folio_offset = offset_in_folio(folio, addr);
> 
> Umm.  Not sure this is safe.  A folio might be mapped misaligned, so
> 'addr' might not give you the right offset within the folio.  I think
> you might need to use addr - (vma->vm_pgoff << PAGE_SHIFT).  But I'd
> defer to others here ... particularly when it comes to anonymous folios.

Ah ok, I didn't realize folios could be misaligned. I'll play around
with your proposed calculation.
Re: [RFC PATCH 1/3] mm/memory.c: convert __copy_remote_vm_str() to folios
Posted by David Hildenbrand 3 months, 2 weeks ago
On 25.06.25 20:00, Matthew Wilcox wrote:
> On Wed, Jun 25, 2025 at 10:48:39AM -0700, Vishal Moola (Oracle) wrote:
>> +++ b/mm/memory.c
>> @@ -6820,9 +6820,10 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
>>   	}
>>   
>>   	while (len) {
>> -		int bytes, offset, retval;
>> +		int bytes, folio_offset, page_offset retval;
> 
> offset_in_folio() returns a size_t so that we can support folios larger
> than 2GB (which is a real possibility here; hugetlbfs might end up with
> a 16GB folio on some architectures).
> 
>> @@ -6837,17 +6838,20 @@ static int __copy_remote_vm_str(struct mm_struct *mm, unsigned long addr,
>>   			goto out;
>>   		}
>>   
>> +		folio = page_folio(page);
>>   		bytes = len;
>> -		offset = addr & (PAGE_SIZE - 1);
>> -		if (bytes > PAGE_SIZE - offset)
>> -			bytes = PAGE_SIZE - offset;
>> +		folio_offset = offset_in_folio(folio, addr);
> 
> Umm.  Not sure this is safe.  A folio might be mapped misaligned, so
> 'addr' might not give you the right offset within the folio.  I think
> you might need to use addr - (vma->vm_pgoff << PAGE_SHIFT).  But I'd
> defer to others here ... particularly when it comes to anonymous folios.

Not special to anon memory I think ... :)

Only the offset within a page is okay to derive (existing code).

-- 
Cheers,

David / dhildenb