On Mon, 23 Mar 2020 at 13:17, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
> > ---
> > 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@xxxxxxxxxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxxxxxxxx
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
