[PATCH][mm/memory.c]: transparent hugepage check condition missed

[PATCH][mm/memory.c]: transparent hugepage check condition missed

[GuanJun He]

For the transparent hugepage module still does not support
tmpfs and cache,the check condition should always be checked
to make sure that it only affect the anonymous maps, the
original check condition missed this, this patch is to fix this.
Otherwise,the hugepage may affect the file-backed maps,
then the cache for the small-size pages will be unuseful,
and till now there is still no implementation for hugepage's cache.

Signed-off-by: Guanjun He <gjhe@suse.com>
---
 mm/memory.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index a56e3ba..79b85fe 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3475,7 +3475,8 @@ int handle_mm_fault(struct mm_struct *mm, struct
vm_area_struct *vma,
 		if (pmd_trans_huge(orig_pmd)) {
 			if (flags & FAULT_FLAG_WRITE &&
 			    !pmd_write(orig_pmd) &&
-			    !pmd_trans_splitting(orig_pmd))
+			    !pmd_trans_splitting(orig_pmd) &&
+			    !vma->vm_ops)
 				return do_huge_pmd_wp_page(mm, vma, address,
 							   pmd, orig_pmd);
 			return 0;
--

Re: [PATCH][mm/memory.c]: transparent hugepage check condition missed

[Andrea Arcangeli]

Hi,

On Mon, Oct 31, 2011 at 06:56:57PM +0800, GuanJun He wrote:
> For the transparent hugepage module still does not support
> tmpfs and cache,the check condition should always be checked
> to make sure that it only affect the anonymous maps, the
> original check condition missed this, this patch is to fix this.
> Otherwise,the hugepage may affect the file-backed maps,
> then the cache for the small-size pages will be unuseful,
> and till now there is still no implementation for hugepage's cache.
> 
> Signed-off-by: Guanjun He <gjhe@suse.com>
> ---
>  mm/memory.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a56e3ba..79b85fe 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3475,7 +3475,8 @@ int handle_mm_fault(struct mm_struct *mm, struct
> vm_area_struct *vma,
>  		if (pmd_trans_huge(orig_pmd)) {
>  			if (flags & FAULT_FLAG_WRITE &&
>  			    !pmd_write(orig_pmd) &&
> -			    !pmd_trans_splitting(orig_pmd))
> +			    !pmd_trans_splitting(orig_pmd) &&
> +			    !vma->vm_ops)
>  				return do_huge_pmd_wp_page(mm, vma, address,
>  							   pmd, orig_pmd);
>  			return 0;

Not needed, this is the cow case and pmd_trans_huge can't be true if
vm_ops is not null. That's already checked a few lines before the
above code and in collapse_huge_page for khugepaged.

Thanks.

Re: [PATCH][mm/memory.c]: transparent hugepage check condition missed

[Shaohua Li]

On Mon, 2011-10-31 at 16:23 +0800, Guanjun He wrote:
> For the transparent hugepage module still does not support
> tmpfs and cache,the check condition should always be checked 
> to make sure that it only affect the anonymous maps, the 
> original check condition missed this, this patch is to fix this.
> Otherwise,the hugepage may affect the file-backed maps,
> then the cache for the small-size pages will be unuseful,
> and till now there is still no implementation for hugepage's cache.
> 
> Signed-off-by: Guanjun He <gjhe@suse.com>
> ---
>  mm/memory.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a56e3ba..79b85fe 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3475,7 +3475,8 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  		if (pmd_trans_huge(orig_pmd)) {
>  			if (flags & FAULT_FLAG_WRITE &&
>  			    !pmd_write(orig_pmd) &&
> -			    !pmd_trans_splitting(orig_pmd))
> +			    !pmd_trans_splitting(orig_pmd) &&
> +			    !vma->vm_ops)
>  				return do_huge_pmd_wp_page(mm, vma, address,
>  							   pmd, orig_pmd);
>  			return 0;
so if vma->vm_ops != NULL, how could the pmd_trans_huge(orig_pmd) be
true? We never enable THP if vma->vm_ops != NULL.

Re: [PATCH][mm/memory.c]: transparent hugepage check condition missed

[Guan Jun He]

>>> On 11/1/2011 at 09:18 AM, in message <1320110288.22361.190.camel@sli10-conroe>,
Shaohua Li <shaohua.li@intel.com> wrote: 
> On Mon, 2011-10-31 at 16:23 +0800, Guanjun He wrote:
>> For the transparent hugepage module still does not support
>> tmpfs and cache,the check condition should always be checked 
>> to make sure that it only affect the anonymous maps, the 
>> original check condition missed this, this patch is to fix this.
>> Otherwise,the hugepage may affect the file-backed maps,
>> then the cache for the small-size pages will be unuseful,
>> and till now there is still no implementation for hugepage's cache.
>> 
>> Signed-off-by: Guanjun He <gjhe@suse.com>
>> ---
>>  mm/memory.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>> 
>> diff --git a/mm/memory.c b/mm/memory.c
>> index a56e3ba..79b85fe 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3475,7 +3475,8 @@ int handle_mm_fault(struct mm_struct *mm, struct 
> vm_area_struct *vma,
>>  		if (pmd_trans_huge(orig_pmd)) {
>>  			if (flags & FAULT_FLAG_WRITE &&
>>  			    !pmd_write(orig_pmd) &&
>> -			    !pmd_trans_splitting(orig_pmd))
>> +			    !pmd_trans_splitting(orig_pmd) &&
>> +			    !vma->vm_ops)
>>  				return do_huge_pmd_wp_page(mm, vma, address,
>>  							   pmd, orig_pmd);
>>  			return 0;
> so if vma->vm_ops != NULL, how could the pmd_trans_huge(orig_pmd) be
> true? We never enable THP if vma->vm_ops != NULL.
acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits, 
it's only a pagesize, not a flag to identity a hugepage.
If I change my default pagesize to PAGE_PSE, then THP will be confused.

There is already a defination:

#define VM_HUGEPAGE	0x01000000	/* MADV_HUGEPAGE marked this vma */

maybe,this can be the flag to identity a hugepage.But the comment marked it only stands for MADV_HUGEPAGE,
and it's still a hugepage.So, I suggest to add the check condition !vma->vm_ops, or turn to 
use VM_HUGEPAGE as the flag.
or
adjust the logic to:
(transparent_hugepage_enabled() use the VM_HUGEPAGE flag)

 if(transparent_hugepage_enabled(vma)){
    if (pmd_none(*pmd){
        ...
    }
    else
    {
      ...
    }
}


the original logic is:

if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) 
{
  ...
}
else
{
  ...
}

Re: [PATCH][mm/memory.c]: transparent hugepage check condition missed

[Shaohua Li]

2011/11/1 Guan Jun He <gjhe@suse.com>:
>
>
>>>> On 11/1/2011 at 09:18 AM, in message <1320110288.22361.190.camel@sli10-conroe>,
> Shaohua Li <shaohua.li@intel.com> wrote:
>> On Mon, 2011-10-31 at 16:23 +0800, Guanjun He wrote:
>>> For the transparent hugepage module still does not support
>>> tmpfs and cache,the check condition should always be checked
>>> to make sure that it only affect the anonymous maps, the
>>> original check condition missed this, this patch is to fix this.
>>> Otherwise,the hugepage may affect the file-backed maps,
>>> then the cache for the small-size pages will be unuseful,
>>> and till now there is still no implementation for hugepage's cache.
>>>
>>> Signed-off-by: Guanjun He <gjhe@suse.com>
>>> ---
>>>  mm/memory.c |    3 ++-
>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index a56e3ba..79b85fe 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -3475,7 +3475,8 @@ int handle_mm_fault(struct mm_struct *mm, struct
>> vm_area_struct *vma,
>>>              if (pmd_trans_huge(orig_pmd)) {
>>>                      if (flags & FAULT_FLAG_WRITE &&
>>>                          !pmd_write(orig_pmd) &&
>>> -                        !pmd_trans_splitting(orig_pmd))
>>> +                        !pmd_trans_splitting(orig_pmd) &&
>>> +                        !vma->vm_ops)
>>>                              return do_huge_pmd_wp_page(mm, vma, address,
>>>                                                         pmd, orig_pmd);
>>>                      return 0;
>> so if vma->vm_ops != NULL, how could the pmd_trans_huge(orig_pmd) be
>> true? We never enable THP if vma->vm_ops != NULL.
> acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits,
> it's only a pagesize, not a flag to identity a hugepage.
> If I change my default pagesize to PAGE_PSE,
Not sure what pagesize means here, assume pmd entry bits.
how could you make the default 'pagesize' to PAGE_PSE?

Re: [PATCH][mm/memory.c]: transparent hugepage check condition missed

[Guan Jun He]

>>> On 11/1/2011 at 04:42 PM, in message
<CANejiEVk41X-P+UyMf96jmPrJJ5-_vbubYtnQgaWXY2FLb41iw@mail.gmail.com>, Shaohua
Li <shaohua.li@intel.com> wrote: 
> 2011/11/1 Guan Jun He <gjhe@suse.com>:
>>
>>
>>>>> On 11/1/2011 at 09:18 AM, in message <1320110288.22361.190.camel@sli10-conroe>,
>> Shaohua Li <shaohua.li@intel.com> wrote:
>>> On Mon, 2011-10-31 at 16:23 +0800, Guanjun He wrote:
>>>> For the transparent hugepage module still does not support
>>>> tmpfs and cache,the check condition should always be checked
>>>> to make sure that it only affect the anonymous maps, the
>>>> original check condition missed this, this patch is to fix this.
>>>> Otherwise,the hugepage may affect the file-backed maps,
>>>> then the cache for the small-size pages will be unuseful,
>>>> and till now there is still no implementation for hugepage's cache.
>>>>
>>>> Signed-off-by: Guanjun He <gjhe@suse.com>
>>>> ---
>>>>  mm/memory.c |    3 ++-
>>>>  1 files changed, 2 insertions(+), 1 deletions(-)
>>>>
>>>> diff --git a/mm/memory.c b/mm/memory.c
>>>> index a56e3ba..79b85fe 100644
>>>> --- a/mm/memory.c
>>>> +++ b/mm/memory.c
>>>> @@ -3475,7 +3475,8 @@ int handle_mm_fault(struct mm_struct *mm, struct
>>> vm_area_struct *vma,
>>>>              if (pmd_trans_huge(orig_pmd)) {
>>>>                      if (flags & FAULT_FLAG_WRITE &&
>>>>                          !pmd_write(orig_pmd) &&
>>>> -                        !pmd_trans_splitting(orig_pmd))
>>>> +                        !pmd_trans_splitting(orig_pmd) &&
>>>> +                        !vma->vm_ops)
>>>>                              return do_huge_pmd_wp_page(mm, vma, address,
>>>>                                                         pmd, orig_pmd);
>>>>                      return 0;
>>> so if vma->vm_ops != NULL, how could the pmd_trans_huge(orig_pmd) be
>>> true? We never enable THP if vma->vm_ops != NULL.
>> acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits,
>> it's only a pagesize, not a flag to identity a hugepage.
>> If I change my default pagesize to PAGE_PSE,
> Not sure what pagesize means here, assume pmd entry bits.
yes, it's  pmd entry bits.
> how could you make the default 'pagesize' to PAGE_PSE?
That requires some work and not so easy and need hardware support... So, recently it won't come.
But one can easily create the same pmd entry bits for some special use;
as comment above, it's a pmd entry bits, only mark a size, not a flag;
and adjust the logic to use the flag can perfect avoid this potential issuse,
and basically no impact to the current code.

Re: [PATCH][mm/memory.c]: transparent hugepage check condition missed

[Guan Jun He]

Excuse me! I got the one email sent 6 times!

best,
Guanjun

>>> On 10/31/2011 at 05:02 PM, in message
<1320051775-13992-1-git-send-email-gjhe@suse.com>, Guanjun He <gjhe@suse.com>
wrote: 
> For the transparent hugepage module still does not support
> tmpfs and cache,the check condition should always be checked 
> to make sure that it only affect the anonymous maps, the 
> original check condition missed this, this patch is to fix this.
> Otherwise,the hugepage may affect the file-backed maps,
> then the cache for the small-size pages will be unuseful,
> and till now there is still no implementation for hugepage's cache.
> 
> Signed-off-by: Guanjun He <gjhe@suse.com>
> ---
>  mm/memory.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a56e3ba..79b85fe 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3475,7 +3475,8 @@ int handle_mm_fault(struct mm_struct *mm, struct 
> vm_area_struct *vma,
>  		if (pmd_trans_huge(orig_pmd)) {
>  			if (flags & FAULT_FLAG_WRITE &&
>  			    !pmd_write(orig_pmd) &&
> -			    !pmd_trans_splitting(orig_pmd))
> +			    !pmd_trans_splitting(orig_pmd) &&
> +			    !vma->vm_ops)
>  				return do_huge_pmd_wp_page(mm, vma, address,
>  							   pmd, orig_pmd);
>  			return 0;