Re: [Intel-gfx] [PATCH] drm/i915/selftests: fixup igt_shrink_thp

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 06/09/2021 10:17, Matthew Auld wrote:
> Since the object might still be active here, the shrink_all will simply
> ignore it, which blows up in the test, since the pages will still be
> there. Currently THP is disabled which should result in the test being
> skipped, but if we ever re-enable THP we might start seeing the failure.
> Fix this by forcing I915_SHRINK_ACTIVE.
> 
> v2: Some machine in the shard runs doesn't seem to have any available
> swap when running this test. Try to handle this.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Cc: Thomas Hellström <thomas.hellstrom@linux.intel.com>
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com> #v1
> ---
>   .../gpu/drm/i915/gem/selftests/huge_pages.c   | 31 ++++++++++++++-----
>   1 file changed, 24 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> index a094f3ce1a90..46ea1997c114 100644
> --- a/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> +++ b/drivers/gpu/drm/i915/gem/selftests/huge_pages.c
> @@ -1519,6 +1519,7 @@ static int igt_shrink_thp(void *arg)
>   	struct i915_vma *vma;
>   	unsigned int flags = PIN_USER;
>   	unsigned int n;
> +	bool should_swap;
>   	int err = 0;
>   
>   	/*
> @@ -1567,23 +1568,39 @@ static int igt_shrink_thp(void *arg)
>   			break;
>   	}
>   	i915_gem_context_unlock_engines(ctx);
> +	/*
> +	 * Nuke everything *before* we unpin the pages so we can be reasonably
> +	 * sure that when later checking get_nr_swap_pages() that some random
> +	 * leftover object doesn't steal the remaining swap space.
> +	 */
> +	i915_gem_shrink(NULL, i915, -1UL, NULL,
> +			I915_SHRINK_BOUND |
> +			I915_SHRINK_UNBOUND |
> +			I915_SHRINK_ACTIVE);
>   	i915_vma_unpin(vma);
>   	if (err)
>   		goto out_put;
>   
> +
>   	/*
> -	 * Now that the pages are *unpinned* shrink-all should invoke
> -	 * shmem to truncate our pages.
> +	 * Now that the pages are *unpinned* shrinking should invoke
> +	 * shmem to truncate our pages, if we have available swap.
>   	 */
> -	i915_gem_shrink_all(i915);
> -	if (i915_gem_object_has_pages(obj)) {
> -		pr_err("shrink-all didn't truncate the pages\n");
> +	should_swap = get_nr_swap_pages() > 0;
> +	i915_gem_shrink(NULL, i915, -1UL, NULL,
> +			I915_SHRINK_BOUND |
> +			I915_SHRINK_UNBOUND |
> +			I915_SHRINK_ACTIVE);
> +	if (should_swap == i915_gem_object_has_pages(obj)) {

Hmm is there any value running the test if no swap (given objects used 
by the test are "willneed"), or you could simplify and just do early skip?

Regards,

Tvrtko

> +		pr_err("unexpected pages mismatch, should_swap=%s\n",
> +		       yesno(should_swap));
>   		err = -EINVAL;
>   		goto out_put;
>   	}
>   
> -	if (obj->mm.page_sizes.sg || obj->mm.page_sizes.phys) {
> -		pr_err("residual page-size bits left\n");
> +	if (should_swap == (obj->mm.page_sizes.sg || obj->mm.page_sizes.phys)) {
> +		pr_err("unexpected residual page-size bits, should_swap=%s\n",
> +		       yesno(should_swap));
>   		err = -EINVAL;
>   		goto out_put;
>   	}
>