On Wed, Aug 07, 2019 at 08:30:02AM +0200, Christoph Hellwig wrote:
On Tue, Aug 06, 2019 at 12:50:10AM -0700, Hugh Dickins wrote:
Though personally I'm averse to managing "f"objects through "m"interfaces, which can get ridiculous (notably, MADV_HUGEPAGE works on the virtual address of a mapping, but the huge-or-not alignment of that mapping must have been decided previously). In Google we do use fcntls F_HUGEPAGE and F_NOHUGEPAGE to override on a per-file basis - one day I'll get to upstreaming those.
Such an interface seems very useful, although the two fcntls seem a bit odd.
But I think the point here is that the i915 has its own somewhat odd instance of tmpfs. If we could pass the equivalent of the huge=* options to shmem_file_setup all that garbage (including the shmem_file_setup_with_mnt function) could go away.
... or follow shmem_file_super() with whatever that fcntl maps to internally. I would really love to get rid of that i915 kludge.
On Thu, 8 Aug 2019, Al Viro wrote:
On Wed, Aug 07, 2019 at 08:30:02AM +0200, Christoph Hellwig wrote:
On Tue, Aug 06, 2019 at 12:50:10AM -0700, Hugh Dickins wrote:
Though personally I'm averse to managing "f"objects through "m"interfaces, which can get ridiculous (notably, MADV_HUGEPAGE works on the virtual address of a mapping, but the huge-or-not alignment of that mapping must have been decided previously). In Google we do use fcntls F_HUGEPAGE and F_NOHUGEPAGE to override on a per-file basis - one day I'll get to upstreaming those.
Such an interface seems very useful, although the two fcntls seem a bit odd.
But I think the point here is that the i915 has its own somewhat odd instance of tmpfs. If we could pass the equivalent of the huge=* options to shmem_file_setup all that garbage (including the shmem_file_setup_with_mnt function) could go away.
... or follow shmem_file_super() with whatever that fcntl maps to internally. I would really love to get rid of that i915 kludge.
As to the immediate problem of i915_gemfs using remount_fs on linux-next, IIUC, all that is necessary at the moment is the deletions patch below (but I'd prefer that to come from the i915 folks). Since gemfs has no need to change the huge option from its default to its default.
As to the future of when they get back to wanting huge pages in gemfs, yes, that can probably best be arranged by using the internals of an fcntl F_HUGEPAGE on those objects that would benefit from it.
Though my intention there was that the "huge=never" default ought to continue to refuse to give huge pages, even when asked by fcntl. So a little hackery may still be required, to allow the i915_gemfs internal mount to get huge pages when a user mount would not.
As to whether shmem_file_setup_with_mnt() needs to live: I've given that no thought, but accept that shm_mnt is such a ragbag of different usages, that i915 is right to prefer their own separate gemfs mount.
Hugh
--- mmotm/drivers/gpu/drm/i915/gem/i915_gemfs.c 2019-07-21 19:40:16.573703780 -0700 +++ linux/drivers/gpu/drm/i915/gem/i915_gemfs.c 2019-08-08 07:19:23.967689058 -0700 @@ -24,28 +24,6 @@ int i915_gemfs_init(struct drm_i915_priv if (IS_ERR(gemfs)) return PTR_ERR(gemfs);
- /* - * Enable huge-pages for objects that are at least HPAGE_PMD_SIZE, most - * likely 2M. Note that within_size may overallocate huge-pages, if say - * we allocate an object of size 2M + 4K, we may get 2M + 2M, but under - * memory pressure shmem should split any huge-pages which can be - * shrunk. - */ - - if (has_transparent_hugepage()) { - struct super_block *sb = gemfs->mnt_sb; - /* FIXME: Disabled until we get W/A for read BW issue. */ - char options[] = "huge=never"; - int flags = 0; - int err; - - err = sb->s_op->remount_fs(sb, &flags, options); - if (err) { - kern_unmount(gemfs); - return err; - } - } - i915->mm.gemfs = gemfs;
return 0;
Quoting Hugh Dickins (2019-08-08 16:54:16)
On Thu, 8 Aug 2019, Al Viro wrote:
On Wed, Aug 07, 2019 at 08:30:02AM +0200, Christoph Hellwig wrote:
On Tue, Aug 06, 2019 at 12:50:10AM -0700, Hugh Dickins wrote:
Though personally I'm averse to managing "f"objects through "m"interfaces, which can get ridiculous (notably, MADV_HUGEPAGE works on the virtual address of a mapping, but the huge-or-not alignment of that mapping must have been decided previously). In Google we do use fcntls F_HUGEPAGE and F_NOHUGEPAGE to override on a per-file basis - one day I'll get to upstreaming those.
Such an interface seems very useful, although the two fcntls seem a bit odd.
But I think the point here is that the i915 has its own somewhat odd instance of tmpfs. If we could pass the equivalent of the huge=* options to shmem_file_setup all that garbage (including the shmem_file_setup_with_mnt function) could go away.
... or follow shmem_file_super() with whatever that fcntl maps to internally. I would really love to get rid of that i915 kludge.
As to the immediate problem of i915_gemfs using remount_fs on linux-next, IIUC, all that is necessary at the moment is the deletions patch below (but I'd prefer that to come from the i915 folks). Since gemfs has no need to change the huge option from its default to its default.
As to the future of when they get back to wanting huge pages in gemfs, yes, that can probably best be arranged by using the internals of an fcntl F_HUGEPAGE on those objects that would benefit from it.
Though my intention there was that the "huge=never" default ought to continue to refuse to give huge pages, even when asked by fcntl. So a little hackery may still be required, to allow the i915_gemfs internal mount to get huge pages when a user mount would not.
As to whether shmem_file_setup_with_mnt() needs to live: I've given that no thought, but accept that shm_mnt is such a ragbag of different usages, that i915 is right to prefer their own separate gemfs mount.
Hugh
--- mmotm/drivers/gpu/drm/i915/gem/i915_gemfs.c 2019-07-21 19:40:16.573703780 -0700 +++ linux/drivers/gpu/drm/i915/gem/i915_gemfs.c 2019-08-08 07:19:23.967689058 -0700 @@ -24,28 +24,6 @@ int i915_gemfs_init(struct drm_i915_priv if (IS_ERR(gemfs)) return PTR_ERR(gemfs);
/** Enable huge-pages for objects that are at least HPAGE_PMD_SIZE, most* likely 2M. Note that within_size may overallocate huge-pages, if say* we allocate an object of size 2M + 4K, we may get 2M + 2M, but under* memory pressure shmem should split any huge-pages which can be* shrunk.*/if (has_transparent_hugepage()) {struct super_block *sb = gemfs->mnt_sb;/* FIXME: Disabled until we get W/A for read BW issue. */char options[] = "huge=never";int flags = 0;int err;err = sb->s_op->remount_fs(sb, &flags, options);if (err) {kern_unmount(gemfs);return err;}}
That's perfectly fine; we should probably leave a hint as to why gemfs exists and include the suggestion of looking at per-file hugepage controls.
Matthew, how does this affect your current plans? If at all? -Chris
On 08/08/2019 17:23, Chris Wilson wrote:
Quoting Hugh Dickins (2019-08-08 16:54:16)
On Thu, 8 Aug 2019, Al Viro wrote:
On Wed, Aug 07, 2019 at 08:30:02AM +0200, Christoph Hellwig wrote:
On Tue, Aug 06, 2019 at 12:50:10AM -0700, Hugh Dickins wrote:
Though personally I'm averse to managing "f"objects through "m"interfaces, which can get ridiculous (notably, MADV_HUGEPAGE works on the virtual address of a mapping, but the huge-or-not alignment of that mapping must have been decided previously). In Google we do use fcntls F_HUGEPAGE and F_NOHUGEPAGE to override on a per-file basis - one day I'll get to upstreaming those.
Such an interface seems very useful, although the two fcntls seem a bit odd.
But I think the point here is that the i915 has its own somewhat odd instance of tmpfs. If we could pass the equivalent of the huge=* options to shmem_file_setup all that garbage (including the shmem_file_setup_with_mnt function) could go away.
... or follow shmem_file_super() with whatever that fcntl maps to internally. I would really love to get rid of that i915 kludge.
As to the immediate problem of i915_gemfs using remount_fs on linux-next, IIUC, all that is necessary at the moment is the deletions patch below (but I'd prefer that to come from the i915 folks). Since gemfs has no need to change the huge option from its default to its default.
As to the future of when they get back to wanting huge pages in gemfs, yes, that can probably best be arranged by using the internals of an fcntl F_HUGEPAGE on those objects that would benefit from it.
Though my intention there was that the "huge=never" default ought to continue to refuse to give huge pages, even when asked by fcntl. So a little hackery may still be required, to allow the i915_gemfs internal mount to get huge pages when a user mount would not.
As to whether shmem_file_setup_with_mnt() needs to live: I've given that no thought, but accept that shm_mnt is such a ragbag of different usages, that i915 is right to prefer their own separate gemfs mount.
Hugh
--- mmotm/drivers/gpu/drm/i915/gem/i915_gemfs.c 2019-07-21 19:40:16.573703780 -0700 +++ linux/drivers/gpu/drm/i915/gem/i915_gemfs.c 2019-08-08 07:19:23.967689058 -0700 @@ -24,28 +24,6 @@ int i915_gemfs_init(struct drm_i915_priv if (IS_ERR(gemfs)) return PTR_ERR(gemfs);
/** Enable huge-pages for objects that are at least HPAGE_PMD_SIZE, most* likely 2M. Note that within_size may overallocate huge-pages, if say* we allocate an object of size 2M + 4K, we may get 2M + 2M, but under* memory pressure shmem should split any huge-pages which can be* shrunk.*/if (has_transparent_hugepage()) {struct super_block *sb = gemfs->mnt_sb;/* FIXME: Disabled until we get W/A for read BW issue. */char options[] = "huge=never";int flags = 0;int err;err = sb->s_op->remount_fs(sb, &flags, options);if (err) {kern_unmount(gemfs);return err;}}That's perfectly fine; we should probably leave a hint as to why gemfs exists and include the suggestion of looking at per-file hugepage controls.
Matthew, how does this affect your current plans? If at all?
Fine with me.
-Chris
dri-devel@lists.freedesktop.org
-
Al Viro -
Chris Wilson -
Hugh Dickins -
Matthew Auld