linux-kernel - RE: [PATCH 2/2] drm/i915: Attempt to get pages without eviction first

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CH0PR11MB544412FB2B166632F4517535E5BE2@CH0PR11MB5444.namprd11.prod.outlook.com>
Date: Mon, 5 Aug 2024 20:11:11 +0000
From: "Cavitt, Jonathan" <jonathan.cavitt@...el.com>
To: David Gow <david@...idgow.net>, Christian König
	<ckoenig.leichtzumerken@...il.com>, Somalapuram Amaranath
	<Amaranath.Somalapuram@....com>, Thomas Hellström
	<thomas.hellstrom@...ux.intel.com>, "De Marchi, Lucas"
	<lucas.demarchi@...el.com>, Andi Shyti <andi.shyti@...ux.intel.com>, "Thomas
 Zimmermann" <tzimmermann@...e.de>
CC: Jani Nikula <jani.nikula@...ux.intel.com>, Joonas Lahtinen
	<joonas.lahtinen@...ux.intel.com>, "Vivi, Rodrigo" <rodrigo.vivi@...el.com>,
	Tvrtko Ursulin <tursulin@...ulin.net>, Ville Syrjälä
	<ville.syrjala@...ux.intel.com>, "intel-gfx@...ts.freedesktop.org"
	<intel-gfx@...ts.freedesktop.org>, "dri-devel@...ts.freedesktop.org"
	<dri-devel@...ts.freedesktop.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "Cavitt, Jonathan"
	<jonathan.cavitt@...el.com>
Subject: RE: [PATCH 2/2] drm/i915: Attempt to get pages without eviction first

-----Original Message-----
From: Intel-gfx <intel-gfx-bounces@...ts.freedesktop.org> On Behalf Of David Gow
Sent: Sunday, August 4, 2024 2:19 AM
To: Christian König <ckoenig.leichtzumerken@...il.com>; Somalapuram Amaranath <Amaranath.Somalapuram@....com>; Thomas Hellström <thomas.hellstrom@...ux.intel.com>; De Marchi, Lucas <lucas.demarchi@...el.com>; Andi Shyti <andi.shyti@...ux.intel.com>; Thomas Zimmermann <tzimmermann@...e.de>
Cc: David Gow <david@...idgow.net>; Jani Nikula <jani.nikula@...ux.intel.com>; Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>; Vivi, Rodrigo <rodrigo.vivi@...el.com>; Tvrtko Ursulin <tursulin@...ulin.net>; Ville Syrjälä <ville.syrjala@...ux.intel.com>; intel-gfx@...ts.freedesktop.org; dri-devel@...ts.freedesktop.org; linux-kernel@...r.kernel.org
Subject: [PATCH 2/2] drm/i915: Attempt to get pages without eviction first
> 
> In commit a78a8da51b36 ("drm/ttm: replace busy placement with flags v6"),
> __i915_ttm_get_pages was updated to use flags instead of the separate
> 'busy' placement list. However, the behaviour was subtly changed.
> Originally, the function would attempt to use the preferred placement
> without eviction, and give an opportunity to restart the operation
> before falling back to allowing eviction.
> 
> This was unintentionally changed, as the preferred placement was not
> given the TTM_PL_FLAG_DESIRED flag, and so eviction could be triggered
> in that first pass. This caused thrashing, and a significant performance
> regression on DG2 systems with small BAR. For example, Minecraft and
> Team Fortress 2 would drop to single-digit framerates.
> 
> Restore the original behaviour by marking the initial placement as
> desired on that first attempt. Also, rework this to use a separate
> struct ttm_palcement, as the individual placements are marked 'const',
> so hot-patching the flags is even more dodgy than before.
> 
> Thanks to Justin Brewer for bisecting this.
> 
> Fixes: a78a8da51b36 ("drm/ttm: replace busy placement with flags v6")
> Link: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/11255
> Signed-off-by: David Gow <david@...idgow.net>

Thank you for the thorough write-up of this issue.
Reviewed-by: Jonathan Cavitt <jonathan.cavitt@...el.com>
-Jonathan Cavitt

> ---
>  drivers/gpu/drm/i915/gem/i915_gem_ttm.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> index fb848fd8ba15..5c72462d1f57 100644
> --- a/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> +++ b/drivers/gpu/drm/i915/gem/i915_gem_ttm.c
> @@ -778,13 +778,16 @@ static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj,
>  		.interruptible = true,
>  		.no_wait_gpu = false,
>  	};
> -	int real_num_busy;
> +	struct ttm_placement initial_placement;
> +	struct ttm_place initial_place;
>  	int ret;
>  
>  	/* First try only the requested placement. No eviction. */
> -	real_num_busy = placement->num_placement;
> -	placement->num_placement = 1;
> -	ret = ttm_bo_validate(bo, placement, &ctx);
> +	initial_placement.num_placement = 1;
> +	memcpy(&initial_place, placement->placement, sizeof(struct ttm_place));
> +	initial_place.flags |= TTM_PL_FLAG_DESIRED;
> +	initial_placement.placement = &initial_place;
> +	ret = ttm_bo_validate(bo, &initial_placement, &ctx);
>  	if (ret) {
>  		ret = i915_ttm_err_to_gem(ret);
>  		/*
> @@ -799,7 +802,6 @@ static int __i915_ttm_get_pages(struct drm_i915_gem_object *obj,
>  		 * If the initial attempt fails, allow all accepted placements,
>  		 * evicting if necessary.
>  		 */
> -		placement->num_placement = real_num_busy;
>  		ret = ttm_bo_validate(bo, placement, &ctx);
>  		if (ret)
>  			return i915_ttm_err_to_gem(ret);
> -- 
> 2.46.0
> 
>