All the mail mirrored from lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops
@ 2016-03-23  6:09 akash.goel
  2016-03-23  6:09 ` [PATCH 2/2] drm/i915: Make pages of GFX allocations movable akash.goel
                   ` (5 more replies)
  0 siblings, 6 replies; 30+ messages in thread
From: akash.goel @ 2016-03-23  6:09 UTC (permalink / raw
  To: intel-gfx; +Cc: Chris Wilson, Hugh Dickins, linux-mm, Sourab Gupta, Akash Goel

From: Chris Wilson <chris@chris-wilson.co.uk>

This provides support for the Drivers or shmem file owners to register
a set of callbacks, which can be invoked from the address space operations
methods implemented by shmem.
This allow the file owners to hook into the shmem address space operations
to do some extra/custom operations in addition to the default ones.

The private_data field of address_space struct is used to store the pointer
to driver specific ops.
Currently only one ops field is defined, which is migratepage, but can be
extended on need basis.

The need for driver specific operations arises since some of the operations
(like migratepage) may not be handled completely within shmem, so as to be
effective, and would need some driver specific handling also.

Specifically, i915.ko would like to participate in migratepage().
i915.ko uses shmemfs to provide swappable backing storage for its user
objects, but when those objects are in use by the GPU it must pin the entire
object until the GPU is idle. As a result, large chunks of memory can be
arbitrarily withdrawn from page migration, resulting in premature
out-of-memory due to fragmentation. However, if i915.ko can receive the
migratepage() request, it can then flush the object from the GPU, remove
its pin and thus enable the migration.

Since Gfx allocations are one of the major consumer of system memory, its
imperative to have such a mechanism to effectively deal with fragmentation.
And therefore the need for such a provision for initiating driver specific
actions during address space operations.

Cc: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 include/linux/shmem_fs.h | 17 +++++++++++++++++
 mm/shmem.c               | 17 ++++++++++++++++-
 2 files changed, 33 insertions(+), 1 deletion(-)

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index 4d4780c..6cfa76a 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -34,11 +34,28 @@ struct shmem_sb_info {
 	struct mempolicy *mpol;     /* default memory policy for mappings */
 };
 
+struct shmem_dev_info {
+	void *dev_private_data;
+	int (*dev_migratepage)(struct address_space *mapping,
+			       struct page *newpage, struct page *page,
+			       enum migrate_mode mode, void *dev_priv_data);
+};
+
 static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
 {
 	return container_of(inode, struct shmem_inode_info, vfs_inode);
 }
 
+static inline int shmem_set_device_ops(struct address_space *mapping,
+				struct shmem_dev_info *info)
+{
+	if (mapping->private_data != NULL)
+		return -EEXIST;
+
+	mapping->private_data = info;
+	return 0;
+}
+
 /*
  * Functions in mm/shmem.c called directly from elsewhere:
  */
diff --git a/mm/shmem.c b/mm/shmem.c
index 440e2a7..f8625c4 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -952,6 +952,21 @@ redirty:
 	return 0;
 }
 
+#ifdef CONFIG_MIGRATION
+static int shmem_migratepage(struct address_space *mapping,
+			     struct page *newpage, struct page *page,
+			     enum migrate_mode mode)
+{
+	struct shmem_dev_info *dev_info = mapping->private_data;
+
+	if (dev_info && dev_info->dev_migratepage)
+		return dev_info->dev_migratepage(mapping, newpage, page,
+				mode, dev_info->dev_private_data);
+
+	return migrate_page(mapping, newpage, page, mode);
+}
+#endif
+
 #ifdef CONFIG_NUMA
 #ifdef CONFIG_TMPFS
 static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
@@ -3168,7 +3183,7 @@ static const struct address_space_operations shmem_aops = {
 	.write_end	= shmem_write_end,
 #endif
 #ifdef CONFIG_MIGRATION
-	.migratepage	= migrate_page,
+	.migratepage	= shmem_migratepage,
 #endif
 	.error_remove_page = generic_error_remove_page,
 };
-- 
1.9.2

--
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>

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/2] drm/i915: Make pages of GFX allocations movable
  2016-03-23  6:09 [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops akash.goel
@ 2016-03-23  6:09 ` akash.goel
  2016-03-23  7:58   ` Chris Wilson
  2016-03-23  7:39 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops Patchwork
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 30+ messages in thread
From: akash.goel @ 2016-03-23  6:09 UTC (permalink / raw
  To: intel-gfx; +Cc: Chris Wilson, Hugh Dickins, linux-mm, Sourab Gupta, Akash Goel

From: Chris Wilson <chris@chris-wilson.co.uk>

On a long run of more than 2-3 days, physical memory tends to get fragmented
severely, which considerably slows down the system. In such a scenario,
Shrinker is also unable to help as lack of memory is not the actual problem,
since it has been observed that there are enough free pages of 0 order.

To address the issue of external fragementation, kernel does a compaction
(which involves migration of pages) but it's efficacy depends upon how many
pages are marked as MOVABLE, as only those pages can be migrated.

Currently the backing pages for GFX buffers are allocated from shmemfs
with GFP_RECLAIMABLE flag, in units of 4KB pages.
In the case of limited Swap space, it may not be possible always to reclaim
or swap-out pages of all the inactive objects, to make way for free space
allowing formation of higher order groups of physically-contiguous pages
on compaction.

Just marking the GFX pages as MOVABLE will not suffice, as i915 Driver
has to pin the pages if they are in use by GPU, which will prevent their
migration. So the migratepage callback in shmem is also hooked up to get a
notification when kernel initiates the page migration. On the notification,
i915 Driver appropriately unpin the pages.
With this Driver can effectively mark the GFX pages as MOVABLE and hence
mitigate the fragmentation problem.

Cc: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h |   3 +
 drivers/gpu/drm/i915/i915_gem.c | 128 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f330a53..28e50c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -52,6 +52,7 @@
 #include <linux/intel-iommu.h>
 #include <linux/kref.h>
 #include <linux/pm_qos.h>
+#include <linux/shmem_fs.h>
 #include "intel_guc.h"
 #include "intel_dpll_mgr.h"
 
@@ -1966,6 +1967,8 @@ struct drm_i915_private {
 
 	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
 
+	struct shmem_dev_info  migrate_info;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8588c83..a4af5b6 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -33,6 +33,7 @@
 #include "i915_trace.h"
 #include "intel_drv.h"
 #include <linux/shmem_fs.h>
+#include <linux/migrate.h>
 #include <linux/slab.h>
 #include <linux/swap.h>
 #include <linux/pci.h>
@@ -2204,6 +2205,7 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 		if (obj->madv == I915_MADV_WILLNEED)
 			mark_page_accessed(page);
 
+		set_page_private(page, 0);
 		page_cache_release(page);
 	}
 	obj->dirty = 0;
@@ -2318,6 +2320,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			sg->length += PAGE_SIZE;
 		}
 		last_pfn = page_to_pfn(page);
+		set_page_private(page, (unsigned long)obj);
 
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
@@ -2343,8 +2346,11 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 
 err_pages:
 	sg_mark_end(sg);
-	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
-		page_cache_release(sg_page_iter_page(&sg_iter));
+	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
+		page = sg_page_iter_page(&sg_iter);
+		set_page_private(page, 0);
+		page_cache_release(page);
+	}
 	sg_free_table(st);
 	kfree(st);
 
@@ -4465,9 +4471,116 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 	.put_pages = i915_gem_object_put_pages_gtt,
 };
 
+#ifdef CONFIG_MIGRATION
+static int i915_migratepage(struct address_space *mapping,
+			    struct page *newpage, struct page *page,
+			    enum migrate_mode mode, void *dev_priv_data)
+{
+	struct drm_i915_private *dev_priv = dev_priv_data;
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_i915_gem_object *obj;
+	unsigned long timeout = msecs_to_jiffies(10) + 1;
+	int ret = 0;
+
+	WARN((page_count(newpage) != 1), "Unexpected ref count for newpage\n");
+
+	/*
+	 * Clear the private field of the new target page as it could have a
+	 * stale value in the private field. Otherwise later on if this page
+	 * itself gets migrated, without getting referred by the Driver
+	 * in between, the stale value would cause the i915_migratepage
+	 * function to go for a toss as object pointer is derived from it.
+	 * This should be safe since at the time of migration, private field
+	 * of the new page (which is actually an independent free 4KB page now)
+	 * should be like a don't care for the kernel.
+	 */
+	set_page_private(newpage, 0);
+
+	/*
+	 * Check the page count, if Driver also has a reference then it should
+	 * be more than 2, as shmem will have one reference and one reference
+	 * would have been taken by the migration path itself. So if reference
+	 * is <=2, we can directly invoke the migration function.
+	 */
+	if (page_count(page) <= 2)
+		goto migrate;
+
+	/*
+	 * Use trylock here, with a timeout, for struct_mutex as
+	 * otherwise there is a possibility of deadlock due to lock
+	 * inversion. This path, which tries to migrate a particular
+	 * page after locking that page, can race with a path which
+	 * truncate/purge pages of the corresponding object (after
+	 * acquiring struct_mutex). Since page truncation will also
+	 * try to lock the page, a scenario of deadlock can arise.
+	 */
+	while (!mutex_trylock(&dev->struct_mutex) && --timeout)
+		schedule_timeout_killable(1);
+	if (timeout == 0) {
+		DRM_DEBUG_DRIVER("Unable to acquire device mutex.\n");
+		return -EBUSY;
+	}
+
+	obj = (struct drm_i915_gem_object *)page_private(page);
+
+	if (!PageSwapCache(page) && obj) {
+		/*
+		 * Avoid the migration of pages if they are being actively used
+		 * by GPU or pinned for Display.
+		 * Also skip the migration for purgeable objects otherwise there
+		 * will be a deadlock when shmem will try to lock the page for
+		 * truncation, which is already locked by the caller before
+		 * migration.
+		 * Also HW access would be required for a bound object for which
+		 * device has to be kept runtime active. But a deadlock scenario
+		 * can arise if the attempt is made to resume the device, when
+		 * either a suspend or a resume operation is already happening
+		 * concurrently from some other path and that only actually
+		 * triggered the compaction. So only unbind if the device is
+		 * currently runtime active.
+		 */
+		if (obj->active || obj->pin_display ||
+		    obj->madv == I915_MADV_DONTNEED ||
+		    !intel_runtime_pm_get_if_in_use(dev_priv)) {
+			ret = -EBUSY;
+		} else {
+			ret = drop_pages(obj);
+			intel_runtime_pm_put(dev_priv);
+		}
+
+		BUG_ON(!ret && page_private(page));
+	}
+
+	mutex_unlock(&dev->struct_mutex);
+	if (ret)
+		return ret;
+
+	/*
+	 * Ideally here we don't expect the page count to be > 2, as driver
+	 * would have dropped its reference, but occasionally it has been seen
+	 * coming as 3 & 4. This leads to a situation of unexpected page count,
+	 * causing migration failure, with -EGAIN error. This then leads to
+	 * multiple attempts by the kernel to migrate the same set of pages.
+	 * And sometimes the repeated attempts proves detrimental for stability.
+	 * Also since we don't know who is the other owner, and for how long its
+	 * gonna keep the reference, its better to return -EBUSY.
+	 */
+	if (page_count(page) > 2)
+		return -EBUSY;
+
+migrate:
+	ret = migrate_page(mapping, newpage, page, mode);
+	if (ret)
+		DRM_DEBUG_DRIVER("page=%p migration returned %d\n", page, ret);
+
+	return ret;
+}
+#endif
+
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	struct address_space *mapping;
 	gfp_t mask;
@@ -4481,7 +4594,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 		return NULL;
 	}
 
-	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
+	mask = GFP_HIGHUSER_MOVABLE;
 	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
 		/* 965gm cannot relocate objects above 4GiB. */
 		mask &= ~__GFP_HIGHMEM;
@@ -4491,6 +4604,10 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	mapping = file_inode(obj->base.filp)->i_mapping;
 	mapping_set_gfp_mask(mapping, mask);
 
+#ifdef CONFIG_MIGRATION
+	shmem_set_device_ops(mapping, &dev_priv->migrate_info);
+#endif
+
 	i915_gem_object_init(obj, &i915_gem_object_ops);
 
 	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
@@ -4993,6 +5110,11 @@ int i915_gem_init(struct drm_device *dev)
 		ret = 0;
 	}
 
+#ifdef CONFIG_MIGRATION
+	dev_priv->migrate_info.dev_private_data = dev_priv;
+	dev_priv->migrate_info.dev_migratepage = i915_migratepage;
+#endif
+
 out_unlock:
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 	mutex_unlock(&dev->struct_mutex);
-- 
1.9.2

--
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>

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops
  2016-03-23  6:09 [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops akash.goel
  2016-03-23  6:09 ` [PATCH 2/2] drm/i915: Make pages of GFX allocations movable akash.goel
@ 2016-03-23  7:39 ` Patchwork
  2016-03-24 12:11   ` Joonas Lahtinen
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2016-03-23  7:39 UTC (permalink / raw
  To: Akash Goel; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops
URL   : https://patchwork.freedesktop.org/series/4780/
State : failure

== Summary ==

Series 4780v1 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/4780/revisions/1/mbox/

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-a:
                pass       -> INCOMPLETE (hsw-gt2)

bdw-nuci7        total:192  pass:180  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:192  pass:171  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:192  pass:153  dwarn:2   dfail:0   fail:0   skip:37 
hsw-brixbox      total:192  pass:170  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:70   pass:62   dwarn:0   dfail:0   fail:0   skip:7  
ivb-t430s        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
skl-i5k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:192  pass:158  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:192  pass:158  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1682/

83ed25fa1b956275542da63eb98dc8fd2291329d drm-intel-nightly: 2016y-03m-22d-15h-20m-55s UTC integration manifest
88691ff2a039ce25efae7c13a1a91d1665bd31a0 drm/i915: Make pages of GFX allocations movable
155017b7fc3763160a721400efd9e6c58ea8bd0c shmem: Support for registration of Driver/file owner specific ops

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] drm/i915: Make pages of GFX allocations movable
  2016-03-23  6:09 ` [PATCH 2/2] drm/i915: Make pages of GFX allocations movable akash.goel
@ 2016-03-23  7:58   ` Chris Wilson
  2016-03-23  8:25     ` Goel, Akash
  0 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2016-03-23  7:58 UTC (permalink / raw
  To: akash.goel; +Cc: intel-gfx, Hugh Dickins, linux-mm, Sourab Gupta

On Wed, Mar 23, 2016 at 11:39:44AM +0530, akash.goel@intel.com wrote:
> +#ifdef CONFIG_MIGRATION
> +static int i915_migratepage(struct address_space *mapping,
> +			    struct page *newpage, struct page *page,
> +			    enum migrate_mode mode, void *dev_priv_data)

If we move this to i915_gem_shrink_migratepage (i.e. i915_gem_shrink),
we can

> +	/*
> +	 * Use trylock here, with a timeout, for struct_mutex as
> +	 * otherwise there is a possibility of deadlock due to lock
> +	 * inversion. This path, which tries to migrate a particular
> +	 * page after locking that page, can race with a path which
> +	 * truncate/purge pages of the corresponding object (after
> +	 * acquiring struct_mutex). Since page truncation will also
> +	 * try to lock the page, a scenario of deadlock can arise.
> +	 */
> +	while (!mutex_trylock(&dev->struct_mutex) && --timeout)
> +		schedule_timeout_killable(1);

replace this with i915_gem_shrinker_lock() and like constructs with the
other shrinkers. Any reason for dropping the early
if (!page_private(obj)) skip?

Similarly there are other patterns here that would benefit from
integration with existing shrinker logic. However, things like tidying
up the pin_display, unbinding, rpm lock inversion are still only on
list.
-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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] drm/i915: Make pages of GFX allocations movable
  2016-03-23  7:58   ` Chris Wilson
@ 2016-03-23  8:25     ` Goel, Akash
  2016-03-24  8:13       ` Chris Wilson
  0 siblings, 1 reply; 30+ messages in thread
From: Goel, Akash @ 2016-03-23  8:25 UTC (permalink / raw
  To: Chris Wilson, intel-gfx, Hugh Dickins, linux-mm; +Cc: Sourab Gupta, akash.goel



On 3/23/2016 1:28 PM, Chris Wilson wrote:
> On Wed, Mar 23, 2016 at 11:39:44AM +0530, akash.goel@intel.com wrote:
>> +#ifdef CONFIG_MIGRATION
>> +static int i915_migratepage(struct address_space *mapping,
>> +			    struct page *newpage, struct page *page,
>> +			    enum migrate_mode mode, void *dev_priv_data)
>
> If we move this to i915_gem_shrink_migratepage (i.e. i915_gem_shrink),
> we can
>
>> +	/*
>> +	 * Use trylock here, with a timeout, for struct_mutex as
>> +	 * otherwise there is a possibility of deadlock due to lock
>> +	 * inversion. This path, which tries to migrate a particular
>> +	 * page after locking that page, can race with a path which
>> +	 * truncate/purge pages of the corresponding object (after
>> +	 * acquiring struct_mutex). Since page truncation will also
>> +	 * try to lock the page, a scenario of deadlock can arise.
>> +	 */
>> +	while (!mutex_trylock(&dev->struct_mutex) && --timeout)
>> +		schedule_timeout_killable(1);
>
> replace this with i915_gem_shrinker_lock() and like constructs with the
> other shrinkers.

fine, will rename the function to gem_shrink_migratepage, move it inside 
the gem_shrinker.c file, and use the existing constructs.

 > Any reason for dropping the early
 > if (!page_private(obj)) skip?
 >

Would this sequence be fine ?

	if (!page_private(page))
		goto migrate; /*skip */

	Loop for locking mutex

	obj = (struct drm_i915_gem_object *)page_private(page);

	if (!PageSwapCache(page) && obj) {


> Similarly there are other patterns here that would benefit from
> integration with existing shrinker logic. However, things like tidying
> up the pin_display, unbinding, rpm lock inversion are still only on
> list.

Tidying, like split that one single if condition into multiple if, else 
if blocks ?

Best regards
Akash
> -Chris
>

--
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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] drm/i915: Make pages of GFX allocations movable
  2016-03-23  8:25     ` Goel, Akash
@ 2016-03-24  8:13       ` Chris Wilson
  2016-03-24 18:22           ` akash.goel
  0 siblings, 1 reply; 30+ messages in thread
From: Chris Wilson @ 2016-03-24  8:13 UTC (permalink / raw
  To: Goel, Akash; +Cc: intel-gfx, Hugh Dickins, linux-mm, Sourab Gupta

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

Yes.

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

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

-- 
Chris Wilson, Intel Open Source Technology Centre

--
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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops
  2016-03-23  6:09 [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops akash.goel
@ 2016-03-24 12:11   ` Joonas Lahtinen
  2016-03-23  7:39 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops Patchwork
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 30+ messages in thread
From: Joonas Lahtinen @ 2016-03-24 12:11 UTC (permalink / raw
  To: akash.goel, intel-gfx; +Cc: linux-mm, Sourab Gupta, Hugh Dickins

On ke, 2016-03-23 at 11:39 +0530, akash.goel@intel.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> This provides support for the Drivers or shmem file owners to register
> a set of callbacks, which can be invoked from the address space operations
> methods implemented by shmem.
> This allow the file owners to hook into the shmem address space operations
> to do some extra/custom operations in addition to the default ones.
> 
> The private_data field of address_space struct is used to store the pointer
> to driver specific ops.
> Currently only one ops field is defined, which is migratepage, but can be
> extended on need basis.
> 
> The need for driver specific operations arises since some of the operations
> (like migratepage) may not be handled completely within shmem, so as to be
> effective, and would need some driver specific handling also.
> 
> Specifically, i915.ko would like to participate in migratepage().
> i915.ko uses shmemfs to provide swappable backing storage for its user
> objects, but when those objects are in use by the GPU it must pin the entire
> object until the GPU is idle. As a result, large chunks of memory can be
> arbitrarily withdrawn from page migration, resulting in premature
> out-of-memory due to fragmentation. However, if i915.ko can receive the
> migratepage() request, it can then flush the object from the GPU, remove
> its pin and thus enable the migration.
> 
> Since Gfx allocations are one of the major consumer of system memory, its
> imperative to have such a mechanism to effectively deal with fragmentation.
> And therefore the need for such a provision for initiating driver specific
> actions during address space operations.
> 
> Cc: Hugh Dickins <hughd@google.com>
> Cc: linux-mm@kvack.org
> Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
> A include/linux/shmem_fs.h | 17 +++++++++++++++++
> A mm/shmem.cA A A A A A A A A A A A A A A | 17 ++++++++++++++++-
> A 2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index 4d4780c..6cfa76a 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -34,11 +34,28 @@ struct shmem_sb_info {
> A 	struct mempolicy *mpol;A A A A A /* default memory policy for mappings */
> A };
> A 
> +struct shmem_dev_info {
> +	void *dev_private_data;
> +	int (*dev_migratepage)(struct address_space *mapping,
> +			A A A A A A A struct page *newpage, struct page *page,
> +			A A A A A A A enum migrate_mode mode, void *dev_priv_data);

One might want to have a separate shmem_dev_operations struct or
similar.

> +};
> +
> A static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
> A {
> A 	return container_of(inode, struct shmem_inode_info, vfs_inode);
> A }
> A 
> +static inline int shmem_set_device_ops(struct address_space *mapping,
> +				struct shmem_dev_info *info)
> +{
> +	if (mapping->private_data != NULL)
> +		return -EEXIST;
> +

I did a quick random peek and most set functions are just void and
override existing data. I'd suggest the same.

> +	mapping->private_data = info;

Also, doesn't this kinda steal the mapping->private_data, might that be
unexpected for the user? I notice currently it's not being touched at
all.

> +	return 0;
> +}
> +
> A /*
> A  * Functions in mm/shmem.c called directly from elsewhere:
> A  */
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 440e2a7..f8625c4 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -952,6 +952,21 @@ redirty:
> A 	return 0;
> A }
> A 
> +#ifdef CONFIG_MIGRATION
> +static int shmem_migratepage(struct address_space *mapping,
> +			A A A A A struct page *newpage, struct page *page,
> +			A A A A A enum migrate_mode mode)
> +{
> +	struct shmem_dev_info *dev_info = mapping->private_data;
> +
> +	if (dev_info && dev_info->dev_migratepage)
> +		return dev_info->dev_migratepage(mapping, newpage, page,
> +				mode, dev_info->dev_private_data);
> +
> +	return migrate_page(mapping, newpage, page, mode);
> +}
> +#endif
> +
> A #ifdef CONFIG_NUMA
> A #ifdef CONFIG_TMPFS
> A static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> @@ -3168,7 +3183,7 @@ static const struct address_space_operations shmem_aops = {
> A 	.write_end	= shmem_write_end,
> A #endif
> A #ifdef CONFIG_MIGRATION
> -	.migratepage	= migrate_page,
> +	.migratepage	= shmem_migratepage,
> A #endif
> A 	.error_remove_page = generic_error_remove_page,
> A };
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

--
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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops
@ 2016-03-24 12:11   ` Joonas Lahtinen
  0 siblings, 0 replies; 30+ messages in thread
From: Joonas Lahtinen @ 2016-03-24 12:11 UTC (permalink / raw
  To: akash.goel, intel-gfx; +Cc: linux-mm, Sourab Gupta, Hugh Dickins

On ke, 2016-03-23 at 11:39 +0530, akash.goel@intel.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> This provides support for the Drivers or shmem file owners to register
> a set of callbacks, which can be invoked from the address space operations
> methods implemented by shmem.
> This allow the file owners to hook into the shmem address space operations
> to do some extra/custom operations in addition to the default ones.
> 
> The private_data field of address_space struct is used to store the pointer
> to driver specific ops.
> Currently only one ops field is defined, which is migratepage, but can be
> extended on need basis.
> 
> The need for driver specific operations arises since some of the operations
> (like migratepage) may not be handled completely within shmem, so as to be
> effective, and would need some driver specific handling also.
> 
> Specifically, i915.ko would like to participate in migratepage().
> i915.ko uses shmemfs to provide swappable backing storage for its user
> objects, but when those objects are in use by the GPU it must pin the entire
> object until the GPU is idle. As a result, large chunks of memory can be
> arbitrarily withdrawn from page migration, resulting in premature
> out-of-memory due to fragmentation. However, if i915.ko can receive the
> migratepage() request, it can then flush the object from the GPU, remove
> its pin and thus enable the migration.
> 
> Since Gfx allocations are one of the major consumer of system memory, its
> imperative to have such a mechanism to effectively deal with fragmentation.
> And therefore the need for such a provision for initiating driver specific
> actions during address space operations.
> 
> Cc: Hugh Dickins <hughd@google.com>
> Cc: linux-mm@kvack.org
> Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> ---
>  include/linux/shmem_fs.h | 17 +++++++++++++++++
>  mm/shmem.c               | 17 ++++++++++++++++-
>  2 files changed, 33 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index 4d4780c..6cfa76a 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -34,11 +34,28 @@ struct shmem_sb_info {
>  	struct mempolicy *mpol;     /* default memory policy for mappings */
>  };
>  
> +struct shmem_dev_info {
> +	void *dev_private_data;
> +	int (*dev_migratepage)(struct address_space *mapping,
> +			       struct page *newpage, struct page *page,
> +			       enum migrate_mode mode, void *dev_priv_data);

One might want to have a separate shmem_dev_operations struct or
similar.

> +};
> +
>  static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
>  {
>  	return container_of(inode, struct shmem_inode_info, vfs_inode);
>  }
>  
> +static inline int shmem_set_device_ops(struct address_space *mapping,
> +				struct shmem_dev_info *info)
> +{
> +	if (mapping->private_data != NULL)
> +		return -EEXIST;
> +

I did a quick random peek and most set functions are just void and
override existing data. I'd suggest the same.

> +	mapping->private_data = info;

Also, doesn't this kinda steal the mapping->private_data, might that be
unexpected for the user? I notice currently it's not being touched at
all.

> +	return 0;
> +}
> +
>  /*
>   * Functions in mm/shmem.c called directly from elsewhere:
>   */
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 440e2a7..f8625c4 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -952,6 +952,21 @@ redirty:
>  	return 0;
>  }
>  
> +#ifdef CONFIG_MIGRATION
> +static int shmem_migratepage(struct address_space *mapping,
> +			     struct page *newpage, struct page *page,
> +			     enum migrate_mode mode)
> +{
> +	struct shmem_dev_info *dev_info = mapping->private_data;
> +
> +	if (dev_info && dev_info->dev_migratepage)
> +		return dev_info->dev_migratepage(mapping, newpage, page,
> +				mode, dev_info->dev_private_data);
> +
> +	return migrate_page(mapping, newpage, page, mode);
> +}
> +#endif
> +
>  #ifdef CONFIG_NUMA
>  #ifdef CONFIG_TMPFS
>  static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
> @@ -3168,7 +3183,7 @@ static const struct address_space_operations shmem_aops = {
>  	.write_end	= shmem_write_end,
>  #endif
>  #ifdef CONFIG_MIGRATION
> -	.migratepage	= migrate_page,
> +	.migratepage	= shmem_migratepage,
>  #endif
>  	.error_remove_page = generic_error_remove_page,
>  };
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v2 2/2] drm/i915: Make pages of GFX allocations movable
  2016-03-24  8:13       ` Chris Wilson
@ 2016-03-24 18:22           ` akash.goel
  0 siblings, 0 replies; 30+ messages in thread
From: akash.goel @ 2016-03-24 18:22 UTC (permalink / raw
  To: intel-gfx; +Cc: Chris Wilson, Hugh Dickins, linux-mm, Sourab Gupta, Akash Goel

From: Chris Wilson <chris@chris-wilson.co.uk>

On a long run of more than 2-3 days, physical memory tends to get fragmented
severely, which considerably slows down the system. In such a scenario,
Shrinker is also unable to help as lack of memory is not the actual problem,
since it has been observed that there are enough free pages of 0 order.

To address the issue of external fragementation, kernel does a compaction
(which involves migration of pages) but it's efficacy depends upon how many
pages are marked as MOVABLE, as only those pages can be migrated.

Currently the backing pages for GFX buffers are allocated from shmemfs
with GFP_RECLAIMABLE flag, in units of 4KB pages.
In the case of limited Swap space, it may not be possible always to reclaim
or swap-out pages of all the inactive objects, to make way for free space
allowing formation of higher order groups of physically-contiguous pages
on compaction.

Just marking the GFX pages as MOVABLE will not suffice, as i915 Driver
has to pin the pages if they are in use by GPU, which will prevent their
migration. So the migratepage callback in shmem is also hooked up to get a
notification when kernel initiates the page migration. On the notification,
i915 Driver appropriately unpin the pages.
With this Driver can effectively mark the GFX pages as MOVABLE and hence
mitigate the fragmentation problem.

v2:
 - Rename the migration routine to gem_shrink_migratepage, move it to the
   shrinker file, and use the existing constructs (Chris)
 - To cleanup, add a new helper function to encapsulate all page migration
   skip conditions (Chris)
 - Add a new local helper function in shrinker file, for dropping the
   backing pages, and call the same from gem_shrink() also (Chris)

Cc: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |   3 +
 drivers/gpu/drm/i915/i915_gem.c          |  16 ++-
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 173 ++++++++++++++++++++++++++++---
 3 files changed, 177 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f330a53..28e50c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -52,6 +52,7 @@
 #include <linux/intel-iommu.h>
 #include <linux/kref.h>
 #include <linux/pm_qos.h>
+#include <linux/shmem_fs.h>
 #include "intel_guc.h"
 #include "intel_dpll_mgr.h"
 
@@ -1966,6 +1967,8 @@ struct drm_i915_private {
 
 	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
 
+	struct shmem_dev_info  migrate_info;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8588c83..59ab58a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2204,6 +2204,7 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 		if (obj->madv == I915_MADV_WILLNEED)
 			mark_page_accessed(page);
 
+		set_page_private(page, 0);
 		page_cache_release(page);
 	}
 	obj->dirty = 0;
@@ -2318,6 +2319,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			sg->length += PAGE_SIZE;
 		}
 		last_pfn = page_to_pfn(page);
+		set_page_private(page, (unsigned long)obj);
 
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
@@ -2343,8 +2345,11 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 
 err_pages:
 	sg_mark_end(sg);
-	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
-		page_cache_release(sg_page_iter_page(&sg_iter));
+	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
+		page = sg_page_iter_page(&sg_iter);
+		set_page_private(page, 0);
+		page_cache_release(page);
+	}
 	sg_free_table(st);
 	kfree(st);
 
@@ -4468,6 +4473,7 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	struct address_space *mapping;
 	gfp_t mask;
@@ -4481,7 +4487,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 		return NULL;
 	}
 
-	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
+	mask = GFP_HIGHUSER_MOVABLE;
 	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
 		/* 965gm cannot relocate objects above 4GiB. */
 		mask &= ~__GFP_HIGHMEM;
@@ -4491,6 +4497,10 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	mapping = file_inode(obj->base.filp)->i_mapping;
 	mapping_set_gfp_mask(mapping, mask);
 
+#ifdef CONFIG_MIGRATION
+	shmem_set_device_ops(mapping, &dev_priv->migrate_info);
+#endif
+
 	i915_gem_object_init(obj, &i915_gem_object_ops);
 
 	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d3c473f..f6c2c20 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -24,6 +24,7 @@
 
 #include <linux/oom.h>
 #include <linux/shmem_fs.h>
+#include <linux/migrate.h>
 #include <linux/slab.h>
 #include <linux/swap.h>
 #include <linux/pci.h>
@@ -87,6 +88,71 @@ static bool can_release_pages(struct drm_i915_gem_object *obj)
 	return swap_available() || obj->madv == I915_MADV_DONTNEED;
 }
 
+static bool can_migrate_page(struct drm_i915_gem_object *obj)
+{
+	/* Avoid the migration of page if being actively used by GPU */
+	if (obj->active)
+		return false;
+
+	/* Skip the migration for purgeable objects otherwise there
+	 * will be a deadlock when shmem will try to lock the page for
+	 * truncation, which is already locked by the caller before
+	 * migration.
+	 */
+	if (obj->madv == I915_MADV_DONTNEED)
+		return false;
+
+	/* Skip the migration for a pinned object */
+	if (obj->pages_pin_count != num_vma_bound(obj))
+		return false;
+
+	return true;
+}
+
+static int
+unsafe_drop_pages(struct drm_i915_gem_object *obj)
+{
+	struct i915_vma *vma, *next;
+	int ret;
+
+	drm_gem_object_reference(&obj->base);
+	list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link)
+		if (i915_vma_unbind(vma))
+			break;
+
+	ret = i915_gem_object_put_pages(obj);
+	drm_gem_object_unreference(&obj->base);
+
+	return ret;
+}
+
+static int
+do_migrate_page(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+	int ret = 0;
+
+	if (!can_migrate_page(obj))
+		return -EBUSY;
+
+	/* HW access would be required for a bound object for which
+	 * device has to be kept runtime active. But a deadlock scenario
+	 * can arise if the attempt is made to resume the device, when
+	 * either a suspend or a resume operation is already happening
+	 * concurrently from some other path and that only actually
+	 * triggered the compaction. So only unbind if the device is
+	 * currently runtime active.
+	 */
+	if (!intel_runtime_pm_get_if_in_use(dev_priv))
+		return -EBUSY;
+
+	if (!unsafe_drop_pages(obj))
+		ret = -EBUSY;
+
+	intel_runtime_pm_put(dev_priv);
+	return ret;
+}
+
 /**
  * i915_gem_shrink - Shrink buffer object caches
  * @dev_priv: i915 device
@@ -156,7 +222,6 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 		INIT_LIST_HEAD(&still_in_list);
 		while (count < target && !list_empty(phase->list)) {
 			struct drm_i915_gem_object *obj;
-			struct i915_vma *vma, *v;
 
 			obj = list_first_entry(phase->list,
 					       typeof(*obj), global_list);
@@ -172,18 +237,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			if (!can_release_pages(obj))
 				continue;
 
-			drm_gem_object_reference(&obj->base);
-
-			/* For the unbound phase, this should be a no-op! */
-			list_for_each_entry_safe(vma, v,
-						 &obj->vma_list, obj_link)
-				if (i915_vma_unbind(vma))
-					break;
-
-			if (i915_gem_object_put_pages(obj) == 0)
+			if (unsafe_drop_pages(obj) == 0)
 				count += obj->base.size >> PAGE_SHIFT;
-
-			drm_gem_object_unreference(&obj->base);
 		}
 		list_splice(&still_in_list, phase->list);
 	}
@@ -356,6 +411,95 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
 	return NOTIFY_DONE;
 }
 
+#ifdef CONFIG_MIGRATION
+static int i915_gem_shrink_migratepage(struct address_space *mapping,
+			    struct page *newpage, struct page *page,
+			    enum migrate_mode mode, void *dev_priv_data)
+{
+	struct drm_i915_private *dev_priv = dev_priv_data;
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_i915_gem_object *obj;
+	unsigned long timeout = msecs_to_jiffies(10) + 1;
+	bool unlock;
+	int ret = 0;
+
+	WARN((page_count(newpage) != 1), "Unexpected ref count for newpage\n");
+
+	/*
+	 * Clear the private field of the new target page as it could have a
+	 * stale value in the private field. Otherwise later on if this page
+	 * itself gets migrated, without getting referred by the Driver
+	 * in between, the stale value would cause the i915_migratepage
+	 * function to go for a toss as object pointer is derived from it.
+	 * This should be safe since at the time of migration, private field
+	 * of the new page (which is actually an independent free 4KB page now)
+	 * should be like a don't care for the kernel.
+	 */
+	set_page_private(newpage, 0);
+
+	if (!page_private(page))
+		goto migrate;
+
+	/*
+	 * Check the page count, if Driver also has a reference then it should
+	 * be more than 2, as shmem will have one reference and one reference
+	 * would have been taken by the migration path itself. So if reference
+	 * is <=2, we can directly invoke the migration function.
+	 */
+	if (page_count(page) <= 2)
+		goto migrate;
+
+	/*
+	 * Use trylock here, with a timeout, for struct_mutex as
+	 * otherwise there is a possibility of deadlock due to lock
+	 * inversion. This path, which tries to migrate a particular
+	 * page after locking that page, can race with a path which
+	 * truncate/purge pages of the corresponding object (after
+	 * acquiring struct_mutex). Since page truncation will also
+	 * try to lock the page, a scenario of deadlock can arise.
+	 */
+	while (!i915_gem_shrinker_lock(dev, &unlock) && --timeout)
+		schedule_timeout_killable(1);
+	if (timeout == 0) {
+		DRM_DEBUG_DRIVER("Unable to acquire device mutex.\n");
+		return -EBUSY;
+	}
+
+	obj = (struct drm_i915_gem_object *)page_private(page);
+
+	if (!PageSwapCache(page) && obj) {
+		ret = do_migrate_page(obj);
+		BUG_ON(!ret && page_private(page));
+	}
+
+	if (unlock)
+		mutex_unlock(&dev->struct_mutex);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * Ideally here we don't expect the page count to be > 2, as driver
+	 * would have dropped its reference, but occasionally it has been seen
+	 * coming as 3 & 4. This leads to a situation of unexpected page count,
+	 * causing migration failure, with -EGAIN error. This then leads to
+	 * multiple attempts by the kernel to migrate the same set of pages.
+	 * And sometimes the repeated attempts proves detrimental for stability.
+	 * Also since we don't know who is the other owner, and for how long its
+	 * gonna keep the reference, its better to return -EBUSY.
+	 */
+	if (page_count(page) > 2)
+		return -EBUSY;
+
+migrate:
+	ret = migrate_page(mapping, newpage, page, mode);
+	if (ret)
+		DRM_DEBUG_DRIVER("page=%p migration returned %d\n", page, ret);
+
+	return ret;
+}
+#endif
+
 /**
  * i915_gem_shrinker_init - Initialize i915 shrinker
  * @dev_priv: i915 device
@@ -371,6 +515,11 @@ void i915_gem_shrinker_init(struct drm_i915_private *dev_priv)
 
 	dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
 	WARN_ON(register_oom_notifier(&dev_priv->mm.oom_notifier));
+
+#ifdef CONFIG_MIGRATION
+	dev_priv->migrate_info.dev_private_data = dev_priv;
+	dev_priv->migrate_info.dev_migratepage = i915_gem_shrink_migratepage;
+#endif
 }
 
 /**
-- 
1.9.2

--
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>

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v2 2/2] drm/i915: Make pages of GFX allocations movable
@ 2016-03-24 18:22           ` akash.goel
  0 siblings, 0 replies; 30+ messages in thread
From: akash.goel @ 2016-03-24 18:22 UTC (permalink / raw
  To: intel-gfx; +Cc: linux-mm, Sourab Gupta, Hugh Dickins, Akash Goel

From: Chris Wilson <chris@chris-wilson.co.uk>

On a long run of more than 2-3 days, physical memory tends to get fragmented
severely, which considerably slows down the system. In such a scenario,
Shrinker is also unable to help as lack of memory is not the actual problem,
since it has been observed that there are enough free pages of 0 order.

To address the issue of external fragementation, kernel does a compaction
(which involves migration of pages) but it's efficacy depends upon how many
pages are marked as MOVABLE, as only those pages can be migrated.

Currently the backing pages for GFX buffers are allocated from shmemfs
with GFP_RECLAIMABLE flag, in units of 4KB pages.
In the case of limited Swap space, it may not be possible always to reclaim
or swap-out pages of all the inactive objects, to make way for free space
allowing formation of higher order groups of physically-contiguous pages
on compaction.

Just marking the GFX pages as MOVABLE will not suffice, as i915 Driver
has to pin the pages if they are in use by GPU, which will prevent their
migration. So the migratepage callback in shmem is also hooked up to get a
notification when kernel initiates the page migration. On the notification,
i915 Driver appropriately unpin the pages.
With this Driver can effectively mark the GFX pages as MOVABLE and hence
mitigate the fragmentation problem.

v2:
 - Rename the migration routine to gem_shrink_migratepage, move it to the
   shrinker file, and use the existing constructs (Chris)
 - To cleanup, add a new helper function to encapsulate all page migration
   skip conditions (Chris)
 - Add a new local helper function in shrinker file, for dropping the
   backing pages, and call the same from gem_shrink() also (Chris)

Cc: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |   3 +
 drivers/gpu/drm/i915/i915_gem.c          |  16 ++-
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 173 ++++++++++++++++++++++++++++---
 3 files changed, 177 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f330a53..28e50c7 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -52,6 +52,7 @@
 #include <linux/intel-iommu.h>
 #include <linux/kref.h>
 #include <linux/pm_qos.h>
+#include <linux/shmem_fs.h>
 #include "intel_guc.h"
 #include "intel_dpll_mgr.h"
 
@@ -1966,6 +1967,8 @@ struct drm_i915_private {
 
 	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
 
+	struct shmem_dev_info  migrate_info;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 8588c83..59ab58a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2204,6 +2204,7 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 		if (obj->madv == I915_MADV_WILLNEED)
 			mark_page_accessed(page);
 
+		set_page_private(page, 0);
 		page_cache_release(page);
 	}
 	obj->dirty = 0;
@@ -2318,6 +2319,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			sg->length += PAGE_SIZE;
 		}
 		last_pfn = page_to_pfn(page);
+		set_page_private(page, (unsigned long)obj);
 
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
@@ -2343,8 +2345,11 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 
 err_pages:
 	sg_mark_end(sg);
-	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
-		page_cache_release(sg_page_iter_page(&sg_iter));
+	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
+		page = sg_page_iter_page(&sg_iter);
+		set_page_private(page, 0);
+		page_cache_release(page);
+	}
 	sg_free_table(st);
 	kfree(st);
 
@@ -4468,6 +4473,7 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	struct address_space *mapping;
 	gfp_t mask;
@@ -4481,7 +4487,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 		return NULL;
 	}
 
-	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
+	mask = GFP_HIGHUSER_MOVABLE;
 	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
 		/* 965gm cannot relocate objects above 4GiB. */
 		mask &= ~__GFP_HIGHMEM;
@@ -4491,6 +4497,10 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	mapping = file_inode(obj->base.filp)->i_mapping;
 	mapping_set_gfp_mask(mapping, mask);
 
+#ifdef CONFIG_MIGRATION
+	shmem_set_device_ops(mapping, &dev_priv->migrate_info);
+#endif
+
 	i915_gem_object_init(obj, &i915_gem_object_ops);
 
 	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d3c473f..f6c2c20 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -24,6 +24,7 @@
 
 #include <linux/oom.h>
 #include <linux/shmem_fs.h>
+#include <linux/migrate.h>
 #include <linux/slab.h>
 #include <linux/swap.h>
 #include <linux/pci.h>
@@ -87,6 +88,71 @@ static bool can_release_pages(struct drm_i915_gem_object *obj)
 	return swap_available() || obj->madv == I915_MADV_DONTNEED;
 }
 
+static bool can_migrate_page(struct drm_i915_gem_object *obj)
+{
+	/* Avoid the migration of page if being actively used by GPU */
+	if (obj->active)
+		return false;
+
+	/* Skip the migration for purgeable objects otherwise there
+	 * will be a deadlock when shmem will try to lock the page for
+	 * truncation, which is already locked by the caller before
+	 * migration.
+	 */
+	if (obj->madv == I915_MADV_DONTNEED)
+		return false;
+
+	/* Skip the migration for a pinned object */
+	if (obj->pages_pin_count != num_vma_bound(obj))
+		return false;
+
+	return true;
+}
+
+static int
+unsafe_drop_pages(struct drm_i915_gem_object *obj)
+{
+	struct i915_vma *vma, *next;
+	int ret;
+
+	drm_gem_object_reference(&obj->base);
+	list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link)
+		if (i915_vma_unbind(vma))
+			break;
+
+	ret = i915_gem_object_put_pages(obj);
+	drm_gem_object_unreference(&obj->base);
+
+	return ret;
+}
+
+static int
+do_migrate_page(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+	int ret = 0;
+
+	if (!can_migrate_page(obj))
+		return -EBUSY;
+
+	/* HW access would be required for a bound object for which
+	 * device has to be kept runtime active. But a deadlock scenario
+	 * can arise if the attempt is made to resume the device, when
+	 * either a suspend or a resume operation is already happening
+	 * concurrently from some other path and that only actually
+	 * triggered the compaction. So only unbind if the device is
+	 * currently runtime active.
+	 */
+	if (!intel_runtime_pm_get_if_in_use(dev_priv))
+		return -EBUSY;
+
+	if (!unsafe_drop_pages(obj))
+		ret = -EBUSY;
+
+	intel_runtime_pm_put(dev_priv);
+	return ret;
+}
+
 /**
  * i915_gem_shrink - Shrink buffer object caches
  * @dev_priv: i915 device
@@ -156,7 +222,6 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 		INIT_LIST_HEAD(&still_in_list);
 		while (count < target && !list_empty(phase->list)) {
 			struct drm_i915_gem_object *obj;
-			struct i915_vma *vma, *v;
 
 			obj = list_first_entry(phase->list,
 					       typeof(*obj), global_list);
@@ -172,18 +237,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			if (!can_release_pages(obj))
 				continue;
 
-			drm_gem_object_reference(&obj->base);
-
-			/* For the unbound phase, this should be a no-op! */
-			list_for_each_entry_safe(vma, v,
-						 &obj->vma_list, obj_link)
-				if (i915_vma_unbind(vma))
-					break;
-
-			if (i915_gem_object_put_pages(obj) == 0)
+			if (unsafe_drop_pages(obj) == 0)
 				count += obj->base.size >> PAGE_SHIFT;
-
-			drm_gem_object_unreference(&obj->base);
 		}
 		list_splice(&still_in_list, phase->list);
 	}
@@ -356,6 +411,95 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
 	return NOTIFY_DONE;
 }
 
+#ifdef CONFIG_MIGRATION
+static int i915_gem_shrink_migratepage(struct address_space *mapping,
+			    struct page *newpage, struct page *page,
+			    enum migrate_mode mode, void *dev_priv_data)
+{
+	struct drm_i915_private *dev_priv = dev_priv_data;
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_i915_gem_object *obj;
+	unsigned long timeout = msecs_to_jiffies(10) + 1;
+	bool unlock;
+	int ret = 0;
+
+	WARN((page_count(newpage) != 1), "Unexpected ref count for newpage\n");
+
+	/*
+	 * Clear the private field of the new target page as it could have a
+	 * stale value in the private field. Otherwise later on if this page
+	 * itself gets migrated, without getting referred by the Driver
+	 * in between, the stale value would cause the i915_migratepage
+	 * function to go for a toss as object pointer is derived from it.
+	 * This should be safe since at the time of migration, private field
+	 * of the new page (which is actually an independent free 4KB page now)
+	 * should be like a don't care for the kernel.
+	 */
+	set_page_private(newpage, 0);
+
+	if (!page_private(page))
+		goto migrate;
+
+	/*
+	 * Check the page count, if Driver also has a reference then it should
+	 * be more than 2, as shmem will have one reference and one reference
+	 * would have been taken by the migration path itself. So if reference
+	 * is <=2, we can directly invoke the migration function.
+	 */
+	if (page_count(page) <= 2)
+		goto migrate;
+
+	/*
+	 * Use trylock here, with a timeout, for struct_mutex as
+	 * otherwise there is a possibility of deadlock due to lock
+	 * inversion. This path, which tries to migrate a particular
+	 * page after locking that page, can race with a path which
+	 * truncate/purge pages of the corresponding object (after
+	 * acquiring struct_mutex). Since page truncation will also
+	 * try to lock the page, a scenario of deadlock can arise.
+	 */
+	while (!i915_gem_shrinker_lock(dev, &unlock) && --timeout)
+		schedule_timeout_killable(1);
+	if (timeout == 0) {
+		DRM_DEBUG_DRIVER("Unable to acquire device mutex.\n");
+		return -EBUSY;
+	}
+
+	obj = (struct drm_i915_gem_object *)page_private(page);
+
+	if (!PageSwapCache(page) && obj) {
+		ret = do_migrate_page(obj);
+		BUG_ON(!ret && page_private(page));
+	}
+
+	if (unlock)
+		mutex_unlock(&dev->struct_mutex);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * Ideally here we don't expect the page count to be > 2, as driver
+	 * would have dropped its reference, but occasionally it has been seen
+	 * coming as 3 & 4. This leads to a situation of unexpected page count,
+	 * causing migration failure, with -EGAIN error. This then leads to
+	 * multiple attempts by the kernel to migrate the same set of pages.
+	 * And sometimes the repeated attempts proves detrimental for stability.
+	 * Also since we don't know who is the other owner, and for how long its
+	 * gonna keep the reference, its better to return -EBUSY.
+	 */
+	if (page_count(page) > 2)
+		return -EBUSY;
+
+migrate:
+	ret = migrate_page(mapping, newpage, page, mode);
+	if (ret)
+		DRM_DEBUG_DRIVER("page=%p migration returned %d\n", page, ret);
+
+	return ret;
+}
+#endif
+
 /**
  * i915_gem_shrinker_init - Initialize i915 shrinker
  * @dev_priv: i915 device
@@ -371,6 +515,11 @@ void i915_gem_shrinker_init(struct drm_i915_private *dev_priv)
 
 	dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
 	WARN_ON(register_oom_notifier(&dev_priv->mm.oom_notifier));
+
+#ifdef CONFIG_MIGRATION
+	dev_priv->migrate_info.dev_private_data = dev_priv;
+	dev_priv->migrate_info.dev_migratepage = i915_gem_shrink_migratepage;
+#endif
 }
 
 /**
-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH v2 2/2] drm/i915: Make pages of GFX allocations movable
  2016-03-24 18:22           ` akash.goel
  (?)
@ 2016-03-24 18:40           ` Chris Wilson
  -1 siblings, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2016-03-24 18:40 UTC (permalink / raw
  To: akash.goel; +Cc: intel-gfx, Hugh Dickins, linux-mm, Sourab Gupta

On Thu, Mar 24, 2016 at 11:52:59PM +0530, akash.goel@intel.com wrote:
> +static int
> +unsafe_drop_pages(struct drm_i915_gem_object *obj)
> +{
> +	struct i915_vma *vma, *next;
> +	int ret;
> +
> +	drm_gem_object_reference(&obj->base);
> +	list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link)
> +		if (i915_vma_unbind(vma))
> +			break;
> +
> +	ret = i915_gem_object_put_pages(obj);
> +	drm_gem_object_unreference(&obj->base);
> +
> +	return ret;
> +}
> +
> +static int
> +do_migrate_page(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
> +	int ret = 0;
> +
> +	if (!can_migrate_page(obj))
> +		return -EBUSY;
> +
> +	/* HW access would be required for a bound object for which
> +	 * device has to be kept runtime active. But a deadlock scenario
> +	 * can arise if the attempt is made to resume the device, when
> +	 * either a suspend or a resume operation is already happening
> +	 * concurrently from some other path and that only actually
> +	 * triggered the compaction. So only unbind if the device is
> +	 * currently runtime active.
> +	 */
> +	if (!intel_runtime_pm_get_if_in_use(dev_priv))
> +		return -EBUSY;
> +
> +	if (!unsafe_drop_pages(obj))
> +		ret = -EBUSY;

Reversed!

> +
> +	intel_runtime_pm_put(dev_priv);
> +	return ret;
> +}
> +
>  /**
>   * i915_gem_shrink - Shrink buffer object caches
>   * @dev_priv: i915 device
> @@ -156,7 +222,6 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  		INIT_LIST_HEAD(&still_in_list);
>  		while (count < target && !list_empty(phase->list)) {
>  			struct drm_i915_gem_object *obj;
> -			struct i915_vma *vma, *v;
>  
>  			obj = list_first_entry(phase->list,
>  					       typeof(*obj), global_list);
> @@ -172,18 +237,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
>  			if (!can_release_pages(obj))
>  				continue;
>  
> -			drm_gem_object_reference(&obj->base);
> -
> -			/* For the unbound phase, this should be a no-op! */
> -			list_for_each_entry_safe(vma, v,
> -						 &obj->vma_list, obj_link)
> -				if (i915_vma_unbind(vma))
> -					break;
> -
> -			if (i915_gem_object_put_pages(obj) == 0)
> +			if (unsafe_drop_pages(obj) == 0)
>  				count += obj->base.size >> PAGE_SHIFT;

But correct here :)
-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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* ✗ Fi.CI.BAT: warning for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops (rev2)
  2016-03-23  6:09 [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops akash.goel
                   ` (2 preceding siblings ...)
  2016-03-24 12:11   ` Joonas Lahtinen
@ 2016-03-25  7:31 ` Patchwork
  2016-04-04 13:14 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops (rev3) Patchwork
  2016-11-04 13:31 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of driver/file owner specific ops (rev4) Patchwork
  5 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2016-03-25  7:31 UTC (permalink / raw
  To: Akash Goel; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops (rev2)
URL   : https://patchwork.freedesktop.org/series/4780/
State : warning

== Summary ==

Series 4780v2 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/4780/revisions/2/mbox/

Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (bsw-nuc-2)

bdw-nuci7        total:192  pass:179  dwarn:0   dfail:0   fail:1   skip:12 
bdw-ultra        total:192  pass:170  dwarn:0   dfail:0   fail:1   skip:21 
bsw-nuc-2        total:192  pass:154  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:192  pass:156  dwarn:1   dfail:0   fail:0   skip:35 
hsw-brixbox      total:192  pass:170  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:192  pass:175  dwarn:0   dfail:0   fail:0   skip:17 
ivb-t430s        total:192  pass:167  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:192  pass:169  dwarn:0   dfail:0   fail:0   skip:23 
snb-dellxps      total:192  pass:158  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:192  pass:158  dwarn:0   dfail:0   fail:1   skip:33 

Results at /archive/results/CI_IGT_test/Patchwork_1715/

f5d413cccefa1f93d64c34f357151d42add63a84 drm-intel-nightly: 2016y-03m-24d-14h-34m-29s UTC integration manifest
bbbf884d78807182d2756e2c7606e92f529c4d71 drm/i915: Make pages of GFX allocations movable
c01a1b101822ef3eb364395db02c1f0aeeddcb02 shmem: Support for registration of Driver/file owner specific ops

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH v3 2/2] drm/i915: Make pages of GFX allocations movable
  2016-03-24 18:22           ` akash.goel
@ 2016-04-04 11:27             ` akash.goel
  -1 siblings, 0 replies; 30+ messages in thread
From: akash.goel @ 2016-04-04 11:27 UTC (permalink / raw
  To: intel-gfx; +Cc: Chris Wilson, Hugh Dickins, linux-mm, Sourab Gupta, Akash Goel

From: Chris Wilson <chris@chris-wilson.co.uk>

On a long run of more than 2-3 days, physical memory tends to get fragmented
severely, which considerably slows down the system. In such a scenario,
Shrinker is also unable to help as lack of memory is not the actual problem,
since it has been observed that there are enough free pages of 0 order.

To address the issue of external fragementation, kernel does a compaction
(which involves migration of pages) but it's efficacy depends upon how many
pages are marked as MOVABLE, as only those pages can be migrated.

Currently the backing pages for GFX buffers are allocated from shmemfs
with GFP_RECLAIMABLE flag, in units of 4KB pages.
In the case of limited Swap space, it may not be possible always to reclaim
or swap-out pages of all the inactive objects, to make way for free space
allowing formation of higher order groups of physically-contiguous pages
on compaction.

Just marking the GFX pages as MOVABLE will not suffice, as i915 Driver
has to pin the pages if they are in use by GPU, which will prevent their
migration. So the migratepage callback in shmem is also hooked up to get a
notification when kernel initiates the page migration. On the notification,
i915 Driver appropriately unpin the pages.
With this Driver can effectively mark the GFX pages as MOVABLE and hence
mitigate the fragmentation problem.

v2:
 - Rename the migration routine to gem_shrink_migratepage, move it to the
   shrinker file, and use the existing constructs (Chris)
 - To cleanup, add a new helper function to encapsulate all page migration
   skip conditions (Chris)
 - Add a new local helper function in shrinker file, for dropping the
   backing pages, and call the same from gem_shrink() also (Chris)

v3:
 - Fix/invert the check on the return value of unsafe_drop_pages (Chris)

Cc: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |   3 +
 drivers/gpu/drm/i915/i915_gem.c          |  16 ++-
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 173 ++++++++++++++++++++++++++++---
 3 files changed, 177 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dd18772..83415f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -52,6 +52,7 @@
 #include <linux/intel-iommu.h>
 #include <linux/kref.h>
 #include <linux/pm_qos.h>
+#include <linux/shmem_fs.h>
 #include "intel_guc.h"
 #include "intel_dpll_mgr.h"
 
@@ -1979,6 +1980,8 @@ struct drm_i915_private {
 
 	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
 
+	struct shmem_dev_info  migrate_info;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ca96fc1..88b717c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2206,6 +2206,7 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 		if (obj->madv == I915_MADV_WILLNEED)
 			mark_page_accessed(page);
 
+		set_page_private(page, 0);
 		page_cache_release(page);
 	}
 	obj->dirty = 0;
@@ -2320,6 +2321,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			sg->length += PAGE_SIZE;
 		}
 		last_pfn = page_to_pfn(page);
+		set_page_private(page, (unsigned long)obj);
 
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
@@ -2345,8 +2347,11 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 
 err_pages:
 	sg_mark_end(sg);
-	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
-		page_cache_release(sg_page_iter_page(&sg_iter));
+	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
+		page = sg_page_iter_page(&sg_iter);
+		set_page_private(page, 0);
+		page_cache_release(page);
+	}
 	sg_free_table(st);
 	kfree(st);
 
@@ -4468,6 +4473,7 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	struct address_space *mapping;
 	gfp_t mask;
@@ -4481,7 +4487,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 		return NULL;
 	}
 
-	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
+	mask = GFP_HIGHUSER_MOVABLE;
 	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
 		/* 965gm cannot relocate objects above 4GiB. */
 		mask &= ~__GFP_HIGHMEM;
@@ -4491,6 +4497,10 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	mapping = file_inode(obj->base.filp)->i_mapping;
 	mapping_set_gfp_mask(mapping, mask);
 
+#ifdef CONFIG_MIGRATION
+	shmem_set_device_ops(mapping, &dev_priv->migrate_info);
+#endif
+
 	i915_gem_object_init(obj, &i915_gem_object_ops);
 
 	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d3c473f..220481e 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -24,6 +24,7 @@
 
 #include <linux/oom.h>
 #include <linux/shmem_fs.h>
+#include <linux/migrate.h>
 #include <linux/slab.h>
 #include <linux/swap.h>
 #include <linux/pci.h>
@@ -87,6 +88,71 @@ static bool can_release_pages(struct drm_i915_gem_object *obj)
 	return swap_available() || obj->madv == I915_MADV_DONTNEED;
 }
 
+static bool can_migrate_page(struct drm_i915_gem_object *obj)
+{
+	/* Avoid the migration of page if being actively used by GPU */
+	if (obj->active)
+		return false;
+
+	/* Skip the migration for purgeable objects otherwise there
+	 * will be a deadlock when shmem will try to lock the page for
+	 * truncation, which is already locked by the caller before
+	 * migration.
+	 */
+	if (obj->madv == I915_MADV_DONTNEED)
+		return false;
+
+	/* Skip the migration for a pinned object */
+	if (obj->pages_pin_count != num_vma_bound(obj))
+		return false;
+
+	return true;
+}
+
+static int
+unsafe_drop_pages(struct drm_i915_gem_object *obj)
+{
+	struct i915_vma *vma, *next;
+	int ret;
+
+	drm_gem_object_reference(&obj->base);
+	list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link)
+		if (i915_vma_unbind(vma))
+			break;
+
+	ret = i915_gem_object_put_pages(obj);
+	drm_gem_object_unreference(&obj->base);
+
+	return ret;
+}
+
+static int
+do_migrate_page(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+	int ret = 0;
+
+	if (!can_migrate_page(obj))
+		return -EBUSY;
+
+	/* HW access would be required for a bound object for which
+	 * device has to be kept runtime active. But a deadlock scenario
+	 * can arise if the attempt is made to resume the device, when
+	 * either a suspend or a resume operation is already happening
+	 * concurrently from some other path and that only actually
+	 * triggered the compaction. So only unbind if the device is
+	 * currently runtime active.
+	 */
+	if (!intel_runtime_pm_get_if_in_use(dev_priv))
+		return -EBUSY;
+
+	if (unsafe_drop_pages(obj))
+		ret = -EBUSY;
+
+	intel_runtime_pm_put(dev_priv);
+	return ret;
+}
+
 /**
  * i915_gem_shrink - Shrink buffer object caches
  * @dev_priv: i915 device
@@ -156,7 +222,6 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 		INIT_LIST_HEAD(&still_in_list);
 		while (count < target && !list_empty(phase->list)) {
 			struct drm_i915_gem_object *obj;
-			struct i915_vma *vma, *v;
 
 			obj = list_first_entry(phase->list,
 					       typeof(*obj), global_list);
@@ -172,18 +237,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			if (!can_release_pages(obj))
 				continue;
 
-			drm_gem_object_reference(&obj->base);
-
-			/* For the unbound phase, this should be a no-op! */
-			list_for_each_entry_safe(vma, v,
-						 &obj->vma_list, obj_link)
-				if (i915_vma_unbind(vma))
-					break;
-
-			if (i915_gem_object_put_pages(obj) == 0)
+			if (unsafe_drop_pages(obj) == 0)
 				count += obj->base.size >> PAGE_SHIFT;
-
-			drm_gem_object_unreference(&obj->base);
 		}
 		list_splice(&still_in_list, phase->list);
 	}
@@ -356,6 +411,95 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
 	return NOTIFY_DONE;
 }
 
+#ifdef CONFIG_MIGRATION
+static int i915_gem_shrink_migratepage(struct address_space *mapping,
+			    struct page *newpage, struct page *page,
+			    enum migrate_mode mode, void *dev_priv_data)
+{
+	struct drm_i915_private *dev_priv = dev_priv_data;
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_i915_gem_object *obj;
+	unsigned long timeout = msecs_to_jiffies(10) + 1;
+	bool unlock;
+	int ret = 0;
+
+	WARN((page_count(newpage) != 1), "Unexpected ref count for newpage\n");
+
+	/*
+	 * Clear the private field of the new target page as it could have a
+	 * stale value in the private field. Otherwise later on if this page
+	 * itself gets migrated, without getting referred by the Driver
+	 * in between, the stale value would cause the i915_migratepage
+	 * function to go for a toss as object pointer is derived from it.
+	 * This should be safe since at the time of migration, private field
+	 * of the new page (which is actually an independent free 4KB page now)
+	 * should be like a don't care for the kernel.
+	 */
+	set_page_private(newpage, 0);
+
+	if (!page_private(page))
+		goto migrate;
+
+	/*
+	 * Check the page count, if Driver also has a reference then it should
+	 * be more than 2, as shmem will have one reference and one reference
+	 * would have been taken by the migration path itself. So if reference
+	 * is <=2, we can directly invoke the migration function.
+	 */
+	if (page_count(page) <= 2)
+		goto migrate;
+
+	/*
+	 * Use trylock here, with a timeout, for struct_mutex as
+	 * otherwise there is a possibility of deadlock due to lock
+	 * inversion. This path, which tries to migrate a particular
+	 * page after locking that page, can race with a path which
+	 * truncate/purge pages of the corresponding object (after
+	 * acquiring struct_mutex). Since page truncation will also
+	 * try to lock the page, a scenario of deadlock can arise.
+	 */
+	while (!i915_gem_shrinker_lock(dev, &unlock) && --timeout)
+		schedule_timeout_killable(1);
+	if (timeout == 0) {
+		DRM_DEBUG_DRIVER("Unable to acquire device mutex.\n");
+		return -EBUSY;
+	}
+
+	obj = (struct drm_i915_gem_object *)page_private(page);
+
+	if (!PageSwapCache(page) && obj) {
+		ret = do_migrate_page(obj);
+		BUG_ON(!ret && page_private(page));
+	}
+
+	if (unlock)
+		mutex_unlock(&dev->struct_mutex);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * Ideally here we don't expect the page count to be > 2, as driver
+	 * would have dropped its reference, but occasionally it has been seen
+	 * coming as 3 & 4. This leads to a situation of unexpected page count,
+	 * causing migration failure, with -EGAIN error. This then leads to
+	 * multiple attempts by the kernel to migrate the same set of pages.
+	 * And sometimes the repeated attempts proves detrimental for stability.
+	 * Also since we don't know who is the other owner, and for how long its
+	 * gonna keep the reference, its better to return -EBUSY.
+	 */
+	if (page_count(page) > 2)
+		return -EBUSY;
+
+migrate:
+	ret = migrate_page(mapping, newpage, page, mode);
+	if (ret)
+		DRM_DEBUG_DRIVER("page=%p migration returned %d\n", page, ret);
+
+	return ret;
+}
+#endif
+
 /**
  * i915_gem_shrinker_init - Initialize i915 shrinker
  * @dev_priv: i915 device
@@ -371,6 +515,11 @@ void i915_gem_shrinker_init(struct drm_i915_private *dev_priv)
 
 	dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
 	WARN_ON(register_oom_notifier(&dev_priv->mm.oom_notifier));
+
+#ifdef CONFIG_MIGRATION
+	dev_priv->migrate_info.dev_private_data = dev_priv;
+	dev_priv->migrate_info.dev_migratepage = i915_gem_shrink_migratepage;
+#endif
 }
 
 /**
-- 
1.9.2

--
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>

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH v3 2/2] drm/i915: Make pages of GFX allocations movable
@ 2016-04-04 11:27             ` akash.goel
  0 siblings, 0 replies; 30+ messages in thread
From: akash.goel @ 2016-04-04 11:27 UTC (permalink / raw
  To: intel-gfx; +Cc: linux-mm, Sourab Gupta, Hugh Dickins, Akash Goel

From: Chris Wilson <chris@chris-wilson.co.uk>

On a long run of more than 2-3 days, physical memory tends to get fragmented
severely, which considerably slows down the system. In such a scenario,
Shrinker is also unable to help as lack of memory is not the actual problem,
since it has been observed that there are enough free pages of 0 order.

To address the issue of external fragementation, kernel does a compaction
(which involves migration of pages) but it's efficacy depends upon how many
pages are marked as MOVABLE, as only those pages can be migrated.

Currently the backing pages for GFX buffers are allocated from shmemfs
with GFP_RECLAIMABLE flag, in units of 4KB pages.
In the case of limited Swap space, it may not be possible always to reclaim
or swap-out pages of all the inactive objects, to make way for free space
allowing formation of higher order groups of physically-contiguous pages
on compaction.

Just marking the GFX pages as MOVABLE will not suffice, as i915 Driver
has to pin the pages if they are in use by GPU, which will prevent their
migration. So the migratepage callback in shmem is also hooked up to get a
notification when kernel initiates the page migration. On the notification,
i915 Driver appropriately unpin the pages.
With this Driver can effectively mark the GFX pages as MOVABLE and hence
mitigate the fragmentation problem.

v2:
 - Rename the migration routine to gem_shrink_migratepage, move it to the
   shrinker file, and use the existing constructs (Chris)
 - To cleanup, add a new helper function to encapsulate all page migration
   skip conditions (Chris)
 - Add a new local helper function in shrinker file, for dropping the
   backing pages, and call the same from gem_shrink() also (Chris)

v3:
 - Fix/invert the check on the return value of unsafe_drop_pages (Chris)

Cc: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h          |   3 +
 drivers/gpu/drm/i915/i915_gem.c          |  16 ++-
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 173 ++++++++++++++++++++++++++++---
 3 files changed, 177 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index dd18772..83415f0 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -52,6 +52,7 @@
 #include <linux/intel-iommu.h>
 #include <linux/kref.h>
 #include <linux/pm_qos.h>
+#include <linux/shmem_fs.h>
 #include "intel_guc.h"
 #include "intel_dpll_mgr.h"
 
@@ -1979,6 +1980,8 @@ struct drm_i915_private {
 
 	struct intel_encoder *dig_port_map[I915_MAX_PORTS];
 
+	struct shmem_dev_info  migrate_info;
+
 	/*
 	 * NOTE: This is the dri1/ums dungeon, don't add stuff here. Your patch
 	 * will be rejected. Instead look for a better place.
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ca96fc1..88b717c 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2206,6 +2206,7 @@ i915_gem_object_put_pages_gtt(struct drm_i915_gem_object *obj)
 		if (obj->madv == I915_MADV_WILLNEED)
 			mark_page_accessed(page);
 
+		set_page_private(page, 0);
 		page_cache_release(page);
 	}
 	obj->dirty = 0;
@@ -2320,6 +2321,7 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 			sg->length += PAGE_SIZE;
 		}
 		last_pfn = page_to_pfn(page);
+		set_page_private(page, (unsigned long)obj);
 
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
@@ -2345,8 +2347,11 @@ i915_gem_object_get_pages_gtt(struct drm_i915_gem_object *obj)
 
 err_pages:
 	sg_mark_end(sg);
-	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0)
-		page_cache_release(sg_page_iter_page(&sg_iter));
+	for_each_sg_page(st->sgl, &sg_iter, st->nents, 0) {
+		page = sg_page_iter_page(&sg_iter);
+		set_page_private(page, 0);
+		page_cache_release(page);
+	}
 	sg_free_table(st);
 	kfree(st);
 
@@ -4468,6 +4473,7 @@ static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 						  size_t size)
 {
+	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_i915_gem_object *obj;
 	struct address_space *mapping;
 	gfp_t mask;
@@ -4481,7 +4487,7 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 		return NULL;
 	}
 
-	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
+	mask = GFP_HIGHUSER_MOVABLE;
 	if (IS_CRESTLINE(dev) || IS_BROADWATER(dev)) {
 		/* 965gm cannot relocate objects above 4GiB. */
 		mask &= ~__GFP_HIGHMEM;
@@ -4491,6 +4497,10 @@ struct drm_i915_gem_object *i915_gem_alloc_object(struct drm_device *dev,
 	mapping = file_inode(obj->base.filp)->i_mapping;
 	mapping_set_gfp_mask(mapping, mask);
 
+#ifdef CONFIG_MIGRATION
+	shmem_set_device_ops(mapping, &dev_priv->migrate_info);
+#endif
+
 	i915_gem_object_init(obj, &i915_gem_object_ops);
 
 	obj->base.write_domain = I915_GEM_DOMAIN_CPU;
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index d3c473f..220481e 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -24,6 +24,7 @@
 
 #include <linux/oom.h>
 #include <linux/shmem_fs.h>
+#include <linux/migrate.h>
 #include <linux/slab.h>
 #include <linux/swap.h>
 #include <linux/pci.h>
@@ -87,6 +88,71 @@ static bool can_release_pages(struct drm_i915_gem_object *obj)
 	return swap_available() || obj->madv == I915_MADV_DONTNEED;
 }
 
+static bool can_migrate_page(struct drm_i915_gem_object *obj)
+{
+	/* Avoid the migration of page if being actively used by GPU */
+	if (obj->active)
+		return false;
+
+	/* Skip the migration for purgeable objects otherwise there
+	 * will be a deadlock when shmem will try to lock the page for
+	 * truncation, which is already locked by the caller before
+	 * migration.
+	 */
+	if (obj->madv == I915_MADV_DONTNEED)
+		return false;
+
+	/* Skip the migration for a pinned object */
+	if (obj->pages_pin_count != num_vma_bound(obj))
+		return false;
+
+	return true;
+}
+
+static int
+unsafe_drop_pages(struct drm_i915_gem_object *obj)
+{
+	struct i915_vma *vma, *next;
+	int ret;
+
+	drm_gem_object_reference(&obj->base);
+	list_for_each_entry_safe(vma, next, &obj->vma_list, obj_link)
+		if (i915_vma_unbind(vma))
+			break;
+
+	ret = i915_gem_object_put_pages(obj);
+	drm_gem_object_unreference(&obj->base);
+
+	return ret;
+}
+
+static int
+do_migrate_page(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *dev_priv = obj->base.dev->dev_private;
+	int ret = 0;
+
+	if (!can_migrate_page(obj))
+		return -EBUSY;
+
+	/* HW access would be required for a bound object for which
+	 * device has to be kept runtime active. But a deadlock scenario
+	 * can arise if the attempt is made to resume the device, when
+	 * either a suspend or a resume operation is already happening
+	 * concurrently from some other path and that only actually
+	 * triggered the compaction. So only unbind if the device is
+	 * currently runtime active.
+	 */
+	if (!intel_runtime_pm_get_if_in_use(dev_priv))
+		return -EBUSY;
+
+	if (unsafe_drop_pages(obj))
+		ret = -EBUSY;
+
+	intel_runtime_pm_put(dev_priv);
+	return ret;
+}
+
 /**
  * i915_gem_shrink - Shrink buffer object caches
  * @dev_priv: i915 device
@@ -156,7 +222,6 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 		INIT_LIST_HEAD(&still_in_list);
 		while (count < target && !list_empty(phase->list)) {
 			struct drm_i915_gem_object *obj;
-			struct i915_vma *vma, *v;
 
 			obj = list_first_entry(phase->list,
 					       typeof(*obj), global_list);
@@ -172,18 +237,8 @@ i915_gem_shrink(struct drm_i915_private *dev_priv,
 			if (!can_release_pages(obj))
 				continue;
 
-			drm_gem_object_reference(&obj->base);
-
-			/* For the unbound phase, this should be a no-op! */
-			list_for_each_entry_safe(vma, v,
-						 &obj->vma_list, obj_link)
-				if (i915_vma_unbind(vma))
-					break;
-
-			if (i915_gem_object_put_pages(obj) == 0)
+			if (unsafe_drop_pages(obj) == 0)
 				count += obj->base.size >> PAGE_SHIFT;
-
-			drm_gem_object_unreference(&obj->base);
 		}
 		list_splice(&still_in_list, phase->list);
 	}
@@ -356,6 +411,95 @@ i915_gem_shrinker_oom(struct notifier_block *nb, unsigned long event, void *ptr)
 	return NOTIFY_DONE;
 }
 
+#ifdef CONFIG_MIGRATION
+static int i915_gem_shrink_migratepage(struct address_space *mapping,
+			    struct page *newpage, struct page *page,
+			    enum migrate_mode mode, void *dev_priv_data)
+{
+	struct drm_i915_private *dev_priv = dev_priv_data;
+	struct drm_device *dev = dev_priv->dev;
+	struct drm_i915_gem_object *obj;
+	unsigned long timeout = msecs_to_jiffies(10) + 1;
+	bool unlock;
+	int ret = 0;
+
+	WARN((page_count(newpage) != 1), "Unexpected ref count for newpage\n");
+
+	/*
+	 * Clear the private field of the new target page as it could have a
+	 * stale value in the private field. Otherwise later on if this page
+	 * itself gets migrated, without getting referred by the Driver
+	 * in between, the stale value would cause the i915_migratepage
+	 * function to go for a toss as object pointer is derived from it.
+	 * This should be safe since at the time of migration, private field
+	 * of the new page (which is actually an independent free 4KB page now)
+	 * should be like a don't care for the kernel.
+	 */
+	set_page_private(newpage, 0);
+
+	if (!page_private(page))
+		goto migrate;
+
+	/*
+	 * Check the page count, if Driver also has a reference then it should
+	 * be more than 2, as shmem will have one reference and one reference
+	 * would have been taken by the migration path itself. So if reference
+	 * is <=2, we can directly invoke the migration function.
+	 */
+	if (page_count(page) <= 2)
+		goto migrate;
+
+	/*
+	 * Use trylock here, with a timeout, for struct_mutex as
+	 * otherwise there is a possibility of deadlock due to lock
+	 * inversion. This path, which tries to migrate a particular
+	 * page after locking that page, can race with a path which
+	 * truncate/purge pages of the corresponding object (after
+	 * acquiring struct_mutex). Since page truncation will also
+	 * try to lock the page, a scenario of deadlock can arise.
+	 */
+	while (!i915_gem_shrinker_lock(dev, &unlock) && --timeout)
+		schedule_timeout_killable(1);
+	if (timeout == 0) {
+		DRM_DEBUG_DRIVER("Unable to acquire device mutex.\n");
+		return -EBUSY;
+	}
+
+	obj = (struct drm_i915_gem_object *)page_private(page);
+
+	if (!PageSwapCache(page) && obj) {
+		ret = do_migrate_page(obj);
+		BUG_ON(!ret && page_private(page));
+	}
+
+	if (unlock)
+		mutex_unlock(&dev->struct_mutex);
+
+	if (ret)
+		return ret;
+
+	/*
+	 * Ideally here we don't expect the page count to be > 2, as driver
+	 * would have dropped its reference, but occasionally it has been seen
+	 * coming as 3 & 4. This leads to a situation of unexpected page count,
+	 * causing migration failure, with -EGAIN error. This then leads to
+	 * multiple attempts by the kernel to migrate the same set of pages.
+	 * And sometimes the repeated attempts proves detrimental for stability.
+	 * Also since we don't know who is the other owner, and for how long its
+	 * gonna keep the reference, its better to return -EBUSY.
+	 */
+	if (page_count(page) > 2)
+		return -EBUSY;
+
+migrate:
+	ret = migrate_page(mapping, newpage, page, mode);
+	if (ret)
+		DRM_DEBUG_DRIVER("page=%p migration returned %d\n", page, ret);
+
+	return ret;
+}
+#endif
+
 /**
  * i915_gem_shrinker_init - Initialize i915 shrinker
  * @dev_priv: i915 device
@@ -371,6 +515,11 @@ void i915_gem_shrinker_init(struct drm_i915_private *dev_priv)
 
 	dev_priv->mm.oom_notifier.notifier_call = i915_gem_shrinker_oom;
 	WARN_ON(register_oom_notifier(&dev_priv->mm.oom_notifier));
+
+#ifdef CONFIG_MIGRATION
+	dev_priv->migrate_info.dev_private_data = dev_priv;
+	dev_priv->migrate_info.dev_migratepage = i915_gem_shrink_migratepage;
+#endif
 }
 
 /**
-- 
1.9.2

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops (rev3)
  2016-03-23  6:09 [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops akash.goel
                   ` (3 preceding siblings ...)
  2016-03-25  7:31 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops (rev2) Patchwork
@ 2016-04-04 13:14 ` Patchwork
  2016-11-04 13:31 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of driver/file owner specific ops (rev4) Patchwork
  5 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2016-04-04 13:14 UTC (permalink / raw
  To: Akash Goel; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops (rev3)
URL   : https://patchwork.freedesktop.org/series/4780/
State : failure

== Summary ==

Series 4780v3 Series without cover letter
http://patchwork.freedesktop.org/api/1.0/series/4780/revisions/3/mbox/

Test gem_sync:
        Subgroup basic-all:
                pass       -> DMESG-FAIL (ilk-hp8440p)
Test kms_flip:
        Subgroup basic-flip-vs-dpms:
                pass       -> DMESG-WARN (ilk-hp8440p) UNSTABLE
        Subgroup basic-flip-vs-modeset:
                pass       -> DMESG-WARN (skl-i7k-2)
Test kms_pipe_crc_basic:
        Subgroup nonblocking-crc-pipe-a-frame-sequence:
                pass       -> FAIL       (ilk-hp8440p)
        Subgroup suspend-read-crc-pipe-a:
                incomplete -> PASS       (skl-nuci5)
Test pm_rpm:
        Subgroup basic-rte:
                pass       -> DMESG-WARN (bsw-nuc-2)

bdw-nuci7        total:196  pass:184  dwarn:0   dfail:0   fail:0   skip:12 
bdw-ultra        total:196  pass:175  dwarn:0   dfail:0   fail:0   skip:21 
bsw-nuc-2        total:196  pass:158  dwarn:1   dfail:0   fail:0   skip:37 
byt-nuc          total:196  pass:161  dwarn:0   dfail:0   fail:0   skip:35 
hsw-brixbox      total:196  pass:174  dwarn:0   dfail:0   fail:0   skip:22 
hsw-gt2          total:196  pass:179  dwarn:0   dfail:0   fail:0   skip:17 
ilk-hp8440p      total:196  pass:129  dwarn:1   dfail:1   fail:1   skip:64 
ivb-t430s        total:196  pass:171  dwarn:0   dfail:0   fail:0   skip:25 
skl-i7k-2        total:196  pass:172  dwarn:1   dfail:0   fail:0   skip:23 
skl-nuci5        total:196  pass:185  dwarn:0   dfail:0   fail:0   skip:11 
snb-dellxps      total:196  pass:162  dwarn:0   dfail:0   fail:0   skip:34 
snb-x220t        total:164  pass:139  dwarn:0   dfail:0   fail:0   skip:25 

Results at /archive/results/CI_IGT_test/Patchwork_1787/

3e353ec38c8fe68e9a243a9388389a8815115451 drm-intel-nightly: 2016y-04m-04d-11h-13m-54s UTC integration manifest
60d6976c7880f278c3421853a46d1a9110c786cf drm/i915: Make pages of GFX allocations movable
3c45c2f983dd2c991fc4f2a4aa54653a4f6b56df shmem: Support for registration of Driver/file owner specific ops

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops
  2016-03-24 12:11   ` Joonas Lahtinen
@ 2016-10-19 15:11     ` akash goel
  -1 siblings, 0 replies; 30+ messages in thread
From: akash goel @ 2016-10-19 15:11 UTC (permalink / raw
  To: Joonas Lahtinen, Chris Wilson
  Cc: intel-gfx, linux-mm, Hugh Dickins, Sourab Gupta, Goel, Akash

On Thu, Mar 24, 2016 at 5:41 PM, Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
> On ke, 2016-03-23 at 11:39 +0530, akash.goel@intel.com wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> This provides support for the Drivers or shmem file owners to register
>> a set of callbacks, which can be invoked from the address space operations
>> methods implemented by shmem.
>> This allow the file owners to hook into the shmem address space operations
>> to do some extra/custom operations in addition to the default ones.
>>
>> The private_data field of address_space struct is used to store the pointer
>> to driver specific ops.
>> Currently only one ops field is defined, which is migratepage, but can be
>> extended on need basis.
>>
>> The need for driver specific operations arises since some of the operations
>> (like migratepage) may not be handled completely within shmem, so as to be
>> effective, and would need some driver specific handling also.
>>
>> Specifically, i915.ko would like to participate in migratepage().
>> i915.ko uses shmemfs to provide swappable backing storage for its user
>> objects, but when those objects are in use by the GPU it must pin the entire
>> object until the GPU is idle. As a result, large chunks of memory can be
>> arbitrarily withdrawn from page migration, resulting in premature
>> out-of-memory due to fragmentation. However, if i915.ko can receive the
>> migratepage() request, it can then flush the object from the GPU, remove
>> its pin and thus enable the migration.
>>
>> Since Gfx allocations are one of the major consumer of system memory, its
>> imperative to have such a mechanism to effectively deal with fragmentation.
>> And therefore the need for such a provision for initiating driver specific
>> actions during address space operations.
>>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: linux-mm@kvack.org
>> Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>  include/linux/shmem_fs.h | 17 +++++++++++++++++
>>  mm/shmem.c               | 17 ++++++++++++++++-
>>  2 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>> index 4d4780c..6cfa76a 100644
>> --- a/include/linux/shmem_fs.h
>> +++ b/include/linux/shmem_fs.h
>> @@ -34,11 +34,28 @@ struct shmem_sb_info {
>>       struct mempolicy *mpol;     /* default memory policy for mappings */
>>  };
>>
>> +struct shmem_dev_info {
>> +     void *dev_private_data;
>> +     int (*dev_migratepage)(struct address_space *mapping,
>> +                            struct page *newpage, struct page *page,
>> +                            enum migrate_mode mode, void *dev_priv_data);
>
> One might want to have a separate shmem_dev_operations struct or
> similar.
>
Sorry for the very late turnaround.

Sorry couldn't get your point here. Are you suggesting to rename the
structure to shmem_dev_operations ?

>> +};
>> +
>>  static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
>>  {
>>       return container_of(inode, struct shmem_inode_info, vfs_inode);
>>  }
>>
>> +static inline int shmem_set_device_ops(struct address_space *mapping,
>> +                             struct shmem_dev_info *info)
>> +{
>> +     if (mapping->private_data != NULL)
>> +             return -EEXIST;
>> +
>
> I did a quick random peek and most set functions are just void and
> override existing data. I'd suggest the same.
>
>> +     mapping->private_data = info;
>
Fine will change the return type to void and remove the check.

> Also, doesn't this kinda steal the mapping->private_data, might that be
> unexpected for the user? I notice currently it's not being touched at
> all.
>
Sorry by User do you mean the shmem client who called shmem_file_setup() ?
It seems clients are not expected to touch mapping->private_data and
so shmemfs can safely use it.

Best regards
Akash

>> +     return 0;
>> +}
>> +
>>  /*
>>   * Functions in mm/shmem.c called directly from elsewhere:
>>   */
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 440e2a7..f8625c4 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -952,6 +952,21 @@ redirty:
>>       return 0;
>>  }
>>
>> +#ifdef CONFIG_MIGRATION
>> +static int shmem_migratepage(struct address_space *mapping,
>> +                          struct page *newpage, struct page *page,
>> +                          enum migrate_mode mode)
>> +{
>> +     struct shmem_dev_info *dev_info = mapping->private_data;
>> +
>> +     if (dev_info && dev_info->dev_migratepage)
>> +             return dev_info->dev_migratepage(mapping, newpage, page,
>> +                             mode, dev_info->dev_private_data);
>> +
>> +     return migrate_page(mapping, newpage, page, mode);
>> +}
>> +#endif
>> +
>>  #ifdef CONFIG_NUMA
>>  #ifdef CONFIG_TMPFS
>>  static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> @@ -3168,7 +3183,7 @@ static const struct address_space_operations shmem_aops = {
>>       .write_end      = shmem_write_end,
>>  #endif
>>  #ifdef CONFIG_MIGRATION
>> -     .migratepage    = migrate_page,
>> +     .migratepage    = shmem_migratepage,
>>  #endif
>>       .error_remove_page = generic_error_remove_page,
>>  };
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

--
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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops
@ 2016-10-19 15:11     ` akash goel
  0 siblings, 0 replies; 30+ messages in thread
From: akash goel @ 2016-10-19 15:11 UTC (permalink / raw
  To: Joonas Lahtinen, Chris Wilson
  Cc: Goel, Akash, linux-mm, intel-gfx, Hugh Dickins, Sourab Gupta

On Thu, Mar 24, 2016 at 5:41 PM, Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
> On ke, 2016-03-23 at 11:39 +0530, akash.goel@intel.com wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> This provides support for the Drivers or shmem file owners to register
>> a set of callbacks, which can be invoked from the address space operations
>> methods implemented by shmem.
>> This allow the file owners to hook into the shmem address space operations
>> to do some extra/custom operations in addition to the default ones.
>>
>> The private_data field of address_space struct is used to store the pointer
>> to driver specific ops.
>> Currently only one ops field is defined, which is migratepage, but can be
>> extended on need basis.
>>
>> The need for driver specific operations arises since some of the operations
>> (like migratepage) may not be handled completely within shmem, so as to be
>> effective, and would need some driver specific handling also.
>>
>> Specifically, i915.ko would like to participate in migratepage().
>> i915.ko uses shmemfs to provide swappable backing storage for its user
>> objects, but when those objects are in use by the GPU it must pin the entire
>> object until the GPU is idle. As a result, large chunks of memory can be
>> arbitrarily withdrawn from page migration, resulting in premature
>> out-of-memory due to fragmentation. However, if i915.ko can receive the
>> migratepage() request, it can then flush the object from the GPU, remove
>> its pin and thus enable the migration.
>>
>> Since Gfx allocations are one of the major consumer of system memory, its
>> imperative to have such a mechanism to effectively deal with fragmentation.
>> And therefore the need for such a provision for initiating driver specific
>> actions during address space operations.
>>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: linux-mm@kvack.org
>> Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> ---
>>  include/linux/shmem_fs.h | 17 +++++++++++++++++
>>  mm/shmem.c               | 17 ++++++++++++++++-
>>  2 files changed, 33 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>> index 4d4780c..6cfa76a 100644
>> --- a/include/linux/shmem_fs.h
>> +++ b/include/linux/shmem_fs.h
>> @@ -34,11 +34,28 @@ struct shmem_sb_info {
>>       struct mempolicy *mpol;     /* default memory policy for mappings */
>>  };
>>
>> +struct shmem_dev_info {
>> +     void *dev_private_data;
>> +     int (*dev_migratepage)(struct address_space *mapping,
>> +                            struct page *newpage, struct page *page,
>> +                            enum migrate_mode mode, void *dev_priv_data);
>
> One might want to have a separate shmem_dev_operations struct or
> similar.
>
Sorry for the very late turnaround.

Sorry couldn't get your point here. Are you suggesting to rename the
structure to shmem_dev_operations ?

>> +};
>> +
>>  static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
>>  {
>>       return container_of(inode, struct shmem_inode_info, vfs_inode);
>>  }
>>
>> +static inline int shmem_set_device_ops(struct address_space *mapping,
>> +                             struct shmem_dev_info *info)
>> +{
>> +     if (mapping->private_data != NULL)
>> +             return -EEXIST;
>> +
>
> I did a quick random peek and most set functions are just void and
> override existing data. I'd suggest the same.
>
>> +     mapping->private_data = info;
>
Fine will change the return type to void and remove the check.

> Also, doesn't this kinda steal the mapping->private_data, might that be
> unexpected for the user? I notice currently it's not being touched at
> all.
>
Sorry by User do you mean the shmem client who called shmem_file_setup() ?
It seems clients are not expected to touch mapping->private_data and
so shmemfs can safely use it.

Best regards
Akash

>> +     return 0;
>> +}
>> +
>>  /*
>>   * Functions in mm/shmem.c called directly from elsewhere:
>>   */
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 440e2a7..f8625c4 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -952,6 +952,21 @@ redirty:
>>       return 0;
>>  }
>>
>> +#ifdef CONFIG_MIGRATION
>> +static int shmem_migratepage(struct address_space *mapping,
>> +                          struct page *newpage, struct page *page,
>> +                          enum migrate_mode mode)
>> +{
>> +     struct shmem_dev_info *dev_info = mapping->private_data;
>> +
>> +     if (dev_info && dev_info->dev_migratepage)
>> +             return dev_info->dev_migratepage(mapping, newpage, page,
>> +                             mode, dev_info->dev_private_data);
>> +
>> +     return migrate_page(mapping, newpage, page, mode);
>> +}
>> +#endif
>> +
>>  #ifdef CONFIG_NUMA
>>  #ifdef CONFIG_TMPFS
>>  static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>> @@ -3168,7 +3183,7 @@ static const struct address_space_operations shmem_aops = {
>>       .write_end      = shmem_write_end,
>>  #endif
>>  #ifdef CONFIG_MIGRATION
>> -     .migratepage    = migrate_page,
>> +     .migratepage    = shmem_migratepage,
>>  #endif
>>       .error_remove_page = generic_error_remove_page,
>>  };
> --
> Joonas Lahtinen
> Open Source Technology Center
> Intel Corporation
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [Intel-gfx] [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops
  2016-10-19 15:11     ` akash goel
@ 2016-10-20 15:15       ` Joonas Lahtinen
  -1 siblings, 0 replies; 30+ messages in thread
From: Joonas Lahtinen @ 2016-10-20 15:15 UTC (permalink / raw
  To: akash goel, Chris Wilson
  Cc: intel-gfx, linux-mm, Hugh Dickins, Sourab Gupta, Goel, Akash

On ke, 2016-10-19 at 20:41 +0530, akash goel wrote:
> On Thu, Mar 24, 2016 at 5:41 PM, Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com> wrote:
> > On ke, 2016-03-23 at 11:39 +0530, akash.goel@intel.com wrote:
> > > @@ -34,11 +34,28 @@ struct shmem_sb_info {
> > > A A A A A A struct mempolicy *mpol;A A A A A /* default memory policy for mappings */
> > > A };
> > > 
> > > +struct shmem_dev_info {
> > > +A A A A A void *dev_private_data;
> > > +A A A A A int (*dev_migratepage)(struct address_space *mapping,
> > > +A A A A A A A A A A A A A A A A A A A A A A A A A A A A struct page *newpage, struct page *page,
> > > +A A A A A A A A A A A A A A A A A A A A A A A A A A A A enum migrate_mode mode, void *dev_priv_data);
> > 
> > One might want to have a separate shmem_dev_operations struct or
> > similar.
> > 
> Sorry for the very late turnaround.
> 
> Sorry couldn't get your point here. Are you suggesting to rename the
> structure to shmem_dev_operations ?

I'm pretty sure I was after putting migratepage function pointer in
shmem_dev_operations struct, but I think that can be done once there
are more functions.

s/dev_private_data/private_data/ and s/dev_priv_data/private_data/
might be in order, too. I should be obvious from context.

> > > +};
> > > +
> > > A static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
> > > A {
> > > A A A A A A return container_of(inode, struct shmem_inode_info, vfs_inode);
> > > A }
> > > 
> > > +static inline int shmem_set_device_ops(struct address_space *mapping,
> > > +A A A A A A A A A A A A A A A A A A A A A A A A A A A A A struct shmem_dev_info *info)
> > > +{

This name could be shmem_set_dev_info, if there will be separate _ops
struct in future.

> > > +A A A A A if (mapping->private_data != NULL)
> > > +A A A A A A A A A A A A A return -EEXIST;
> > > +
> > 
> > I did a quick random peek and most set functions are just void and
> > override existing data. I'd suggest the same.
> > 
> > > 
> > > +A A A A A mapping->private_data = info;
> > 
> Fine will change the return type to void and remove the check.
> 
> > 
> > Also, doesn't this kinda steal the mapping->private_data, might that be
> > unexpected for the user? I notice currently it's not being touched at
> > all.
> > 
> Sorry by User do you mean the shmem client who called shmem_file_setup() ?
> It seems clients are not expected to touch mapping->private_data and
> so shmemfs can safely use it.

If it's not used by others, should be fine. Not sure if WARN would be
in place, Chris?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

--
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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops
@ 2016-10-20 15:15       ` Joonas Lahtinen
  0 siblings, 0 replies; 30+ messages in thread
From: Joonas Lahtinen @ 2016-10-20 15:15 UTC (permalink / raw
  To: akash goel, Chris Wilson
  Cc: Goel, Akash, linux-mm, intel-gfx, Hugh Dickins, Sourab Gupta

On ke, 2016-10-19 at 20:41 +0530, akash goel wrote:
> On Thu, Mar 24, 2016 at 5:41 PM, Joonas Lahtinen
> > <joonas.lahtinen@linux.intel.com> wrote:
> > On ke, 2016-03-23 at 11:39 +0530, akash.goel@intel.com wrote:
> > > @@ -34,11 +34,28 @@ struct shmem_sb_info {
> > >       struct mempolicy *mpol;     /* default memory policy for mappings */
> > >  };
> > > 
> > > +struct shmem_dev_info {
> > > +     void *dev_private_data;
> > > +     int (*dev_migratepage)(struct address_space *mapping,
> > > +                            struct page *newpage, struct page *page,
> > > +                            enum migrate_mode mode, void *dev_priv_data);
> > 
> > One might want to have a separate shmem_dev_operations struct or
> > similar.
> > 
> Sorry for the very late turnaround.
> 
> Sorry couldn't get your point here. Are you suggesting to rename the
> structure to shmem_dev_operations ?

I'm pretty sure I was after putting migratepage function pointer in
shmem_dev_operations struct, but I think that can be done once there
are more functions.

s/dev_private_data/private_data/ and s/dev_priv_data/private_data/
might be in order, too. I should be obvious from context.

> > > +};
> > > +
> > >  static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
> > >  {
> > >       return container_of(inode, struct shmem_inode_info, vfs_inode);
> > >  }
> > > 
> > > +static inline int shmem_set_device_ops(struct address_space *mapping,
> > > +                             struct shmem_dev_info *info)
> > > +{

This name could be shmem_set_dev_info, if there will be separate _ops
struct in future.

> > > +     if (mapping->private_data != NULL)
> > > +             return -EEXIST;
> > > +
> > 
> > I did a quick random peek and most set functions are just void and
> > override existing data. I'd suggest the same.
> > 
> > > 
> > > +     mapping->private_data = info;
> > 
> Fine will change the return type to void and remove the check.
> 
> > 
> > Also, doesn't this kinda steal the mapping->private_data, might that be
> > unexpected for the user? I notice currently it's not being touched at
> > all.
> > 
> Sorry by User do you mean the shmem client who called shmem_file_setup() ?
> It seems clients are not expected to touch mapping->private_data and
> so shmemfs can safely use it.

If it's not used by others, should be fine. Not sure if WARN would be
in place, Chris?

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/2] shmem: Support for registration of driver/file owner specific ops
  2016-10-20 15:15       ` Joonas Lahtinen
  (?)
@ 2016-11-04 12:48       ` akash.goel
  2016-11-04 12:48         ` [PATCH 2/2] drm/i915: Make GPU pages movable akash.goel
  -1 siblings, 1 reply; 30+ messages in thread
From: akash.goel @ 2016-11-04 12:48 UTC (permalink / raw
  To: intel-gfx
  Cc: Chris Wilson, Hugh Dickins, linux-mm, linux-kernel, Sourab Gupta,
	Akash Goel

From: Chris Wilson <chris@chris-wilson.co.uk>

This provides support for the drivers or shmem file owners to register
a set of callbacks, which can be invoked from the address space
operations methods implemented by shmem.  This allow the file owners to
hook into the shmem address space operations to do some extra/custom
operations in addition to the default ones.

The private_data field of address_space struct is used to store the
pointer to driver specific ops.  Currently only one ops field is defined,
which is migratepage, but can be extended on an as-needed basis.

The need for driver specific operations arises since some of the
operations (like migratepage) may not be handled completely within shmem,
so as to be effective, and would need some driver specific handling also.
Specifically, i915.ko would like to participate in migratepage().
i915.ko uses shmemfs to provide swappable backing storage for its user
objects, but when those objects are in use by the GPU it must pin the
entire object until the GPU is idle.  As a result, large chunks of memory
can be arbitrarily withdrawn from page migration, resulting in premature
out-of-memory due to fragmentation.  However, if i915.ko can receive the
migratepage() request, it can then flush the object from the GPU, remove
its pin and thus enable the migration.

Since gfx allocations are one of the major consumer of system memory, its
imperative to have such a mechanism to effectively deal with
fragmentation.  And therefore the need for such a provision for initiating
driver specific actions during address space operations.

v2:
- Drop dev_ prefix from the members of shmem_dev_info structure. (Joonas)
- Change the return type of shmem_set_device_op() to void and remove the
  check for pre-existing data. (Joonas)
- Rename shmem_set_device_op() to shmem_set_dev_info() to be consistent
  with shmem_dev_info structure. (Joonas)

Cc: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.linux.org
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 include/linux/shmem_fs.h | 13 +++++++++++++
 mm/shmem.c               | 17 ++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index ff078e7..22796a0 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -39,11 +39,24 @@ struct shmem_sb_info {
 	unsigned long shrinklist_len; /* Length of shrinklist */
 };
 
+struct shmem_dev_info {
+	void *private_data;
+	int (*migratepage)(struct address_space *mapping,
+			   struct page *newpage, struct page *page,
+			   enum migrate_mode mode, void *dev_priv_data);
+};
+
 static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
 {
 	return container_of(inode, struct shmem_inode_info, vfs_inode);
 }
 
+static inline void shmem_set_dev_info(struct address_space *mapping,
+					 struct shmem_dev_info *info)
+{
+	mapping->private_data = info;
+}
+
 /*
  * Functions in mm/shmem.c called directly from elsewhere:
  */
diff --git a/mm/shmem.c b/mm/shmem.c
index ad7813d..bf71ddd 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1290,6 +1290,21 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 	return 0;
 }
 
+#ifdef CONFIG_MIGRATION
+static int shmem_migratepage(struct address_space *mapping,
+			     struct page *newpage, struct page *page,
+			     enum migrate_mode mode)
+{
+	struct shmem_dev_info *dev_info = mapping->private_data;
+
+	if (dev_info && dev_info->migratepage)
+		return dev_info->migratepage(mapping, newpage, page,
+				mode, dev_info->private_data);
+
+	return migrate_page(mapping, newpage, page, mode);
+}
+#endif
+
 #if defined(CONFIG_NUMA) && defined(CONFIG_TMPFS)
 static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
@@ -3654,7 +3669,7 @@ static void shmem_destroy_inodecache(void)
 	.write_end	= shmem_write_end,
 #endif
 #ifdef CONFIG_MIGRATION
-	.migratepage	= migrate_page,
+	.migratepage	= shmem_migratepage,
 #endif
 	.error_remove_page = generic_error_remove_page,
 };
-- 
1.9.2

--
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>

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* [PATCH 2/2] drm/i915: Make GPU pages movable
  2016-11-04 12:48       ` [PATCH 1/2] shmem: Support for registration of driver/file " akash.goel
@ 2016-11-04 12:48         ` akash.goel
  2016-11-04 13:37             ` Chris Wilson
  0 siblings, 1 reply; 30+ messages in thread
From: akash.goel @ 2016-11-04 12:48 UTC (permalink / raw
  To: intel-gfx; +Cc: Chris Wilson, Hugh Dickins, linux-mm, Sourab Gupta, Akash Goel

From: Chris Wilson <chris@chris-wilson.co.uk>

On a long run of more than 2-3 days, physical memory tends to get
fragmented severely, which considerably slows down the system. In such a
scenario, the shrinker is also unable to help as lack of memory is not
the actual problem, since it has been observed that there are enough free
pages of 0 order. This also manifests itself when an indiviual zone in
the mm runs out of pages and if we cannot migrate pages between zones,
the kernel hits an out-of-memory even though there are free pages (and
often all of swap) available.

To address the issue of external fragementation, kernel does a compaction
(which involves migration of pages) but it's efficacy depends upon how
many pages are marked as MOVABLE, as only those pages can be migrated.

Currently the backing pages for GPU buffers are allocated from shmemfs
with GFP_RECLAIMABLE flag, in units of 4KB pages.  In the case of limited
swap space, it may not be possible always to reclaim or swap-out pages of
all the inactive objects, to make way for free space allowing formation
of higher order groups of physically-contiguous pages on compaction.

Just marking the GPU pages as MOVABLE will not suffice, as i915.ko has to
pin the pages if they are in use by GPU, which will prevent their
migration. So the migratepage callback in shmem is also hooked up to get
a notification when kernel initiates the page migration. On the
notification, i915.ko appropriately unpin the pages.  With this we can
effectively mark the GPU pages as MOVABLE and hence mitigate the
fragmentation problem.

v2:
 - Rename the migration routine to gem_shrink_migratepage, move it to the
   shrinker file, and use the existing constructs (Chris)
 - To cleanup, add a new helper function to encapsulate all page migration
   skip conditions (Chris)
 - Add a new local helper function in shrinker file, for dropping the
   backing pages, and call the same from gem_shrink() also (Chris)

v3:
 - Fix/invert the check on the return value of unsafe_drop_pages (Chris)

v4:
 - Minor tidy

v5:
 - Fix unsafe usage of unsafe_drop_pages()
 - Rebase onto vmap-notifier

Testcase: igt/gem_shrink
Bugzilla: (e.g.) https://bugs.freedesktop.org/show_bug.cgi?id=90254
Cc: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h          |   2 +
 drivers/gpu/drm/i915/i915_gem.c          |   9 ++-
 drivers/gpu/drm/i915/i915_gem_shrinker.c | 134 +++++++++++++++++++++++++++++++
 3 files changed, 144 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 4735b417..7f2717b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1357,6 +1357,8 @@ struct intel_l3_parity {
 };
 
 struct i915_gem_mm {
+	struct shmem_dev_info shmem_info;
+
 	/** Memory allocator for GTT stolen memory */
 	struct drm_mm stolen;
 	/** Protects the usage of the GTT stolen memory allocator. This is
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 1f995ce..f0d4ce7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2164,6 +2164,7 @@ void __i915_gem_object_invalidate(struct drm_i915_gem_object *obj)
 		if (obj->mm.madv == I915_MADV_WILLNEED)
 			mark_page_accessed(page);
 
+		set_page_private(page, 0);
 		put_page(page);
 	}
 	obj->mm.dirty = false;
@@ -2310,6 +2311,7 @@ static unsigned int swiotlb_max_size(void)
 			sg->length += PAGE_SIZE;
 		}
 		last_pfn = page_to_pfn(page);
+		set_page_private(page, (unsigned long)obj);
 
 		/* Check that the i965g/gm workaround works. */
 		WARN_ON((gfp & __GFP_DMA32) && (last_pfn >= 0x00100000UL));
@@ -2334,8 +2336,10 @@ static unsigned int swiotlb_max_size(void)
 
 err_pages:
 	sg_mark_end(sg);
-	for_each_sgt_page(page, sgt_iter, st)
+	for_each_sgt_page(page, sgt_iter, st) {
+		set_page_private(page, 0);
 		put_page(page);
+	}
 	sg_free_table(st);
 	kfree(st);
 
@@ -4185,6 +4189,8 @@ struct drm_i915_gem_object *
 		goto fail;
 
 	mask = GFP_HIGHUSER | __GFP_RECLAIMABLE;
+	if (IS_ENABLED(MIGRATION))
+		mask |= __GFP_MOVABLE;
 	if (IS_CRESTLINE(dev_priv) || IS_BROADWATER(dev_priv)) {
 		/* 965gm cannot relocate objects above 4GiB. */
 		mask &= ~__GFP_HIGHMEM;
@@ -4193,6 +4199,7 @@ struct drm_i915_gem_object *
 
 	mapping = obj->base.filp->f_mapping;
 	mapping_set_gfp_mask(mapping, mask);
+	shmem_set_dev_info(mapping, &dev_priv->mm.shmem_info);
 
 	i915_gem_object_init(obj, &i915_gem_object_ops);
 
diff --git a/drivers/gpu/drm/i915/i915_gem_shrinker.c b/drivers/gpu/drm/i915/i915_gem_shrinker.c
index a6fc1bd..2862523 100644
--- a/drivers/gpu/drm/i915/i915_gem_shrinker.c
+++ b/drivers/gpu/drm/i915/i915_gem_shrinker.c
@@ -24,6 +24,7 @@
 
 #include <linux/oom.h>
 #include <linux/shmem_fs.h>
+#include <linux/migrate.h>
 #include <linux/slab.h>
 #include <linux/swap.h>
 #include <linux/pci.h>
@@ -473,6 +474,134 @@ struct shrinker_lock_uninterruptible {
 	return NOTIFY_DONE;
 }
 
+#ifdef CONFIG_MIGRATION
+static bool can_migrate_page(struct drm_i915_gem_object *obj)
+{
+	/* Avoid the migration of page if being actively used by GPU */
+	if (i915_gem_object_is_active(obj))
+		return false;
+
+	/* Skip the migration for purgeable objects otherwise there
+	 * will be a deadlock when shmem will try to lock the page for
+	 * truncation, which is already locked by the caller before
+	 * migration.
+	 */
+	if (obj->mm.madv == I915_MADV_DONTNEED)
+		return false;
+
+	/* Skip the migration for a pinned object */
+	if (atomic_read(&obj->mm.pages_pin_count) > obj->bind_count)
+		return false;
+
+	if (any_vma_pinned(obj))
+		return false;
+
+	return true;
+}
+
+static int do_migrate_page(struct drm_i915_gem_object *obj)
+{
+	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
+	int ret = 0;
+
+	if (!can_migrate_page(obj))
+		return -EBUSY;
+
+	/* HW access would be required for a GGTT bound object, for which
+	 * device has to be kept awake. But a deadlock scenario can arise if
+	 * the attempt is made to resume the device, when either a suspend
+	 * or a resume operation is already happening concurrently from some
+	 * other path and that only also triggers compaction. So only unbind
+	 * if the device is currently awake.
+	 */
+	if (!intel_runtime_pm_get_if_in_use(dev_priv))
+		return -EBUSY;
+
+	i915_gem_object_get(obj);
+	if (!unsafe_drop_pages(obj))
+		ret = -EBUSY;
+	i915_gem_object_put(obj);
+
+	intel_runtime_pm_put(dev_priv);
+	return ret;
+}
+
+static int i915_gem_shrinker_migratepage(struct address_space *mapping,
+					 struct page *newpage,
+					 struct page *page,
+					 enum migrate_mode mode,
+					 void *dev_priv_data)
+{
+	struct drm_i915_private *dev_priv = dev_priv_data;
+	struct shrinker_lock_uninterruptible slu;
+	int ret;
+
+	/*
+	 * Clear the private field of the new target page as it could have a
+	 * stale value in the private field. Otherwise later on if this page
+	 * itself gets migrated, without getting referred by the Driver
+	 * in between, the stale value would cause the i915_migratepage
+	 * function to go for a toss as object pointer is derived from it.
+	 * This should be safe since at the time of migration, private field
+	 * of the new page (which is actually an independent free 4KB page now)
+	 * should be like a don't care for the kernel.
+	 */
+	set_page_private(newpage, 0);
+
+	if (!page_private(page))
+		goto migrate;
+
+	/*
+	 * Check the page count, if Driver also has a reference then it should
+	 * be more than 2, as shmem will have one reference and one reference
+	 * would have been taken by the migration path itself. So if reference
+	 * is <=2, we can directly invoke the migration function.
+	 */
+	if (page_count(page) <= 2)
+		goto migrate;
+
+	/*
+	 * Use trylock here, with a timeout, for struct_mutex as
+	 * otherwise there is a possibility of deadlock due to lock
+	 * inversion. This path, which tries to migrate a particular
+	 * page after locking that page, can race with a path which
+	 * truncate/purge pages of the corresponding object (after
+	 * acquiring struct_mutex). Since page truncation will also
+	 * try to lock the page, a scenario of deadlock can arise.
+	 */
+	if (!i915_gem_shrinker_lock_uninterruptible(dev_priv, &slu, 10))
+		return -EBUSY;
+
+	ret = 0;
+	if (!PageSwapCache(page) && page_private(page)) {
+		struct drm_i915_gem_object *obj =
+			(struct drm_i915_gem_object *)page_private(page);
+
+		ret = do_migrate_page(obj);
+	}
+
+	i915_gem_shrinker_unlock_uninterruptible(dev_priv, &slu);
+	if (ret)
+		return ret;
+
+	/*
+	 * Ideally here we don't expect the page count to be > 2, as driver
+	 * would have dropped its reference, but occasionally it has been seen
+	 * coming as 3 & 4. This leads to a situation of unexpected page count,
+	 * causing migration failure, with -EGAIN error. This then leads to
+	 * multiple attempts by the kernel to migrate the same set of pages.
+	 * And sometimes the repeated attempts proves detrimental for stability.
+	 * Also since we don't know who is the other owner, and for how long its
+	 * gonna keep the reference, its better to return -EBUSY.
+	 */
+	if (page_count(page) > 2)
+		return -EBUSY;
+
+migrate:
+	return migrate_page(mapping, newpage, page, mode);
+}
+#endif
+
 /**
  * i915_gem_shrinker_init - Initialize i915 shrinker
  * @dev_priv: i915 device
@@ -491,6 +620,11 @@ void i915_gem_shrinker_init(struct drm_i915_private *dev_priv)
 
 	dev_priv->mm.vmap_notifier.notifier_call = i915_gem_shrinker_vmap;
 	WARN_ON(register_vmap_purge_notifier(&dev_priv->mm.vmap_notifier));
+
+	dev_priv->mm.shmem_info.private_data = dev_priv;
+#ifdef CONFIG_MIGRATION
+	dev_priv->mm.shmem_info.migratepage = i915_gem_shrinker_migratepage;
+#endif
 }
 
 /**
-- 
1.9.2

--
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>

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of driver/file owner specific ops (rev4)
  2016-03-23  6:09 [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops akash.goel
                   ` (4 preceding siblings ...)
  2016-04-04 13:14 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops (rev3) Patchwork
@ 2016-11-04 13:31 ` Patchwork
  5 siblings, 0 replies; 30+ messages in thread
From: Patchwork @ 2016-11-04 13:31 UTC (permalink / raw
  To: Akash Goel; +Cc: intel-gfx

== Series Details ==

Series: series starting with [1/2] shmem: Support for registration of driver/file owner specific ops (rev4)
URL   : https://patchwork.freedesktop.org/series/4780/
State : failure

== Summary ==

drivers/gpu/drm/i915/i915_drv.h: At top level:
drivers/gpu/drm/i915/i915_drv.h:58:1: error: expected identifier or ‘(’ before ‘==’ token
 =======
 ^
In file included from drivers/gpu/drm/i915/intel_guc.h:27:0,
                 from drivers/gpu/drm/i915/i915_drv.h:60,
                 from drivers/gpu/drm/i915/intel_csr.c:25:
drivers/gpu/drm/i915/intel_guc_fwif.h:222:1: warning: empty declaration
 } __packed;
 ^
In file included from drivers/gpu/drm/i915/intel_csr.c:25:0:
drivers/gpu/drm/i915/i915_drv.h:61:1: error: expected identifier or ‘(’ before ‘>>’ token
 >>>>>>> drm/i915: Make pages of GFX allocations movable
 ^
  LD      net/key/built-in.o
scripts/Makefile.build:290: recipe for target 'drivers/gpu/drm/i915/i915_sysfs.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_sysfs.o] Error 1
  LD      drivers/acpi/acpica/acpi.o
scripts/Makefile.build:290: recipe for target 'drivers/gpu/drm/i915/i915_suspend.o' failed
make[4]: *** [drivers/gpu/drm/i915/i915_suspend.o] Error 1
  LD      drivers/thermal/thermal_sys.o
  LD      drivers/thermal/built-in.o
  LD      drivers/iommu/built-in.o
  LD      net/netlink/built-in.o
  LD      drivers/video/console/built-in.o
  LD      drivers/pci/pcie/pcieportdrv.o
scripts/Makefile.build:290: recipe for target 'drivers/gpu/drm/i915/intel_csr.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_csr.o] Error 1
scripts/Makefile.build:475: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:475: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:475: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
make[1]: *** Waiting for unfinished jobs....
  LD      drivers/spi/built-in.o
  LD      kernel/sched/built-in.o
  LD      drivers/video/built-in.o
  LD      drivers/acpi/acpica/built-in.o
  LD      drivers/tty/serial/8250/8250.o
  LD      kernel/built-in.o
  LD      drivers/acpi/built-in.o
  LD [M]  drivers/net/ethernet/intel/igbvf/igbvf.o
  LD      lib/raid6/raid6_pq.o
  LD      lib/raid6/built-in.o
  LD [M]  drivers/mmc/core/mmc_core.o
  LD      drivers/mmc/built-in.o
  LD      drivers/pci/pcie/aer/aerdriver.o
  LD      drivers/usb/gadget/libcomposite.o
  LD      net/unix/unix.o
  LD      drivers/pci/pcie/aer/built-in.o
  LD      net/unix/built-in.o
  LD      drivers/pci/pcie/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000/e1000.o
  LD [M]  sound/pci/hda/snd-hda-codec-generic.o
  LD      sound/pci/built-in.o
  LD      net/packet/built-in.o
  LD      sound/built-in.o
  LD      drivers/usb/core/usbcore.o
  LD      drivers/scsi/sd_mod.o
  LD      drivers/scsi/built-in.o
  LD      drivers/tty/serial/8250/8250_base.o
  LD      drivers/tty/serial/8250/built-in.o
  LD      drivers/usb/core/built-in.o
  LD      drivers/pci/built-in.o
  LD      drivers/tty/serial/built-in.o
  LD      drivers/usb/gadget/udc/udc-core.o
  LD      drivers/usb/gadget/udc/built-in.o
  LD      drivers/usb/gadget/built-in.o
  LD      net/xfrm/built-in.o
  CC      arch/x86/kernel/cpu/capflags.o
  LD      arch/x86/kernel/cpu/built-in.o
  LD      arch/x86/kernel/built-in.o
  LD      arch/x86/built-in.o
  AR      lib/lib.a
  EXPORTS lib/lib-ksyms.o
  LD      drivers/md/md-mod.o
  LD      drivers/tty/vt/built-in.o
  LD [M]  drivers/net/ethernet/intel/igb/igb.o
  LD      lib/built-in.o
  LD      drivers/md/built-in.o
  LD      drivers/tty/built-in.o
  LD      drivers/usb/host/xhci-hcd.o
  LD      fs/btrfs/btrfs.o
  LD      fs/ext4/ext4.o
  LD      net/ipv6/ipv6.o
  LD      fs/ext4/built-in.o
  LD      fs/btrfs/built-in.o
  LD      net/ipv6/built-in.o
  LD      fs/built-in.o
  LD      drivers/usb/host/built-in.o
  LD      drivers/usb/built-in.o
  LD      net/core/built-in.o
  LD      net/ipv4/built-in.o
  LD      net/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000e/e1000e.o
  LD      drivers/net/ethernet/built-in.o
  LD      drivers/net/built-in.o
Makefile:983: recipe for target 'drivers' failed
make: *** [drivers] Error 2

Full logs at /archive/deploy/logs/Patchwork_2905

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] drm/i915: Make GPU pages movable
  2016-11-04 12:48         ` [PATCH 2/2] drm/i915: Make GPU pages movable akash.goel
@ 2016-11-04 13:37             ` Chris Wilson
  0 siblings, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2016-11-04 13:37 UTC (permalink / raw
  To: akash.goel; +Cc: intel-gfx, Hugh Dickins, linux-mm, Sourab Gupta

Best if we send these as a new series to unconfuse CI.

On Fri, Nov 04, 2016 at 06:18:26PM +0530, akash.goel@intel.com wrote:
> +static int do_migrate_page(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> +	int ret = 0;
> +
> +	if (!can_migrate_page(obj))
> +		return -EBUSY;
> +
> +	/* HW access would be required for a GGTT bound object, for which
> +	 * device has to be kept awake. But a deadlock scenario can arise if
> +	 * the attempt is made to resume the device, when either a suspend
> +	 * or a resume operation is already happening concurrently from some
> +	 * other path and that only also triggers compaction. So only unbind
> +	 * if the device is currently awake.
> +	 */
> +	if (!intel_runtime_pm_get_if_in_use(dev_priv))
> +		return -EBUSY;
> +
> +	i915_gem_object_get(obj);
> +	if (!unsafe_drop_pages(obj))
> +		ret = -EBUSY;
> +	i915_gem_object_put(obj);

Since the object release changes, we can now do this without the
i915_gem_object_get / i915_gem_object_put (as we are guarded by the BKL
struct_mutex).
-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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] drm/i915: Make GPU pages movable
@ 2016-11-04 13:37             ` Chris Wilson
  0 siblings, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2016-11-04 13:37 UTC (permalink / raw
  To: akash.goel; +Cc: linux-mm, intel-gfx, Hugh Dickins, Sourab Gupta

Best if we send these as a new series to unconfuse CI.

On Fri, Nov 04, 2016 at 06:18:26PM +0530, akash.goel@intel.com wrote:
> +static int do_migrate_page(struct drm_i915_gem_object *obj)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
> +	int ret = 0;
> +
> +	if (!can_migrate_page(obj))
> +		return -EBUSY;
> +
> +	/* HW access would be required for a GGTT bound object, for which
> +	 * device has to be kept awake. But a deadlock scenario can arise if
> +	 * the attempt is made to resume the device, when either a suspend
> +	 * or a resume operation is already happening concurrently from some
> +	 * other path and that only also triggers compaction. So only unbind
> +	 * if the device is currently awake.
> +	 */
> +	if (!intel_runtime_pm_get_if_in_use(dev_priv))
> +		return -EBUSY;
> +
> +	i915_gem_object_get(obj);
> +	if (!unsafe_drop_pages(obj))
> +		ret = -EBUSY;
> +	i915_gem_object_put(obj);

Since the object release changes, we can now do this without the
i915_gem_object_get / i915_gem_object_put (as we are guarded by the BKL
struct_mutex).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 2/2] drm/i915: Make GPU pages movable
  2016-11-04 13:37             ` Chris Wilson
  (?)
@ 2016-11-04 13:53             ` Goel, Akash
  -1 siblings, 0 replies; 30+ messages in thread
From: Goel, Akash @ 2016-11-04 13:53 UTC (permalink / raw
  To: Chris Wilson, intel-gfx, Hugh Dickins, linux-mm
  Cc: Sourab Gupta, akash.goels, akash.goel



On 11/4/2016 7:07 PM, Chris Wilson wrote:
> Best if we send these as a new series to unconfuse CI.
>
Okay will send as a new series.

> On Fri, Nov 04, 2016 at 06:18:26PM +0530, akash.goel@intel.com wrote:
>> +static int do_migrate_page(struct drm_i915_gem_object *obj)
>> +{
>> +	struct drm_i915_private *dev_priv = to_i915(obj->base.dev);
>> +	int ret = 0;
>> +
>> +	if (!can_migrate_page(obj))
>> +		return -EBUSY;
>> +
>> +	/* HW access would be required for a GGTT bound object, for which
>> +	 * device has to be kept awake. But a deadlock scenario can arise if
>> +	 * the attempt is made to resume the device, when either a suspend
>> +	 * or a resume operation is already happening concurrently from some
>> +	 * other path and that only also triggers compaction. So only unbind
>> +	 * if the device is currently awake.
>> +	 */
>> +	if (!intel_runtime_pm_get_if_in_use(dev_priv))
>> +		return -EBUSY;
>> +
>> +	i915_gem_object_get(obj);
>> +	if (!unsafe_drop_pages(obj))
>> +		ret = -EBUSY;
>> +	i915_gem_object_put(obj);
>
> Since the object release changes, we can now do this without the
> i915_gem_object_get / i915_gem_object_put (as we are guarded by the BKL
> struct_mutex).
Fine will remove object_get/put as with struct_mutex protection object 
can't disappear across unsafe_drop_pages().

Best regards
Akash


> -Chris
>

--
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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* [PATCH 1/2] shmem: Support for registration of driver/file owner specific ops
@ 2016-11-04 15:02 akash.goel
  2016-11-10  5:36 ` Hugh Dickins
  0 siblings, 1 reply; 30+ messages in thread
From: akash.goel @ 2016-11-04 15:02 UTC (permalink / raw
  To: intel-gfx
  Cc: Chris Wilson, Hugh Dickins, linux-mm, linux-kernel, Sourab Gupta,
	Akash Goel

From: Chris Wilson <chris@chris-wilson.co.uk>

This provides support for the drivers or shmem file owners to register
a set of callbacks, which can be invoked from the address space
operations methods implemented by shmem.  This allow the file owners to
hook into the shmem address space operations to do some extra/custom
operations in addition to the default ones.

The private_data field of address_space struct is used to store the
pointer to driver specific ops.  Currently only one ops field is defined,
which is migratepage, but can be extended on an as-needed basis.

The need for driver specific operations arises since some of the
operations (like migratepage) may not be handled completely within shmem,
so as to be effective, and would need some driver specific handling also.
Specifically, i915.ko would like to participate in migratepage().
i915.ko uses shmemfs to provide swappable backing storage for its user
objects, but when those objects are in use by the GPU it must pin the
entire object until the GPU is idle.  As a result, large chunks of memory
can be arbitrarily withdrawn from page migration, resulting in premature
out-of-memory due to fragmentation.  However, if i915.ko can receive the
migratepage() request, it can then flush the object from the GPU, remove
its pin and thus enable the migration.

Since gfx allocations are one of the major consumer of system memory, its
imperative to have such a mechanism to effectively deal with
fragmentation.  And therefore the need for such a provision for initiating
driver specific actions during address space operations.

v2:
- Drop dev_ prefix from the members of shmem_dev_info structure. (Joonas)
- Change the return type of shmem_set_device_op() to void and remove the
  check for pre-existing data. (Joonas)
- Rename shmem_set_device_op() to shmem_set_dev_info() to be consistent
  with shmem_dev_info structure. (Joonas)

Cc: Hugh Dickins <hughd@google.com>
Cc: linux-mm@kvack.org
Cc: linux-kernel@vger.linux.org
Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
Signed-off-by: Akash Goel <akash.goel@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 include/linux/shmem_fs.h | 13 +++++++++++++
 mm/shmem.c               | 17 ++++++++++++++++-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
index ff078e7..454c3ba 100644
--- a/include/linux/shmem_fs.h
+++ b/include/linux/shmem_fs.h
@@ -39,11 +39,24 @@ struct shmem_sb_info {
 	unsigned long shrinklist_len; /* Length of shrinklist */
 };
 
+struct shmem_dev_info {
+	void *private_data;
+	int (*migratepage)(struct address_space *mapping,
+			   struct page *newpage, struct page *page,
+			   enum migrate_mode mode, void *dev_priv_data);
+};
+
 static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
 {
 	return container_of(inode, struct shmem_inode_info, vfs_inode);
 }
 
+static inline void shmem_set_dev_info(struct address_space *mapping,
+				      struct shmem_dev_info *info)
+{
+	mapping->private_data = info;
+}
+
 /*
  * Functions in mm/shmem.c called directly from elsewhere:
  */
diff --git a/mm/shmem.c b/mm/shmem.c
index ad7813d..fce8de3 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -1290,6 +1290,21 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
 	return 0;
 }
 
+#ifdef CONFIG_MIGRATION
+static int shmem_migratepage(struct address_space *mapping,
+			     struct page *newpage, struct page *page,
+			     enum migrate_mode mode)
+{
+	struct shmem_dev_info *dev_info = mapping->private_data;
+
+	if (dev_info && dev_info->migratepage)
+		return dev_info->migratepage(mapping, newpage, page,
+					     mode, dev_info->private_data);
+
+	return migrate_page(mapping, newpage, page, mode);
+}
+#endif
+
 #if defined(CONFIG_NUMA) && defined(CONFIG_TMPFS)
 static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
 {
@@ -3654,7 +3669,7 @@ static void shmem_destroy_inodecache(void)
 	.write_end	= shmem_write_end,
 #endif
 #ifdef CONFIG_MIGRATION
-	.migratepage	= migrate_page,
+	.migratepage	= shmem_migratepage,
 #endif
 	.error_remove_page = generic_error_remove_page,
 };
-- 
1.9.2

--
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>

^ permalink raw reply related	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] shmem: Support for registration of driver/file owner specific ops
  2016-11-04 15:02 [PATCH 1/2] shmem: Support for registration of driver/file owner specific ops akash.goel
@ 2016-11-10  5:36 ` Hugh Dickins
  2016-11-10 16:22   ` Goel, Akash
  0 siblings, 1 reply; 30+ messages in thread
From: Hugh Dickins @ 2016-11-10  5:36 UTC (permalink / raw
  To: Akash Goel
  Cc: intel-gfx, Chris Wilson, Hugh Dickins, linux-mm, linux-kernel,
	Sourab Gupta

On Fri, 4 Nov 2016, akash.goel@intel.com wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> This provides support for the drivers or shmem file owners to register
> a set of callbacks, which can be invoked from the address space
> operations methods implemented by shmem.  This allow the file owners to
> hook into the shmem address space operations to do some extra/custom
> operations in addition to the default ones.
> 
> The private_data field of address_space struct is used to store the
> pointer to driver specific ops.  Currently only one ops field is defined,
> which is migratepage, but can be extended on an as-needed basis.
> 
> The need for driver specific operations arises since some of the
> operations (like migratepage) may not be handled completely within shmem,
> so as to be effective, and would need some driver specific handling also.
> Specifically, i915.ko would like to participate in migratepage().
> i915.ko uses shmemfs to provide swappable backing storage for its user
> objects, but when those objects are in use by the GPU it must pin the
> entire object until the GPU is idle.  As a result, large chunks of memory
> can be arbitrarily withdrawn from page migration, resulting in premature
> out-of-memory due to fragmentation.  However, if i915.ko can receive the
> migratepage() request, it can then flush the object from the GPU, remove
> its pin and thus enable the migration.
> 
> Since gfx allocations are one of the major consumer of system memory, its
> imperative to have such a mechanism to effectively deal with
> fragmentation.  And therefore the need for such a provision for initiating
> driver specific actions during address space operations.

Thank you for persisting with this, and sorry for all my delay.

> 
> v2:
> - Drop dev_ prefix from the members of shmem_dev_info structure. (Joonas)
> - Change the return type of shmem_set_device_op() to void and remove the
>   check for pre-existing data. (Joonas)
> - Rename shmem_set_device_op() to shmem_set_dev_info() to be consistent
>   with shmem_dev_info structure. (Joonas)
> 
> Cc: Hugh Dickins <hughd@google.com>
> Cc: linux-mm@kvack.org
> Cc: linux-kernel@vger.linux.org
> Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> Signed-off-by: Akash Goel <akash.goel@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

That doesn't seem quite right: the From line above implies that Chris
wrote it, and should be first Signer; but perhaps the From line is wrong.

> ---
>  include/linux/shmem_fs.h | 13 +++++++++++++
>  mm/shmem.c               | 17 ++++++++++++++++-
>  2 files changed, 29 insertions(+), 1 deletion(-)
> 
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index ff078e7..454c3ba 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -39,11 +39,24 @@ struct shmem_sb_info {
>  	unsigned long shrinklist_len; /* Length of shrinklist */
>  };
>  
> +struct shmem_dev_info {
> +	void *private_data;
> +	int (*migratepage)(struct address_space *mapping,
> +			   struct page *newpage, struct page *page,
> +			   enum migrate_mode mode, void *dev_priv_data);

Aren't the private_data field and dev_priv_data arg a little bit
confusing and redundant?  Can't the migratepage() deduce dev_priv
for itself from mapping->private_data (perhaps wrapped by a
shmem_get_dev_info()), by using container_of()?

> +};
> +
>  static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
>  {
>  	return container_of(inode, struct shmem_inode_info, vfs_inode);
>  }
>  
> +static inline void shmem_set_dev_info(struct address_space *mapping,
> +				      struct shmem_dev_info *info)
> +{
> +	mapping->private_data = info;

Nit: if this stays as is, I'd prefer dev_info there and above,
since shmem.c uses info all over for its shmem_inode_info pointer.
But in second patch I suggest obj_info may be better than dev_info.

> +}
> +
>  /*
>   * Functions in mm/shmem.c called directly from elsewhere:
>   */
> diff --git a/mm/shmem.c b/mm/shmem.c
> index ad7813d..fce8de3 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -1290,6 +1290,21 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_MIGRATION
> +static int shmem_migratepage(struct address_space *mapping,
> +			     struct page *newpage, struct page *page,
> +			     enum migrate_mode mode)
> +{
> +	struct shmem_dev_info *dev_info = mapping->private_data;
> +
> +	if (dev_info && dev_info->migratepage)
> +		return dev_info->migratepage(mapping, newpage, page,
> +					     mode, dev_info->private_data);
> +
> +	return migrate_page(mapping, newpage, page, mode);
> +}
> +#endif
> +
>  #if defined(CONFIG_NUMA) && defined(CONFIG_TMPFS)
>  static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>  {
> @@ -3654,7 +3669,7 @@ static void shmem_destroy_inodecache(void)
>  	.write_end	= shmem_write_end,
>  #endif
>  #ifdef CONFIG_MIGRATION
> -	.migratepage	= migrate_page,
> +	.migratepage	= shmem_migratepage,
>  #endif
>  	.error_remove_page = generic_error_remove_page,
>  };
> -- 
> 1.9.2

I didn't like this very much; but every time I tried to "improve" it,
found good reasons why you chose the way you did (modularity of i915,
constness of a_ops, reluctance to copy and modify a_ops, reluctance
to export those shmem methods separately).

I think perhaps later we just add a gem_ops pointer to shmem_inode_info,
for i915 or other gems to fill in as they wish (and shmem divert off to
them if set, as you've done); but for now you're trying to avoid
enlarging the shmem inode, okay.

Hugh

--
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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] shmem: Support for registration of driver/file owner specific ops
  2016-11-10  5:36 ` Hugh Dickins
@ 2016-11-10 16:22   ` Goel, Akash
  2016-11-11 13:50       ` Chris Wilson
  0 siblings, 1 reply; 30+ messages in thread
From: Goel, Akash @ 2016-11-10 16:22 UTC (permalink / raw
  To: Hugh Dickins
  Cc: intel-gfx, Chris Wilson, linux-mm, linux-kernel, Sourab Gupta,
	akash.goels, akash.goel



On 11/10/2016 11:06 AM, Hugh Dickins wrote:
> On Fri, 4 Nov 2016, akash.goel@intel.com wrote:
>> From: Chris Wilson <chris@chris-wilson.co.uk>
>>
>> This provides support for the drivers or shmem file owners to register
>> a set of callbacks, which can be invoked from the address space
>> operations methods implemented by shmem.  This allow the file owners to
>> hook into the shmem address space operations to do some extra/custom
>> operations in addition to the default ones.
>>
>> The private_data field of address_space struct is used to store the
>> pointer to driver specific ops.  Currently only one ops field is defined,
>> which is migratepage, but can be extended on an as-needed basis.
>>
>> The need for driver specific operations arises since some of the
>> operations (like migratepage) may not be handled completely within shmem,
>> so as to be effective, and would need some driver specific handling also.
>> Specifically, i915.ko would like to participate in migratepage().
>> i915.ko uses shmemfs to provide swappable backing storage for its user
>> objects, but when those objects are in use by the GPU it must pin the
>> entire object until the GPU is idle.  As a result, large chunks of memory
>> can be arbitrarily withdrawn from page migration, resulting in premature
>> out-of-memory due to fragmentation.  However, if i915.ko can receive the
>> migratepage() request, it can then flush the object from the GPU, remove
>> its pin and thus enable the migration.
>>
>> Since gfx allocations are one of the major consumer of system memory, its
>> imperative to have such a mechanism to effectively deal with
>> fragmentation.  And therefore the need for such a provision for initiating
>> driver specific actions during address space operations.
>
> Thank you for persisting with this, and sorry for all my delay.
>
>>
>> v2:
>> - Drop dev_ prefix from the members of shmem_dev_info structure. (Joonas)
>> - Change the return type of shmem_set_device_op() to void and remove the
>>   check for pre-existing data. (Joonas)
>> - Rename shmem_set_device_op() to shmem_set_dev_info() to be consistent
>>   with shmem_dev_info structure. (Joonas)
>>
>> Cc: Hugh Dickins <hughd@google.com>
>> Cc: linux-mm@kvack.org
>> Cc: linux-kernel@vger.linux.org
>> Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
>> Signed-off-by: Akash Goel <akash.goel@intel.com>
>> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
>
> That doesn't seem quite right: the From line above implies that Chris
> wrote it, and should be first Signer; but perhaps the From line is wrong.
>
Chris only wrote this patch initially, will do the required correction.

>> ---
>>  include/linux/shmem_fs.h | 13 +++++++++++++
>>  mm/shmem.c               | 17 ++++++++++++++++-
>>  2 files changed, 29 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>> index ff078e7..454c3ba 100644
>> --- a/include/linux/shmem_fs.h
>> +++ b/include/linux/shmem_fs.h
>> @@ -39,11 +39,24 @@ struct shmem_sb_info {
>>  	unsigned long shrinklist_len; /* Length of shrinklist */
>>  };
>>
>> +struct shmem_dev_info {
>> +	void *private_data;
>> +	int (*migratepage)(struct address_space *mapping,
>> +			   struct page *newpage, struct page *page,
>> +			   enum migrate_mode mode, void *dev_priv_data);
>
> Aren't the private_data field and dev_priv_data arg a little bit
> confusing and redundant?  Can't the migratepage() deduce dev_priv
> for itself from mapping->private_data (perhaps wrapped by a
> shmem_get_dev_info()), by using container_of()?
>
Yes looks like migratepage() can deduce dev_priv from mapping->private_data.
Can we keep the private_data as a placeholder ?. Will 
s/dev_priv_data/private_data/.

As per your suggestion, in the other patch, object pointer can be 
derived from mapping->private_data (container_of) and dev_priv in turn 
can be derived from object pointer.

>> +};
>> +
>>  static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
>>  {
>>  	return container_of(inode, struct shmem_inode_info, vfs_inode);
>>  }
>>
>> +static inline void shmem_set_dev_info(struct address_space *mapping,
>> +				      struct shmem_dev_info *info)
>> +{
>> +	mapping->private_data = info;
>
> Nit: if this stays as is, I'd prefer dev_info there and above,
> since shmem.c uses info all over for its shmem_inode_info pointer.
> But in second patch I suggest obj_info may be better than dev_info.
>
Fine will s/info/dev_info.

Best regards
Akash

>> +}
>> +
>>  /*
>>   * Functions in mm/shmem.c called directly from elsewhere:
>>   */
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index ad7813d..fce8de3 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -1290,6 +1290,21 @@ static int shmem_writepage(struct page *page, struct writeback_control *wbc)
>>  	return 0;
>>  }
>>
>> +#ifdef CONFIG_MIGRATION
>> +static int shmem_migratepage(struct address_space *mapping,
>> +			     struct page *newpage, struct page *page,
>> +			     enum migrate_mode mode)
>> +{
>> +	struct shmem_dev_info *dev_info = mapping->private_data;
>> +
>> +	if (dev_info && dev_info->migratepage)
>> +		return dev_info->migratepage(mapping, newpage, page,
>> +					     mode, dev_info->private_data);
>> +
>> +	return migrate_page(mapping, newpage, page, mode);
>> +}
>> +#endif
>> +
>>  #if defined(CONFIG_NUMA) && defined(CONFIG_TMPFS)
>>  static void shmem_show_mpol(struct seq_file *seq, struct mempolicy *mpol)
>>  {
>> @@ -3654,7 +3669,7 @@ static void shmem_destroy_inodecache(void)
>>  	.write_end	= shmem_write_end,
>>  #endif
>>  #ifdef CONFIG_MIGRATION
>> -	.migratepage	= migrate_page,
>> +	.migratepage	= shmem_migratepage,
>>  #endif
>>  	.error_remove_page = generic_error_remove_page,
>>  };
>> --
>> 1.9.2
>
> I didn't like this very much; but every time I tried to "improve" it,
> found good reasons why you chose the way you did (modularity of i915,
> constness of a_ops, reluctance to copy and modify a_ops, reluctance
> to export those shmem methods separately).
>
> I think perhaps later we just add a gem_ops pointer to shmem_inode_info,
> for i915 or other gems to fill in as they wish (and shmem divert off to
> them if set, as you've done); but for now you're trying to avoid
> enlarging the shmem inode, okay.
>
> Hugh
>

--
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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] shmem: Support for registration of driver/file owner specific ops
  2016-11-10 16:22   ` Goel, Akash
@ 2016-11-11 13:50       ` Chris Wilson
  0 siblings, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2016-11-11 13:50 UTC (permalink / raw
  To: Goel, Akash
  Cc: Hugh Dickins, intel-gfx, linux-mm, linux-kernel, Sourab Gupta,
	akash.goels

On Thu, Nov 10, 2016 at 09:52:34PM +0530, Goel, Akash wrote:
> 
> 
> On 11/10/2016 11:06 AM, Hugh Dickins wrote:
> >On Fri, 4 Nov 2016, akash.goel@intel.com wrote:
> >>Cc: Hugh Dickins <hughd@google.com>
> >>Cc: linux-mm@kvack.org
> >>Cc: linux-kernel@vger.linux.org
> >>Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> >>Signed-off-by: Akash Goel <akash.goel@intel.com>
> >>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> >That doesn't seem quite right: the From line above implies that Chris
> >wrote it, and should be first Signer; but perhaps the From line is wrong.
> >
> Chris only wrote this patch initially, will do the required correction.

Akash is being modest. I gave him an idea I had been toying with to help
reduce premature oom, he is the one who deserves credit for turning it
into a functional patch and putting it into production.
-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>

^ permalink raw reply	[flat|nested] 30+ messages in thread

* Re: [PATCH 1/2] shmem: Support for registration of driver/file owner specific ops
@ 2016-11-11 13:50       ` Chris Wilson
  0 siblings, 0 replies; 30+ messages in thread
From: Chris Wilson @ 2016-11-11 13:50 UTC (permalink / raw
  To: Goel, Akash; +Cc: intel-gfx, Hugh Dickins, linux-mm, Sourab Gupta, linux-kernel

On Thu, Nov 10, 2016 at 09:52:34PM +0530, Goel, Akash wrote:
> 
> 
> On 11/10/2016 11:06 AM, Hugh Dickins wrote:
> >On Fri, 4 Nov 2016, akash.goel@intel.com wrote:
> >>Cc: Hugh Dickins <hughd@google.com>
> >>Cc: linux-mm@kvack.org
> >>Cc: linux-kernel@vger.linux.org
> >>Signed-off-by: Sourab Gupta <sourab.gupta@intel.com>
> >>Signed-off-by: Akash Goel <akash.goel@intel.com>
> >>Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> >
> >That doesn't seem quite right: the From line above implies that Chris
> >wrote it, and should be first Signer; but perhaps the From line is wrong.
> >
> Chris only wrote this patch initially, will do the required correction.

Akash is being modest. I gave him an idea I had been toying with to help
reduce premature oom, he is the one who deserves credit for turning it
into a functional patch and putting it into production.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2016-11-11 13:50 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-03-23  6:09 [PATCH 1/2] shmem: Support for registration of Driver/file owner specific ops akash.goel
2016-03-23  6:09 ` [PATCH 2/2] drm/i915: Make pages of GFX allocations movable akash.goel
2016-03-23  7:58   ` Chris Wilson
2016-03-23  8:25     ` Goel, Akash
2016-03-24  8:13       ` Chris Wilson
2016-03-24 18:22         ` [PATCH v2 " akash.goel
2016-03-24 18:22           ` akash.goel
2016-03-24 18:40           ` Chris Wilson
2016-04-04 11:27           ` [PATCH v3 " akash.goel
2016-04-04 11:27             ` akash.goel
2016-03-23  7:39 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops Patchwork
2016-03-24 12:11 ` [Intel-gfx] [PATCH 1/2] " Joonas Lahtinen
2016-03-24 12:11   ` Joonas Lahtinen
2016-10-19 15:11   ` [Intel-gfx] " akash goel
2016-10-19 15:11     ` akash goel
2016-10-20 15:15     ` [Intel-gfx] " Joonas Lahtinen
2016-10-20 15:15       ` Joonas Lahtinen
2016-11-04 12:48       ` [PATCH 1/2] shmem: Support for registration of driver/file " akash.goel
2016-11-04 12:48         ` [PATCH 2/2] drm/i915: Make GPU pages movable akash.goel
2016-11-04 13:37           ` Chris Wilson
2016-11-04 13:37             ` Chris Wilson
2016-11-04 13:53             ` Goel, Akash
2016-03-25  7:31 ` ✗ Fi.CI.BAT: warning for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops (rev2) Patchwork
2016-04-04 13:14 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of Driver/file owner specific ops (rev3) Patchwork
2016-11-04 13:31 ` ✗ Fi.CI.BAT: failure for series starting with [1/2] shmem: Support for registration of driver/file owner specific ops (rev4) Patchwork
  -- strict thread matches above, loose matches on Subject: below --
2016-11-04 15:02 [PATCH 1/2] shmem: Support for registration of driver/file owner specific ops akash.goel
2016-11-10  5:36 ` Hugh Dickins
2016-11-10 16:22   ` Goel, Akash
2016-11-11 13:50     ` Chris Wilson
2016-11-11 13:50       ` Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.