Re: [PATCH 3/3] drm/i915: Remove __GFP_NORETRY from our buffer allocator

From: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
To: Chris Wilson <chris@chris-wilson.co.uk>,
	intel-gfx@lists.freedesktop.org, "Ursulin,
	Tvrtko" <tvrtko.ursulin@intel.com>,
	"Kuoppala, Mika" <mika.kuoppala@intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Subject: Re: [PATCH 3/3] drm/i915: Remove __GFP_NORETRY from our buffer allocator
Date: Fri, 02 Jun 2017 13:28:49 +0300	[thread overview]
Message-ID: <1496399329.3795.5.camel@linux.intel.com> (raw)
In-Reply-To: <20170601133331.5973-3-chris@chris-wilson.co.uk>

On to, 2017-06-01 at 14:33 +0100, Chris Wilson wrote:
> I tried __GFP_NORETRY in the belief that __GFP_RECLAIM was effective. It
> struggles with handling reclaim via kswapd (through inconsistency within
> throttle_direct_reclaim() and even then the race between multiple
> allocators makes the two step of reclaim then allocate fragile), and as
> our buffers are always dirty (with very few exceptions), we required
> kswapd to perform pageout on them. The only effective means of waiting
> on kswapd is to retry the allocations (i.e. not set __GFP_NORETRY). That
> leaves us with the dilemma of invoking the oomkiller instead of
> propagating the allocation failure back to userspace where it can be
> handled more gracefully (one hopes). We cheat and note that __GFP_THISNODE
> has the side-effect of preventing oom and has no consequence for our final
> attempt at allocation.
> 
> Fixes: 24f8e00a8a2e ("drm/i915: Prefer to report ENOMEM rather than incur the oom for gfx allocations")
> Testcase: igt/gem_tiled_swapping
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>

<SNIP>

> @@ -2406,7 +2406,21 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
>  			if (!*s) {
>  				/* reclaim and warn, but no oom */
>  				gfp = mapping_gfp_mask(mapping);
> -				gfp |= __GFP_NORETRY;
> +
> +				/* Our bo are always dirty and so we require
> +				 * kswapd to reclaim our pages (direct reclaim
> +				 * performs no swapping on its own). However,
> +				 * direct reclaim is meant to wait for kswapd
> +				 * when under pressure, this is broken. As a
> +				 * result __GFP_RECLAIM is unreliable and fails
> +				 * to actually reclaim dirty pages -- unless
> +				 * you try over and over again with
> +				 * !__GFP_NORETRY. However, we still want to
> +				 * fail this allocation rather than trigger
> +				 * the out-of-memory killer and for this we
> +				 * subvert __GFP_THISNODE for that side effect.

"side effect" sounds bad immediately, Daniel is on vacation so adding
Tvrtko and Mika here too.

Was this discussed with core MM?

Regards, Joonas

> +				 */
> +				gfp |= __GFP_THISNODE;
>  			}
>  		} while (1);
>  
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx