[PATCH 1/3] Identification of huge pages mapping (Take 3)

[PATCH 1/3] Identification of huge pages mapping (Take 3)

[Alexey Korolev]

This patch changes a little bit the procedures of huge pages file
identification. We need this because we may have huge page mapping for
files which are not on hugetlbfs (the same case in ipc/shm.c).
Just file operations check will not work as drivers should have own
file operations. So if we need to identify if file has huge pages
mapping, we need to check the file mapping flags.
New identification procedure obsoletes existing workaround for hugetlb
file identification in ipc/shm.c
Also having huge page mapping for files which are not on hugetlbfs do
not allow us to get hstate based on file dentry, we need to be based
on file mapping instead.

fs/hugetlbfs/inode.c    |    1 +
include/linux/hugetlb.h |   15 ++-------------
include/linux/pagemap.h |   13 +++++++++++++
ipc/shm.c               |   12 ------------
4 files changed, 16 insertions(+), 25 deletions(-)

---
Signed-off-by: Alexey Korolev <akorolev@infradead.org>

diff -aurp clean/fs/hugetlbfs/inode.c patched/fs/hugetlbfs/inode.c
--- clean/fs/hugetlbfs/inode.c	2009-09-10 17:48:38.000000000 +1200
+++ patched/fs/hugetlbfs/inode.c	2009-09-11 15:12:17.000000000 +1200
@@ -521,6 +521,7 @@ static struct inode *hugetlbfs_get_inode
 		case S_IFREG:
 			inode->i_op = &hugetlbfs_inode_operations;
 			inode->i_fop = &hugetlbfs_file_operations;
+			mapping_set_hugetlb(inode->i_mapping);
 			break;
 		case S_IFDIR:
 			inode->i_op = &hugetlbfs_dir_inode_operations;
diff -aurp clean/include/linux/hugetlb.h patched/include/linux/hugetlb.h
--- clean/include/linux/hugetlb.h	2009-09-10 17:48:28.000000000 +1200
+++ patched/include/linux/hugetlb.h	2009-09-11 15:15:30.000000000 +1200
@@ -169,22 +169,11 @@ void hugetlb_put_quota(struct address_sp

 static inline int is_file_hugepages(struct file *file)
 {
-	if (file->f_op == &hugetlbfs_file_operations)
-		return 1;
-	if (is_file_shm_hugepages(file))
-		return 1;
-
-	return 0;
-}
-
-static inline void set_file_hugepages(struct file *file)
-{
-	file->f_op = &hugetlbfs_file_operations;
+	return mapping_hugetlb(file->f_mapping);
 }
 #else /* !CONFIG_HUGETLBFS */

 #define is_file_hugepages(file)			0
-#define set_file_hugepages(file)		BUG()
 #define hugetlb_file_setup(name,size,acct,user,creat)	ERR_PTR(-ENOSYS)

 #endif /* !CONFIG_HUGETLBFS */
@@ -245,7 +234,7 @@ static inline struct hstate *hstate_inod

 static inline struct hstate *hstate_file(struct file *f)
 {
-	return hstate_inode(f->f_dentry->d_inode);
+	return hstate_inode(f->f_mapping->host);
 }

 static inline struct hstate *hstate_vma(struct vm_area_struct *vma)
diff -aurp clean/include/linux/pagemap.h patched/include/linux/pagemap.h
--- clean/include/linux/pagemap.h	2009-09-06 11:38:12.000000000 +1200
+++ patched/include/linux/pagemap.h	2009-09-11 15:17:04.000000000 +1200
@@ -23,6 +23,7 @@ enum mapping_flags {
 	AS_ENOSPC	= __GFP_BITS_SHIFT + 1,	/* ENOSPC on async write */
 	AS_MM_ALL_LOCKS	= __GFP_BITS_SHIFT + 2,	/* under mm_take_all_locks() */
 	AS_UNEVICTABLE	= __GFP_BITS_SHIFT + 3,	/* e.g., ramdisk, SHM_LOCK */
+	AS_HUGETLB	= __GFP_BITS_SHIFT + 4,	/* under HUGE TLB */
 };

 static inline void mapping_set_error(struct address_space *mapping, int error)
@@ -52,6 +53,18 @@ static inline int mapping_unevictable(st
 	return !!mapping;
 }

+static inline void mapping_set_hugetlb(struct address_space *mapping)
+{
+	set_bit(AS_HUGETLB, &mapping->flags);
+}
+
+static inline int mapping_hugetlb(struct address_space *mapping)
+{
+	if (likely(mapping))
+		return test_bit(AS_HUGETLB, &mapping->flags);
+	return 0;
+}
+
 static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
 {
 	return (__force gfp_t)mapping->flags & __GFP_BITS_MASK;
diff -aurp clean/ipc/shm.c patched/ipc/shm.c
--- clean/ipc/shm.c	2009-09-10 17:48:23.000000000 +1200
+++ patched/ipc/shm.c	2009-09-11 15:17:04.000000000 +1200
@@ -293,18 +293,6 @@ static unsigned long shm_get_unmapped_ar
 	return get_unmapped_area(sfd->file, addr, len, pgoff, flags);
 }

-int is_file_shm_hugepages(struct file *file)
-{
-	int ret = 0;
-
-	if (file->f_op == &shm_file_operations) {
-		struct shm_file_data *sfd;
-		sfd = shm_file_data(file);
-		ret = is_file_hugepages(sfd->file);
-	}
-	return ret;
-}
-
 static const struct file_operations shm_file_operations = {
 	.mmap		= shm_mmap,
 	.fsync		= shm_fsync,

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

Re: [PATCH 1/3] Identification of huge pages mapping (Take 3)

[Mel Gorman]

I suggest a subject change to

"Identify huge page mappings from address_space->flags instead of file_operations comparison"

for the purposes of having an easier-to-understand changelog.


On Mon, Sep 14, 2009 at 05:16:13PM +1200, Alexey Korolev wrote:
> This patch changes a little bit the procedures of huge pages file
> identification. We need this because we may have huge page mapping for
> files which are not on hugetlbfs (the same case in ipc/shm.c).

Is this strictly-speaking true as there is still a file on hugetlbfs for
the driver? Maybe something like

This patch identifies whether a mapping uses huge pages based on the
address_space flags instead of the file operations. A later patch allows
a driver to manage an underlying hugetlbfs file while exposing it via a
different file_operations structure.

I haven't read the rest of the series yet so take the suggestion with a
grain of salt.

> Just file operations check will not work as drivers should have own
> file operations. So if we need to identify if file has huge pages
> mapping, we need to check the file mapping flags.
> New identification procedure obsoletes existing workaround for hugetlb
> file identification in ipc/shm.c
> Also having huge page mapping for files which are not on hugetlbfs do
> not allow us to get hstate based on file dentry, we need to be based
> on file mapping instead.
> 

Can you clarify this a bit more? I think the reasoning is as follows but
confirmation would be nice.

"As part of this, the hstate for a given file as implemented by hstate_file()
must be based on file mapping instead of dentry. Even if a driver is
maintaining an underlying hugetlbfs file, the mmap() operation is still
taking place on a device-specific file. That dentry is unlikely to be on
a hugetlbfs file. A device driver must ensure that file->f_mapping->host
resolves correctly."

If this is accurate, a comment in hstate_file() wouldn't hurt in case
someone later decides that dentry really was the way to go.

> fs/hugetlbfs/inode.c    |    1 +
> include/linux/hugetlb.h |   15 ++-------------
> include/linux/pagemap.h |   13 +++++++++++++
> ipc/shm.c               |   12 ------------
> 4 files changed, 16 insertions(+), 25 deletions(-)
> 
> ---
> Signed-off-by: Alexey Korolev <akorolev@infradead.org>
> 
> diff -aurp clean/fs/hugetlbfs/inode.c patched/fs/hugetlbfs/inode.c
> --- clean/fs/hugetlbfs/inode.c	2009-09-10 17:48:38.000000000 +1200
> +++ patched/fs/hugetlbfs/inode.c	2009-09-11 15:12:17.000000000 +1200
> @@ -521,6 +521,7 @@ static struct inode *hugetlbfs_get_inode
>  		case S_IFREG:
>  			inode->i_op = &hugetlbfs_inode_operations;
>  			inode->i_fop = &hugetlbfs_file_operations;
> +			mapping_set_hugetlb(inode->i_mapping);
>  			break;
>  		case S_IFDIR:
>  			inode->i_op = &hugetlbfs_dir_inode_operations;
> diff -aurp clean/include/linux/hugetlb.h patched/include/linux/hugetlb.h
> --- clean/include/linux/hugetlb.h	2009-09-10 17:48:28.000000000 +1200
> +++ patched/include/linux/hugetlb.h	2009-09-11 15:15:30.000000000 +1200
> @@ -169,22 +169,11 @@ void hugetlb_put_quota(struct address_sp
> 
>  static inline int is_file_hugepages(struct file *file)
>  {
> -	if (file->f_op == &hugetlbfs_file_operations)
> -		return 1;
> -	if (is_file_shm_hugepages(file))
> -		return 1;
> -
> -	return 0;
> -}
> -
> -static inline void set_file_hugepages(struct file *file)
> -{
> -	file->f_op = &hugetlbfs_file_operations;
> +	return mapping_hugetlb(file->f_mapping);
>  }
>  #else /* !CONFIG_HUGETLBFS */
> 
>  #define is_file_hugepages(file)			0
> -#define set_file_hugepages(file)		BUG()
>  #define hugetlb_file_setup(name,size,acct,user,creat)	ERR_PTR(-ENOSYS)
> 

Why do you remove this BUG()? It still seems to be a valid check.

>  #endif /* !CONFIG_HUGETLBFS */
> @@ -245,7 +234,7 @@ static inline struct hstate *hstate_inod
> 
>  static inline struct hstate *hstate_file(struct file *f)
>  {
> -	return hstate_inode(f->f_dentry->d_inode);
> +	return hstate_inode(f->f_mapping->host);
>  }
> 
>  static inline struct hstate *hstate_vma(struct vm_area_struct *vma)
> diff -aurp clean/include/linux/pagemap.h patched/include/linux/pagemap.h
> --- clean/include/linux/pagemap.h	2009-09-06 11:38:12.000000000 +1200
> +++ patched/include/linux/pagemap.h	2009-09-11 15:17:04.000000000 +1200
> @@ -23,6 +23,7 @@ enum mapping_flags {
>  	AS_ENOSPC	= __GFP_BITS_SHIFT + 1,	/* ENOSPC on async write */
>  	AS_MM_ALL_LOCKS	= __GFP_BITS_SHIFT + 2,	/* under mm_take_all_locks() */
>  	AS_UNEVICTABLE	= __GFP_BITS_SHIFT + 3,	/* e.g., ramdisk, SHM_LOCK */
> +	AS_HUGETLB	= __GFP_BITS_SHIFT + 4,	/* under HUGE TLB */
>  };
> 
>  static inline void mapping_set_error(struct address_space *mapping, int error)
> @@ -52,6 +53,18 @@ static inline int mapping_unevictable(st
>  	return !!mapping;
>  }
> 
> +static inline void mapping_set_hugetlb(struct address_space *mapping)
> +{
> +	set_bit(AS_HUGETLB, &mapping->flags);
> +}
> +
> +static inline int mapping_hugetlb(struct address_space *mapping)
> +{
> +	if (likely(mapping))
> +		return test_bit(AS_HUGETLB, &mapping->flags);
> +	return 0;
> +}

Is mapping_hugetlb necessary? Why not just make that the implementation
of is_file_hugepages()

> +
>  static inline gfp_t mapping_gfp_mask(struct address_space * mapping)
>  {
>  	return (__force gfp_t)mapping->flags & __GFP_BITS_MASK;
> diff -aurp clean/ipc/shm.c patched/ipc/shm.c
> --- clean/ipc/shm.c	2009-09-10 17:48:23.000000000 +1200
> +++ patched/ipc/shm.c	2009-09-11 15:17:04.000000000 +1200
> @@ -293,18 +293,6 @@ static unsigned long shm_get_unmapped_ar
>  	return get_unmapped_area(sfd->file, addr, len, pgoff, flags);
>  }
> 
> -int is_file_shm_hugepages(struct file *file)
> -{
> -	int ret = 0;
> -
> -	if (file->f_op == &shm_file_operations) {
> -		struct shm_file_data *sfd;
> -		sfd = shm_file_data(file);
> -		ret = is_file_hugepages(sfd->file);
> -	}
> -	return ret;
> -}

What about the declarations and definitions in include/linux/shm.h?

> -
>  static const struct file_operations shm_file_operations = {
>  	.mmap		= shm_mmap,
>  	.fsync		= shm_fsync,
> 

Still some ironing to do but I think this part of the series is getting
there.

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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

Re: [PATCH 1/3] Identification of huge pages mapping (Take 3)

[Alexey Korolev]

Mel,

> I suggest a subject change to
>
> "Identify huge page mappings from address_space->flags instead of file_operations comparison"
>
> for the purposes of having an easier-to-understand changelog.
>
Yes. It is a bit longer but it is definitely clear. Will be corrected.

> On Mon, Sep 14, 2009 at 05:16:13PM +1200, Alexey Korolev wrote:
>> This patch changes a little bit the procedures of huge pages file
>> identification. We need this because we may have huge page mapping for
>> files which are not on hugetlbfs (the same case in ipc/shm.c).
>
> Is this strictly-speaking true as there is still a file on hugetlbfs for
> the driver? Maybe something like
>
> This patch identifies whether a mapping uses huge pages based on the
> address_space flags instead of the file operations. A later patch allows
> a driver to manage an underlying hugetlbfs file while exposing it via a
> different file_operations structure.
>
> I haven't read the rest of the series yet so take the suggestion with a
> grain of salt.

You understood properly. Thanks for the comments. I need to work on
the description more, it seems not to be completely clear.

>> Just file operations check will not work as drivers should have own
>> file operations. So if we need to identify if file has huge pages
>> mapping, we need to check the file mapping flags.
>> New identification procedure obsoletes existing workaround for hugetlb
>> file identification in ipc/shm.c
>> Also having huge page mapping for files which are not on hugetlbfs do
>> not allow us to get hstate based on file dentry, we need to be based
>> on file mapping instead.
>
> Can you clarify this a bit more? I think the reasoning is as follows but
> confirmation would be nice.
>
> "As part of this, the hstate for a given file as implemented by hstate_file()
> must be based on file mapping instead of dentry. Even if a driver is
> maintaining an underlying hugetlbfs file, the mmap() operation is still
> taking place on a device-specific file. That dentry is unlikely to be on
> a hugetlbfs file. A device driver must ensure that file->f_mapping->host
> resolves correctly."
>
> If this is accurate, a comment in hstate_file() wouldn't hurt in case
> someone later decides that dentry really was the way to go.
>
Right. Getting hstate via mapping instead of dentry is important here, so it is
necessary to add a comment in order to prevent people breaking this.
A comment will be added.

>>
>>  static inline int is_file_hugepages(struct file *file)
>>  {
>> -     if (file->f_op == &hugetlbfs_file_operations)
>> -             return 1;
>> -     if (is_file_shm_hugepages(file))
>> -             return 1;
>> -
>> -     return 0;
>> -}
>> -
>> -static inline void set_file_hugepages(struct file *file)
>> -{
>> -     file->f_op = &hugetlbfs_file_operations;
>> +     return mapping_hugetlb(file->f_mapping);
>>  }
>>  #else /* !CONFIG_HUGETLBFS */
>>
>>  #define is_file_hugepages(file)                      0
>> -#define set_file_hugepages(file)             BUG()
>>  #define hugetlb_file_setup(name,size,acct,user,creat)        ERR_PTR(-ENOSYS)
>>
>
> Why do you remove this BUG()? It still seems to be a valid check.
I removed this function - because it has not been called since 2.6.15 and
it is confusing the user a bit after applying new changes. I think it
was necessary to write about this little change in description, sorry
about that.
>>
>> +static inline void mapping_set_hugetlb(struct address_space *mapping)
>> +{
>> +     set_bit(AS_HUGETLB, &mapping->flags);
>> +}
>> +
>> +static inline int mapping_hugetlb(struct address_space *mapping)
>> +{
>> +     if (likely(mapping))
>> +             return test_bit(AS_HUGETLB, &mapping->flags);
>> +     return 0;
>> +}
>
> Is mapping_hugetlb necessary? Why not just make that the implementation
> of is_file_hugepages()
No. It is not necessary. The reason I wrote these functions is just
there are the
similar function for other mapping flags. I see no problem to have
only: is_file_hugepages and
set_file_huge_pages in hugetlb.h instead of mapping_set_hugetlb and
mapping_hugetlb.

>> -     if (file->f_op == &shm_file_operations) {
>> -             struct shm_file_data *sfd;
>> -             sfd = shm_file_data(file);
>> -             ret = is_file_hugepages(sfd->file);
>> -     }
>> -     return ret;
>> -}
>
> What about the declarations and definitions in include/linux/shm.h?

Ahh. Thank you! Will be fixed.

Thanks,
Alexey

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

Re: [PATCH 1/3] Identification of huge pages mapping (Take 3)

[Mel Gorman]

On Thu, Sep 17, 2009 at 12:11:33AM +1200, Alexey Korolev wrote:
> Mel,
> 
> > I suggest a subject change to
> >
> > "Identify huge page mappings from address_space->flags instead of file_operations comparison"
> >
> > for the purposes of having an easier-to-understand changelog.
> >
>
> Yes. It is a bit longer but it is definitely clear. Will be corrected.
> 

Thanks

> > On Mon, Sep 14, 2009 at 05:16:13PM +1200, Alexey Korolev wrote:
> >> This patch changes a little bit the procedures of huge pages file
> >> identification. We need this because we may have huge page mapping for
> >> files which are not on hugetlbfs (the same case in ipc/shm.c).
> >
> > Is this strictly-speaking true as there is still a file on hugetlbfs for
> > the driver? Maybe something like
> >
> > This patch identifies whether a mapping uses huge pages based on the
> > address_space flags instead of the file operations. A later patch allows
> > a driver to manage an underlying hugetlbfs file while exposing it via a
> > different file_operations structure.
> >
> > I haven't read the rest of the series yet so take the suggestion with a
> > grain of salt.
> 
> You understood properly. Thanks for the comments. I need to work on
> the description more, it seems not to be completely clear.
> 
> >> Just file operations check will not work as drivers should have own
> >> file operations. So if we need to identify if file has huge pages
> >> mapping, we need to check the file mapping flags.
> >> New identification procedure obsoletes existing workaround for hugetlb
> >> file identification in ipc/shm.c
> >> Also having huge page mapping for files which are not on hugetlbfs do
> >> not allow us to get hstate based on file dentry, we need to be based
> >> on file mapping instead.
> >
> > Can you clarify this a bit more? I think the reasoning is as follows but
> > confirmation would be nice.
> >
> > "As part of this, the hstate for a given file as implemented by hstate_file()
> > must be based on file mapping instead of dentry. Even if a driver is
> > maintaining an underlying hugetlbfs file, the mmap() operation is still
> > taking place on a device-specific file. That dentry is unlikely to be on
> > a hugetlbfs file. A device driver must ensure that file->f_mapping->host
> > resolves correctly."
> >
> > If this is accurate, a comment in hstate_file() wouldn't hurt in case
> > someone later decides that dentry really was the way to go.
> >
>
> Right. Getting hstate via mapping instead of dentry is important here, so it is
> necessary to add a comment in order to prevent people breaking this.
> A comment will be added.
> 

Thanks

> >>
> >>  static inline int is_file_hugepages(struct file *file)
> >>  {
> >> -     if (file->f_op == &hugetlbfs_file_operations)
> >> -             return 1;
> >> -     if (is_file_shm_hugepages(file))
> >> -             return 1;
> >> -
> >> -     return 0;
> >> -}
> >> -
> >> -static inline void set_file_hugepages(struct file *file)
> >> -{
> >> -     file->f_op = &hugetlbfs_file_operations;
> >> +     return mapping_hugetlb(file->f_mapping);
> >>  }
> >>  #else /* !CONFIG_HUGETLBFS */
> >>
> >>  #define is_file_hugepages(file)                      0
> >> -#define set_file_hugepages(file)             BUG()
> >>  #define hugetlb_file_setup(name,size,acct,user,creat)        ERR_PTR(-ENOSYS)
> >>
> >
> > Why do you remove this BUG()? It still seems to be a valid check.
>
> I removed this function - because it has not been called since 2.6.15 and
> it is confusing the user a bit after applying new changes. I think it
> was necessary to write about this little change in description, sorry
> about that.

If it's really confusing, it should be a separate patch. It doesn't need to
be folded into this one.

> >>
> >> +static inline void mapping_set_hugetlb(struct address_space *mapping)
> >> +{
> >> +     set_bit(AS_HUGETLB, &mapping->flags);
> >> +}
> >> +
> >> +static inline int mapping_hugetlb(struct address_space *mapping)
> >> +{
> >> +     if (likely(mapping))
> >> +             return test_bit(AS_HUGETLB, &mapping->flags);
> >> +     return 0;
> >> +}
> >
> > Is mapping_hugetlb necessary? Why not just make that the implementation
> > of is_file_hugepages()
>
> No. It is not necessary. The reason I wrote these functions is just
> there are the
> similar function for other mapping flags. I see no problem to have
> only: is_file_hugepages and
> set_file_huge_pages in hugetlb.h instead of mapping_set_hugetlb and
> mapping_hugetlb.
> 

I'd also be ok with you converting is_file_hugepages and set_file_huge_pages()
to mapping_set_hugetlb and mapping_hugetlb to bring hugetlb functions more
in line with the core VM.

> >> -     if (file->f_op == &shm_file_operations) {
> >> -             struct shm_file_data *sfd;
> >> -             sfd = shm_file_data(file);
> >> -             ret = is_file_hugepages(sfd->file);
> >> -     }
> >> -     return ret;
> >> -}
> >
> > What about the declarations and definitions in include/linux/shm.h?
> 
> Ahh. Thank you! Will be fixed.
> 
> Thanks,
> Alexey
> 

-- 
Mel Gorman
Part-time Phd Student                          Linux Technology Center
University of Limerick                         IBM Dublin Software Lab

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