[PATCH] THP: need is_vma_temporary_stack() when reference transparent_hugepage_enabled

[PATCH] THP: need is_vma_temporary_stack() when reference transparent_hugepage_enabled

[Alex Shi]

When the transparent_hugepage_enabled() used out of mm/,
is_vma_temporary_stack() need be referenced. Otherwise, it has compile
error.

Signed-off-by: Alex Shi <alex.shi@intel.com>
---
 include/linux/huge_mm.h |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index c8af7a2..10254ac 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -59,6 +59,7 @@ extern pmd_t *page_check_address_pmd(struct page *page,
 #define HPAGE_PMD_MASK HPAGE_MASK
 #define HPAGE_PMD_SIZE HPAGE_SIZE
 
+extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
 #define transparent_hugepage_enabled(__vma)				\
 	((transparent_hugepage_flags &					\
 	  (1<<TRANSPARENT_HUGEPAGE_FLAG) ||				\
-- 
1.7.5.4

Re: [PATCH] THP: need is_vma_temporary_stack() when reference transparent_hugepage_enabled

[Andrew Morton]

On Sat, 28 Apr 2012 14:33:15 +0800
Alex Shi <alex.shi@intel.com> wrote:

> When the transparent_hugepage_enabled() used out of mm/,
> is_vma_temporary_stack() need be referenced. Otherwise, it has compile
> error.

This is a poor changelog - it doesn't tell us how this compilation
error comes about.  Is there some known build error in the mainline
kernel, or did you discover this when altering the kernel, or what?

One of the several reasons for this information is to permit others to
work out which kernel version(s) should be fixed.


> diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
> index c8af7a2..10254ac 100644
> --- a/include/linux/huge_mm.h
> +++ b/include/linux/huge_mm.h
> @@ -59,6 +59,7 @@ extern pmd_t *page_check_address_pmd(struct page *page,
>  #define HPAGE_PMD_MASK HPAGE_MASK
>  #define HPAGE_PMD_SIZE HPAGE_SIZE
>  
> +extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
>  #define transparent_hugepage_enabled(__vma)				\
>  	((transparent_hugepage_flags &					\
>  	  (1<<TRANSPARENT_HUGEPAGE_FLAG) ||				\

is_vma_temporary_stack() is already declared in rmap.h.  We should not
declare it in two places.

include/linux/huge_mm.h doesn't include any headers at all.  It
is one of those files which require its user to set up the
preconditions.

So, lacking any additional infomation I'd say that your mystery build
breakage was caused by a failure to include rmap.h.

Re: [PATCH] THP: need is_vma_temporary_stack() when reference transparent_hugepage_enabled

[Alex Shi]

On 05/01/2012 07:05 AM, Andrew Morton wrote:

> On Sat, 28 Apr 2012 14:33:15 +0800
> Alex Shi <alex.shi@intel.com> wrote:
> 
>> When the transparent_hugepage_enabled() used out of mm/,
>> is_vma_temporary_stack() need be referenced. Otherwise, it has compile
>> error.
> 
> This is a poor changelog - it doesn't tell us how this compilation
> error comes about.  Is there some known build error in the mainline
> kernel, or did you discover this when altering the kernel, or what?

> 

> One of the several reasons for this information is to permit others to
> work out which kernel version(s) should be fixed.
> 




I am sorry for the unclear log!
When I try to transparent_hugepage_enabled() in arch/x86/mm/tlb.c with
huge_mm.h include: +#include <linux/huge_mm.h>. make give me the following
error:
...
  CC      arch/x86/mm/srat.o
arch/x86/mm/tlb.c: In function ‘flush_tlb_range’:
arch/x86/mm/tlb.c:324:4: error: implicit declaration of function ‘is_vma_temporary_stack’ [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors
...
Since it is not convenitant for user to include 2 head files just for one 
target function, I send this patch. 

> is_vma_temporary_stack() is already declared in rmap.h.  We should not
> declare it in two places.




Oh, yes. Since is_vma_temporay_stack is just used in rmap.c and huge_memory.c
. is it better to move it to huge_mm.h?

---
>From 7c208e10b3f4fc2f4f9c41068ea4d65a1119970e Mon Sep 17 00:00:00 2001
From: Alex Shi <alex.shi@intel.com>
Date: Wed, 2 May 2012 11:04:04 +0800
Subject: [PATCH] mm/THP: need is_vma_temporary_stack() when reference
 transparent_hugepage_enabled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When the transparent_hugepage_enabled() using out of mm/,
like in a altering arch/x86/xx/tlb.c:

+       if (!cpu_has_invlpg || vma->vm_flags & VM_HUGETLB
+                       || transparent_hugepage_enabled(vma)) {
+               flush_tlb_mm(vma->vm_mm);

is_vma_temporary_stack() isn't referenced in huge_mm.h, so it has compile
errors:
arch/x86/mm/tlb.c: In function ‘flush_tlb_range’:
arch/x86/mm/tlb.c:324:4: error: implicit declaration of function ‘is_vma_temporary_stack’ [-Werror=implicit-function-declaration]
cc1: some warnings being treated as errors

Since is_vma_temporay_stack() is just used in rmap.c and huge_memory.c.
It is better to move it to huge_mm.h from rmap.h to avoid such error.

Signed-off-by: Alex Shi <alex.shi@intel.com>
---
 include/linux/huge_mm.h |    2 ++
 include/linux/rmap.h    |    2 --
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/huge_mm.h b/include/linux/huge_mm.h
index 1b92129..acf3ab1 100644
--- a/include/linux/huge_mm.h
+++ b/include/linux/huge_mm.h
@@ -56,6 +56,8 @@ extern pmd_t *page_check_address_pmd(struct page *page,
 #define HPAGE_PMD_MASK HPAGE_MASK
 #define HPAGE_PMD_SIZE HPAGE_SIZE
 
+extern bool is_vma_temporary_stack(struct vm_area_struct *vma);
+
 #define transparent_hugepage_enabled(__vma)				\
 	((transparent_hugepage_flags &					\
 	  (1<<TRANSPARENT_HUGEPAGE_FLAG) ||				\
diff --git a/include/linux/rmap.h b/include/linux/rmap.h
index 1cdd62a..267fb6b 100644
--- a/include/linux/rmap.h
+++ b/include/linux/rmap.h
@@ -174,8 +174,6 @@ enum ttu_flags {
 };
 #define TTU_ACTION(x) ((x) & TTU_ACTION_MASK)
 
-bool is_vma_temporary_stack(struct vm_area_struct *vma);
-
 int try_to_unmap(struct page *, enum ttu_flags flags);
 int try_to_unmap_one(struct page *, struct vm_area_struct *,
 			unsigned long address, enum ttu_flags flags);
-- 
1.7.5.1

Re: [PATCH] THP: need is_vma_temporary_stack() when reference transparent_hugepage_enabled

[Alex Shi]

>

> Oh, yes. Since is_vma_temporay_stack is just used in rmap.c and huge_memory.c
> . is it better to move it to huge_mm.h?



If you still concern someone just want to use is_vma_temporay_stack
function. I have no better idea. and please drop this patch if so. :)

Re: [PATCH] THP: need is_vma_temporary_stack() when reference transparent_hugepage_enabled

[Andrea Arcangeli]

On Wed, May 02, 2012 at 11:17:23AM +0800, Alex Shi wrote:
> On 05/01/2012 07:05 AM, Andrew Morton wrote:
> 
> > On Sat, 28 Apr 2012 14:33:15 +0800
> > Alex Shi <alex.shi@intel.com> wrote:
> > 
> >> When the transparent_hugepage_enabled() used out of mm/,
> >> is_vma_temporary_stack() need be referenced. Otherwise, it has compile
> >> error.
> > 
> > This is a poor changelog - it doesn't tell us how this compilation
> > error comes about.  Is there some known build error in the mainline
> > kernel, or did you discover this when altering the kernel, or what?
> 
> > 
> 
> > One of the several reasons for this information is to permit others to
> > work out which kernel version(s) should be fixed.
> > 

I wanted to ask the same question.

> I am sorry for the unclear log!
> When I try to transparent_hugepage_enabled() in arch/x86/mm/tlb.c with
> huge_mm.h include: +#include <linux/huge_mm.h>. make give me the following
> error:

I already guessed it was for development code out of tree code but I
wasn't sure, thanks for the clarification.

> ...
>   CC      arch/x86/mm/srat.o
> arch/x86/mm/tlb.c: In function ‘flush_tlb_range’:
> arch/x86/mm/tlb.c:324:4: error: implicit declaration of function ‘is_vma_temporary_stack’ [-Werror=implicit-function-declaration]
> cc1: some warnings being treated as errors
> ...
> Since it is not convenitant for user to include 2 head files just for one 
> target function, I send this patch. 
> 
> > is_vma_temporary_stack() is already declared in rmap.h.  We should not
> > declare it in two places.

My preference would still be to remove the is_vma_temporary_stack and
use two vmas during mremap of execve, that would remove the "vma"
parameter from transparent_hugepage_enabled() but others prefers to
skip a vma allocation in execve and stick to is_vma_temporary_stack,
which is fair enough argument.

> Oh, yes. Since is_vma_temporay_stack is just used in rmap.c and huge_memory.c
> . is it better to move it to huge_mm.h?

I guess it was cleaner on rmap.h as this is not related to THP, but
clearly it works better in huge_mm.h (and rmap.h->mm.h->huge_mm.h is
included automatically) so the patch looks fine to me.

Thanks,
Andrea

Re: [PATCH] THP: need is_vma_temporary_stack() when reference transparent_hugepage_enabled

[Alex Shi]

> My preference would still be to remove the is_vma_temporary_stack and
> use two vmas during mremap of execve, that would remove the "vma"
> parameter from transparent_hugepage_enabled() but others prefers to
> skip a vma allocation in execve and stick to is_vma_temporary_stack,
> which is fair enough argument.


Actually, current transparent_hugepage_enabled just means the vma is in
THP enable ENV, the vma is just possibly has some large page, no grantee
really has.  But in lots situations, user wants to know if a vma or a
part of memory really include a large page. not the possibility.

So, it will be great to see a real large page checking function appearing.

> 
>> Oh, yes. Since is_vma_temporay_stack is just used in rmap.c and huge_memory.c
>> . is it better to move it to huge_mm.h?
> 
> I guess it was cleaner on rmap.h as this is not related to THP, but
> clearly it works better in huge_mm.h (and rmap.h->mm.h->huge_mm.h is
> included automatically) so the patch looks fine to me.


Thanks for point out.

Re: [PATCH] THP: need is_vma_temporary_stack() when reference transparent_hugepage_enabled

[Andrea Arcangeli]

Hi,

On Thu, May 03, 2012 at 08:56:57AM +0800, Alex Shi wrote:
> 
> > My preference would still be to remove the is_vma_temporary_stack and
> > use two vmas during mremap of execve, that would remove the "vma"
> > parameter from transparent_hugepage_enabled() but others prefers to
> > skip a vma allocation in execve and stick to is_vma_temporary_stack,
> > which is fair enough argument.
> 
> 
> Actually, current transparent_hugepage_enabled just means the vma is in
> THP enable ENV, the vma is just possibly has some large page, no grantee
> really has.  But in lots situations, user wants to know if a vma or a
> part of memory really include a large page. not the possibility.
> 
> So, it will be great to see a real large page checking function appearing.

Well, to know if a VMA (or a memory range) really includes a THP, it'd
require to hold the page_table_lock and a loop on all pmds in the
range, but by the time you relase the lock things may have already
changed as split_huge_page can run at any time, madvise(MADV_DONTNEED)
too if you only hold the mmap_sem in read mode and the THP page
fault. In fact while holding the mmap_sem in read mode (the usual read
lock you need to take to lookup and stabilize the vma) a THP can be
freed and reallocated under it, and that's what pmd_trans_unstable is
about.

Re: [PATCH] THP: need is_vma_temporary_stack() when reference transparent_hugepage_enabled

[Alex Shi]

On 05/03/2012 07:25 PM, Andrea Arcangeli wrote:

> Hi,
> 
> On Thu, May 03, 2012 at 08:56:57AM +0800, Alex Shi wrote:
>>
>>> My preference would still be to remove the is_vma_temporary_stack and
>>> use two vmas during mremap of execve, that would remove the "vma"
>>> parameter from transparent_hugepage_enabled() but others prefers to
>>> skip a vma allocation in execve and stick to is_vma_temporary_stack,
>>> which is fair enough argument.
>>
>>
>> Actually, current transparent_hugepage_enabled just means the vma is in
>> THP enable ENV, the vma is just possibly has some large page, no grantee
>> really has.  But in lots situations, user wants to know if a vma or a
>> part of memory really include a large page. not the possibility.
>>
>> So, it will be great to see a real large page checking function appearing.
> 
> Well, to know if a VMA (or a memory range) really includes a THP, it'd
> require to hold the page_table_lock and a loop on all pmds in the
> range, but by the time you relase the lock things may have already
> changed as split_huge_page can run at any time, madvise(MADV_DONTNEED)
> too if you only hold the mmap_sem in read mode and the THP page
> fault. In fact while holding the mmap_sem in read mode (the usual read
> lock you need to take to lookup and stabilize the vma) a THP can be
> freed and reallocated under it, and that's what pmd_trans_unstable is
> about.


Appreciate for explanation!