diff mbox

[01/15] drm/i915: really simple gemfs

Message ID 20170531185210.29189-2-matthew.auld@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Matthew Auld May 31, 2017, 6:51 p.m. UTC 
Not a fully blown gemfs, just our very own tmpfs kernel mount. Doing so
moves us away from the shmemfs shm_mnt, and gives us the much needed
flexibility to do things like set our own mount options, namely huge=
which should allow us to enable the use of transparent-huge-pages for
our shmem backed objects.

Signed-off-by: Matthew Auld <matthew.auld@intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/Makefile                    |   1 +
 drivers/gpu/drm/i915/i915_drv.h                  |  13 +++
 drivers/gpu/drm/i915/i915_gem.c                  |  30 ++++++-
 drivers/gpu/drm/i915/i915_gemfs.c                | 105 +++++++++++++++++++++++
 drivers/gpu/drm/i915/selftests/mock_gem_device.c |  12 ++-
 5 files changed, 159 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/i915_gemfs.c

Comments

Chris Wilson May 31, 2017, 7:16 p.m. UTC | #1 
On Wed, May 31, 2017 at 07:51:56PM +0100, Matthew Auld wrote:
> Not a fully blown gemfs, just our very own tmpfs kernel mount. Doing so
> moves us away from the shmemfs shm_mnt, and gives us the much needed
> flexibility to do things like set our own mount options, namely huge=
> which should allow us to enable the use of transparent-huge-pages for
> our shmem backed objects.

After we get the kinks worked out, this should be in drm_gem.c With a
default mountpoint, it should present the same interface to the existing
users. But we will not be the only ones who will custom mount options.
Even for ttm who may only use shmemfs for swap, with the advent of fast
swapping for huge pages that will be win (if any improvement to swap
hell can be a win!).

Just food for thought, not the actual review.
-Chris
Joonas Lahtinen June 1, 2017, 10:49 a.m. UTC | #2 
On ke, 2017-05-31 at 19:51 +0100, Matthew Auld wrote:
> Not a fully blown gemfs, just our very own tmpfs kernel mount. Doing so
> moves us away from the shmemfs shm_mnt, and gives us the much needed
> flexibility to do things like set our own mount options, namely huge=
> which should allow us to enable the use of transparent-huge-pages for
> our shmem backed objects.
> 
> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -2227,6 +2227,9 @@ struct drm_i915_private {
>  	DECLARE_HASHTABLE(mm_structs, 7);
>  	struct mutex mm_lock;
>  
> +	/* Our tmpfs instance used for shmem backed objects */
> +	struct vfsmount *gemfs_mnt;

"gemfs" might be good enough, should not cause any confusion?

> @@ -4169,4 +4172,14 @@ static inline bool i915_gem_object_is_coherent(struct drm_i915_gem_object *obj)
>  		HAS_LLC(to_i915(obj->base.dev)));
>  }
>  
> +/* i915_gemfs.c */

i915_gemfs.h please. Lets not bloat i915_drv.h more when effort is in
place to strip it down.

> +struct vfsmount *i915_gemfs_create(void);

Not "int gemfs_init(struct drm_i915_privat *i915)" and _fini?

I doubt we should be creating more of these.

> @@ -4268,7 +4286,7 @@ i915_gem_object_create(struct drm_i915_private *dev_priv, u64 size)
>  	if (obj == NULL)
>  		return ERR_PTR(-ENOMEM);
>  
> -	ret = drm_gem_object_init(&dev_priv->drm, &obj->base, size);
> +	ret = i915_drm_gem_object_init(&dev_priv->drm, &obj->base, size);
>  	if (ret)
>  		goto fail;

As Chris mentioned, smells bit like we could be targeting DRM scope in
the future.

> @@ -4383,6 +4401,9 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
>  			drm_prime_gem_destroy(&obj->base, NULL);
>  
>  		reservation_object_fini(&obj->__builtin_resv);
> +

For code below, do drop a note here that we want to do part of
drm_gem_object_release's work in advance. Or rather than commenting,
make it explicitly clear by having i915_gem_object_release() with this
hunk of code.

> +		if (obj->base.filp)
> +			i915_gemfs_unlink(obj->base.filp);
>  		drm_gem_object_release(&obj->base);
>  		i915_gem_info_remove_obj(i915, obj->base.size);
>  
> @@ -4843,6 +4864,10 @@ i915_gem_load_init(struct drm_i915_private *dev_priv)
>  {
>  	int err = -ENOMEM;
>  
> +	dev_priv->gemfs_mnt = i915_gemfs_create();
> +	if (IS_ERR(dev_priv->gemfs_mnt))
> +		return PTR_ERR(dev_priv->gemfs_mnt);

	err = i915_gemfs_init(dev_priv);
	if (err)
		return err;

> +
>  	dev_priv->objects = KMEM_CACHE(drm_i915_gem_object, SLAB_HWCACHE_ALIGN);
>  	if (!dev_priv->objects)

		err = -ENOMEM;
		goto err_gemfs;
 
> @@ -4930,6 +4956,8 @@ void i915_gem_load_cleanup(struct drm_i915_private *dev_priv)
>  
>  	/* And ensure that our DESTROY_BY_RCU slabs are truly destroyed */
>  	rcu_barrier();
> +
> +	i915_gemfs_destroy(dev_priv->gemfs_mnt);

	i915_gemfs_fini();

> +struct file *i915_gemfs_file_setup(struct vfsmount *gemfs_mnt,
> +				   const char *name, size_t size)
> +{

<SNIP>

> +
> +	inode = d_inode(path.dentry);
> +	inode->i_size = size;
> +
> +	res = alloc_file(&path, FMODE_WRITE | FMODE_READ, inode->i_fop);

shmem is passing their own fops, we don't need to? shmem_mmap seems to
have some transparent huge page code at least which would be missed,
no?

> +	if (IS_ERR(res))
> +		goto unlink;
> +
> +	return res;
> +
> +unlink:
> +	dir->i_op->unlink(dir, path.dentry);
> +put_path:
> +	path_put(&path);

Might throw newline here.

Regards, Joonas
Matthew Auld June 1, 2017, 12:33 p.m. UTC | #3 
On 1 June 2017 at 11:49, Joonas Lahtinen
<joonas.lahtinen@linux.intel.com> wrote:
> On ke, 2017-05-31 at 19:51 +0100, Matthew Auld wrote:
>> Not a fully blown gemfs, just our very own tmpfs kernel mount. Doing so
>> moves us away from the shmemfs shm_mnt, and gives us the much needed
>> flexibility to do things like set our own mount options, namely huge=
>> which should allow us to enable the use of transparent-huge-pages for
>> our shmem backed objects.
>>
>> Signed-off-by: Matthew Auld <matthew.auld@intel.com>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>
> <SNIP>
>
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -2227,6 +2227,9 @@ struct drm_i915_private {
>>       DECLARE_HASHTABLE(mm_structs, 7);
>>       struct mutex mm_lock;
>>
>> +     /* Our tmpfs instance used for shmem backed objects */
>> +     struct vfsmount *gemfs_mnt;
>
> "gemfs" might be good enough, should not cause any confusion?
>
>> @@ -4169,4 +4172,14 @@ static inline bool i915_gem_object_is_coherent(struct drm_i915_gem_object *obj)
>>               HAS_LLC(to_i915(obj->base.dev)));
>>  }
>>
>> +/* i915_gemfs.c */
>
> i915_gemfs.h please. Lets not bloat i915_drv.h more when effort is in
> place to strip it down.
>
>> +struct vfsmount *i915_gemfs_create(void);
>
> Not "int gemfs_init(struct drm_i915_privat *i915)" and _fini?
>
> I doubt we should be creating more of these.
>
>> @@ -4268,7 +4286,7 @@ i915_gem_object_create(struct drm_i915_private *dev_priv, u64 size)
>>       if (obj == NULL)
>>               return ERR_PTR(-ENOMEM);
>>
>> -     ret = drm_gem_object_init(&dev_priv->drm, &obj->base, size);
>> +     ret = i915_drm_gem_object_init(&dev_priv->drm, &obj->base, size);
>>       if (ret)
>>               goto fail;
>
> As Chris mentioned, smells bit like we could be targeting DRM scope in
> the future.
>
>> @@ -4383,6 +4401,9 @@ static void __i915_gem_free_objects(struct drm_i915_private *i915,
>>                       drm_prime_gem_destroy(&obj->base, NULL);
>>
>>               reservation_object_fini(&obj->__builtin_resv);
>> +
>
> For code below, do drop a note here that we want to do part of
> drm_gem_object_release's work in advance. Or rather than commenting,
> make it explicitly clear by having i915_gem_object_release() with this
> hunk of code.
>
>> +             if (obj->base.filp)
>> +                     i915_gemfs_unlink(obj->base.filp);
>>               drm_gem_object_release(&obj->base);
>>               i915_gem_info_remove_obj(i915, obj->base.size);
>>
>> @@ -4843,6 +4864,10 @@ i915_gem_load_init(struct drm_i915_private *dev_priv)
>>  {
>>       int err = -ENOMEM;
>>
>> +     dev_priv->gemfs_mnt = i915_gemfs_create();
>> +     if (IS_ERR(dev_priv->gemfs_mnt))
>> +             return PTR_ERR(dev_priv->gemfs_mnt);
>
>         err = i915_gemfs_init(dev_priv);
>         if (err)
>                 return err;
>
>> +
>>       dev_priv->objects = KMEM_CACHE(drm_i915_gem_object, SLAB_HWCACHE_ALIGN);
>>       if (!dev_priv->objects)
>
>                 err = -ENOMEM;
>                 goto err_gemfs;
>
>> @@ -4930,6 +4956,8 @@ void i915_gem_load_cleanup(struct drm_i915_private *dev_priv)
>>
>>       /* And ensure that our DESTROY_BY_RCU slabs are truly destroyed */
>>       rcu_barrier();
>> +
>> +     i915_gemfs_destroy(dev_priv->gemfs_mnt);
>
>         i915_gemfs_fini();
>
>> +struct file *i915_gemfs_file_setup(struct vfsmount *gemfs_mnt,
>> +                                const char *name, size_t size)
>> +{
>
> <SNIP>
>
>> +
>> +     inode = d_inode(path.dentry);
>> +     inode->i_size = size;
>> +
>> +     res = alloc_file(&path, FMODE_WRITE | FMODE_READ, inode->i_fop);
>
> shmem is passing their own fops, we don't need to? shmem_mmap seems to
> have some transparent huge page code at least which would be missed,
> no?
We pass the same fops, since shmem_file_operations == inode->i_fop.
See shmem_get_inode.

I'll try to address the other comments, thanks.
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 16dccf550412..1a70af7d51ec 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -46,6 +46,7 @@  i915-y += i915_cmd_parser.o \
 	  i915_gem_tiling.o \
 	  i915_gem_timeline.o \
 	  i915_gem_userptr.o \
+	  i915_gemfs.o \
 	  i915_trace_points.o \
 	  i915_vma.o \
 	  intel_breadcrumbs.o \
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d2a57493ac2e..ca3196e2566f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2227,6 +2227,9 @@  struct drm_i915_private {
 	DECLARE_HASHTABLE(mm_structs, 7);
 	struct mutex mm_lock;
 
+	/* Our tmpfs instance used for shmem backed objects */
+	struct vfsmount *gemfs_mnt;
+
 	/* The hw wants to have a stable context identifier for the lifetime
 	 * of the context (for OA, PASID, faults, etc). This is limited
 	 * in execlists to 21 bits.
@@ -4169,4 +4172,14 @@  static inline bool i915_gem_object_is_coherent(struct drm_i915_gem_object *obj)
 		HAS_LLC(to_i915(obj->base.dev)));
 }
 
+/* i915_gemfs.c */
+struct vfsmount *i915_gemfs_create(void);
+
+void i915_gemfs_destroy(struct vfsmount *gemfs_mnt);
+
+struct file *i915_gemfs_file_setup(struct vfsmount *gemfs_mnt,
+				   const char *name, size_t size);
+
+int i915_gemfs_unlink(struct file *filp);
+
 #endif
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7ab47a84671f..30f9af590969 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4245,6 +4245,24 @@  static const struct drm_i915_gem_object_ops i915_gem_object_ops = {
 	.pwrite = i915_gem_object_pwrite_gtt,
 };
 
+static int i915_drm_gem_object_init(struct drm_device *dev,
+				    struct drm_gem_object *obj,
+				    size_t size)
+{
+	struct drm_i915_private *i915 = to_i915(dev);
+	struct file *filp;
+
+	drm_gem_private_object_init(dev, obj, size);
+
+	filp = i915_gemfs_file_setup(i915->gemfs_mnt, "i915 mm object", size);
+	if (IS_ERR(filp))
+		return PTR_ERR(filp);
+
+	obj->filp = filp;
+
+	return 0;
+}
+
 struct drm_i915_gem_object *
 i915_gem_object_create(struct drm_i915_private *dev_priv, u64 size)
 {
@@ -4268,7 +4286,7 @@  i915_gem_object_create(struct drm_i915_private *dev_priv, u64 size)
 	if (obj == NULL)
 		return ERR_PTR(-ENOMEM);
 
-	ret = drm_gem_object_init(&dev_priv->drm, &obj->base, size);
+	ret = i915_drm_gem_object_init(&dev_priv->drm, &obj->base, size);
 	if (ret)
 		goto fail;
 
@@ -4383,6 +4401,9 @@  static void __i915_gem_free_objects(struct drm_i915_private *i915,
 			drm_prime_gem_destroy(&obj->base, NULL);
 
 		reservation_object_fini(&obj->__builtin_resv);
+
+		if (obj->base.filp)
+			i915_gemfs_unlink(obj->base.filp);
 		drm_gem_object_release(&obj->base);
 		i915_gem_info_remove_obj(i915, obj->base.size);
 
@@ -4843,6 +4864,10 @@  i915_gem_load_init(struct drm_i915_private *dev_priv)
 {
 	int err = -ENOMEM;
 
+	dev_priv->gemfs_mnt = i915_gemfs_create();
+	if (IS_ERR(dev_priv->gemfs_mnt))
+		return PTR_ERR(dev_priv->gemfs_mnt);
+
 	dev_priv->objects = KMEM_CACHE(drm_i915_gem_object, SLAB_HWCACHE_ALIGN);
 	if (!dev_priv->objects)
 		goto err_out;
@@ -4908,6 +4933,7 @@  i915_gem_load_init(struct drm_i915_private *dev_priv)
 err_objects:
 	kmem_cache_destroy(dev_priv->objects);
 err_out:
+	i915_gemfs_destroy(dev_priv->gemfs_mnt);
 	return err;
 }
 
@@ -4930,6 +4956,8 @@  void i915_gem_load_cleanup(struct drm_i915_private *dev_priv)
 
 	/* And ensure that our DESTROY_BY_RCU slabs are truly destroyed */
 	rcu_barrier();
+
+	i915_gemfs_destroy(dev_priv->gemfs_mnt);
 }
 
 int i915_gem_freeze(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_gemfs.c b/drivers/gpu/drm/i915/i915_gemfs.c
new file mode 100644
index 000000000000..e1b2af1d9946
--- /dev/null
+++ b/drivers/gpu/drm/i915/i915_gemfs.c
@@ -0,0 +1,105 @@ 
+/*
+ * Copyright © 2017 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ *
+ */
+
+#include <linux/fs.h>
+#include <linux/file.h>
+#include <linux/mount.h>
+
+static const struct dentry_operations anon_ops = {
+	.d_dname = simple_dname
+};
+
+struct vfsmount *i915_gemfs_create(void)
+{
+	struct file_system_type *type;
+	struct vfsmount *gemfs_mnt;
+
+	type = get_fs_type("tmpfs");
+	if (!type)
+		return ERR_PTR(-EINVAL);
+
+	gemfs_mnt = kern_mount(type);
+
+	return gemfs_mnt;
+}
+
+void i915_gemfs_destroy(struct vfsmount *gemfs_mnt)
+{
+	kern_unmount(gemfs_mnt);
+}
+
+struct file *i915_gemfs_file_setup(struct vfsmount *gemfs_mnt,
+				   const char *name, size_t size)
+{
+	struct super_block *sb = gemfs_mnt->mnt_sb;
+	struct inode *dir = d_inode(sb->s_root);
+	struct inode *inode;
+	struct path path;
+	struct qstr this;
+	struct file *res;
+	int ret;
+
+	if (size < 0 || size > MAX_LFS_FILESIZE)
+		return ERR_PTR(-EINVAL);
+
+	this.name = name;
+	this.len = strlen(name);
+	this.hash = 0;
+
+	path.mnt = mntget(gemfs_mnt);
+	path.dentry = d_alloc_pseudo(sb, &this);
+	if (!path.dentry) {
+		res = ERR_PTR(-ENOMEM);
+		goto put_path;
+	}
+	d_set_d_op(path.dentry, &anon_ops);
+
+	ret = dir->i_op->create(dir, path.dentry, S_IFREG | S_IRWXUGO, false);
+	if (ret) {
+		res = ERR_PTR(ret);
+		goto put_path;
+	}
+
+	inode = d_inode(path.dentry);
+	inode->i_size = size;
+
+	res = alloc_file(&path, FMODE_WRITE | FMODE_READ, inode->i_fop);
+	if (IS_ERR(res))
+		goto unlink;
+
+	return res;
+
+unlink:
+	dir->i_op->unlink(dir, path.dentry);
+put_path:
+	path_put(&path);
+	return res;
+}
+
+int i915_gemfs_unlink(struct file *filp)
+{
+	struct inode *dir = d_inode(filp->f_inode->i_sb->s_root);
+
+	return dir->i_op->unlink(dir, filp->f_path.dentry);
+}
diff --git a/drivers/gpu/drm/i915/selftests/mock_gem_device.c b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
index 627e2aa09766..7f038ea15ef5 100644
--- a/drivers/gpu/drm/i915/selftests/mock_gem_device.c
+++ b/drivers/gpu/drm/i915/selftests/mock_gem_device.c
@@ -80,6 +80,8 @@  static void mock_device_release(struct drm_device *dev)
 	kmem_cache_destroy(i915->vmas);
 	kmem_cache_destroy(i915->objects);
 
+	i915_gemfs_destroy(i915->gemfs_mnt);
+
 	drm_dev_fini(&i915->drm);
 	put_device(&i915->drm.pdev->dev);
 }
@@ -167,9 +169,15 @@  struct drm_i915_private *mock_gem_device(void)
 
 	i915->gt.awake = true;
 
+	i915->gemfs_mnt = i915_gemfs_create();
+	if (IS_ERR(i915->gemfs_mnt)) {
+		err = PTR_ERR(i915->gemfs_mnt);
+		goto err_wq;
+	}
+
 	i915->objects = KMEM_CACHE(mock_object, SLAB_HWCACHE_ALIGN);
 	if (!i915->objects)
-		goto err_wq;
+		goto err_gemfs;
 
 	i915->vmas = KMEM_CACHE(i915_vma, SLAB_HWCACHE_ALIGN);
 	if (!i915->vmas)
@@ -227,6 +235,8 @@  struct drm_i915_private *mock_gem_device(void)
 	kmem_cache_destroy(i915->vmas);
 err_objects:
 	kmem_cache_destroy(i915->objects);
+err_gemfs:
+	i915_gemfs_destroy(i915->gemfs_mnt);
 err_wq:
 	destroy_workqueue(i915->wq);
 put_device: