[PATCH v1 33/36] mm/gup: drop nth_page() usage in unpin_user_page_range_dirty_lock()

David Hildenbrand posted 36 patches 1 month ago
There is a newer version of this series
[PATCH v1 33/36] mm/gup: drop nth_page() usage in unpin_user_page_range_dirty_lock()
Posted by David Hildenbrand 1 month ago
There is the concern that unpin_user_page_range_dirty_lock() might do
some weird merging of PFN ranges -- either now or in the future -- such
that PFN range is contiguous but the page range might not be.

Let's sanity-check for that and drop the nth_page() usage.

Signed-off-by: David Hildenbrand <david@redhat.com>
---
 mm/gup.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mm/gup.c b/mm/gup.c
index 89ca0813791ab..c24f6009a7a44 100644
--- a/mm/gup.c
+++ b/mm/gup.c
@@ -237,7 +237,7 @@ void folio_add_pin(struct folio *folio)
 static inline struct folio *gup_folio_range_next(struct page *start,
 		unsigned long npages, unsigned long i, unsigned int *ntails)
 {
-	struct page *next = nth_page(start, i);
+	struct page *next = start + i;
 	struct folio *folio = page_folio(next);
 	unsigned int nr = 1;
 
@@ -342,6 +342,9 @@ EXPORT_SYMBOL(unpin_user_pages_dirty_lock);
  * "gup-pinned page range" refers to a range of pages that has had one of the
  * pin_user_pages() variants called on that page.
  *
+ * The page range must be truly contiguous: the page range corresponds
+ * to a contiguous PFN range and all pages can be iterated naturally.
+ *
  * For the page ranges defined by [page .. page+npages], make that range (or
  * its head pages, if a compound page) dirty, if @make_dirty is true, and if the
  * page range was previously listed as clean.
@@ -359,6 +362,8 @@ void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
 	struct folio *folio;
 	unsigned int nr;
 
+	VM_WARN_ON_ONCE(!page_range_contiguous(page, npages));
+
 	for (i = 0; i < npages; i += nr) {
 		folio = gup_folio_range_next(page, npages, i, &nr);
 		if (make_dirty && !folio_test_dirty(folio)) {
-- 
2.50.1
Re: [PATCH v1 33/36] mm/gup: drop nth_page() usage in unpin_user_page_range_dirty_lock()
Posted by Lorenzo Stoakes 1 month ago
On Thu, Aug 28, 2025 at 12:01:37AM +0200, David Hildenbrand wrote:
> There is the concern that unpin_user_page_range_dirty_lock() might do
> some weird merging of PFN ranges -- either now or in the future -- such
> that PFN range is contiguous but the page range might not be.
>
> Let's sanity-check for that and drop the nth_page() usage.
>
> Signed-off-by: David Hildenbrand <david@redhat.com>

Seems one user uses SG and the other is IOMMU and in each instance you'd
expect physical contiguity (maybe Jason G. or somebody else more familiar
with these uses can also chime in).

Anyway, on that basis, LGTM (though 1 small nit below), so:

Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@oracle.com>

> ---
>  mm/gup.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/mm/gup.c b/mm/gup.c
> index 89ca0813791ab..c24f6009a7a44 100644
> --- a/mm/gup.c
> +++ b/mm/gup.c
> @@ -237,7 +237,7 @@ void folio_add_pin(struct folio *folio)
>  static inline struct folio *gup_folio_range_next(struct page *start,
>  		unsigned long npages, unsigned long i, unsigned int *ntails)
>  {
> -	struct page *next = nth_page(start, i);
> +	struct page *next = start + i;
>  	struct folio *folio = page_folio(next);
>  	unsigned int nr = 1;
>
> @@ -342,6 +342,9 @@ EXPORT_SYMBOL(unpin_user_pages_dirty_lock);
>   * "gup-pinned page range" refers to a range of pages that has had one of the
>   * pin_user_pages() variants called on that page.
>   *
> + * The page range must be truly contiguous: the page range corresponds

NIT: maybe 'physically contiguous'?

> + * to a contiguous PFN range and all pages can be iterated naturally.
> + *
>   * For the page ranges defined by [page .. page+npages], make that range (or
>   * its head pages, if a compound page) dirty, if @make_dirty is true, and if the
>   * page range was previously listed as clean.
> @@ -359,6 +362,8 @@ void unpin_user_page_range_dirty_lock(struct page *page, unsigned long npages,
>  	struct folio *folio;
>  	unsigned int nr;
>
> +	VM_WARN_ON_ONCE(!page_range_contiguous(page, npages));
> +
>  	for (i = 0; i < npages; i += nr) {
>  		folio = gup_folio_range_next(page, npages, i, &nr);
>  		if (make_dirty && !folio_test_dirty(folio)) {
> --
> 2.50.1
>
Re: [PATCH v1 33/36] mm/gup: drop nth_page() usage in unpin_user_page_range_dirty_lock()
Posted by David Hildenbrand 1 month ago
On 28.08.25 20:09, Lorenzo Stoakes wrote:
> On Thu, Aug 28, 2025 at 12:01:37AM +0200, David Hildenbrand wrote:
>> There is the concern that unpin_user_page_range_dirty_lock() might do
>> some weird merging of PFN ranges -- either now or in the future -- such
>> that PFN range is contiguous but the page range might not be.
>>
>> Let's sanity-check for that and drop the nth_page() usage.
>>
>> Signed-off-by: David Hildenbrand <david@redhat.com>
> 
> Seems one user uses SG and the other is IOMMU and in each instance you'd
> expect physical contiguity (maybe Jason G. or somebody else more familiar
> with these uses can also chime in).

Right, and I added the sanity-check so we can identify and fix any such 
wrong merging of ranges.

Thanks!

-- 
Cheers

David / dhildenb