Skip to contentSkip to conversation

hugetlb: Remove is_file_hugepages() macro

PATCH New Adam Litke 1 participant 7 messages Jan 31, 2007

Subsystems (5)
Memory management VFS Memory mapping HugeTLB +1 more subsystems
lore
Share & tools
lore

Adam Litke

author
#0 lore raw

cover hugetlb: Remove is_file_hugepages() macro

The kernel code is currently peppered with special casing for hugetlbfs mappings. In many places we check a struct file's f_op member to see if it points to the hugetlbfs file_operations in which case we'll employ some sort of workaround. The need to check file_operations in this manner suggests that we are either missing f_op operations, or have deficient abstraction elsewhere.

I am motivated to clean this up for two reasons: 1) The community has asked for huge pages to be kept "on the side" of the main VM. I believe these patches advance that goal. 2) Proper abstraction of hugetlbfs allows the underlying implementation to be changed without disturbing the main VM.

Removing the is_file_hugepages() macro involved finding all the call sites, determining the actual incompatibility that huge page mappings introduce, and applying a relatively trivial fix for the problem. The following patches perform this surgery.

When converting these, I have tried to use as general of a solution as possible. Review of some of the design decisions would be appreciated -- specifically the use of backing_dev_info for the note about special accounting, and hugetlbfs sharing the inode_info struct with shmem.

Thanks to Andy Whitcroft and others for review of the preliminary patches.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#1 lore raw

PATCH 1/6 Define the shmem_inode_info flags directly

Defining flags in terms of other flags is always confusing. Give them literal values instead of defining them in terms of VM_flags. While we're at it, move them to a header file so they can be used by a later patch in this series.

Signed-off-by Adam Litke <[email protected]>

---

 include/linux/shmem_fs.h |    4 ++++
 mm/shmem.c               |    4 ----
 2 files changed, 4 insertions(+), 4 deletions(-)
include/linux/shmem_fs.h +4 -0
index f3c5189..3ea0b6e 100644
@@ -8,6 +8,10 @@
 
 #define SHMEM_NR_DIRECT 16
 
+/* These info->flags are used to handle pagein/truncate races efficiently */
+#define SHMEM_PAGEIN	0x00000001
+#define SHMEM_TRUNCATE	0x00000002
+
 struct shmem_inode_info {
 	spinlock_t		lock;
 	unsigned long		flags;
mm/shmem.c +0 -4
index 70da7a0..a9bdb0d 100644
@@ -66,10 +66,6 @@
 
 #define VM_ACCT(size)    (PAGE_CACHE_ALIGN(size) >> PAGE_SHIFT)
 
-/* info->flags needs VM_flags to handle pagein/truncate races efficiently */
-#define SHMEM_PAGEIN	 VM_READ
-#define SHMEM_TRUNCATE	 VM_WRITE
-
 /* Definition to limit shmem_truncate's steps between cond_rescheds */
 #define LATENCY_LIMIT	 64

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#2 lore raw

PATCH 2/6 hugetlb: share shmem_inode_info

In reply to Adam Litke

The definition of the inode_info struct is different between hugetlbfs and shmem even though most of the code that operates on this data is shared between the two.

Define hugetlbfs_inode_info in terms of shmem_inode_info which creates a handy place to mark shm segments hugetlb (without the ugly file_operations check). Sharing this structure also goes a long way towards removing all hugetlbfs special casing in the shm code (should that be desired in the future).

Signed-off-by Adam Litke <[email protected]>

---

 fs/hugetlbfs/inode.c     |    9 +++++----
 include/linux/hugetlb.h  |   11 ++++-------
 include/linux/shmem_fs.h |    1 +
 3 files changed, 10 insertions(+), 11 deletions(-)
fs/hugetlbfs/inode.c +5 -4
index 4f4cd13..bd54e7e 100644
@@ -346,7 +346,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb, uid_t uid,
 
 	inode = new_inode(sb);
 	if (inode) {
-		struct hugetlbfs_inode_info *info;
+		hugetlbfs_inode_info *info;
 		inode->i_mode = mode;
 		inode->i_uid = uid;
 		inode->i_gid = gid;

@@ -356,6 +356,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb, uid_t uid,
 		inode->i_atime = inode->i_mtime = inode->i_ctime = CURRENT_TIME;
 		INIT_LIST_HEAD(&inode->i_mapping->private_list);
 		info = HUGETLBFS_I(inode);
+		memset(info, 0, offsetof(hugetlbfs_inode_info, vfs_inode));
 		mpol_shared_policy_init(&info->policy, MPOL_DEFAULT, NULL);
 		switch (mode & S_IFMT) {
 		default:

@@ -518,7 +519,7 @@ static struct kmem_cache *hugetlbfs_inode_cachep;
 static struct inode *hugetlbfs_alloc_inode(struct super_block *sb)
 {
 	struct hugetlbfs_sb_info *sbinfo = HUGETLBFS_SB(sb);
-	struct hugetlbfs_inode_info *p;
+	hugetlbfs_inode_info *p;
 
 	if (unlikely(!hugetlbfs_dec_free_inodes(sbinfo)))
 		return NULL;

@@ -547,7 +548,7 @@ static const struct address_space_operations hugetlbfs_aops = {
 
 static void init_once(void *foo, struct kmem_cache *cachep, unsigned long flags)
 {
-	struct hugetlbfs_inode_info *ei = (struct hugetlbfs_inode_info *)foo;
+	hugetlbfs_inode_info *ei = (hugetlbfs_inode_info *)foo;
 
 	if ((flags & (SLAB_CTOR_VERIFY|SLAB_CTOR_CONSTRUCTOR)) ==
 	    SLAB_CTOR_CONSTRUCTOR)

@@ -798,7 +799,7 @@ static int __init init_hugetlbfs_fs(void)
 	struct vfsmount *vfsmount;
 
 	hugetlbfs_inode_cachep = kmem_cache_create("hugetlbfs_inode_cache",
-					sizeof(struct hugetlbfs_inode_info),
+					sizeof(hugetlbfs_inode_info),
 					0, 0, init_once, NULL);
 	if (hugetlbfs_inode_cachep == NULL)
 		return -ENOMEM;
include/linux/hugetlb.h +4 -7
index a60995a..a184933 100644
@@ -4,6 +4,7 @@
 #ifdef CONFIG_HUGETLB_PAGE
 
 #include <linux/mempolicy.h>
+#include <linux/shmem_fs.h>
 #include <asm/tlbflush.h>
 
 struct ctl_table;

@@ -144,15 +145,11 @@ struct hugetlbfs_sb_info {
 	spinlock_t	stat_lock;
 };
 
+typedef struct shmem_inode_info hugetlbfs_inode_info;
 
-struct hugetlbfs_inode_info {
-	struct shared_policy policy;
-	struct inode vfs_inode;
-};
-
-static inline struct hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
+static inline hugetlbfs_inode_info *HUGETLBFS_I(struct inode *inode)
 {
-	return container_of(inode, struct hugetlbfs_inode_info, vfs_inode);
+	return container_of(inode, hugetlbfs_inode_info, vfs_inode);
 }
 
 static inline struct hugetlbfs_sb_info *HUGETLBFS_SB(struct super_block *sb)
include/linux/shmem_fs.h +1 -0
index 3ea0b6e..23707f1 100644
@@ -12,6 +12,7 @@
 #define SHMEM_PAGEIN	0x00000001
 #define SHMEM_TRUNCATE	0x00000002
 
+/* Hugetlbfs is now using this structure definition */
 struct shmem_inode_info {
 	spinlock_t		lock;
 	unsigned long		flags;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#3 lore raw

PATCH 3/6 Use inode_info to annotate hugetlbfs shm segments

In reply to Adam Litke

Now that hugetlbfs and shmem share the same inode_info struct, add a SHMEM_flag to mark the hugetlb shm segments as special. We can then check that flag (rather than using file_operations) for hugetlb special cases.

Signed-off-by Adam Litke <[email protected]>

---

 fs/hugetlbfs/inode.c     |    1 +
 include/linux/shmem_fs.h |   10 ++++++++++
 ipc/shm.c                |   12 ++++++------
 3 files changed, 17 insertions(+), 6 deletions(-)
fs/hugetlbfs/inode.c +1 -0
index bd54e7e..c95dc47 100644
@@ -357,6 +357,7 @@ static struct inode *hugetlbfs_get_inode(struct super_block *sb, uid_t uid,
 		INIT_LIST_HEAD(&inode->i_mapping->private_list);
 		info = HUGETLBFS_I(inode);
 		memset(info, 0, offsetof(hugetlbfs_inode_info, vfs_inode));
+		info->flags |= SHMEM_HUGETLBFS;
 		mpol_shared_policy_init(&info->policy, MPOL_DEFAULT, NULL);
 		switch (mode & S_IFMT) {
 		default:
include/linux/shmem_fs.h +10 -0
index 23707f1..c6ae0c8 100644
@@ -3,6 +3,7 @@
 
 #include <linux/swap.h>
 #include <linux/mempolicy.h>
+#include <linux/shm.h>
 
 /* inode in-kernel data */
 

@@ -11,6 +12,7 @@
 /* These info->flags are used to handle pagein/truncate races efficiently */
 #define SHMEM_PAGEIN	0x00000001
 #define SHMEM_TRUNCATE	0x00000002
+#define SHMEM_HUGETLBFS	0x00000004 /* Backed by hugetlbfs */
 
 /* Hugetlbfs is now using this structure definition */
 struct shmem_inode_info {

@@ -45,6 +47,14 @@ static inline struct shmem_inode_info *SHMEM_I(struct inode *inode)
 	return container_of(inode, struct shmem_inode_info, vfs_inode);
 }
 
+static inline int is_shm_hugetlbfs(struct shmid_kernel *shp)
+{
+	struct shmem_inode_info *info;
+
+	info = SHMEM_I(shp->shm_file->f_path.dentry->d_inode);
+	return info->flags & SHMEM_HUGETLBFS;
+}
+
 #ifdef CONFIG_TMPFS_POSIX_ACL
 int shmem_permission(struct inode *, int, struct nameidata *);
 int shmem_acl_init(struct inode *, struct inode *);
ipc/shm.c +6 -6
index f8e10a2..6054b16 100644
@@ -184,7 +184,7 @@ static void shm_destroy(struct ipc_namespace *ns, struct shmid_kernel *shp)
 	ns->shm_tot -= (shp->shm_segsz + PAGE_SIZE - 1) >> PAGE_SHIFT;
 	shm_rmid(ns, shp->id);
 	shm_unlock(shp);
-	if (!is_file_hugepages(shp->shm_file))
+	if (!is_shm_hugetlbfs(shp))
 		shmem_lock(shp->shm_file, 0, shp->mlock_user);
 	else
 		user_shm_unlock(shp->shm_file->f_path.dentry->d_inode->i_size,

@@ -497,7 +497,7 @@ static void shm_get_stat(struct ipc_namespace *ns, unsigned long *rss,
 
 		inode = shp->shm_file->f_path.dentry->d_inode;
 
-		if (is_file_hugepages(shp->shm_file)) {
+		if (is_shm_hugetlbfs(shp)) {
 			struct address_space *mapping = inode->i_mapping;
 			*rss += (HPAGE_SIZE/PAGE_SIZE)*mapping->nrpages;
 		} else {

@@ -607,7 +607,7 @@ asmlinkage long sys_shmctl (int shmid, int cmd, struct shmid_ds __user *buf)
 		tbuf.shm_ctime	= shp->shm_ctim;
 		tbuf.shm_cpid	= shp->shm_cprid;
 		tbuf.shm_lpid	= shp->shm_lprid;
-		if (!is_file_hugepages(shp->shm_file))
+		if (!is_shm_hugetlbfs(shp))
 			tbuf.shm_nattch	= shp->shm_nattch;
 		else
 			tbuf.shm_nattch = file_count(shp->shm_file) - 1;

@@ -650,14 +650,14 @@ asmlinkage long sys_shmctl (int shmid, int cmd, struct shmid_ds __user *buf)
 		
 		if(cmd==SHM_LOCK) {
 			struct user_struct * user = current->user;
-			if (!is_file_hugepages(shp->shm_file)) {
+			if (!is_shm_hugetlbfs(shp)) {
 				err = shmem_lock(shp->shm_file, 1, user);
 				if (!err) {
 					shp->shm_perm.mode |= SHM_LOCKED;
 					shp->mlock_user = user;
 				}
 			}
-		} else if (!is_file_hugepages(shp->shm_file)) {
+		} else if (!is_shm_hugetlbfs(shp)) {
 			shmem_lock(shp->shm_file, 0, shp->mlock_user);
 			shp->shm_perm.mode &= ~SHM_LOCKED;
 			shp->mlock_user = NULL;

@@ -1004,7 +1004,7 @@ static int sysvipc_shm_proc_show(struct seq_file *s, void *it)
 			  shp->shm_segsz,
 			  shp->shm_cprid,
 			  shp->shm_lprid,
-			  is_file_hugepages(shp->shm_file) ? (file_count(shp->shm_file) - 1) : shp->shm_nattch,
+			  is_shm_hugetlbfs(shp) ? (file_count(shp->shm_file) - 1) : shp->shm_nattch,
 			  shp->shm_perm.uid,
 			  shp->shm_perm.gid,
 			  shp->shm_perm.cuid,

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#4 lore raw

PATCH 4/6 hugetlb: hugetlbfs handles overcommit accounting privately

In reply to Adam Litke

In do_mmap_pgoff, is_file_hugepages() is used to determine whether the default overcommit accounting checks should be performed. The underlying question is whether the "device" handles the overcommit logic instead of using the generic logic. Add a backing_dev_info .capability flag which allows the underlying "device" to specify the answer to this question directly.

This lets us remove another call to is_file_hugepages().

Signed-off-by Adam Litke <[email protected]>

---

 fs/hugetlbfs/inode.c        |    3 ++-
 include/linux/backing-dev.h |    6 ++++++
 mm/mmap.c                   |    2 +-
 3 files changed, 9 insertions(+), 2 deletions(-)
fs/hugetlbfs/inode.c +2 -1
index c95dc47..b61592f 100644
@@ -41,7 +41,8 @@ static struct inode_operations hugetlbfs_inode_operations;
 
 static struct backing_dev_info hugetlbfs_backing_dev_info = {
 	.ra_pages	= 0,	/* No readahead */
-	.capabilities	= BDI_CAP_NO_ACCT_DIRTY | BDI_CAP_NO_WRITEBACK,
+	.capabilities	= BDI_CAP_NO_ACCT_DIRTY | BDI_CAP_NO_WRITEBACK | \
+			  BDI_CAP_PRIVATE_ACCT,
 };
 
 int sysctl_hugetlb_shm_group;
include/linux/backing-dev.h +6 -0
index 7011d62..73ef6e5 100644
@@ -50,6 +50,7 @@ struct backing_dev_info {
 #define BDI_CAP_READ_MAP	0x00000010	/* Can be mapped for reading */
 #define BDI_CAP_WRITE_MAP	0x00000020	/* Can be mapped for writing */
 #define BDI_CAP_EXEC_MAP	0x00000040	/* Can be mapped for execution */
+#define BDI_CAP_PRIVATE_ACCT	0x00000080	/* Overcommit accounting handled privately */
 #define BDI_CAP_VMFLAGS \
 	(BDI_CAP_READ_MAP | BDI_CAP_WRITE_MAP | BDI_CAP_EXEC_MAP)
 

@@ -101,11 +102,16 @@ void congestion_end(int rw);
 #define bdi_cap_account_dirty(bdi) \
 	(!((bdi)->capabilities & BDI_CAP_NO_ACCT_DIRTY))
 
+#define bdi_cap_private_acct(bdi) \
+	((bdi)->capabilities & BDI_CAP_PRIVATE_ACCT)
+
 #define mapping_cap_writeback_dirty(mapping) \
 	bdi_cap_writeback_dirty((mapping)->backing_dev_info)
 
 #define mapping_cap_account_dirty(mapping) \
 	bdi_cap_account_dirty((mapping)->backing_dev_info)
 
+#define mapping_cap_private_acct(mapping) \
+	bdi_cap_private_acct((mapping)->backing_dev_info)
 
 #endif		/* _LINUX_BACKING_DEV_H */
mm/mmap.c +1 -1
index cc3a208..a5cb0a5 100644
@@ -994,7 +994,7 @@ unsigned long do_mmap_pgoff(struct file * file, unsigned long addr,
 					return -EPERM;
 				vm_flags &= ~VM_MAYEXEC;
 			}
-			if (is_file_hugepages(file))
+			if (mapping_cap_private_acct(file->f_mapping))
 				accountable = 0;
 
 			if (!file->f_op || !file->f_op->mmap)

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#5 lore raw

PATCH 5/6 Abstract is_hugepage_only_range

In reply to Adam Litke

Some architectures define regions of the address space that can be used exclusively for either normal pages or hugetlb pages. Currently, prepare_hugepage_range() is used to validate an unmapped_area for use with hugepages and is_hugepage_only_range() is used to validate an unmapped_area for normal pages.

Introduce a prepare_unmapped_area() file operation to abstract the validation of unmapped areas. If prepare_unmapped_area() is not specified, the default behavior is to require the area to not overlap any "special" areas.

Buh-bye to another is_file_hugepages() call.

Signed-off-by Adam Litke <[email protected]>

---

 fs/hugetlbfs/inode.c |    1 +
 include/linux/fs.h   |    1 +
 mm/mmap.c            |   23 ++++++++++-------------
 3 files changed, 12 insertions(+), 13 deletions(-)
fs/hugetlbfs/inode.c +1 -0
index b61592f..3eea7a5 100644
@@ -561,6 +561,7 @@ const struct file_operations hugetlbfs_file_operations = {
 	.mmap			= hugetlbfs_file_mmap,
 	.fsync			= simple_sync_file,
 	.get_unmapped_area	= hugetlb_get_unmapped_area,
+	.prepare_unmapped_area	= prepare_hugepage_range,
 };
 
 static struct inode_operations hugetlbfs_dir_inode_operations = {
include/linux/fs.h +1 -0
index 1410e53..853a4f4 100644
@@ -1094,6 +1094,7 @@ struct file_operations {
 	ssize_t (*sendfile) (struct file *, loff_t *, size_t, read_actor_t, void *);
 	ssize_t (*sendpage) (struct file *, struct page *, int, size_t, loff_t *, int);
 	unsigned long (*get_unmapped_area)(struct file *, unsigned long, unsigned long, unsigned long, unsigned long);
+	int (*prepare_unmapped_area)(unsigned long addr, unsigned long len, pgoff_t pgoff);
 	int (*check_flags)(int);
 	int (*dir_notify)(struct file *filp, unsigned long arg);
 	int (*flock) (struct file *, int, struct file_lock *);
mm/mmap.c +10 -13
index a5cb0a5..f8e0bd0 100644
@@ -1374,20 +1374,17 @@ get_unmapped_area(struct file *file, unsigned long addr, unsigned long len,
 		return -ENOMEM;
 	if (addr & ~PAGE_MASK)
 		return -EINVAL;
-	if (file && is_file_hugepages(file))  {
-		/*
-		 * Check if the given range is hugepage aligned, and
-		 * can be made suitable for hugepages.
-		 */
-		ret = prepare_hugepage_range(addr, len, pgoff);
-	} else {
-		/*
-		 * Ensure that a normal request is not falling in a
-		 * reserved hugepage range.  For some archs like IA-64,
-		 * there is a separate region for hugepages.
-		 */
+	/*
+	 * This file may only be able to be mapped into special areas of the
+	 * addess space (eg. hugetlb pages).  If prepare_unmapped_area() is
+	 * specified, use it to validate the selected range.  If not, just
+	 * make sure the range does not overlap any special ranges.
+	 */
+	if (file && file->f_op && file->f_op->prepare_unmapped_area)
+		ret = file->f_op->prepare_unmapped_area(addr, len, pgoff);
+	else
 		ret = is_hugepage_only_range(current->mm, addr, len);
-	}
+
 	if (ret)
 		return -EINVAL;
 	return addr;

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>

author
#6 lore raw

PATCH 6/6 hugetlb: Remove is_file_hugepages()

In reply to Adam Litke

Now that they have no users, remove the is_file_hugepages() and set_file_hugepages() macros.

This unshackles the file_operations structure and permits future interfaces to hugetlb pages to define their own file_operations, distinct from those currently used by hugetlbfs.

Signed-off-by Adam Litke <[email protected]>

---

 include/linux/hugetlb.h |   12 ------------
 1 files changed, 0 insertions(+), 12 deletions(-)
include/linux/hugetlb.h +0 -12
index a184933..ec3ce89 100644
@@ -162,20 +162,8 @@ extern struct vm_operations_struct hugetlb_vm_ops;
 struct file *hugetlb_zero_setup(size_t);
 int hugetlb_get_quota(struct address_space *mapping);
 void hugetlb_put_quota(struct address_space *mapping);
-
-static inline int is_file_hugepages(struct file *file)
-{
-	return file->f_op == &hugetlbfs_file_operations;
-}
-
-static inline void set_file_hugepages(struct file *file)
-{
-	file->f_op = &hugetlbfs_file_operations;
-}
 #else /* !CONFIG_HUGETLBFS */
 
-#define is_file_hugepages(file)		0
-#define set_file_hugepages(file)	BUG()
 #define hugetlb_zero_setup(size)	ERR_PTR(-ENOSYS)
 
 #endif /* !CONFIG_HUGETLBFS */

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to [email protected]. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"[email protected]"> [email protected] </a>