Re: [PATCH 1/3] hugetlbfs: Allow the creation of files suitable for MAP_PRIVATE on the vfs internal mount

Re: [PATCH 1/3] hugetlbfs: Allow the creation of files suitable for MAP_PRIVATE on the vfs internal mount

[Mel Gorman]

On Wed, Aug 26, 2009 at 11:44:51AM +0100, Eric B Munson wrote:
> There are two means of creating mappings backed by huge pages:
> 
>         1. mmap() a file created on hugetlbfs
>         2. Use shm which creates a file on an internal mount which essentially
>            maps it MAP_SHARED
> 
> The internal mount is only used for shared mappings but there is very
> little that stops it being used for private mappings. This patch extends
> hugetlbfs_file_setup() to deal with the creation of files that will be
> mapped MAP_PRIVATE on the internal hugetlbfs mount. This extended API is
> used in a subsequent patch to implement the MAP_HUGETLB mmap() flag.
> 

Hi Eric,

I ran these patches through a series of small tests and I have just one
concern with the changes made to can_do_hugetlb_shm(). If that returns false
because of MAP_HUGETLB, we then proceed to call user_shm_lock(). I think your
intention might have been something like the following patch on top of yours?

For what it's worth, once this was applied, I didn't spot any other
problems, run-time or otherwise.

=====
hugetlbfs: Do not call user_shm_lock() for MAP_HUGETLB

The patch
hugetlbfs-allow-the-creation-of-files-suitable-for-map_private-on-the-vfs-internal-mount.patch
alters can_do_hugetlb_shm() to check if a file is being created for shared
memory or mmap(). If this returns false, we then unconditionally call
user_shm_lock() triggering a warning. This block should never be entered
for MAP_HUGETLB. This patch partially reverts the problem and fixes the check.

This patch should be considered a fix to
hugetlbfs-allow-the-creation-of-files-suitable-for-map_private-on-the-vfs-internal-mount.patch.

Signed-off-by: Mel Gorman <mel@csn.ul.ie>
--- 
 fs/hugetlbfs/inode.c |   12 +++---------
 1 file changed, 3 insertions(+), 9 deletions(-)

diff --git a/fs/hugetlbfs/inode.c b/fs/hugetlbfs/inode.c
index 49d2bf9..c944cc1 100644
--- a/fs/hugetlbfs/inode.c
+++ b/fs/hugetlbfs/inode.c
@@ -910,15 +910,9 @@ static struct file_system_type hugetlbfs_fs_type = {
 
 static struct vfsmount *hugetlbfs_vfsmount;
 
-static int can_do_hugetlb_shm(int creat_flags)
+static int can_do_hugetlb_shm(void)
 {
-	if (creat_flags != HUGETLB_SHMFS_INODE)
-		return 0;
-	if (capable(CAP_IPC_LOCK))
-		return 1;
-	if (in_group_p(sysctl_hugetlb_shm_group))
-		return 1;
-	return 0;
+	return capable(CAP_IPC_LOCK) || in_group_p(sysctl_hugetlb_shm_group);
 }
 
 struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag,
@@ -934,7 +928,7 @@ struct file *hugetlb_file_setup(const char *name, size_t size, int acctflag,
 	if (!hugetlbfs_vfsmount)
 		return ERR_PTR(-ENOENT);
 
-	if (!can_do_hugetlb_shm(creat_flags)) {
+	if (creat_flags == HUGETLB_SHMFS_INODE && !can_do_hugetlb_shm()) {
 		*user = current_user();
 		if (user_shm_lock(size, *user)) {
 			WARN_ONCE(1,

Re: [PATCH 2/3] Add MAP_HUGETLB for mmaping pseudo-anonymous huge page regions

[Hugh Dickins]

On Wed, 26 Aug 2009, Eric B Munson wrote:
> This patch adds a flag for mmap that will be used to request a huge
> page region that will look like anonymous memory to user space.  This
> is accomplished by using a file on the internal vfsmount.  MAP_HUGETLB
> is a modifier of MAP_ANONYMOUS and so must be specified with it.  The
> region will behave the same as a MAP_ANONYMOUS region using small pages.
> 
> Signed-off-by: Eric B Munson <ebmunson@us.ibm.com>
> ---
>  include/asm-generic/mman-common.h |    1 +
>  include/linux/hugetlb.h           |    7 +++++++
>  mm/mmap.c                         |   19 +++++++++++++++++++
>  3 files changed, 27 insertions(+), 0 deletions(-)
> 
> diff --git a/include/asm-generic/mman-common.h b/include/asm-generic/mman-common.h
> index 3b69ad3..12f5982 100644
> --- a/include/asm-generic/mman-common.h
> +++ b/include/asm-generic/mman-common.h
> @@ -19,6 +19,7 @@
>  #define MAP_TYPE	0x0f		/* Mask for type of mapping */
>  #define MAP_FIXED	0x10		/* Interpret addr exactly */
>  #define MAP_ANONYMOUS	0x20		/* don't use a file */
> +#define MAP_HUGETLB	0x40		/* create a huge page mapping */
>  
>  #define MS_ASYNC	1		/* sync memory asynchronously */
>  #define MS_INVALIDATE	2		/* invalidate the caches */

I'm afraid you can't put MAP_HUGETLB in mman-common.h: that is picked
up by most or all architectures (which is of course what you wanted!)
but conflicts with a definition in at least one of them.  When I boot
up mmotm on powerpc, I get a warning:

Using mlock ulimits for SHM_HUGETLB deprecated
------------[ cut here ]------------
Badness at fs/hugetlbfs/inode.c:941
NIP: c0000000001f3038 LR: c0000000001f3034 CTR: 0000000000000000
REGS: c0000000275d7960 TRAP: 0700   Not tainted  (2.6.31-rc7-mm2)
MSR: 9000000000029032 <EE,ME,CE,IR,DR>  CR: 24000484  XER: 00000000
TASK = c000000029fa94a0[1321] 'console-kit-dae' THREAD: c0000000275d4000 CPU: 3
GPR00: c0000000001f3034 c0000000275d7be0 c00000000071a908 0000000000000032 
GPR04: 0000000000000000 ffffffffffffffff ffffffffffffffff 0000000000000000 
GPR08: c0000000297dc1d0 c0000000275d4000 d00008008247fa08 0000000000000000 
GPR12: 0000000024000442 c00000000074ba00 000000000fedb9a4 000000001049cd18 
GPR16: 00000000100365d0 00000000104a9100 000000000fefc350 00000000104a9098 
GPR20: 00000000104a9160 000000000fefc238 0000000000000000 0000000000200000 
GPR24: 0000000000000000 0000000001000000 c0000000275d7d20 0000000001000000 
GPR28: c00000000058c738 ffffffffffffffb5 c0000000006a93d0 c000000000791400 
NIP [c0000000001f3038] .hugetlb_file_setup+0xd0/0x254
LR [c0000000001f3034] .hugetlb_file_setup+0xcc/0x254
Call Trace:
[c0000000275d7be0] [c0000000001f3034] .hugetlb_file_setup+0xcc/0x254 (unreliable)
[c0000000275d7cb0] [c0000000000ee240] .do_mmap_pgoff+0x184/0x424
[c0000000275d7d80] [c00000000000a9c8] .sys_mmap+0xc4/0x13c
[c0000000275d7e30] [c0000000000075ac] syscall_exit+0x0/0x40
Instruction dump:
f89a0000 4bef7111 60000000 2c230000 41820034 e93e8018 80090014 2f800000 
40fe0030 e87e80b0 4823ff09 60000000 <0fe00000> e93e8018 38000001 90090014 

Which won't be coming from any use of MAP_HUGETLB, but presumably
from something using MAP_NORESERVE, defined as 0x40 in
arch/powerpc/include/asm/mman.h.

I think you have to put your #define MAP_HUGETLB into
include/asm-generic/mman.h (seems used by only three architectures),
and into the arch/whatever/include/asm/mman.h of each architecture
which uses asm-generic/mman-common.h without asm-generic/mman.h.

Hugh

Re: [PATCH 2/3] Add MAP_HUGETLB for mmaping pseudo-anonymous huge page regions

[Hugh Dickins]

On Tue, 1 Sep 2009, Eric B Munson wrote:
> On Mon, 31 Aug 2009, Hugh Dickins wrote:
> > On Wed, 26 Aug 2009, Eric B Munson wrote:
> > > This patch adds a flag for mmap that will be used to request a huge
> > > page region that will look like anonymous memory to user space.  This
> > > is accomplished by using a file on the internal vfsmount.  MAP_HUGETLB
> > > is a modifier of MAP_ANONYMOUS and so must be specified with it.  The
> > > region will behave the same as a MAP_ANONYMOUS region using small pages.
> > > 
> > > Signed-off-by: Eric B Munson <ebmunson@us.ibm.com>
> > > ---
> > >  include/asm-generic/mman-common.h |    1 +
> > >  include/linux/hugetlb.h           |    7 +++++++
> > >  mm/mmap.c                         |   19 +++++++++++++++++++
> > >  3 files changed, 27 insertions(+), 0 deletions(-)
> > > 
> > > diff --git a/include/asm-generic/mman-common.h b/include/asm-generic/mman-common.h
> > > index 3b69ad3..12f5982 100644
> > > --- a/include/asm-generic/mman-common.h
> > > +++ b/include/asm-generic/mman-common.h
> > > @@ -19,6 +19,7 @@
> > >  #define MAP_TYPE	0x0f		/* Mask for type of mapping */
> > >  #define MAP_FIXED	0x10		/* Interpret addr exactly */
> > >  #define MAP_ANONYMOUS	0x20		/* don't use a file */
> > > +#define MAP_HUGETLB	0x40		/* create a huge page mapping */
> > >  
> > >  #define MS_ASYNC	1		/* sync memory asynchronously */
> > >  #define MS_INVALIDATE	2		/* invalidate the caches */
> > 
> > I'm afraid you can't put MAP_HUGETLB in mman-common.h: that is picked
> > up by most or all architectures (which is of course what you wanted!)
> > but conflicts with a definition in at least one of them.  When I boot
> > up mmotm on powerpc, I get a warning:
> > 
> > Using mlock ulimits for SHM_HUGETLB deprecated
> > ------------[ cut here ]------------
> > Badness at fs/hugetlbfs/inode.c:941
> > NIP: c0000000001f3038 LR: c0000000001f3034 CTR: 0000000000000000
> > REGS: c0000000275d7960 TRAP: 0700   Not tainted  (2.6.31-rc7-mm2)
> > MSR: 9000000000029032 <EE,ME,CE,IR,DR>  CR: 24000484  XER: 00000000
> > TASK = c000000029fa94a0[1321] 'console-kit-dae' THREAD: c0000000275d4000 CPU: 3
> > GPR00: c0000000001f3034 c0000000275d7be0 c00000000071a908 0000000000000032 
> > GPR04: 0000000000000000 ffffffffffffffff ffffffffffffffff 0000000000000000 
> > GPR08: c0000000297dc1d0 c0000000275d4000 d00008008247fa08 0000000000000000 
> > GPR12: 0000000024000442 c00000000074ba00 000000000fedb9a4 000000001049cd18 
> > GPR16: 00000000100365d0 00000000104a9100 000000000fefc350 00000000104a9098 
> > GPR20: 00000000104a9160 000000000fefc238 0000000000000000 0000000000200000 
> > GPR24: 0000000000000000 0000000001000000 c0000000275d7d20 0000000001000000 
> > GPR28: c00000000058c738 ffffffffffffffb5 c0000000006a93d0 c000000000791400 
> > NIP [c0000000001f3038] .hugetlb_file_setup+0xd0/0x254
> > LR [c0000000001f3034] .hugetlb_file_setup+0xcc/0x254
> > Call Trace:
> > [c0000000275d7be0] [c0000000001f3034] .hugetlb_file_setup+0xcc/0x254 (unreliable)
> > [c0000000275d7cb0] [c0000000000ee240] .do_mmap_pgoff+0x184/0x424
> > [c0000000275d7d80] [c00000000000a9c8] .sys_mmap+0xc4/0x13c
> > [c0000000275d7e30] [c0000000000075ac] syscall_exit+0x0/0x40
> > Instruction dump:
> > f89a0000 4bef7111 60000000 2c230000 41820034 e93e8018 80090014 2f800000 
> > 40fe0030 e87e80b0 4823ff09 60000000 <0fe00000> e93e8018 38000001 90090014 
> > 
> > Which won't be coming from any use of MAP_HUGETLB, but presumably
> > from something using MAP_NORESERVE, defined as 0x40 in
> > arch/powerpc/include/asm/mman.h.
> > 
> > I think you have to put your #define MAP_HUGETLB into
> > include/asm-generic/mman.h (seems used by only three architectures),
> > and into the arch/whatever/include/asm/mman.h of each architecture
> > which uses asm-generic/mman-common.h without asm-generic/mman.h.
> > 
> > Hugh
> > 
> 
> This problem is the same that Mel Gorman reported (and fixed) in response to patch
> 1 of this series.  I have forwarded the patch that addresses this problem on,
> but it has not been picked up.
> 
> The bug is not where MAP_HUGETLB is defined, rather how the patch handled
> can_do_hugetlb_shm().  If MAP_HUGETLB was specified, can_do_hugetlb_shm() returned
> 0 forcing a call to user_shm_lock() which is responisble for the warning about
> SHM_HUGETLB and mlock ulimits.  The fix is to check if the file is to be used
> for SHM_HUGETLB and if not, skip the calls to can_do_hugetlb_shm() and
> user_shm_lock().

Sorry, no, I disagree.

I agree that the fs/hugetlbfs/inode.c:941 message and backtrace in
themselves are symptoms of the can_do_hugetlb_shm() bug that Mel
reported and fixed (I'm agreeing a little too readily, I've not
actually studied that bug and fix, I'm taking it on trust).

But that does not explain how last year's openSUSE 11.1 userspace
was trying for a MAP_HUGETLB mapping at startup on PowerPC (but
not on x86), while you're only introducing MAP_HUGETLB now.

That is explained by you #defining MAP_HUGETLB in include/asm-generic/
mman-common.h to a number which is already being used for other MAP_s
on some architectures.  That's a separate bug which needs to be fixed
by distributing the MAP_HUGETLB definition across various asm*/mman.h.

Hugh

Re: [PATCH 2/3] Add MAP_HUGETLB for mmaping pseudo-anonymous huge page regions

[Hugh Dickins]

On Tue, 1 Sep 2009, Eric B Munson wrote:
> On Tue, 01 Sep 2009, Hugh Dickins wrote:
> > 
> > That is explained by you #defining MAP_HUGETLB in include/asm-generic/
> > mman-common.h to a number which is already being used for other MAP_s
> > on some architectures.  That's a separate bug which needs to be fixed
> > by distributing the MAP_HUGETLB definition across various asm*/mman.h.
> 
> Would it be okay to keep the define in include/asm-generic/mman.h
> if a value that is known free across all architectures is used?
> 0x080000 is not used by any arch and, AFAICT would work just as well.

That's a very sensible suggestion, but departs from how we have
assigned new numbers up until now: so include/asm-generic/mman-common.h
isn't actually where we'd expect to find a Linux-specific MAP_ define.

I'd say, yes, do that for now, so as not to hit this conflict while
testing in mmotm.  But whether it should stay that way, or later the
arch/*/include/asm/mman.h's be updated as I'd imagined, I don't know.

Arnd, Michael, do you have any views on this?

Thanks,
Hugh

Re: [PATCH 2/3] Add MAP_HUGETLB for mmaping pseudo-anonymous huge page regions

[Arnd Bergmann]

On Tuesday 01 September 2009, Hugh Dickins wrote:
> On Tue, 1 Sep 2009, Eric B Munson wrote:
> > On Tue, 01 Sep 2009, Hugh Dickins wrote:
> > > 
> > > That is explained by you #defining MAP_HUGETLB in include/asm-generic/
> > > mman-common.h to a number which is already being used for other MAP_s
> > > on some architectures.  That's a separate bug which needs to be fixed
> > > by distributing the MAP_HUGETLB definition across various asm*/mman.h.
> > 
> > Would it be okay to keep the define in include/asm-generic/mman.h
> > if a value that is known free across all architectures is used?
> > 0x080000 is not used by any arch and, AFAICT would work just as well.
> 
> That's a very sensible suggestion, but departs from how we have
> assigned new numbers up until now: so include/asm-generic/mman-common.h
> isn't actually where we'd expect to find a Linux-specific MAP_ define.
> 
> I'd say, yes, do that for now, so as not to hit this conflict while
> testing in mmotm.  But whether it should stay that way, or later the
> arch/*/include/asm/mman.h's be updated as I'd imagined, I don't know.
> 
> Arnd, Michael, do you have any views on this?

The minimal procedure would be to add it to mman-common.h, plus
the asm/mman.h files for xtensa, mips, parisc and alpha, which all
use a version that is compatible to a Unix variant, but that would
be confusing the next person that needs to add a flag.

I'd use the number 0x40000 for all architectures except alpha,
because that makes the most sense for asm-generic/mman.h. Alpha
is weird anyway here, so we don't need to avoid conflicts with it.

With a few exceptions (sparc, powerpc), I think we should change
all architectures to use asm-generic/mman.h instead of mman-common.h
in the long run. If you touch those anyway, one option would be
to do it in one step.

	Arnd <><

Re: [PATCH 2/3] Add MAP_HUGETLB for mmaping pseudo-anonymous huge page regions

[Andrew Morton]

On Thu, 17 Sep 2009 15:44:04 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> mm/mmap.c: In function 'do_mmap_pgoff':
> mm/mmap.c:953: error: 'MAP_HUGETLB' undeclared (first use in this function)

mips breaks as well.

I don't know how many other architectures broke.  I disabled the patches.

Re: [PATCH 2/3] Add MAP_HUGETLB for mmaping pseudo-anonymous huge page regions

[Andrew Morton]

On Tue, 25 Aug 2009 12:14:53 +0100
Eric B Munson <ebmunson@us.ibm.com> wrote:

> This patch adds a flag for mmap that will be used to request a huge
> page region that will look like anonymous memory to user space.  This
> is accomplished by using a file on the internal vfsmount.  MAP_HUGETLB
> is a modifier of MAP_ANONYMOUS and so must be specified with it.  The
> region will behave the same as a MAP_ANONYMOUS region using small pages.
> 
> Signed-off-by: Eric B Munson <ebmunson@us.ibm.com>
> ---
>  include/asm-generic/mman-common.h |    1 +
>  include/linux/hugetlb.h           |    7 +++++++
>  mm/mmap.c                         |   19 +++++++++++++++++++

alpha fix:

From: Andrew Morton <akpm@linux-foundation.org>

mm/mmap.c: In function 'do_mmap_pgoff':
mm/mmap.c:953: error: 'MAP_HUGETLB' undeclared (first use in this function)
mm/mmap.c:953: error: (Each undeclared identifier is reported only once
mm/mmap.c:953: error: for each function it appears in.)

Cc: Adam Litke <agl@us.ibm.com>
Cc: David Gibson <david@gibson.dropbear.id.au>
Cc: David Rientjes <rientjes@google.com>
Cc: Eric B Munson <ebmunson@us.ibm.com>
Cc: Hugh Dickins <hugh.dickins@tiscali.co.uk>
Cc: Lee Schermerhorn <lee.schermerhorn@hp.com>
Cc: Mel Gorman <mel@csn.ul.ie>
Cc: Nick Piggin <nickpiggin@yahoo.com.au>
Cc: Ivan Kokshaysky <ink@jurassic.park.msu.ru>
Cc: Richard Henderson <rth@twiddle.net>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 arch/alpha/include/asm/mman.h |    1 +
 1 file changed, 1 insertion(+)

diff -puN arch/alpha/include/asm/mman.h~hugetlb-add-map_hugetlb-for-mmaping-pseudo-anonymous-huge-page-regions-alpha-fix arch/alpha/include/asm/mman.h
--- a/arch/alpha/include/asm/mman.h~hugetlb-add-map_hugetlb-for-mmaping-pseudo-anonymous-huge-page-regions-alpha-fix
+++ a/arch/alpha/include/asm/mman.h
@@ -28,6 +28,7 @@
 #define MAP_NORESERVE	0x10000		/* don't check for reservations */
 #define MAP_POPULATE	0x20000		/* populate (prefault) pagetables */
 #define MAP_NONBLOCK	0x40000		/* do not block on IO */
+#define MAP_HUGETLB	0x80000		/* create a huge page mapping */
 
 #define MS_ASYNC	1		/* sync memory asynchronously */
 #define MS_SYNC		2		/* synchronous memory sync */
_