Hi Janusz,
On 2026-10-12 at 20:48:10 +0200, Janusz Krzysztofik wrote:
> Hi Krzysztof,
>
> This version finally looks good to me, however ...
>
> On Tue, 2026-10-06 at 09:58 +0000, Krzysztof Karas wrote:
> > Before addition of commit 029ae067431a
> > ("drm/i915: Fix potential overflow of shmem scatterlist length")
> > and after folios were introduced complete folios were always
> > allocated, possibly overloading the scatterlist capacity which
> > was never truly limited to PAGE_SIZE when requested via
> > max_segment. The above commit addressed scatterlist overloading,
> > but unintentionally disabled PAGE_SIZE as a valid max_segment
> > value and failed to take care of remaining pages from folios
> > above max_segment boundary.
> > This created a state, where multitude of scatterlists were used
> > for the same folio, but never counting enough of its pages to
> > jump to the next folio.
> >
> > Furthermore, current do-while loop goes over the same folio
> > multiple times, increasing the refcount each time, and breaks
> > get/put symmetry: many shmem_read_folio_gfp() gets and only one
> > put in i915_gem_object_put_pages_shmem() per 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")
> > Cc: [email protected]
> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/work_items/15816
> > Assisted-by: LLM
> > Signed-off-by: Krzysztof Karas <[email protected]>
> > ---
> > v7:
> > * Derive the page offset within the current folio from
> > next_pfn. (Janusz)
> > * Update next_pfn using that offset and clarify its role in
> > merging contiguous physical pages. (Andi)
> >
> > drivers/gpu/drm/i915/gem/i915_gem_shmem.c | 108 +++++++++++++---------
> > 1 file changed, 63 insertions(+), 45 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > index e35ee4bd456f..645da248e25d 100644
> > --- a/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > +++ b/drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> > @@ -70,7 +70,9 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> > struct sg_table *st,
> > unsigned int page_count; /* restricted by sg_alloc_table */
> > unsigned long i;
> > struct scatterlist *sg;
> > - unsigned long next_pfn = 0; /* suppress gcc warning */
> > + /* suppress gcc warnings */
> > + unsigned long next_pfn = 0;
>
> Why plural? Why moved to a separate line? Do we really need such
> comments? I'm not asking for dropping it, only not touching as not
> related, unless no longer applicable after your changes.
Nah, this is on me, I had more variables declared there in
previous versions and forgot to remove this change.
>
> > + struct folio *folio;
>
> However, if initialization of next_pfn is needed because its lack can still
> trigger a gcc warning, shouldn't we now also initialize folio before first
> use?
I'll check that.
>
> Thanks,
> Janusz
>
> > gfp_t noreclaim;
> > int ret;
> >
> > @@ -104,7 +106,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;
> > unsigned long nr_pages;
> > const unsigned int shrink[] = {
> > I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
> > @@ -112,51 +114,61 @@ 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);
> > + if (!i || (next_pfn >= folio_pfn(folio) +
> > folio_nr_pages(folio))) {
> > + /* Grab the next folio if we exhausted the current one.
> > */
> > + do {
> > + cond_resched();
> > + folio = shmem_read_folio_gfp(mapping, i, gfp);
> > + folio_page_index = 0;
> > + 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.
> > + * 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.
> > */
> > - gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> > - }
> > - } while (1);
> > + 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);
> > + } else {
> > + /*
> > + * Otherwise, we are still inside the same folio,
> > + * so just update its page index.
> > + */
> > + folio_page_index = next_pfn - folio_pfn(folio);
> > + }
> >
> > - nr_pages = min3(folio_nr_pages(folio),
> > + nr_pages = min3(folio_nr_pages(folio) - folio_page_index,
> > page_count - i, max_segment / PAGE_SIZE);
> >
> > if (!i ||
> > @@ -166,13 +178,19 @@ int shmem_sg_alloc_table(struct drm_i915_private
> > *i915, struct sg_table *st,
> > 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(nr_pages, (max_segment - sg->length) /
> > PAGE_SIZE);
> >
> > sg->length += nr_pages * PAGE_SIZE;
> > }
> > - next_pfn = folio_pfn(folio) + nr_pages;
> > +
> > + /*
> > + * Store the next PFN, so contiguous physical pages
> > + * can be merged into the same SG entry.
> > + */
> > + next_pfn = folio_pfn(folio) + folio_page_index + nr_pages;
> > i += nr_pages - 1;
> >
> > /* Check that the i965g/gm workaround works. */
--
Best Regards,
Krzysztof
