* [Intel-gfx] [bug report] drm/i915: Allow compaction upto SWIOTLB max segment size
@ 2023-02-06 14:19 Dan Carpenter
2023-02-06 16:59 ` Tvrtko Ursulin
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-02-06 14:19 UTC (permalink / raw)
To: chris; +Cc: intel-gfx
[ 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)
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.
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?
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.
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.
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [bug report] drm/i915: Allow compaction upto SWIOTLB max segment size
2023-02-06 14:19 [Intel-gfx] [bug report] drm/i915: Allow compaction upto SWIOTLB max segment size Dan Carpenter
@ 2023-02-06 16:59 ` Tvrtko Ursulin
2023-02-07 8:49 ` Dan Carpenter
0 siblings, 1 reply; 4+ messages in thread
From: Tvrtko Ursulin @ 2023-02-06 16:59 UTC (permalink / raw)
To: Dan Carpenter, chris; +Cc: intel-gfx
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [bug report] drm/i915: Allow compaction upto SWIOTLB max segment size
2023-02-06 16:59 ` Tvrtko Ursulin
@ 2023-02-07 8:49 ` Dan Carpenter
2023-02-07 9:29 ` Tvrtko Ursulin
0 siblings, 1 reply; 4+ messages in thread
From: Dan Carpenter @ 2023-02-07 8:49 UTC (permalink / raw)
To: Tvrtko Ursulin; +Cc: intel-gfx, chris
On Mon, Feb 06, 2023 at 04:59:36PM +0000, Tvrtko Ursulin wrote:
>
> 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..
Reading through your comments, it seems like you're saying the NULL
check should be deleted. I don't really consider that a "false positive".
Hopefully, we both agree that by the time we get to the check the "sg"
pointer has been dereferenced.
> >
> > 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.
>
Yeah. This doesn't matter. When I originally wrote this, I thought it
mattered but then I re-read the code but forgot to delete this comment.
In theory Smatch is supposed to be able to be tracking that "If
sg_alloc_table() succeeds, then "st->sgl" is non-NULL," but
__sg_alloc_table() has a do { } while() loop and Smatch is bad at
parsing loops.
However, Smatch does say that if sg_alloc_table() succeeds it means
page_count is non-zero.
> >
> > 96 st->nents = 0;
> > 97 for (i = 0; i < page_count; i++) {
Since page_count is non-zero we definitely enter this loop.
> > 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.
None of that really matters. What matters is that we dereference "sg"
at the end of every iteration through the loop.
Technically, it does slightly matter. If Smatch were able to determine
that a dereference is safe, then it doesn't print a warning. But Smatch
is probably always never going to know that sg_next() can't return NULL
here.
>
> >
> > 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 */
^^^^^^
> > 165 sg_mark_end(sg);
> >
> > 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.
Right. So this if statement can be deleted?
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [Intel-gfx] [bug report] drm/i915: Allow compaction upto SWIOTLB max segment size
2023-02-07 8:49 ` Dan Carpenter
@ 2023-02-07 9:29 ` Tvrtko Ursulin
0 siblings, 0 replies; 4+ messages in thread
From: Tvrtko Ursulin @ 2023-02-07 9:29 UTC (permalink / raw)
To: Dan Carpenter; +Cc: intel-gfx, chris
On 07/02/2023 08:49, Dan Carpenter wrote:
> On Mon, Feb 06, 2023 at 04:59:36PM +0000, Tvrtko Ursulin wrote:
>>
>> 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..
>
> Reading through your comments, it seems like you're saying the NULL
> check should be deleted. I don't really consider that a "false positive".
> Hopefully, we both agree that by the time we get to the check the "sg"
> pointer has been dereferenced.
>
>>>
>>> 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.
>>
>
> Yeah. This doesn't matter. When I originally wrote this, I thought it
> mattered but then I re-read the code but forgot to delete this comment.
>
> In theory Smatch is supposed to be able to be tracking that "If
> sg_alloc_table() succeeds, then "st->sgl" is non-NULL," but
> __sg_alloc_table() has a do { } while() loop and Smatch is bad at
> parsing loops.
>
> However, Smatch does say that if sg_alloc_table() succeeds it means
> page_count is non-zero.
>
>>>
>>> 96 st->nents = 0;
>>> 97 for (i = 0; i < page_count; i++) {
>
> Since page_count is non-zero we definitely enter this loop.
>
>>> 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.
>
> None of that really matters. What matters is that we dereference "sg"
> at the end of every iteration through the loop.
>
> Technically, it does slightly matter. If Smatch were able to determine
> that a dereference is safe, then it doesn't print a warning. But Smatch
> is probably always never going to know that sg_next() can't return NULL
> here.
>
>>
>>>
>>> 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 */
> ^^^^^^
>
>>> 165 sg_mark_end(sg);
>
>>>
>>> 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.
>
> Right. So this if statement can be deleted?
I think so, I don't see loop can exit with a null sg. Sg_mark_end()
still has to stay in case of i915_sg_trim below is not able to
re-allocate a more compact list.
Regards,
Tvrtko
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-02-07 9:29 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-02-06 14:19 [Intel-gfx] [bug report] drm/i915: Allow compaction upto SWIOTLB max segment size Dan Carpenter
2023-02-06 16:59 ` Tvrtko Ursulin
2023-02-07 8:49 ` Dan Carpenter
2023-02-07 9:29 ` Tvrtko Ursulin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox