Re: How best to pin pages in physical memory?

Re: How best to pin pages in physical memory?

[Hugh Dickins]

On Sun, 26 Sep 2010, Chris Wilson wrote:

> This morning I came to the sickening conclusion that there is
> nothing in the drm/i915 driver that prevents the VM from swapping out
> pages mapped into the GTT (i.e. pages that are currently being written
> to or read from the GPU).

I'd expect that to be done by getting or raising the page_count on each
page needed, as get_user_pages() does for pages mapped into userspace.

And i915_gem_object_bind_to_gtt() calls a function
i915_get_object_get_pages() which appears to do just what's needed.
With i915_gem_object_unbind() calling i915_gem_object_put_pages().

> 
> The following patch is what I hastily threw together after grepping the
> sources for likely methods. A couple of considerations that need to be
> taken into account are:

Ingenious (or perhaps natural once you grep for shmem and lock: most
of us have forgotten mapping_set_unevictable if we ever knew it), but
potentially racy: what of pages already on their way to being evicted
when you call this?  SHM locking can tolerate that race, GEM cannot.
 
> 
> 1. Not all pages allocated through the shmfs get_pages() allocator that
> backs each GEM buffer object is mapped into the GTT. Though the actual
> quantity of such pages are small and temporary
> 
> 2. The GTT may be as large as 2GiB + a separate 2GiB that can be used
> for a per-process GTT.

There's a lot to be said for putting an amount that large on the
unevictable page list, instead of leaving it clogging up the lists
which vmscan searches for pages to evict: so your patch may be a
very good idea in the long run...

> 
> 3. Forced eviction is currently the shrinker, which may wait upon the
> GPU to finish and then unbind the pages from the GTT (and attempt to
> return the memory to the system). It might be useful to throttle the GPU
> and return the pages earlier to prevent the system from swapping?
> 
> If this looks like the continuation of the memory corruption saga caused
> by i915.ko during suspend, it is.

... but if you're trying to fix an i915 memory corruption issue, I bet
you are not all that interested in tuning mm's pageout heuristics at
the moment, and rightly so.

If i915_get_object_get_pages() isn't doing its job, I think you need to
wonder why not - maybe somewhere doing a put_page or page_cache_release,
freeing one or all pages too soon?

Hugh

> -Chris
> 
> ---
> From b3844cc3bd6fcbee7bbad640c91323f26904c1ce Mon Sep 17 00:00:00 2001
> From: Chris Wilson <chris@chris-wilson.co.uk>
> Date: Sun, 26 Sep 2010 10:11:53 +0100
> Subject: [PATCH] drm/i915: Mark pages mapped into the GTT as unevictable
> 
> If the GPU is currently reading and writing to pages, we need to prevent
> the VM from swapping those out to disk...
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/i915_gem.c |   19 +++++++++++++++++++
>  1 files changed, 19 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 9f547ab..5fa6227 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1532,6 +1532,23 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
>  	return 0;
>  }
>  
> +static void
> +i915_gem_object_lock_pages(struct drm_i915_gem_object *obj, bool lock)
> +{
> +	struct address_space *mapping;
> +	struct inode *inode;
> +
> +	inode = obj->base.filp->f_path.dentry->d_inode;
> +	mapping = inode->i_mapping;
> +
> +	if (lock) {
> +		mapping_set_unevictable(mapping);
> +	} else {
> +		mapping_clear_unevictable(mapping);
> +		scan_mapping_unevictable_pages(mapping);
> +	}
> +}
> +
>  void
>  i915_gem_object_put_pages(struct drm_gem_object *obj)
>  {
> @@ -2150,6 +2167,7 @@ i915_gem_object_unbind(struct drm_gem_object *obj)
>  
>  	list_del_init(&obj_priv->list);
>  
> +	i915_gem_object_lock_pages(obj_priv, false);
>  	if (i915_gem_object_is_purgeable(obj_priv))
>  		i915_gem_object_truncate(obj);
>  
> @@ -2751,6 +2769,7 @@ i915_gem_object_bind_to_gtt(struct drm_gem_object *obj,
>  
>  	obj_priv->mappable =
>  		obj_priv->gtt_offset + obj->size <= dev_priv->mm.gtt_mappable_end;
> +	i915_gem_object_lock_pages(obj_priv, true);
>  
>  	return 0;
>  }
> -- 
> 1.7.1
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: How best to pin pages in physical memory?

[Chris Wilson]

On Sun, 26 Sep 2010 16:29:23 -0700 (PDT), Hugh Dickins <hughd@google.com> wrote:
> If i915_get_object_get_pages() isn't doing its job, I think you need to
> wonder why not - maybe somewhere doing a put_page or page_cache_release,
> freeing one or all pages too soon?

Thanks, that's a very useful bit of review. I surmised that after seeing
corruption in a batch buffer after a swap storm that the pages were not as
safe required. The fact that swapping was involved could have been a
coincidence, except for the growing number of reports from other users
reporting crashes in conjunction with swapping.

So back to seeing whether i915_gem_object_[get/put]_pages is sufficient
for our uses.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>