[Intel-gfx] [PATCH 2/2] drm/i915: Make pages of GFX allocations movable

[Chris Wilson]

On Wed, Mar 23, 2016 at 01:55:14PM +0530, Goel, Akash wrote:
> 
> 
> On 3/23/2016 1:28 PM, Chris Wilson wrote:
> >On Wed, Mar 23, 2016 at 11:39:44AM +0530, akash.goel at intel.com wrote:
> >>+#ifdef CONFIG_MIGRATION
> >>+static int i915_migratepage(struct address_space *mapping,
> >>+			    struct page *newpage, struct page *page,
> >>+			    enum migrate_mode mode, void *dev_priv_data)
> >
> >If we move this to i915_gem_shrink_migratepage (i.e. i915_gem_shrink),
> >we can
> >
> >>+	/*
> >>+	 * Use trylock here, with a timeout, for struct_mutex as
> >>+	 * otherwise there is a possibility of deadlock due to lock
> >>+	 * inversion. This path, which tries to migrate a particular
> >>+	 * page after locking that page, can race with a path which
> >>+	 * truncate/purge pages of the corresponding object (after
> >>+	 * acquiring struct_mutex). Since page truncation will also
> >>+	 * try to lock the page, a scenario of deadlock can arise.
> >>+	 */
> >>+	while (!mutex_trylock(&dev->struct_mutex) && --timeout)
> >>+		schedule_timeout_killable(1);
> >
> >replace this with i915_gem_shrinker_lock() and like constructs with the
> >other shrinkers.
> 
> fine, will rename the function to gem_shrink_migratepage, move it
> inside the gem_shrinker.c file, and use the existing constructs.
> 
> > Any reason for dropping the early
> > if (!page_private(obj)) skip?
> >
> 
> Would this sequence be fine ?
> 
> 	if (!page_private(page))
> 		goto migrate; /*skip */
> 
> 	Loop for locking mutex
> 
> 	obj = (struct drm_i915_gem_object *)page_private(page);
> 
> 	if (!PageSwapCache(page) && obj) {

Yes.

> >Similarly there are other patterns here that would benefit from
> >integration with existing shrinker logic. However, things like tidying
> >up the pin_display, unbinding, rpm lock inversion are still only on
> >list.
> 
> Tidying, like split that one single if condition into multiple if,
> else if blocks ?

Just outstanding patches that simplify the condition and work we have to
do here.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre