[Intel-gfx] [PATCH] drm/i915/selftests: mark huge_gem_object as not shrinkable

[Intel-gfx] [PATCH] drm/i915/selftests: mark huge_gem_object as not shrinkable

[Matthew Auld]

It looks like the callers expect a non-volatile object, but it looks the
shrinker will discard the object pages anyway, thinking that the pages
can be swapped out if the object is marked as WILLNEED. If that's true
then it might be better to mark it as volatile and fix the callers
instead, but on the other hand huge_gem_objects are fairly unique in
that they duplicate pages for the backing store, so maybe shrinking is
not that applicable.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
---
 drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
index fa16f2c3f3ac..2b46c6530da9 100644
--- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
+++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
@@ -88,8 +88,7 @@ static void huge_put_pages(struct drm_i915_gem_object *obj,
 }
 
 static const struct drm_i915_gem_object_ops huge_ops = {
-	.flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
-		 I915_GEM_OBJECT_IS_SHRINKABLE,
+	.flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
 	.get_pages = huge_get_pages,
 	.put_pages = huge_put_pages,
 };
-- 
2.20.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/selftests: mark huge_gem_object as not shrinkable

[Chris Wilson]

Quoting Matthew Auld (2020-03-23 13:08:21)
> It looks like the callers expect a non-volatile object, but it looks the
> shrinker will discard the object pages anyway, thinking that the pages
> can be swapped out if the object is marked as WILLNEED. If that's true
> then it might be better to mark it as volatile and fix the callers
> instead, but on the other hand huge_gem_objects are fairly unique in
> that they duplicate pages for the backing store, so maybe shrinking is
> not that applicable.

Duplication of backing store is irrelevant for the shrinker -- it just
deals with trying to make room by releasing objects. If we release the
entire object, all duplicate references are released and the pages
become recoverable.

Now as to whether the callers were expecting the object to be volatile
(for the backing pages to be discarded on swapping) is another question.
The answer would be that originally it was used with perma-pinned pages,
so it was never a problem. But looking at the users, they do *not*
expect to lose data on swapping.

So we need to fix the huge object to not gleefully throw away data,
which also means that we cannot shrink it (as there is no backing
storage to copy the pages to).

So both making the pages as DONTNEED and IS_SHRINKABLE are technically
incorrect.

> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> ---
>  drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> index fa16f2c3f3ac..2b46c6530da9 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> @@ -88,8 +88,7 @@ static void huge_put_pages(struct drm_i915_gem_object *obj,
>  }
>  
>  static const struct drm_i915_gem_object_ops huge_ops = {
> -       .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
> -                I915_GEM_OBJECT_IS_SHRINKABLE,
> +       .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

but I think the changelog can be clarified and we either include the
DONTNEED fixes or follow up.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/selftests: mark huge_gem_object as not shrinkable

[Matthew Auld]

On Mon, 23 Mar 2020 at 13:17, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>
> Quoting Matthew Auld (2020-03-23 13:08:21)
> > It looks like the callers expect a non-volatile object, but it looks the
> > shrinker will discard the object pages anyway, thinking that the pages
> > can be swapped out if the object is marked as WILLNEED. If that's true
> > then it might be better to mark it as volatile and fix the callers
> > instead, but on the other hand huge_gem_objects are fairly unique in
> > that they duplicate pages for the backing store, so maybe shrinking is
> > not that applicable.
>
> Duplication of backing store is irrelevant for the shrinker -- it just
> deals with trying to make room by releasing objects. If we release the
> entire object, all duplicate references are released and the pages
> become recoverable.

Ok. My rough thinking was that the shrinker page accounting(i.e what
we return) is currently based on obj->base.size, which might be pure
lies for huge_gem_object.

>
> Now as to whether the callers were expecting the object to be volatile
> (for the backing pages to be discarded on swapping) is another question.
> The answer would be that originally it was used with perma-pinned pages,
> so it was never a problem. But looking at the users, they do *not*
> expect to lose data on swapping.
>
> So we need to fix the huge object to not gleefully throw away data,
> which also means that we cannot shrink it (as there is no backing
> storage to copy the pages to).
>
> So both making the pages as DONTNEED and IS_SHRINKABLE are technically
> incorrect.

Do you mean WILLNEED and IS_SHRINKABLE, if object doesn't also support
swapping? DONTNEED and IS_SHRINKABLE is correct for volatile objects.

GEM_BUG_ON(is_shrinkable(obj) && !is_swappable(obj) && obj->mm.madv ==
WILLNEED);

>
> > Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> > ---
> >  drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c | 3 +--
> >  1 file changed, 1 insertion(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> > index fa16f2c3f3ac..2b46c6530da9 100644
> > --- a/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> > +++ b/drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
> > @@ -88,8 +88,7 @@ static void huge_put_pages(struct drm_i915_gem_object *obj,
> >  }
> >
> >  static const struct drm_i915_gem_object_ops huge_ops = {
> > -       .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE |
> > -                I915_GEM_OBJECT_IS_SHRINKABLE,
> > +       .flags = I915_GEM_OBJECT_HAS_STRUCT_PAGE,
>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> but I think the changelog can be clarified and we either include the
> DONTNEED fixes or follow up.
> -Chris
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [Intel-gfx] [PATCH] drm/i915/selftests: mark huge_gem_object as not shrinkable

[Chris Wilson]

Quoting Matthew Auld (2020-03-23 13:53:13)
> On Mon, 23 Mar 2020 at 13:17, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> >
> > Quoting Matthew Auld (2020-03-23 13:08:21)
> > > It looks like the callers expect a non-volatile object, but it looks the
> > > shrinker will discard the object pages anyway, thinking that the pages
> > > can be swapped out if the object is marked as WILLNEED. If that's true
> > > then it might be better to mark it as volatile and fix the callers
> > > instead, but on the other hand huge_gem_objects are fairly unique in
> > > that they duplicate pages for the backing store, so maybe shrinking is
> > > not that applicable.
> >
> > Duplication of backing store is irrelevant for the shrinker -- it just
> > deals with trying to make room by releasing objects. If we release the
> > entire object, all duplicate references are released and the pages
> > become recoverable.
> 
> Ok. My rough thinking was that the shrinker page accounting(i.e what
> we return) is currently based on obj->base.size, which might be pure
> lies for huge_gem_object.
> 
> >
> > Now as to whether the callers were expecting the object to be volatile
> > (for the backing pages to be discarded on swapping) is another question.
> > The answer would be that originally it was used with perma-pinned pages,
> > so it was never a problem. But looking at the users, they do *not*
> > expect to lose data on swapping.
> >
> > So we need to fix the huge object to not gleefully throw away data,
> > which also means that we cannot shrink it (as there is no backing
> > storage to copy the pages to).
> >
> > So both making the pages as DONTNEED and IS_SHRINKABLE are technically
> > incorrect.
> 
> Do you mean WILLNEED and IS_SHRINKABLE, if object doesn't also support
> swapping? DONTNEED and IS_SHRINKABLE is correct for volatile objects.
> 
> GEM_BUG_ON(is_shrinkable(obj) && !is_swappable(obj) && obj->mm.madv ==
> WILLNEED);

For the moment, we can just drop IS_SHRINKABLE here. But there is room
for a huge object that can take part in shrinker ops by being volatile.

But for the moment there's no pressing need, so we can defer the API
problem. (I'm thinking we'll need to expose i915_gem_object_madvise.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[Intel-gfx] ✗ Fi.CI.BAT: failure for drm/i915/selftests: mark huge_gem_object as not shrinkable

[Patchwork]

== Series Details ==

Series: drm/i915/selftests: mark huge_gem_object as not shrinkable
URL   : https://patchwork.freedesktop.org/series/74972/
State : failure

== Summary ==

Applying: drm/i915/selftests: mark huge_gem_object as not shrinkable
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/gem/selftests/huge_gem_object.c
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx