Shrinker improvements

Shrinker improvements

[Chris Wilson]

Recent-ish changes to the core shrinker have made the code more prone to
hit OOM, and worse do so without even touching swap. My hypothesis is
that this is a balancing issue between shrinking the VM lists and the
slabs, such that writeback of our objects is not performed starving the
system of pages. Patch 5/5 introduces manual writeback handling for our
shrinker, but afaict it should be unnecessary. Nevertheless, I think it
will come in useful.

I am having difficulty in getting results from QA, but so far these have
improved mempressure handling on my machines.
-Chris

[PATCH 1/5] shmemfs: Report ENOMEM for page allocation failures outside of tmpfs faults

[Chris Wilson]

The intention of returning ENOSPC for a page allocation failure due to
memory exhausstion in shmem_getpage_gfp() is purely "so that a failure
on a sparse tmpfs mapping will give SIGBUS not OOM." However, for other
callers, for example i915.ko, we want to distinguish the error message
reported to userspace between ENOSPC (meaning that we were unable to fit
the object/execbuffer into the aperture) and ENOMEM (meaning that we
were unable to allocate pages for the object).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Hugh Dickins <hughd@google.com>
---
 mm/shmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 1f18c9d0d93e..142b0bc085e1 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1179,7 +1179,7 @@ repeat:
 
 	} else {
 		if (shmem_acct_block(info->flags)) {
-			error = -ENOSPC;
+			error = fault_type ? -ENOSPC : -ENOMEM;
 			goto failed;
 		}
 		if (sbinfo->max_blocks) {
-- 
1.9.0

[PATCH 2/5] shmemfs: Use redirty_page_for_writepage()

[Chris Wilson]

"When we cannot write a page we should use redirty_page_for_writepage()
instead of plain set_page_dirty(). That tells writeback code we have
problems, redirties only the page (redirtying buffers is not needed),
and updates mm accounting of failed page writes."

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Hugh Dickins <hughd@google.com>
---
 mm/shmem.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 142b0bc085e1..18aa88eff8e3 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -872,7 +872,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 	mutex_unlock(&shmem_swaplist_mutex);
 	swapcache_free(swap, NULL);
 redirty:
-	set_page_dirty(page);
+	redirty_page_for_writepage(wbc, page);
 	if (wbc->for_reclaim)
 		return AOP_WRITEPAGE_ACTIVATE;	/* Return with page locked */
 	unlock_page(page);
-- 
1.9.0

[PATCH 3/5] 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 f8f7a59ab076..e9609d03e49d 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -1734,8 +1734,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);
@@ -1786,8 +1786,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 7ef0a720ea71..815fc6d25732 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1094,7 +1094,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 6978e692cd82..2aa72ee78a16 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);
@@ -4621,10 +4621,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);
 }
 
 /*
@@ -4882,13 +4882,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;
@@ -4910,10 +4920,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;
 	}
 
@@ -4986,12 +4994,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.0

[PATCH 4/5] 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 2aa72ee78a16..e4117aec716a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4882,6 +4882,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;
@@ -4901,18 +4917,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)
@@ -5000,17 +5009,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.0

[PATCH 5/5] drm/i915: Writeback 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.

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

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index e4117aec716a..fb97626d55c6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -34,6 +34,7 @@
 #include <linux/shmem_fs.h>
 #include <linux/slab.h>
 #include <linux/swap.h>
+#include <linux/writeback.h>
 #include <linux/pci.h>
 #include <linux/dma-buf.h>
 
@@ -60,7 +61,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 bool cpu_cache_is_coherent(struct drm_device *dev,
 				  enum i915_cache_level level)
@@ -1666,12 +1666,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)
@@ -1682,16 +1686,36 @@ 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)
-{
-	return obj->madv == I915_MADV_DONTNEED;
+/* Try to discard unwanted pages */
+static void
+i915_gem_object_writeback(struct drm_i915_gem_object *obj)
+{
+	struct writeback_control wbc = {
+		.nr_to_write = LONG_MAX,
+		.sync_mode = WB_SYNC_NONE,
+		.range_start = 0,
+		.range_end = LLONG_MAX,
+		.for_reclaim = true,
+	};
+	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,
+	generic_writepages(mapping, &wbc);
+	invalidate_mapping_pages(mapping, 0, (loff_t)-1);
 }
 
 static void
@@ -1756,8 +1780,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_writeback(obj);
 
 	return 0;
 }
@@ -4171,6 +4194,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.0

Re: [PATCH 1/5] shmemfs: Report ENOMEM for page allocation failures outside of tmpfs faults

[Hugh Dickins]

On Wed, 5 Mar 2014, Chris Wilson wrote:

> The intention of returning ENOSPC for a page allocation failure due to
> memory exhausstion in shmem_getpage_gfp() is purely "so that a failure
> on a sparse tmpfs mapping will give SIGBUS not OOM." However, for other
> callers, for example i915.ko, we want to distinguish the error message
> reported to userspace between ENOSPC (meaning that we were unable to fit
> the object/execbuffer into the aperture) and ENOMEM (meaning that we
> were unable to allocate pages for the object).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Hugh Dickins <hughd@google.com>

I'm not keen on this: perhaps because it looks like a hack of yours -
and draws attention to what might be thought a hack of mine ;)

Nor am I thrilled by what was there before (we have three cases which
ought to be distinguished, but only ENOMEM and ENOSPC to distinguish
between them); but would rather it stay as is, than change what's
reported to the user after all these years.

But I do see your point, and asking you to convert ENOSPC to ENOMEM
in all your drivers/gpu calls might be tiresome.

I think we have a reasonable compromise: shmem_read_mapping_page_gfp()
is already the wrapper provided just for you guys, how about posting
a patch to map ENOSPC to ENOMEM there?

(You're using the MS_KERNMOUNT, so won't hit a max_blocks limit.)

Hugh

> ---
>  mm/shmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 1f18c9d0d93e..142b0bc085e1 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1179,7 +1179,7 @@ repeat:
>  
>  	} else {
>  		if (shmem_acct_block(info->flags)) {
> -			error = -ENOSPC;
> +			error = fault_type ? -ENOSPC : -ENOMEM;
>  			goto failed;
>  		}
>  		if (sbinfo->max_blocks) {
> -- 
> 1.9.0

Re: [PATCH 2/5] shmemfs: Use redirty_page_for_writepage()

[Hugh Dickins]

On Wed, 5 Mar 2014, Chris Wilson wrote:

> "When we cannot write a page we should use redirty_page_for_writepage()
> instead of plain set_page_dirty(). That tells writeback code we have
> problems, redirties only the page (redirtying buffers is not needed),
> and updates mm accounting of failed page writes."

I didn't locate the origin of that quotation, but it's talking about
the usual filesystem cap_account_dirty/cap_account_writeback protocol.

shmem doesn't participate it that: it only writes out (to swap) under
memory pressure, not for sync, and follows a much simpler path.  Using
redirty_page_for_writepage() would lead it into complications, some of
which we prefer to avoid for efficiency, some of which would actually
be wrong (unless/until there's reason to convert shmem over to the
full protocol).

> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Hugh Dickins <hughd@google.com>

So NAK.

But you didn't explain why you want to make this change.  I presume
it's for the 5/5 which you didn't Cc to me, but I've looked up on
lists.freedesktop.org/archives/intel-gfx.

That's a bigger change and worrying: I've not thought through
the consequences of shmem page writeback from generic_writepages()
called from within a slab shrinker: we're used to doing shmem page
writeback from pageout() in mm/vmscan.c and nowhere else.

One thing that would certainly be wrong (liable to deadlock) would
be to do that when shrink_control's gfp_mask does not have __GFP_IO.
But I'm not comfortable with page writeback from this level at all.

The shmem_truncate_range() (you've had for a long time) should be safe,
and the new invalidate_mapping_pages() too: neither of those gets into
I/O, and invalidate_mapping_pages() looks as if it does all that's
necessary for those pages to be put under writeback at the next
shrink_inactive_list() of the lruvec.

Or perhaps there's a gotcha or two, which we can fix up.
But please try to stick to truncation and invalidation.

Hugh

> ---
>  mm/shmem.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 142b0bc085e1..18aa88eff8e3 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -872,7 +872,7 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  	mutex_unlock(&shmem_swaplist_mutex);
>  	swapcache_free(swap, NULL);
>  redirty:
> -	set_page_dirty(page);
> +	redirty_page_for_writepage(wbc, page);
>  	if (wbc->for_reclaim)
>  		return AOP_WRITEPAGE_ACTIVATE;	/* Return with page locked */
>  	unlock_page(page);
> -- 
> 1.9.0

Re: [PATCH 2/5] shmemfs: Use redirty_page_for_writepage()

[Chris Wilson]

On Wed, Mar 05, 2014 at 08:06:23PM -0800, Hugh Dickins wrote:
> On Wed, 5 Mar 2014, Chris Wilson wrote:
> 
> > "When we cannot write a page we should use redirty_page_for_writepage()
> > instead of plain set_page_dirty(). That tells writeback code we have
> > problems, redirties only the page (redirtying buffers is not needed),
> > and updates mm accounting of failed page writes."
> 
> I didn't locate the origin of that quotation, but it's talking about
> the usual filesystem cap_account_dirty/cap_account_writeback protocol.
> 
> shmem doesn't participate it that: it only writes out (to swap) under
> memory pressure, not for sync, and follows a much simpler path.  Using
> redirty_page_for_writepage() would lead it into complications, some of
> which we prefer to avoid for efficiency, some of which would actually
> be wrong (unless/until there's reason to convert shmem over to the
> full protocol).
> 
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Hugh Dickins <hughd@google.com>
> 
> So NAK.

No worries, I was only concerned with that it seem to be doing magic
with writeback accounting, which seemed relevant.
 
> But you didn't explain why you want to make this change.  I presume
> it's for the 5/5 which you didn't Cc to me, but I've looked up on
> lists.freedesktop.org/archives/intel-gfx.
> 
> That's a bigger change and worrying: I've not thought through
> the consequences of shmem page writeback from generic_writepages()
> called from within a slab shrinker: we're used to doing shmem page
> writeback from pageout() in mm/vmscan.c and nowhere else.
> 
> One thing that would certainly be wrong (liable to deadlock) would
> be to do that when shrink_control's gfp_mask does not have __GFP_IO.
> But I'm not comfortable with page writeback from this level at all.
> 
> The shmem_truncate_range() (you've had for a long time) should be safe,
> and the new invalidate_mapping_pages() too: neither of those gets into
> I/O, and invalidate_mapping_pages() looks as if it does all that's
> necessary for those pages to be put under writeback at the next
> shrink_inactive_list() of the lruvec.
> 
> Or perhaps there's a gotcha or two, which we can fix up.
> But please try to stick to truncation and invalidation.

Thank you for your feedback. I am just trying to poke things to
understand how we manage to end up calling oom-killer with an empty
swap. My presumption was that we were failing to trigger writeback. I
shall see how we fare with just invalidating the inode.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] shmemfs: Report ENOMEM for page allocation failures outside of tmpfs faults

[Chris Wilson]

On Wed, Mar 05, 2014 at 07:14:49PM -0800, Hugh Dickins wrote:
> On Wed, 5 Mar 2014, Chris Wilson wrote:
> 
> > The intention of returning ENOSPC for a page allocation failure due to
> > memory exhausstion in shmem_getpage_gfp() is purely "so that a failure
> > on a sparse tmpfs mapping will give SIGBUS not OOM." However, for other
> > callers, for example i915.ko, we want to distinguish the error message
> > reported to userspace between ENOSPC (meaning that we were unable to fit
> > the object/execbuffer into the aperture) and ENOMEM (meaning that we
> > were unable to allocate pages for the object).
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Hugh Dickins <hughd@google.com>
> 
> I'm not keen on this: perhaps because it looks like a hack of yours -
> and draws attention to what might be thought a hack of mine ;)
> 
> Nor am I thrilled by what was there before (we have three cases which
> ought to be distinguished, but only ENOMEM and ENOSPC to distinguish
> between them); but would rather it stay as is, than change what's
> reported to the user after all these years.
> 
> But I do see your point, and asking you to convert ENOSPC to ENOMEM
> in all your drivers/gpu calls might be tiresome.
> 
> I think we have a reasonable compromise: shmem_read_mapping_page_gfp()
> is already the wrapper provided just for you guys, how about posting
> a patch to map ENOSPC to ENOMEM there?
> 
> (You're using the MS_KERNMOUNT, so won't hit a max_blocks limit.)

Actually, I only need to the conversion along a single error path. So if
you are happy that the only reason shmem_get_page() would report
-ENOSPC would be for a hard memory failure, it seems permissible to
simply fix it up in our driver:

git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index f2efed9955bd..cfc4fcd90b6b 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2099,7 +2099,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 GTT
+        * 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

Whether the other drivers (gma500, msm, omapdrm, udl, armada) care about
the strict distinction between ENOSPC and ENOMEM, I do not know.

Would it be possible for us to use sbinfo->max_blocks to reject large
objects in drm_gem_object_init()?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/5] shmemfs: Report ENOMEM for page allocation failures outside of tmpfs faults

[Hugh Dickins]

On Thu, 6 Mar 2014, Chris Wilson wrote:
> On Wed, Mar 05, 2014 at 07:14:49PM -0800, Hugh Dickins wrote:
> > On Wed, 5 Mar 2014, Chris Wilson wrote:
> > 
> > > The intention of returning ENOSPC for a page allocation failure due to
> > > memory exhausstion in shmem_getpage_gfp() is purely "so that a failure
> > > on a sparse tmpfs mapping will give SIGBUS not OOM." However, for other
> > > callers, for example i915.ko, we want to distinguish the error message
> > > reported to userspace between ENOSPC (meaning that we were unable to fit
> > > the object/execbuffer into the aperture) and ENOMEM (meaning that we
> > > were unable to allocate pages for the object).
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > Cc: Hugh Dickins <hughd@google.com>
> > 
> > I'm not keen on this: perhaps because it looks like a hack of yours -
> > and draws attention to what might be thought a hack of mine ;)
> > 
> > Nor am I thrilled by what was there before (we have three cases which
> > ought to be distinguished, but only ENOMEM and ENOSPC to distinguish
> > between them); but would rather it stay as is, than change what's
> > reported to the user after all these years.
> > 
> > But I do see your point, and asking you to convert ENOSPC to ENOMEM
> > in all your drivers/gpu calls might be tiresome.
> > 
> > I think we have a reasonable compromise: shmem_read_mapping_page_gfp()
> > is already the wrapper provided just for you guys, how about posting
> > a patch to map ENOSPC to ENOMEM there?
> > 
> > (You're using the MS_KERNMOUNT, so won't hit a max_blocks limit.)
> 
> Actually, I only need to the conversion along a single error path. So if
> you are happy that the only reason shmem_get_page() would report
> -ENOSPC would be for a hard memory failure, it seems permissible to
> simply fix it up in our driver:
> 
> git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index f2efed9955bd..cfc4fcd90b6b 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2099,7 +2099,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 GTT
> +        * 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);
>  }

Yes, that's fine by me: it's your territory, and shmem is not suddenly
going to use ENOSPC for something else altogether.  I assumed you were
wanting to do it once for all the GPU drivers, but if the others don't
care at present, yes, do it here for i915 - we can always move the
fixup to shmem_read_mapping_page_gfp() later on if there's a need,

>  
>  /* Ensure that the associated pages are gathered from the backing storage
> 
> Whether the other drivers (gma500, msm, omapdrm, udl, armada) care about
> the strict distinction between ENOSPC and ENOMEM, I do not know.
> 
> Would it be possible for us to use sbinfo->max_blocks to reject large
> objects in drm_gem_object_init()?

No, not without using your own separate shm mount: shmem_file_setup()
uses the internal shm_mnt (used for anon shared mmappings and SysV SHM),
and (rightly or wrongly) we have never imposed a max_blocks limit on
that mount.

But your question does suggest one possibility.  drm_gem.c is doing
the shmem_file_setup() with VM_NORESERVE flag, which gives you the
behaviour of tmpfs files (page by page accounting, perhaps ENOSPC
if !vm_enough_memory for a page) rather than SysV SHM accounting
(succeed or fail according to the initial shmem_file_setup size,
without page by page accounting thereafter).

You might want to consider whether the SHM-style accounting would
suit you better: I don't know what dictated the choice of VM_NORESERVE
originally, and I don't know if it would be a good idea to change over
or not - it would be a terrible idea if you use large sparse objects.

Hugh