[Intel-gfx] [bug report] drm/i915: Allow compaction upto SWIOTLB max segment size
Tvrtko Ursulin
tvrtko.ursulin at linux.intel.com
Mon Feb 6 16:59:36 UTC 2023
On 06/02/2023 14:19, Dan Carpenter wrote:
> [ Ancient code but the warning showed up again because the function was
> renamed or something? - dan ]
>
> Hello Chris Wilson,
>
> The patch 871dfbd67d4e: "drm/i915: Allow compaction upto SWIOTLB max
> segment size" from Oct 11, 2016, leads to the following Smatch static
> checker warning:
>
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c:164 shmem_sg_alloc_table()
> warn: variable dereferenced before check 'sg' (see line 155)
Is smatch getting confused here? Not entirely sure but lets see below..
>
> drivers/gpu/drm/i915/gem/i915_gem_shmem.c
> 58 int shmem_sg_alloc_table(struct drm_i915_private *i915, struct sg_table *st,
> 59 size_t size, struct intel_memory_region *mr,
> 60 struct address_space *mapping,
> 61 unsigned int max_segment)
> 62 {
> 63 unsigned int page_count; /* restricted by sg_alloc_table */
> 64 unsigned long i;
> 65 struct scatterlist *sg;
> 66 struct page *page;
> 67 unsigned long last_pfn = 0; /* suppress gcc warning */
> 68 gfp_t noreclaim;
> 69 int ret;
> 70
> 71 if (overflows_type(size / PAGE_SIZE, page_count))
> 72 return -E2BIG;
> 73
> 74 page_count = size / PAGE_SIZE;
> 75 /*
> 76 * If there's no chance of allocating enough pages for the whole
> 77 * object, bail early.
> 78 */
> 79 if (size > resource_size(&mr->region))
> 80 return -ENOMEM;
> 81
> 82 if (sg_alloc_table(st, page_count, GFP_KERNEL | __GFP_NOWARN))
> 83 return -ENOMEM;
> 84
> 85 /*
> 86 * Get the list of pages out of our struct file. They'll be pinned
> 87 * at this point until we release them.
> 88 *
> 89 * Fail silently without starting the shrinker
> 90 */
> 91 mapping_set_unevictable(mapping);
> 92 noreclaim = mapping_gfp_constraint(mapping, ~__GFP_RECLAIM);
> 93 noreclaim |= __GFP_NORETRY | __GFP_NOWARN;
> 94
> 95 sg = st->sgl;
> ^^^^^^^^^^^^
> "sg" set here.
It is guaranteed to be non-NULL since sg_alloc_table succeeded.
>
> 96 st->nents = 0;
> 97 for (i = 0; i < page_count; i++) {
> 98 const unsigned int shrink[] = {
> 99 I915_SHRINK_BOUND | I915_SHRINK_UNBOUND,
> 100 0,
> 101 }, *s = shrink;
> 102 gfp_t gfp = noreclaim;
> 103
> 104 do {
> 105 cond_resched();
> 106 page = shmem_read_mapping_page_gfp(mapping, i, gfp);
> 107 if (!IS_ERR(page))
> 108 break;
>
> This should probably break out of the outer loop instead of the inner
> loop?
Don't think so, the loop has allocated a page and wants to break out the
inner loop so the page can be appended to the sg list.
>
> 109
> 110 if (!*s) {
> 111 ret = PTR_ERR(page);
> 112 goto err_sg;
> 113 }
> 114
> 115 i915_gem_shrink(NULL, i915, 2 * page_count, NULL, *s++);
> 116
> 117 /*
> 118 * We've tried hard to allocate the memory by reaping
> 119 * our own buffer, now let the real VM do its job and
> 120 * go down in flames if truly OOM.
> 121 *
> 122 * However, since graphics tend to be disposable,
> 123 * defer the oom here by reporting the ENOMEM back
> 124 * to userspace.
> 125 */
> 126 if (!*s) {
> 127 /* reclaim and warn, but no oom */
> 128 gfp = mapping_gfp_mask(mapping);
> 129
> 130 /*
> 131 * Our bo are always dirty and so we require
> 132 * kswapd to reclaim our pages (direct reclaim
> 133 * does not effectively begin pageout of our
> 134 * buffers on its own). However, direct reclaim
> 135 * only waits for kswapd when under allocation
> 136 * congestion. So as a result __GFP_RECLAIM is
> 137 * unreliable and fails to actually reclaim our
> 138 * dirty pages -- unless you try over and over
> 139 * again with !__GFP_NORETRY. However, we still
> 140 * want to fail this allocation rather than
> 141 * trigger the out-of-memory killer and for
> 142 * this we want __GFP_RETRY_MAYFAIL.
> 143 */
> 144 gfp |= __GFP_RETRY_MAYFAIL | __GFP_NOWARN;
> 145 }
> 146 } while (1);
> 147
> 148 if (!i ||
> 149 sg->length >= max_segment ||
> 150 page_to_pfn(page) != last_pfn + 1) {
> 151 if (i)
> 152 sg = sg_next(sg);
> 153
> 154 st->nents++;
> 155 sg_set_page(sg, page, PAGE_SIZE, 0);
> ^^
> Dereferenced.
Does smatch worry about the sg = sg_next(sg) here, or the initially
highlighted state? Even for the former we know we have allocated enough
entries (always allocate assuming worst possible fragmentation) so this
will still be valid whatever happens.
>
> 156 } else {
> 157 sg->length += PAGE_SIZE;
> ^^
> Here too.
>
> 158 }
> 159 last_pfn = page_to_pfn(page);
> 160
> 161 /* Check that the i965g/gm workaround works. */
> 162 GEM_BUG_ON(gfp & __GFP_DMA32 && last_pfn >= 0x00100000UL);
> 163 }
> --> 164 if (sg) /* loop terminated early; short sg table */
>
> If "sg" were NULL then we are already toasted.
AFAICT it can never be since the loop can never try to iterate past the
last sg entry.
Regards,
Tvrtko
>
> 165 sg_mark_end(sg);
> 166
> 167 /* Trim unused sg entries to avoid wasting memory. */
> 168 i915_sg_trim(st);
> 169
> 170 return 0;
> 171 err_sg:
> 172 sg_mark_end(sg);
> 173 if (sg != st->sgl) {
> 174 shmem_sg_free_table(st, mapping, false, false);
> 175 } else {
> 176 mapping_clear_unevictable(mapping);
> 177 sg_free_table(st);
> 178 }
> 179
> 180 /*
> 181 * shmemfs first checks if there is enough memory to allocate the page
> 182 * and reports ENOSPC should there be insufficient, along with the usual
> 183 * ENOMEM for a genuine allocation failure.
> 184 *
> 185 * We use ENOSPC in our driver to mean that we have run out of aperture
> 186 * space and so want to translate the error from shmemfs back to our
> 187 * usual understanding of ENOMEM.
> 188 */
> 189 if (ret == -ENOSPC)
> 190 ret = -ENOMEM;
> 191
> 192 return ret;
> 193 }
>
> regards,
> dan carpenter
More information about the Intel-gfx
mailing list