Hi Janusz,
[...]
> >
> > - do {
> > - cond_resched();
> > - folio = shmem_read_folio_gfp(mapping, i, gfp);
> > - if (!IS_ERR(folio))
> > - break;
> > + /* Grab the next folio if we exhausted the current one. */
>
> I think that even with folio_start < i < folio_end,
> shmem_read_folio_gfp(..., i, ...) should always return the right folio, so
> we shouldn't worry about getting the same folio again in a relatively rare
> case of the folio size exceeding a scatterlist capacity, we only need to
> take care of tracking an offset within that folio. Then, ...
>
> > + 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;
> > - }
> > + 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);
> > + 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);
> > +
> > + 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;
> > + }
>
> ... the existing code above, including the existing 'do' loop, may be left
> untouched, I believe, an your folio_page_index can easily be calculated
> from an already maintained next_pfn as:
>
> + folio_page_index = next_pfn - folio_pfn(folio);
> + if (folio_page_index < 0 || folio_page_index >=
> folio_nr_pages(folio))
> + folio_page_index = 0;
>
> IOW, we need to calculate and apply an offset within the folio only if
> next_pfn is still within the folio's PFN range, otherwise that must be a
> new folio and the offset we apply must be 0.
>
> Then, unless I'm missing something, I believe the patch could be much more
> compact while still correct with my approach. However, if other reviewers
> are more OK with your proposed changes rather than what I suggest then I
> won't oppose.
Leaving the current do/while loop as is means we get multiple
references to a folio per folio with shmem_read_folio_gfp(), but
only one put in i915_gem_object_put_pages_shmem().
Looking at this again, it should be addressed explicitly in
commit message.
>
> >
> > nr_pages = min_array(((unsigned long[]) {
> > - folio_nr_pages(folio),
> > + folio_nr_pages(folio) -
> > folio_page_index,
> > page_count - i,
> > max_segment / PAGE_SIZE,
> > }), 3);
> >
> > if (!i ||
> > sg->length >= max_segment ||
> > - folio_pfn(folio) != next_pfn) {
> > + folio_pfn(folio) + folio_page_index != next_pfn) {
>
> I think the existing condition was correct, and still applicable even when
> tracking potential offsets within folios: when there is still some room in
> the current scatterlist, enter the else part that fills up the scatterlist
> with more data only if we've got a new folio that starts exactly where
> the previous one ended to have contiguity of pages preserved, otherwise
> switch to a new scatterlist.
You are right.
>
> > 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 {
> > + /*
> > + * If our prediction about folio placement is true and
> > + * scatterlist still has space left for more pages,
> > + * then we land here.
> > + */
> > nr_pages = min_t(unsigned long, nr_pages,
> > (max_segment - sg->length) /
> > PAGE_SIZE);
> >
> > sg->length += nr_pages * PAGE_SIZE;
> > }
> > - next_pfn = folio_pfn(folio) + nr_pages;
> > +
> > + /*
> > + * We assume folios are placed one after the other in memory
> > + * and predict where the next folio begins.
> > + */
> > + next_pfn = folio_pfn(folio) + folio_page_index + nr_pages;
>
>
> > i += nr_pages - 1;
> >
> > /* Check that the i965g/gm workaround works. */
> > @@ -186,6 +216,7 @@ int shmem_sg_alloc_table(struct drm_i915_private *i915,
> > struct sg_table *st,
> > i915_sg_trim(st);
> >
> > return 0;
> > +
>
> Not related, I wouldn't mix it in, unless you have a good justification.
Yes, Andi also mentioned this in his review, so I'll remove this
diff.
Thank you for looking at this!
>
> Thanks,
> Janusz
>
> > err_sg:
> > sg_mark_end(sg);
> > if (sg != st->sgl) {
--
Best Regards,
Krzysztof
