| Message ID | 20200408170456.399604-1-matthew.auld@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/evict: watch out for unevictable nodes | expand |
Quoting Matthew Auld (2020-04-08 18:04:56) > In an address space there can be sprinkling of I915_COLOR_UNEVICTABLE > nodes, which lack a parent vma. For platforms with cache coloring we > might be very unlucky and abut with such a node thinking we can simply > unbind the vma. I did notice this myself recently (from observation); it's highly unlikely to ever matter. > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > --- > drivers/gpu/drm/i915/i915_gem_evict.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > index 4518b9b35c3d..9e462c6a4c6a 100644 > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > @@ -227,6 +227,10 @@ i915_gem_evict_something(struct i915_address_space *vm, > } > > while (ret == 0 && (node = drm_mm_scan_color_evict(&scan))) { > + /* If we find any non-objects (!vma), we cannot evict them */ > + if (node->color == I915_COLOR_UNEVICTABLE) > + return -ENOSPC; Returning error immediately looks ok, I was worried as around here we usually have lists to clean up, but this is after those. However, I would suggest that setting ret = -ENOSPC would be more consistent with the flow. Fwiw, we can actually pull this logic into evict_for_node for a bit of simplification. -Chris
Quoting Chris Wilson (2020-04-08 20:44:43) > Quoting Matthew Auld (2020-04-08 18:04:56) > > In an address space there can be sprinkling of I915_COLOR_UNEVICTABLE > > nodes, which lack a parent vma. For platforms with cache coloring we > > might be very unlucky and abut with such a node thinking we can simply > > unbind the vma. > > I did notice this myself recently (from observation); it's highly > unlikely to ever matter. > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > --- > > drivers/gpu/drm/i915/i915_gem_evict.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c > > index 4518b9b35c3d..9e462c6a4c6a 100644 > > --- a/drivers/gpu/drm/i915/i915_gem_evict.c > > +++ b/drivers/gpu/drm/i915/i915_gem_evict.c > > @@ -227,6 +227,10 @@ i915_gem_evict_something(struct i915_address_space *vm, > > } > > > > while (ret == 0 && (node = drm_mm_scan_color_evict(&scan))) { > > + /* If we find any non-objects (!vma), we cannot evict them */ > > + if (node->color == I915_COLOR_UNEVICTABLE) > > + return -ENOSPC; > > Returning error immediately looks ok, I was worried as around here we > usually have lists to clean up, but this is after those. However, I > would suggest that setting ret = -ENOSPC would be more consistent with > the flow. Actually, while this prevents an explosion [good], it now returns an unexpected error [bad]. No explosion is the lesser evil. -Chris
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c index 4518b9b35c3d..9e462c6a4c6a 100644 --- a/drivers/gpu/drm/i915/i915_gem_evict.c +++ b/drivers/gpu/drm/i915/i915_gem_evict.c @@ -227,6 +227,10 @@ i915_gem_evict_something(struct i915_address_space *vm, } while (ret == 0 && (node = drm_mm_scan_color_evict(&scan))) { + /* If we find any non-objects (!vma), we cannot evict them */ + if (node->color == I915_COLOR_UNEVICTABLE) + return -ENOSPC; + vma = container_of(node, struct i915_vma, node); ret = __i915_vma_unbind(vma); }
In an address space there can be sprinkling of I915_COLOR_UNEVICTABLE nodes, which lack a parent vma. For platforms with cache coloring we might be very unlucky and abut with such a node thinking we can simply unbind the vma. Signed-off-by: Matthew Auld <matthew.auld@intel.com> --- drivers/gpu/drm/i915/i915_gem_evict.c | 4 ++++ 1 file changed, 4 insertions(+)