linux-kernel - Re: [PATCHv2 2/3] i915: convert to new mount API

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 08 Aug 2019 17:23:57 +0100
From:   Chris Wilson <chris@...is-wilson.co.uk>
To:     Al Viro <viro@...iv.linux.org.uk>, Hugh Dickins <hughd@...gle.com>
Cc:     Christoph Hellwig <hch@....de>, Hugh Dickins <hughd@...gle.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Jani Nikula <jani.nikula@...ux.intel.com>,
        Joonas Lahtinen <joonas.lahtinen@...ux.intel.com>,
        Rodrigo Vivi <rodrigo.vivi@...el.com>,
        David Howells <dhowells@...hat.com>,
        David Airlie <airlied@...ux.ie>,
        Daniel Vetter <daniel@...ll.ch>,
        intel-gfx@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        Matthew Auld <matthew.auld@...el.com>
Subject: Re: [PATCHv2 2/3] i915: convert to new mount API

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