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

[Eric B Munson <ebmunson@us.ibm.com>]

[-- Attachment #1: Type: text/plain, Size: 3323 bytes --]

On Thu, 27 Aug 2009, Mel Gorman wrote:

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

I am seeing the same thing, terminal says segfault with no memory, dmesg
complains about SHM.  Your patch fixes the issue.  Thanks.


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

-- 
Eric B Munson
IBM Linux Technology Center
ebmunson@us.ibm.com


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 197 bytes --]