[PATCH 1/4] drm/i915: Translate ENOSPC from shmem_get_page() to ENOMEM

[PATCH 1/4] drm/i915: Translate ENOSPC from shmem_get_page() to ENOMEM

[Chris Wilson]

shmemfs first checks if there is enough memory to allocate the page
and reports ENOSPC should there be insufficient, along with
the usual ENOMEM for a genuine allocation failure.

We use ENOSPC in our driver to mean that we have run out of aperture
space and so want to translate the error from shmemfs back to
our usual understanding of ENOMEM. None of the the other GEM users
appear to distinguish between ENOMEM and ENOSPC in their error handling,
hence it is easiest to do the fixup in i915.ko

Cc: Hugh Dickins <hughd@google.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 33bbaa0d4412..c23034021753 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -1976,7 +1976,19 @@ err_pages:
 		page_cache_release(sg_page_iter_page(&sg_iter));
 	sg_free_table(st);
 	kfree(st);
-	return PTR_ERR(page);
+
+	/* shmemfs first checks if there is enough memory to allocate the page
+	 * and reports ENOSPC should there be insufficient, along with the usual
+	 * ENOMEM for a genuine allocation failure.
+	 *
+	 * We use ENOSPC in our driver to mean that we have run out of aperture
+	 * space and so want to translate the error from shmemfs back to our
+	 * usual understanding of ENOMEM.
+	 */
+	if (PTR_ERR(page) == -ENOSPC)
+		return -ENOMEM;
+	else
+		return PTR_ERR(page);
 }
 
 /* Ensure that the associated pages are gathered from the backing storage
-- 
1.9.1

[PATCH 2/4] drm/i915: Include bound and active pages in the count of shrinkable objects

[Chris Wilson]

When the machine is under a lot of memory pressure and being stressed by
multiple GPU threads, we quite often report fewer than shrinker->batch
(i.e. SHRINK_BATCH) pages to be freed. This causes the shrink_control to
skip calling into i915.ko to release pages, despite the GPU holding onto
most of the physical pages in its active lists.

References: https://bugs.freedesktop.org/show_bug.cgi?id=72742
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_dma.c |  8 ++++----
 drivers/gpu/drm/i915/i915_drv.h |  2 +-
 drivers/gpu/drm/i915/i915_gem.c | 42 +++++++++++++++++++++++------------------
 3 files changed, 29 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index 4e0a26a83500..5a37c75f4b3d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1739,8 +1739,8 @@ out_power_well:
 	intel_power_domains_remove(dev_priv);
 	drm_vblank_cleanup(dev);
 out_gem_unload:
-	if (dev_priv->mm.inactive_shrinker.scan_objects)
-		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
+	if (dev_priv->mm.shrinker.scan_objects)
+		unregister_shrinker(&dev_priv->mm.shrinker);
 
 	if (dev->pdev->msi_enabled)
 		pci_disable_msi(dev->pdev);
@@ -1791,8 +1791,8 @@ int i915_driver_unload(struct drm_device *dev)
 
 	i915_teardown_sysfs(dev);
 
-	if (dev_priv->mm.inactive_shrinker.scan_objects)
-		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
+	if (dev_priv->mm.shrinker.scan_objects)
+		unregister_shrinker(&dev_priv->mm.shrinker);
 
 	io_mapping_free(dev_priv->gtt.mappable);
 	arch_phys_wc_del(dev_priv->gtt.mtrr);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index a7ad864f1154..cb4bb171e6cc 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -986,7 +986,7 @@ struct i915_gem_mm {
 	/** PPGTT used for aliasing the PPGTT with the GTT */
 	struct i915_hw_ppgtt *aliasing_ppgtt;
 
-	struct shrinker inactive_shrinker;
+	struct shrinker shrinker;
 	bool shrinker_no_lock_stealing;
 
 	/** LRU list of objects with fence regs on them. */
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index c23034021753..219fe35f9c45 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -54,9 +54,9 @@ static void i915_gem_object_update_fence(struct drm_i915_gem_object *obj,
 					 struct drm_i915_fence_reg *fence,
 					 bool enable);
 
-static unsigned long i915_gem_inactive_count(struct shrinker *shrinker,
+static unsigned long i915_gem_shrinker_count(struct shrinker *shrinker,
 					     struct shrink_control *sc);
-static unsigned long i915_gem_inactive_scan(struct shrinker *shrinker,
+static unsigned long i915_gem_shrinker_scan(struct shrinker *shrinker,
 					    struct shrink_control *sc);
 static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long target);
 static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
@@ -4651,10 +4651,10 @@ i915_gem_load(struct drm_device *dev)
 
 	dev_priv->mm.interruptible = true;
 
-	dev_priv->mm.inactive_shrinker.scan_objects = i915_gem_inactive_scan;
-	dev_priv->mm.inactive_shrinker.count_objects = i915_gem_inactive_count;
-	dev_priv->mm.inactive_shrinker.seeks = DEFAULT_SEEKS;
-	register_shrinker(&dev_priv->mm.inactive_shrinker);
+	dev_priv->mm.shrinker.scan_objects = i915_gem_shrinker_scan;
+	dev_priv->mm.shrinker.count_objects = i915_gem_shrinker_count;
+	dev_priv->mm.shrinker.seeks = DEFAULT_SEEKS;
+	register_shrinker(&dev_priv->mm.shrinker);
 }
 
 /*
@@ -4913,13 +4913,23 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
 #endif
 }
 
+static int num_vma_bound(struct drm_i915_gem_object *obj)
+{
+	struct i915_vma *vma;
+	int count = 0;
+
+	list_for_each_entry(vma, &obj->vma_list, vma_link)
+		if (drm_mm_node_allocated(&vma->node))
+			count++;
+
+	return count;
+}
+
 static unsigned long
-i915_gem_inactive_count(struct shrinker *shrinker, struct shrink_control *sc)
+i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 {
 	struct drm_i915_private *dev_priv =
-		container_of(shrinker,
-			     struct drm_i915_private,
-			     mm.inactive_shrinker);
+		container_of(shrinker, struct drm_i915_private, mm.shrinker);
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_i915_gem_object *obj;
 	bool unlock = true;
@@ -4941,10 +4951,8 @@ i915_gem_inactive_count(struct shrinker *shrinker, struct shrink_control *sc)
 			count += obj->base.size >> PAGE_SHIFT;
 
 	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
-		if (obj->active)
-			continue;
-
-		if (!i915_gem_obj_is_pinned(obj) && obj->pages_pin_count == 0)
+		if (!i915_gem_obj_is_pinned(obj) &&
+		    obj->pages_pin_count == num_vma_bound(obj))
 			count += obj->base.size >> PAGE_SHIFT;
 	}
 
@@ -5017,12 +5025,10 @@ unsigned long i915_gem_obj_size(struct drm_i915_gem_object *o,
 }
 
 static unsigned long
-i915_gem_inactive_scan(struct shrinker *shrinker, struct shrink_control *sc)
+i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 {
 	struct drm_i915_private *dev_priv =
-		container_of(shrinker,
-			     struct drm_i915_private,
-			     mm.inactive_shrinker);
+		container_of(shrinker, struct drm_i915_private, mm.shrinker);
 	struct drm_device *dev = dev_priv->dev;
 	unsigned long freed;
 	bool unlock = true;
-- 
1.9.1

Re: [PATCH 2/4] drm/i915: Include bound and active pages in the count of shrinkable objects

[Barbalho, Rafael]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Chris Wilson
> Sent: Tuesday, March 25, 2014 1:23 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Hugh Dickins
> Subject: [Intel-gfx] [PATCH 2/4] drm/i915: Include bound and active pages in
> the count of shrinkable objects
> 
> When the machine is under a lot of memory pressure and being stressed by
> multiple GPU threads, we quite often report fewer than shrinker->batch (i.e.
> SHRINK_BATCH) pages to be freed. This causes the shrink_control to skip
> calling into i915.ko to release pages, despite the GPU holding onto most of
> the physical pages in its active lists.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=72742
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Rafael Barbalho <rafael.barbalho@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_dma.c |  8 ++++----
> drivers/gpu/drm/i915/i915_drv.h |  2 +-  drivers/gpu/drm/i915/i915_gem.c |
> 42 +++++++++++++++++++++++------------------
>  3 files changed, 29 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c
> b/drivers/gpu/drm/i915/i915_dma.c index 4e0a26a83500..5a37c75f4b3d
> 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -1739,8 +1739,8 @@ out_power_well:
>  	intel_power_domains_remove(dev_priv);
>  	drm_vblank_cleanup(dev);
>  out_gem_unload:
> -	if (dev_priv->mm.inactive_shrinker.scan_objects)
> -		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
> +	if (dev_priv->mm.shrinker.scan_objects)
> +		unregister_shrinker(&dev_priv->mm.shrinker);
> 
>  	if (dev->pdev->msi_enabled)
>  		pci_disable_msi(dev->pdev);
> @@ -1791,8 +1791,8 @@ int i915_driver_unload(struct drm_device *dev)
> 
>  	i915_teardown_sysfs(dev);
> 
> -	if (dev_priv->mm.inactive_shrinker.scan_objects)
> -		unregister_shrinker(&dev_priv->mm.inactive_shrinker);
> +	if (dev_priv->mm.shrinker.scan_objects)
> +		unregister_shrinker(&dev_priv->mm.shrinker);
> 
>  	io_mapping_free(dev_priv->gtt.mappable);
>  	arch_phys_wc_del(dev_priv->gtt.mtrr);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h index a7ad864f1154..cb4bb171e6cc
> 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -986,7 +986,7 @@ struct i915_gem_mm {
>  	/** PPGTT used for aliasing the PPGTT with the GTT */
>  	struct i915_hw_ppgtt *aliasing_ppgtt;
> 
> -	struct shrinker inactive_shrinker;
> +	struct shrinker shrinker;
>  	bool shrinker_no_lock_stealing;
> 
>  	/** LRU list of objects with fence regs on them. */ diff --git
> a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index c23034021753..219fe35f9c45 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -54,9 +54,9 @@ static void i915_gem_object_update_fence(struct
> drm_i915_gem_object *obj,
>  					 struct drm_i915_fence_reg *fence,
>  					 bool enable);
> 
> -static unsigned long i915_gem_inactive_count(struct shrinker *shrinker,
> +static unsigned long i915_gem_shrinker_count(struct shrinker *shrinker,
>  					     struct shrink_control *sc);
> -static unsigned long i915_gem_inactive_scan(struct shrinker *shrinker,
> +static unsigned long i915_gem_shrinker_scan(struct shrinker *shrinker,
>  					    struct shrink_control *sc);
>  static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv,
> long target);  static unsigned long i915_gem_shrink_all(struct
> drm_i915_private *dev_priv); @@ -4651,10 +4651,10 @@
> i915_gem_load(struct drm_device *dev)
> 
>  	dev_priv->mm.interruptible = true;
> 
> -	dev_priv->mm.inactive_shrinker.scan_objects =
> i915_gem_inactive_scan;
> -	dev_priv->mm.inactive_shrinker.count_objects =
> i915_gem_inactive_count;
> -	dev_priv->mm.inactive_shrinker.seeks = DEFAULT_SEEKS;
> -	register_shrinker(&dev_priv->mm.inactive_shrinker);
> +	dev_priv->mm.shrinker.scan_objects = i915_gem_shrinker_scan;
> +	dev_priv->mm.shrinker.count_objects = i915_gem_shrinker_count;
> +	dev_priv->mm.shrinker.seeks = DEFAULT_SEEKS;
> +	register_shrinker(&dev_priv->mm.shrinker);
>  }
> 
>  /*
> @@ -4913,13 +4913,23 @@ static bool mutex_is_locked_by(struct mutex
> *mutex, struct task_struct *task)  #endif  }
> 
> +static int num_vma_bound(struct drm_i915_gem_object *obj) {
> +	struct i915_vma *vma;
> +	int count = 0;
> +
> +	list_for_each_entry(vma, &obj->vma_list, vma_link)
> +		if (drm_mm_node_allocated(&vma->node))
> +			count++;
> +
> +	return count;
> +}
> +
>  static unsigned long
> -i915_gem_inactive_count(struct shrinker *shrinker, struct shrink_control
> *sc)
> +i915_gem_shrinker_count(struct shrinker *shrinker, struct
> +shrink_control *sc)
>  {
>  	struct drm_i915_private *dev_priv =
> -		container_of(shrinker,
> -			     struct drm_i915_private,
> -			     mm.inactive_shrinker);
> +		container_of(shrinker, struct drm_i915_private,
> mm.shrinker);
>  	struct drm_device *dev = dev_priv->dev;
>  	struct drm_i915_gem_object *obj;
>  	bool unlock = true;
> @@ -4941,10 +4951,8 @@ i915_gem_inactive_count(struct shrinker
> *shrinker, struct shrink_control *sc)
>  			count += obj->base.size >> PAGE_SHIFT;
> 
>  	list_for_each_entry(obj, &dev_priv->mm.bound_list, global_list) {
> -		if (obj->active)
> -			continue;
> -
> -		if (!i915_gem_obj_is_pinned(obj) && obj->pages_pin_count
> == 0)
> +		if (!i915_gem_obj_is_pinned(obj) &&
> +		    obj->pages_pin_count == num_vma_bound(obj))
>  			count += obj->base.size >> PAGE_SHIFT;
>  	}
> 
> @@ -5017,12 +5025,10 @@ unsigned long i915_gem_obj_size(struct
> drm_i915_gem_object *o,  }
> 
>  static unsigned long
> -i915_gem_inactive_scan(struct shrinker *shrinker, struct shrink_control *sc)
> +i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control
> +*sc)
>  {
>  	struct drm_i915_private *dev_priv =
> -		container_of(shrinker,
> -			     struct drm_i915_private,
> -			     mm.inactive_shrinker);
> +		container_of(shrinker, struct drm_i915_private,
> mm.shrinker);
>  	struct drm_device *dev = dev_priv->dev;
>  	unsigned long freed;
>  	bool unlock = true;
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/4] drm/i915: Refactor common lock handling between shrinker count/scan

[Chris Wilson]

We can share a few lines of tricky lock handling we need to use for both
shrinker routines and in the process fix the return value for count()
when reporting a deadlock.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 42 +++++++++++++++++++++--------------------
 1 file changed, 22 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 219fe35f9c45..135ee8bd55f6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4913,6 +4913,22 @@ static bool mutex_is_locked_by(struct mutex *mutex, struct task_struct *task)
 #endif
 }
 
+static bool i915_gem_shrinker_lock(struct drm_device *dev, bool *unlock)
+{
+	if (!mutex_trylock(&dev->struct_mutex)) {
+		if (!mutex_is_locked_by(&dev->struct_mutex, current))
+			return false;
+
+		if (to_i915(dev)->mm.shrinker_no_lock_stealing)
+			return false;
+
+		*unlock = false;
+	} else
+		*unlock = true;
+
+	return true;
+}
+
 static int num_vma_bound(struct drm_i915_gem_object *obj)
 {
 	struct i915_vma *vma;
@@ -4932,18 +4948,11 @@ i915_gem_shrinker_count(struct shrinker *shrinker, struct shrink_control *sc)
 		container_of(shrinker, struct drm_i915_private, mm.shrinker);
 	struct drm_device *dev = dev_priv->dev;
 	struct drm_i915_gem_object *obj;
-	bool unlock = true;
 	unsigned long count;
+	bool unlock;
 
-	if (!mutex_trylock(&dev->struct_mutex)) {
-		if (!mutex_is_locked_by(&dev->struct_mutex, current))
-			return 0;
-
-		if (dev_priv->mm.shrinker_no_lock_stealing)
-			return 0;
-
-		unlock = false;
-	}
+	if (!i915_gem_shrinker_lock(dev, &unlock))
+		return 0;
 
 	count = 0;
 	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
@@ -5031,17 +5040,10 @@ i915_gem_shrinker_scan(struct shrinker *shrinker, struct shrink_control *sc)
 		container_of(shrinker, struct drm_i915_private, mm.shrinker);
 	struct drm_device *dev = dev_priv->dev;
 	unsigned long freed;
-	bool unlock = true;
+	bool unlock;
 
-	if (!mutex_trylock(&dev->struct_mutex)) {
-		if (!mutex_is_locked_by(&dev->struct_mutex, current))
-			return SHRINK_STOP;
-
-		if (dev_priv->mm.shrinker_no_lock_stealing)
-			return SHRINK_STOP;
-
-		unlock = false;
-	}
+	if (!i915_gem_shrinker_lock(dev, &unlock))
+		return SHRINK_STOP;
 
 	freed = i915_gem_purge(dev_priv, sc->nr_to_scan);
 	if (freed < sc->nr_to_scan)
-- 
1.9.1

Re: [PATCH 3/4] drm/i915: Refactor common lock handling between shrinker count/scan

[Barbalho, Rafael]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Chris Wilson
> Sent: Tuesday, March 25, 2014 1:23 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Hugh Dickins
> Subject: [Intel-gfx] [PATCH 3/4] drm/i915: Refactor common lock handling
> between shrinker count/scan
> 
> We can share a few lines of tricky lock handling we need to use for both
> shrinker routines and in the process fix the return value for count() when
> reporting a deadlock.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Rafael Barbalho <rafael.barbalho@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 42 +++++++++++++++++++++------------
> --------
>  1 file changed, 22 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 219fe35f9c45..135ee8bd55f6
> 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4913,6 +4913,22 @@ static bool mutex_is_locked_by(struct mutex
> *mutex, struct task_struct *task)  #endif  }
> 
> +static bool i915_gem_shrinker_lock(struct drm_device *dev, bool
> +*unlock) {
> +	if (!mutex_trylock(&dev->struct_mutex)) {
> +		if (!mutex_is_locked_by(&dev->struct_mutex, current))
> +			return false;
> +
> +		if (to_i915(dev)->mm.shrinker_no_lock_stealing)
> +			return false;
> +
> +		*unlock = false;
> +	} else
> +		*unlock = true;
> +
> +	return true;
> +}
> +
>  static int num_vma_bound(struct drm_i915_gem_object *obj)  {
>  	struct i915_vma *vma;
> @@ -4932,18 +4948,11 @@ i915_gem_shrinker_count(struct shrinker
> *shrinker, struct shrink_control *sc)
>  		container_of(shrinker, struct drm_i915_private,
> mm.shrinker);
>  	struct drm_device *dev = dev_priv->dev;
>  	struct drm_i915_gem_object *obj;
> -	bool unlock = true;
>  	unsigned long count;
> +	bool unlock;
> 
> -	if (!mutex_trylock(&dev->struct_mutex)) {
> -		if (!mutex_is_locked_by(&dev->struct_mutex, current))
> -			return 0;
> -
> -		if (dev_priv->mm.shrinker_no_lock_stealing)
> -			return 0;
> -
> -		unlock = false;
> -	}
> +	if (!i915_gem_shrinker_lock(dev, &unlock))
> +		return 0;
> 
>  	count = 0;
>  	list_for_each_entry(obj, &dev_priv->mm.unbound_list, global_list)
> @@ -5031,17 +5040,10 @@ i915_gem_shrinker_scan(struct shrinker
> *shrinker, struct shrink_control *sc)
>  		container_of(shrinker, struct drm_i915_private,
> mm.shrinker);
>  	struct drm_device *dev = dev_priv->dev;
>  	unsigned long freed;
> -	bool unlock = true;
> +	bool unlock;
> 
> -	if (!mutex_trylock(&dev->struct_mutex)) {
> -		if (!mutex_is_locked_by(&dev->struct_mutex, current))
> -			return SHRINK_STOP;
> -
> -		if (dev_priv->mm.shrinker_no_lock_stealing)
> -			return SHRINK_STOP;
> -
> -		unlock = false;
> -	}
> +	if (!i915_gem_shrinker_lock(dev, &unlock))
> +		return SHRINK_STOP;
> 
>  	freed = i915_gem_purge(dev_priv, sc->nr_to_scan);
>  	if (freed < sc->nr_to_scan)
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Chris Wilson]

Try to flush out dirty pages into the swapcache (and from there into the
swapfile) when under memory pressure and forced to drop GEM objects from
memory. In effect, this should just allow us to discard unused pages for
memory reclaim and to start writeback earlier.

v2: Hugh Dickins warned that explicitly starting writeback from
shrink_slab was prone to deadlocks within shmemfs.

Cc: Hugh Dickins <hughd@google.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 135ee8bd55f6..8287fd6701c6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -60,7 +60,6 @@ static unsigned long i915_gem_shrinker_scan(struct shrinker *shrinker,
 					    struct shrink_control *sc);
 static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long target);
 static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
-static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
 static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
 
 static bool cpu_cache_is_coherent(struct drm_device *dev,
@@ -1685,12 +1684,16 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
 	return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
 }
 
+static inline int
+i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
+{
+	return obj->madv == I915_MADV_DONTNEED;
+}
+
 /* Immediately discard the backing storage */
 static void
 i915_gem_object_truncate(struct drm_i915_gem_object *obj)
 {
-	struct inode *inode;
-
 	i915_gem_object_free_mmap_offset(obj);
 
 	if (obj->base.filp == NULL)
@@ -1701,16 +1704,28 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
 	 * To do this we must instruct the shmfs to drop all of its
 	 * backing pages, *now*.
 	 */
-	inode = file_inode(obj->base.filp);
-	shmem_truncate_range(inode, 0, (loff_t)-1);
-
+	shmem_truncate_range(file_inode(obj->base.filp), 0, (loff_t)-1);
 	obj->madv = __I915_MADV_PURGED;
 }
 
-static inline int
-i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
+/* Try to discard unwanted pages */
+static void
+i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
 {
-	return obj->madv == I915_MADV_DONTNEED;
+	struct address_space *mapping;
+
+	switch (obj->madv) {
+	case I915_MADV_DONTNEED:
+		i915_gem_object_truncate(obj);
+	case __I915_MADV_PURGED:
+		return;
+	}
+
+	if (obj->base.filp == NULL)
+		return;
+
+	mapping = file_inode(obj->base.filp)->i_mapping,
+	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
 }
 
 static void
@@ -1775,8 +1790,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
 	ops->put_pages(obj);
 	obj->pages = NULL;
 
-	if (i915_gem_object_is_purgeable(obj))
-		i915_gem_object_truncate(obj);
+	i915_gem_object_invalidate(obj);
 
 	return 0;
 }
@@ -4201,6 +4215,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
 
 	if (WARN_ON(obj->pages_pin_count))
 		obj->pages_pin_count = 0;
+	if (obj->madv != __I915_MADV_PURGED)
+		obj->madv = I915_MADV_DONTNEED;
 	i915_gem_object_put_pages(obj);
 	i915_gem_object_free_mmap_offset(obj);
 	i915_gem_object_release_stolen(obj);
-- 
1.9.1

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Chris Wilson]

On Tue, Mar 25, 2014 at 01:23:06PM +0000, Chris Wilson wrote:
> Try to flush out dirty pages into the swapcache (and from there into the
> swapfile) when under memory pressure and forced to drop GEM objects from
> memory. In effect, this should just allow us to discard unused pages for
> memory reclaim and to start writeback earlier.
> 
> v2: Hugh Dickins warned that explicitly starting writeback from
> shrink_slab was prone to deadlocks within shmemfs.
> 
> Cc: Hugh Dickins <hughd@google.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Good news! QA have declared that this series really does prevent the
random OOM where we have completely unused swap. So all it needs is
someone brave enough to review.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Daniel Vetter]

On Fri, Apr 11, 2014 at 09:30:20AM +0100, Chris Wilson wrote:
> On Tue, Mar 25, 2014 at 01:23:06PM +0000, Chris Wilson wrote:
> > Try to flush out dirty pages into the swapcache (and from there into the
> > swapfile) when under memory pressure and forced to drop GEM objects from
> > memory. In effect, this should just allow us to discard unused pages for
> > memory reclaim and to start writeback earlier.
> > 
> > v2: Hugh Dickins warned that explicitly starting writeback from
> > shrink_slab was prone to deadlocks within shmemfs.
> > 
> > Cc: Hugh Dickins <hughd@google.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Good news! QA have declared that this series really does prevent the
> random OOM where we have completely unused swap. So all it needs is
> someone brave enough to review.

Awesome work.

Do we need the additional patch you've just posted to improve the
writeback stalling after calling shrinkers too, or is that not required?

wrt reviewers I'm poking people to get this done from our side. For
merging an ack on the mm pieces would be good so that I can pull it in
through drm-intel trees. My plan is to merge it in 3.16 if it all checks
out.

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Chris Wilson]

On Fri, Apr 11, 2014 at 10:38:25AM +0200, Daniel Vetter wrote:
> On Fri, Apr 11, 2014 at 09:30:20AM +0100, Chris Wilson wrote:
> > On Tue, Mar 25, 2014 at 01:23:06PM +0000, Chris Wilson wrote:
> > > Try to flush out dirty pages into the swapcache (and from there into the
> > > swapfile) when under memory pressure and forced to drop GEM objects from
> > > memory. In effect, this should just allow us to discard unused pages for
> > > memory reclaim and to start writeback earlier.
> > > 
> > > v2: Hugh Dickins warned that explicitly starting writeback from
> > > shrink_slab was prone to deadlocks within shmemfs.
> > > 
> > > Cc: Hugh Dickins <hughd@google.com>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Good news! QA have declared that this series really does prevent the
> > random OOM where we have completely unused swap. So all it needs is
> > someone brave enough to review.
> 
> Awesome work.
> 
> Do we need the additional patch you've just posted to improve the
> writeback stalling after calling shrinkers too, or is that not required?

That looks to be required (or at least I hope it provokes the mm gods
into doing something sensible) for a different problem. However, that
test is still behaving strangely (inactive_anon =~ 2x shmem, it should
be almost equal) as it appears that there is severe overallocation, or a
leak.

But that we can generate massive amounts of writeback from our shrinker
which may not be cleared in time for the allocation to succeed is a
problem (addressed by that mm patch).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Robert Beckett]

On 25/03/2014 13:23, Chris Wilson wrote:
> Try to flush out dirty pages into the swapcache (and from there into the
> swapfile) when under memory pressure and forced to drop GEM objects from
> memory. In effect, this should just allow us to discard unused pages for
> memory reclaim and to start writeback earlier.
>
> v2: Hugh Dickins warned that explicitly starting writeback from
> shrink_slab was prone to deadlocks within shmemfs.
>
> Cc: Hugh Dickins <hughd@google.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++++++++-----------
>   1 file changed, 27 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 135ee8bd55f6..8287fd6701c6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -60,7 +60,6 @@ static unsigned long i915_gem_shrinker_scan(struct shrinker *shrinker,
>   					    struct shrink_control *sc);
>   static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long target);
>   static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
> -static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
>   static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
>
>   static bool cpu_cache_is_coherent(struct drm_device *dev,
> @@ -1685,12 +1684,16 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
>   	return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
>   }
>
> +static inline int
> +i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
> +{
> +	return obj->madv == I915_MADV_DONTNEED;
> +}
> +
>   /* Immediately discard the backing storage */
>   static void
>   i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>   {
> -	struct inode *inode;
> -
>   	i915_gem_object_free_mmap_offset(obj);
>
>   	if (obj->base.filp == NULL)
> @@ -1701,16 +1704,28 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>   	 * To do this we must instruct the shmfs to drop all of its
>   	 * backing pages, *now*.
>   	 */
> -	inode = file_inode(obj->base.filp);
> -	shmem_truncate_range(inode, 0, (loff_t)-1);
> -
> +	shmem_truncate_range(file_inode(obj->base.filp), 0, (loff_t)-1);
>   	obj->madv = __I915_MADV_PURGED;
>   }
>
> -static inline int
> -i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
> +/* Try to discard unwanted pages */
> +static void
> +i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
>   {
> -	return obj->madv == I915_MADV_DONTNEED;
> +	struct address_space *mapping;
> +
> +	switch (obj->madv) {
> +	case I915_MADV_DONTNEED:
> +		i915_gem_object_truncate(obj);
> +	case __I915_MADV_PURGED:
> +		return;
> +	}
> +
> +	if (obj->base.filp == NULL)
> +		return;
> +
> +	mapping = file_inode(obj->base.filp)->i_mapping,
> +	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
>   }
>
>   static void
> @@ -1775,8 +1790,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>   	ops->put_pages(obj);
>   	obj->pages = NULL;
>
> -	if (i915_gem_object_is_purgeable(obj))
> -		i915_gem_object_truncate(obj);
> +	i915_gem_object_invalidate(obj);
>
>   	return 0;
>   }
> @@ -4201,6 +4215,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>
>   	if (WARN_ON(obj->pages_pin_count))
>   		obj->pages_pin_count = 0;
> +	if (obj->madv != __I915_MADV_PURGED)
> +		obj->madv = I915_MADV_DONTNEED;
>   	i915_gem_object_put_pages(obj);
>   	i915_gem_object_free_mmap_offset(obj);
>   	i915_gem_object_release_stolen(obj);
>

Functionally it looks good to me.

Though, you may want a /* fall-through */ comment (some people cant 
mentally parse fallthroughs without being prompted) and a default: 
break; (to avoid any static code analysis complaints) in the switch in 
i915_gem_object_invalidate.

Reviewed-by: Robert Beckett <robert.beckett@intel.com>

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Jani Nikula]

On Wed, 16 Apr 2014, Robert Beckett <robert.beckett@intel.com> wrote:
> On 25/03/2014 13:23, Chris Wilson wrote:
>> Try to flush out dirty pages into the swapcache (and from there into the
>> swapfile) when under memory pressure and forced to drop GEM objects from
>> memory. In effect, this should just allow us to discard unused pages for
>> memory reclaim and to start writeback earlier.
>>
>> v2: Hugh Dickins warned that explicitly starting writeback from
>> shrink_slab was prone to deadlocks within shmemfs.
>>
>> Cc: Hugh Dickins <hughd@google.com>
>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>> ---
>>   drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++++++++-----------
>>   1 file changed, 27 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>> index 135ee8bd55f6..8287fd6701c6 100644
>> --- a/drivers/gpu/drm/i915/i915_gem.c
>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>> @@ -60,7 +60,6 @@ static unsigned long i915_gem_shrinker_scan(struct shrinker *shrinker,
>>   					    struct shrink_control *sc);
>>   static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long target);
>>   static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
>> -static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
>>   static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
>>
>>   static bool cpu_cache_is_coherent(struct drm_device *dev,
>> @@ -1685,12 +1684,16 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
>>   	return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
>>   }
>>
>> +static inline int
>> +i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
>> +{
>> +	return obj->madv == I915_MADV_DONTNEED;
>> +}
>> +
>>   /* Immediately discard the backing storage */
>>   static void
>>   i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>>   {
>> -	struct inode *inode;
>> -
>>   	i915_gem_object_free_mmap_offset(obj);
>>
>>   	if (obj->base.filp == NULL)
>> @@ -1701,16 +1704,28 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>>   	 * To do this we must instruct the shmfs to drop all of its
>>   	 * backing pages, *now*.
>>   	 */
>> -	inode = file_inode(obj->base.filp);
>> -	shmem_truncate_range(inode, 0, (loff_t)-1);
>> -
>> +	shmem_truncate_range(file_inode(obj->base.filp), 0, (loff_t)-1);
>>   	obj->madv = __I915_MADV_PURGED;
>>   }
>>
>> -static inline int
>> -i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
>> +/* Try to discard unwanted pages */
>> +static void
>> +i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
>>   {
>> -	return obj->madv == I915_MADV_DONTNEED;
>> +	struct address_space *mapping;
>> +
>> +	switch (obj->madv) {
>> +	case I915_MADV_DONTNEED:
>> +		i915_gem_object_truncate(obj);
>> +	case __I915_MADV_PURGED:
>> +		return;
>> +	}
>> +
>> +	if (obj->base.filp == NULL)
>> +		return;
>> +
>> +	mapping = file_inode(obj->base.filp)->i_mapping,
>> +	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
>>   }
>>
>>   static void
>> @@ -1775,8 +1790,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>>   	ops->put_pages(obj);
>>   	obj->pages = NULL;
>>
>> -	if (i915_gem_object_is_purgeable(obj))
>> -		i915_gem_object_truncate(obj);
>> +	i915_gem_object_invalidate(obj);
>>
>>   	return 0;
>>   }
>> @@ -4201,6 +4215,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>>
>>   	if (WARN_ON(obj->pages_pin_count))
>>   		obj->pages_pin_count = 0;
>> +	if (obj->madv != __I915_MADV_PURGED)
>> +		obj->madv = I915_MADV_DONTNEED;
>>   	i915_gem_object_put_pages(obj);
>>   	i915_gem_object_free_mmap_offset(obj);
>>   	i915_gem_object_release_stolen(obj);
>>
>
> Functionally it looks good to me.
>
> Though, you may want a /* fall-through */ comment (some people cant 
> mentally parse fallthroughs without being prompted) and a default: 
> break; (to avoid any static code analysis complaints) in the switch in 
> i915_gem_object_invalidate.

Or just two if statements.

Jani.



>
> Reviewed-by: Robert Beckett <robert.beckett@intel.com>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Daniel Vetter]

On Thu, Apr 17, 2014 at 07:34:31PM +0300, Jani Nikula wrote:
> On Wed, 16 Apr 2014, Robert Beckett <robert.beckett@intel.com> wrote:
> > On 25/03/2014 13:23, Chris Wilson wrote:
> >> Try to flush out dirty pages into the swapcache (and from there into the
> >> swapfile) when under memory pressure and forced to drop GEM objects from
> >> memory. In effect, this should just allow us to discard unused pages for
> >> memory reclaim and to start writeback earlier.
> >>
> >> v2: Hugh Dickins warned that explicitly starting writeback from
> >> shrink_slab was prone to deadlocks within shmemfs.
> >>
> >> Cc: Hugh Dickins <hughd@google.com>
> >> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >> ---
> >>   drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++++++++-----------
> >>   1 file changed, 27 insertions(+), 11 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> >> index 135ee8bd55f6..8287fd6701c6 100644
> >> --- a/drivers/gpu/drm/i915/i915_gem.c
> >> +++ b/drivers/gpu/drm/i915/i915_gem.c
> >> @@ -60,7 +60,6 @@ static unsigned long i915_gem_shrinker_scan(struct shrinker *shrinker,
> >>   					    struct shrink_control *sc);
> >>   static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long target);
> >>   static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
> >> -static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
> >>   static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
> >>
> >>   static bool cpu_cache_is_coherent(struct drm_device *dev,
> >> @@ -1685,12 +1684,16 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
> >>   	return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
> >>   }
> >>
> >> +static inline int
> >> +i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
> >> +{
> >> +	return obj->madv == I915_MADV_DONTNEED;
> >> +}
> >> +
> >>   /* Immediately discard the backing storage */
> >>   static void
> >>   i915_gem_object_truncate(struct drm_i915_gem_object *obj)
> >>   {
> >> -	struct inode *inode;
> >> -
> >>   	i915_gem_object_free_mmap_offset(obj);
> >>
> >>   	if (obj->base.filp == NULL)
> >> @@ -1701,16 +1704,28 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
> >>   	 * To do this we must instruct the shmfs to drop all of its
> >>   	 * backing pages, *now*.
> >>   	 */
> >> -	inode = file_inode(obj->base.filp);
> >> -	shmem_truncate_range(inode, 0, (loff_t)-1);
> >> -
> >> +	shmem_truncate_range(file_inode(obj->base.filp), 0, (loff_t)-1);
> >>   	obj->madv = __I915_MADV_PURGED;
> >>   }
> >>
> >> -static inline int
> >> -i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
> >> +/* Try to discard unwanted pages */
> >> +static void
> >> +i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
> >>   {
> >> -	return obj->madv == I915_MADV_DONTNEED;
> >> +	struct address_space *mapping;
> >> +
> >> +	switch (obj->madv) {
> >> +	case I915_MADV_DONTNEED:
> >> +		i915_gem_object_truncate(obj);
> >> +	case __I915_MADV_PURGED:
> >> +		return;
> >> +	}
> >> +
> >> +	if (obj->base.filp == NULL)
> >> +		return;
> >> +
> >> +	mapping = file_inode(obj->base.filp)->i_mapping,
> >> +	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
> >>   }
> >>
> >>   static void
> >> @@ -1775,8 +1790,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
> >>   	ops->put_pages(obj);
> >>   	obj->pages = NULL;
> >>
> >> -	if (i915_gem_object_is_purgeable(obj))
> >> -		i915_gem_object_truncate(obj);
> >> +	i915_gem_object_invalidate(obj);
> >>
> >>   	return 0;
> >>   }
> >> @@ -4201,6 +4215,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
> >>
> >>   	if (WARN_ON(obj->pages_pin_count))
> >>   		obj->pages_pin_count = 0;
> >> +	if (obj->madv != __I915_MADV_PURGED)
> >> +		obj->madv = I915_MADV_DONTNEED;
> >>   	i915_gem_object_put_pages(obj);
> >>   	i915_gem_object_free_mmap_offset(obj);
> >>   	i915_gem_object_release_stolen(obj);
> >>
> >
> > Functionally it looks good to me.
> >
> > Though, you may want a /* fall-through */ comment (some people cant 
> > mentally parse fallthroughs without being prompted) and a default: 
> > break; (to avoid any static code analysis complaints) in the switch in 
> > i915_gem_object_invalidate.
> 
> Or just two if statements.

	if (MADV_DONTNEED)
		i915_gem_object_truncate(obj);
	if (!MADV_WILLNEED)
		return;

would indeed looka a bit cleaner.

And a question to Bob: Is your r-b for the entire series or just this
patch? Generally the assumption is that an r-b tag is only for the patch
replied to, except when otherwise stated. Usually people just slap r-b
tags onto patches as they go through a series.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Robert Beckett]

On 22 April 2014 20:06:14, Daniel Vetter wrote:
> On Thu, Apr 17, 2014 at 07:34:31PM +0300, Jani Nikula wrote:
>> On Wed, 16 Apr 2014, Robert Beckett <robert.beckett@intel.com> wrote:
>>> On 25/03/2014 13:23, Chris Wilson wrote:
>>>> Try to flush out dirty pages into the swapcache (and from there into the
>>>> swapfile) when under memory pressure and forced to drop GEM objects from
>>>> memory. In effect, this should just allow us to discard unused pages for
>>>> memory reclaim and to start writeback earlier.
>>>>
>>>> v2: Hugh Dickins warned that explicitly starting writeback from
>>>> shrink_slab was prone to deadlocks within shmemfs.
>>>>
>>>> Cc: Hugh Dickins <hughd@google.com>
>>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>>> ---
>>>>    drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++++++++-----------
>>>>    1 file changed, 27 insertions(+), 11 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
>>>> index 135ee8bd55f6..8287fd6701c6 100644
>>>> --- a/drivers/gpu/drm/i915/i915_gem.c
>>>> +++ b/drivers/gpu/drm/i915/i915_gem.c
>>>> @@ -60,7 +60,6 @@ static unsigned long i915_gem_shrinker_scan(struct shrinker *shrinker,
>>>>    					    struct shrink_control *sc);
>>>>    static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long target);
>>>>    static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
>>>> -static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
>>>>    static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
>>>>
>>>>    static bool cpu_cache_is_coherent(struct drm_device *dev,
>>>> @@ -1685,12 +1684,16 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
>>>>    	return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
>>>>    }
>>>>
>>>> +static inline int
>>>> +i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
>>>> +{
>>>> +	return obj->madv == I915_MADV_DONTNEED;
>>>> +}
>>>> +
>>>>    /* Immediately discard the backing storage */
>>>>    static void
>>>>    i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>>>>    {
>>>> -	struct inode *inode;
>>>> -
>>>>    	i915_gem_object_free_mmap_offset(obj);
>>>>
>>>>    	if (obj->base.filp == NULL)
>>>> @@ -1701,16 +1704,28 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>>>>    	 * To do this we must instruct the shmfs to drop all of its
>>>>    	 * backing pages, *now*.
>>>>    	 */
>>>> -	inode = file_inode(obj->base.filp);
>>>> -	shmem_truncate_range(inode, 0, (loff_t)-1);
>>>> -
>>>> +	shmem_truncate_range(file_inode(obj->base.filp), 0, (loff_t)-1);
>>>>    	obj->madv = __I915_MADV_PURGED;
>>>>    }
>>>>
>>>> -static inline int
>>>> -i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
>>>> +/* Try to discard unwanted pages */
>>>> +static void
>>>> +i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
>>>>    {
>>>> -	return obj->madv == I915_MADV_DONTNEED;
>>>> +	struct address_space *mapping;
>>>> +
>>>> +	switch (obj->madv) {
>>>> +	case I915_MADV_DONTNEED:
>>>> +		i915_gem_object_truncate(obj);
>>>> +	case __I915_MADV_PURGED:
>>>> +		return;
>>>> +	}
>>>> +
>>>> +	if (obj->base.filp == NULL)
>>>> +		return;
>>>> +
>>>> +	mapping = file_inode(obj->base.filp)->i_mapping,
>>>> +	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
>>>>    }
>>>>
>>>>    static void
>>>> @@ -1775,8 +1790,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>>>>    	ops->put_pages(obj);
>>>>    	obj->pages = NULL;
>>>>
>>>> -	if (i915_gem_object_is_purgeable(obj))
>>>> -		i915_gem_object_truncate(obj);
>>>> +	i915_gem_object_invalidate(obj);
>>>>
>>>>    	return 0;
>>>>    }
>>>> @@ -4201,6 +4215,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>>>>
>>>>    	if (WARN_ON(obj->pages_pin_count))
>>>>    		obj->pages_pin_count = 0;
>>>> +	if (obj->madv != __I915_MADV_PURGED)
>>>> +		obj->madv = I915_MADV_DONTNEED;
>>>>    	i915_gem_object_put_pages(obj);
>>>>    	i915_gem_object_free_mmap_offset(obj);
>>>>    	i915_gem_object_release_stolen(obj);
>>>>
>>>
>>> Functionally it looks good to me.
>>>
>>> Though, you may want a /* fall-through */ comment (some people cant
>>> mentally parse fallthroughs without being prompted) and a default:
>>> break; (to avoid any static code analysis complaints) in the switch in
>>> i915_gem_object_invalidate.
>>
>> Or just two if statements.
>
> 	if (MADV_DONTNEED)
> 		i915_gem_object_truncate(obj);
> 	if (!MADV_WILLNEED)
> 		return;
>
> would indeed looka a bit cleaner.
>
> And a question to Bob: Is your r-b for the entire series or just this
> patch? Generally the assumption is that an r-b tag is only for the patch
> replied to, except when otherwise stated. Usually people just slap r-b
> tags onto patches as they go through a series.
> -Daniel

It was meant for the whole series.

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Barbalho, Rafael]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Chris Wilson
> Sent: Tuesday, March 25, 2014 1:23 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Hugh Dickins
> Subject: [Intel-gfx] [PATCH 4/4] drm/i915: Invalidate our pages under
> memory pressure
> 
> Try to flush out dirty pages into the swapcache (and from there into the
> swapfile) when under memory pressure and forced to drop GEM objects
> from memory. In effect, this should just allow us to discard unused pages for
> memory reclaim and to start writeback earlier.
> 
> v2: Hugh Dickins warned that explicitly starting writeback from shrink_slab
> was prone to deadlocks within shmemfs.
> 
> Cc: Hugh Dickins <hughd@google.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Rafael Barbalho <rafael.barbalho@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++++++++--
> ---------
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 135ee8bd55f6..8287fd6701c6
> 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -60,7 +60,6 @@ static unsigned long i915_gem_shrinker_scan(struct
> shrinker *shrinker,
>  					    struct shrink_control *sc);
>  static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv,
> long target);  static unsigned long i915_gem_shrink_all(struct
> drm_i915_private *dev_priv); -static void i915_gem_object_truncate(struct
> drm_i915_gem_object *obj);  static void
> i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
> 
>  static bool cpu_cache_is_coherent(struct drm_device *dev, @@ -1685,12
> +1684,16 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void
> *data,
>  	return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
> }
> 
> +static inline int
> +i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj) {
> +	return obj->madv == I915_MADV_DONTNEED; }
> +
>  /* Immediately discard the backing storage */  static void
> i915_gem_object_truncate(struct drm_i915_gem_object *obj)  {
> -	struct inode *inode;
> -
>  	i915_gem_object_free_mmap_offset(obj);
> 
>  	if (obj->base.filp == NULL)
> @@ -1701,16 +1704,28 @@ i915_gem_object_truncate(struct
> drm_i915_gem_object *obj)
>  	 * To do this we must instruct the shmfs to drop all of its
>  	 * backing pages, *now*.
>  	 */
> -	inode = file_inode(obj->base.filp);
> -	shmem_truncate_range(inode, 0, (loff_t)-1);
> -
> +	shmem_truncate_range(file_inode(obj->base.filp), 0, (loff_t)-1);
>  	obj->madv = __I915_MADV_PURGED;
>  }
> 
> -static inline int
> -i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
> +/* Try to discard unwanted pages */
> +static void
> +i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
>  {
> -	return obj->madv == I915_MADV_DONTNEED;
> +	struct address_space *mapping;
> +
> +	switch (obj->madv) {
> +	case I915_MADV_DONTNEED:
> +		i915_gem_object_truncate(obj);
> +	case __I915_MADV_PURGED:
> +		return;
> +	}
> +
> +	if (obj->base.filp == NULL)
> +		return;
> +
> +	mapping = file_inode(obj->base.filp)->i_mapping,
> +	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
>  }
> 
>  static void
> @@ -1775,8 +1790,7 @@ i915_gem_object_put_pages(struct
> drm_i915_gem_object *obj)
>  	ops->put_pages(obj);
>  	obj->pages = NULL;
> 
> -	if (i915_gem_object_is_purgeable(obj))
> -		i915_gem_object_truncate(obj);
> +	i915_gem_object_invalidate(obj);
> 
>  	return 0;
>  }
> @@ -4201,6 +4215,8 @@ void i915_gem_free_object(struct
> drm_gem_object *gem_obj)
> 
>  	if (WARN_ON(obj->pages_pin_count))
>  		obj->pages_pin_count = 0;
> +	if (obj->madv != __I915_MADV_PURGED)
> +		obj->madv = I915_MADV_DONTNEED;
>  	i915_gem_object_put_pages(obj);
>  	i915_gem_object_free_mmap_offset(obj);
>  	i915_gem_object_release_stolen(obj);
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Daniel Vetter]

On Tue, Mar 25, 2014 at 01:23:06PM +0000, Chris Wilson wrote:
> Try to flush out dirty pages into the swapcache (and from there into the
> swapfile) when under memory pressure and forced to drop GEM objects from
> memory. In effect, this should just allow us to discard unused pages for
> memory reclaim and to start writeback earlier.
> 
> v2: Hugh Dickins warned that explicitly starting writeback from
> shrink_slab was prone to deadlocks within shmemfs.
> 
> Cc: Hugh Dickins <hughd@google.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Merged all four patches. Can you please go through bugzilla and poke all
relevant patches for retesting? I think I've made a sufficient fool of
myself yesterday to not attempt this ;-)

Thanks, Daniel
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 38 +++++++++++++++++++++++++++-----------
>  1 file changed, 27 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 135ee8bd55f6..8287fd6701c6 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -60,7 +60,6 @@ static unsigned long i915_gem_shrinker_scan(struct shrinker *shrinker,
>  					    struct shrink_control *sc);
>  static unsigned long i915_gem_purge(struct drm_i915_private *dev_priv, long target);
>  static unsigned long i915_gem_shrink_all(struct drm_i915_private *dev_priv);
> -static void i915_gem_object_truncate(struct drm_i915_gem_object *obj);
>  static void i915_gem_retire_requests_ring(struct intel_ring_buffer *ring);
>  
>  static bool cpu_cache_is_coherent(struct drm_device *dev,
> @@ -1685,12 +1684,16 @@ i915_gem_mmap_gtt_ioctl(struct drm_device *dev, void *data,
>  	return i915_gem_mmap_gtt(file, dev, args->handle, &args->offset);
>  }
>  
> +static inline int
> +i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
> +{
> +	return obj->madv == I915_MADV_DONTNEED;
> +}
> +
>  /* Immediately discard the backing storage */
>  static void
>  i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>  {
> -	struct inode *inode;
> -
>  	i915_gem_object_free_mmap_offset(obj);
>  
>  	if (obj->base.filp == NULL)
> @@ -1701,16 +1704,28 @@ i915_gem_object_truncate(struct drm_i915_gem_object *obj)
>  	 * To do this we must instruct the shmfs to drop all of its
>  	 * backing pages, *now*.
>  	 */
> -	inode = file_inode(obj->base.filp);
> -	shmem_truncate_range(inode, 0, (loff_t)-1);
> -
> +	shmem_truncate_range(file_inode(obj->base.filp), 0, (loff_t)-1);
>  	obj->madv = __I915_MADV_PURGED;
>  }
>  
> -static inline int
> -i915_gem_object_is_purgeable(struct drm_i915_gem_object *obj)
> +/* Try to discard unwanted pages */
> +static void
> +i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
>  {
> -	return obj->madv == I915_MADV_DONTNEED;
> +	struct address_space *mapping;
> +
> +	switch (obj->madv) {
> +	case I915_MADV_DONTNEED:
> +		i915_gem_object_truncate(obj);
> +	case __I915_MADV_PURGED:
> +		return;
> +	}
> +
> +	if (obj->base.filp == NULL)
> +		return;
> +
> +	mapping = file_inode(obj->base.filp)->i_mapping,
> +	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
>  }
>  
>  static void
> @@ -1775,8 +1790,7 @@ i915_gem_object_put_pages(struct drm_i915_gem_object *obj)
>  	ops->put_pages(obj);
>  	obj->pages = NULL;
>  
> -	if (i915_gem_object_is_purgeable(obj))
> -		i915_gem_object_truncate(obj);
> +	i915_gem_object_invalidate(obj);
>  
>  	return 0;
>  }
> @@ -4201,6 +4215,8 @@ void i915_gem_free_object(struct drm_gem_object *gem_obj)
>  
>  	if (WARN_ON(obj->pages_pin_count))
>  		obj->pages_pin_count = 0;
> +	if (obj->madv != __I915_MADV_PURGED)
> +		obj->madv = I915_MADV_DONTNEED;
>  	i915_gem_object_put_pages(obj);
>  	i915_gem_object_free_mmap_offset(obj);
>  	i915_gem_object_release_stolen(obj);
> -- 
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Chris Wilson]

On Tue, May 20, 2014 at 09:53:28AM +0200, Daniel Vetter wrote:
> On Tue, Mar 25, 2014 at 01:23:06PM +0000, Chris Wilson wrote:
> > Try to flush out dirty pages into the swapcache (and from there into the
> > swapfile) when under memory pressure and forced to drop GEM objects from
> > memory. In effect, this should just allow us to discard unused pages for
> > memory reclaim and to start writeback earlier.
> > 
> > v2: Hugh Dickins warned that explicitly starting writeback from
> > shrink_slab was prone to deadlocks within shmemfs.
> > 
> > Cc: Hugh Dickins <hughd@google.com>
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Merged all four patches. Can you please go through bugzilla and poke all
> relevant patches for retesting? I think I've made a sufficient fool of
> myself yesterday to not attempt this ;-)

4 patches? The series tested had 5...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 4/4] drm/i915: Invalidate our pages under memory pressure

[Daniel Vetter]

On Tue, May 20, 2014 at 08:56:08AM +0100, Chris Wilson wrote:
> On Tue, May 20, 2014 at 09:53:28AM +0200, Daniel Vetter wrote:
> > On Tue, Mar 25, 2014 at 01:23:06PM +0000, Chris Wilson wrote:
> > > Try to flush out dirty pages into the swapcache (and from there into the
> > > swapfile) when under memory pressure and forced to drop GEM objects from
> > > memory. In effect, this should just allow us to discard unused pages for
> > > memory reclaim and to start writeback earlier.
> > > 
> > > v2: Hugh Dickins warned that explicitly starting writeback from
> > > shrink_slab was prone to deadlocks within shmemfs.
> > > 
> > > Cc: Hugh Dickins <hughd@google.com>
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Merged all four patches. Can you please go through bugzilla and poke all
> > relevant patches for retesting? I think I've made a sufficient fool of
> > myself yesterday to not attempt this ;-)
> 
> 4 patches? The series tested had 5...

Those 5 didn't show up till just today ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/4] drm/i915: Translate ENOSPC from shmem_get_page() to ENOMEM

[Barbalho, Rafael]

> -----Original Message-----
> From: Intel-gfx [mailto:intel-gfx-bounces@lists.freedesktop.org] On Behalf
> Of Chris Wilson
> Sent: Tuesday, March 25, 2014 1:23 PM
> To: intel-gfx@lists.freedesktop.org
> Cc: Hugh Dickins
> Subject: [Intel-gfx] [PATCH 1/4] drm/i915: Translate ENOSPC from
> shmem_get_page() to ENOMEM
> 
> shmemfs first checks if there is enough memory to allocate the page and
> reports ENOSPC should there be insufficient, along with the usual ENOMEM
> for a genuine allocation failure.
> 
> We use ENOSPC in our driver to mean that we have run out of aperture
> space and so want to translate the error from shmemfs back to our usual
> understanding of ENOMEM. None of the the other GEM users appear to
> distinguish between ENOMEM and ENOSPC in their error handling, hence it is
> easiest to do the fixup in i915.ko
> 
> Cc: Hugh Dickins <hughd@google.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Reviewed-by: Rafael Barbalho <rafael.barbalho@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_gem.c | 14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c
> b/drivers/gpu/drm/i915/i915_gem.c index 33bbaa0d4412..c23034021753
> 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -1976,7 +1976,19 @@ err_pages:
>  		page_cache_release(sg_page_iter_page(&sg_iter));
>  	sg_free_table(st);
>  	kfree(st);
> -	return PTR_ERR(page);
> +
> +	/* shmemfs first checks if there is enough memory to allocate the
> page
> +	 * and reports ENOSPC should there be insufficient, along with the
> usual
> +	 * ENOMEM for a genuine allocation failure.
> +	 *
> +	 * We use ENOSPC in our driver to mean that we have run out of
> aperture
> +	 * space and so want to translate the error from shmemfs back to our
> +	 * usual understanding of ENOMEM.
> +	 */
> +	if (PTR_ERR(page) == -ENOSPC)
> +		return -ENOMEM;
> +	else
> +		return PTR_ERR(page);
>  }
> 
>  /* Ensure that the associated pages are gathered from the backing storage
> --
> 1.9.1
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx