Re: [PATCH v3 1/5] drm/i915/gem: Count mapped pages in a folio

[Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>]

Hi Krzysztof,

Since reading your commit description I was not really able to understand
what the issue you were trying to fix was about, I'm providing some 
comments based on my understanding of the issue.

On Mon, 2026-07-13 at 09:58 +0000, Krzysztof Karas wrote:
> With addition of commit 029ae067431a
> ("drm/i915: Fix potential overflow of shmem scatterlist length")
> max_segment size was included in calculating a number of pages
> for the scatterlist. This meant that segment sizes considerably
> smaller than number of pages in a folio (see shmem_get_pages(),
> rebuild_st label for context), 

Two values of max_segment only could and still can be expected, either 
maximum capacity of a scatterlist, or PAGE_SIZE.  Before the blamed 
commit, that argument was mostly (unintentionally?) not respected when 
allocating pages from a folio to a scatterlist.  IOW, complete folios were 
always allocated, possibly overloading the scatterlist capacity,  
virtually never limited to PAGE_SIZE when requested via max_segment.

The blamed commit took care of scatterlist overloading, and
unintentionally also of respecting the max_segment == PAGE_SIZE case, by 
limiting the number of folio pages allocated to a single scatterlist not
to exceed max_segment, but failed to take care of correctly allocating 
remaining pages from folios larger than max_segment.  That's what this 
commit description should tell us about, I believe.

> were not enough to jump to the
> next folio, which has never been a problem before folios have
> been intoduced. In result, sg_set_folio() was called multiple
> times with nr_pages smaller than folio size, using multitude of
> scatterlists, 

I don't think it matters how many times sg_set_folio() was called or how 
many scatterlists were used, since we may expect them to be huge when 
shmem_sg_alloc_table() is called  with max_segment == PAGE_SIZE.

> all pointing to the beginning pages of the folio
> and never fully covering its range of pages.

Yeah, that's the real issue.  An offset within a folio should be tracked
and used when allocating pages still left in the folio.

> Track how many pages have already been counted in a folio

... and use that number as an offset on consecutive allocations from the 
same folio ...

> to ensure it is fully covered before reading next folio.

> Fixes: 029ae067431a ("drm/i915: Fix potential overflow of shmem scatterlist length")
> Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/15816
> Signed-off-by: Krzysztof Karas <krzysztof.karas@intel.com>
> ---
> v3:
>  * Fixed a bug that caused first folio to never be considered.
> 
>  drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 120 +++++++++++++---------
>  1 file changed, 70 insertions(+), 50 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> index 06543ae60706..0011d76f5b8c 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> @@ -68,10 +68,13 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  			 unsigned int max_segment)
>  {
>  	unsigned int page_count; /* restricted by sg_alloc_table */
> -	unsigned long i;
> +	unsigned long next_pfn = 0; /* suppress gcc warning */
> +	unsigned long folio_start = 0;
> +	unsigned long folio_end = 0;
> +	struct folio *folio = NULL;
>  	struct scatterlist *sg;
> -	unsigned long next_pfn = 0;	/* suppress gcc warning */
>  	gfp_t noreclaim;
> +	unsigned long i;
>  	int ret;
>  
>  	if (overflows_type(size / PAGE_SIZE, page_count))
> @@ -101,7 +104,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  	sg = st->sgl;
>  	st->nents = 0;
>  	for (i = 0; i < page_count; i++) {
> -		struct folio *folio;
> +		unsigned long folio_page_index = 0;
>  		unsigned long nr_pages;
>  		const unsigned int shrink[] = {
>  			I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
> @@ -109,71 +112,87 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  		}, *s = shrink;
>  		gfp_t gfp = noreclaim;
>  
> -		do {
> -			cond_resched();
> -			folio = shmem_read_folio_gfp(mapping, i, gfp);
> -			if (!IS_ERR(folio))
> -				break;
> -
> -			if (!*s) {
> -				ret = PTR_ERR(folio);
> -				goto err_sg;
> -			}
> -
> -			i915_gem_shrink(NULL, i915, 2 * page_count, NULL, *s++);
> -
> -			/*
> -			 * We've tried hard to allocate the memory by reaping
> -			 * our own buffer, now let the real VM do its job and
> -			 * go down in flames if truly OOM.
> -			 *
> -			 * However, since graphics tend to be disposable,
> -			 * defer the oom here by reporting the ENOMEM back
> -			 * to userspace.
> -			 */
> -			if (!*s) {
> -				/* reclaim and warn, but no oom */
> -				gfp = mapping_gfp_mask(mapping);
> +		/* Grab the next folio if we exhausted the current one. */
> +		if (!i || i > folio_end) {
> +			do {
> +				cond_resched();
> +				folio = shmem_read_folio_gfp(mapping, i, gfp);
> +				if (!IS_ERR(folio))
> +					break;
> +
> +				if (!*s) {
> +					ret = PTR_ERR(folio);
> +					goto err_sg;
> +				}
> +
> +				i915_gem_shrink(NULL, i915, 2 * page_count, NULL, *s++);
>  
>  				/*
> -				 * Our bo are always dirty and so we require
> -				 * kswapd to reclaim our pages (direct reclaim
> -				 * does not effectively begin pageout of our
> -				 * buffers on its own). However, direct reclaim
> -				 * only waits for kswapd when under allocation
> -				 * congestion. So as a result __GFP_RECLAIM is
> -				 * unreliable and fails to actually reclaim our
> -				 * dirty pages -- unless you try over and over
> -				 * again with !__GFP_NORETRY. However, we still
> -				 * want to fail this allocation rather than
> -				 * trigger the out-of-memory killer and for
> -				 * this we want __GFP_RETRY_MAYFAIL.
> -				 */
> -				gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> -			}
> -		} while (1);
> +				* We've tried hard to allocate the memory by reaping
> +				* our own buffer, now let the real VM do its job and
> +				* go down in flames if truly OOM.
> +				*
> +				* However, since graphics tend to be disposable,
> +				* defer the oom here by reporting the ENOMEM back
> +				* to userspace.
> +				*/
> +				if (!*s) {
> +					/* reclaim and warn, but no oom */
> +					gfp = mapping_gfp_mask(mapping);
> +
> +					/*
> +					 * Our bo are always dirty and so we require
> +					 * kswapd to reclaim our pages (direct reclaim
> +					 * does not effectively begin pageout of our
> +					 * buffers on its own). However, direct reclaim
> +					 * only waits for kswapd when under allocation
> +					 * congestion. So as a result __GFP_RECLAIM is
> +					 * unreliable and fails to actually reclaim our
> +					 * dirty pages -- unless you try over and over
> +					 * again with !__GFP_NORETRY. However, we still
> +					 * want to fail this allocation rather than
> +					 * trigger the out-of-memory killer and for
> +					 * this we want __GFP_RETRY_MAYFAIL.
> +					 */
> +					gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> +				}
> +			} while (1);
> +
> +			folio_start = folio_pgoff(folio);
> +			folio_end = folio_start + folio_nr_pages(folio) - 1;
> +		}
> +
> +		folio_page_index = i - folio_start;
> +		if (WARN_ON_ONCE(folio_page_index >= folio_nr_pages(folio))) {
> +			ret = -EINVAL;
> +			folio_put(folio);
> +			goto err_sg;
> +		}
>  
>  		nr_pages = min_array(((unsigned long[]) {
> -					folio_nr_pages(folio),
> +					folio_nr_pages(folio) - folio_page_index,
>  					page_count - i,
> -					max_segment / PAGE_SIZE,
> +					max_t(unsigned int, 1, max_segment / PAGE_SIZE),

Why?  This function is never called with max_segment less than PAGE_SIZE, 
then max_segment / PAGE_SIZE can't drop to 0.  If you want to address the 
unexpected 0 case then I think we should just validate max_segment before 
using it.

>  				      }), 3);
>  
>  		if (!i ||
>  		    sg->length >= max_segment ||
> -		    folio_pfn(folio) != next_pfn) {
> +		    folio_pfn(folio) + folio_page_index != next_pfn) {
>  			if (i)
>  				sg = sg_next(sg);
>  
>  			st->nents++;
> -			sg_set_folio(sg, folio, nr_pages * PAGE_SIZE, 0);
> +			sg_set_page(sg, folio_page(folio, folio_page_index),
> +				    nr_pages * PAGE_SIZE, 0);
>  		} else {
>  			nr_pages = min_t(unsigned long, nr_pages,
> -					 (max_segment - sg->length) / PAGE_SIZE);
> +					 max_t(unsigned long, 1,
> +					       (max_segment - sg->length) / PAGE_SIZE));

The same as above, we may exepect max_segment not less than PAGE_SIZE.

>  
>  			sg->length += nr_pages * PAGE_SIZE;

The only case for appending more pages to a not yet full scatterlist I can 
imagine is when first page of next folio is next to the last page of a 
previous folio assigned here before so contiguity of pages can be 
preserved.  Then, I think the above if conditions should better reflect 
that only case for clarity.

Thanks,
Janusz

>  		}
> -		next_pfn = folio_pfn(folio) + nr_pages;
> +
> +		next_pfn = folio_pfn(folio) + folio_page_index + nr_pages;
>  		i += nr_pages - 1;
>  
>  		/* Check that the i965g/gm workaround works. */
> @@ -186,6 +205,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
>  	i915_sg_trim(st);
>  
>  	return 0;
> +
>  err_sg:
>  	sg_mark_end(sg);
>  	if (sg != st->sgl) {