All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH][mm] adjust the logic of checking THP
       [not found] <pebolle@tiscali.nl;linux-kernel@vger.kernel.org;linux-mm@kvack.org;gjhe@suse.com>
@ 2011-11-02  6:34 ` Guanjun He
  2011-11-02 12:17   ` Hillf Danton
  2011-11-23  0:04   ` Andrea Arcangeli
  0 siblings, 2 replies; 11+ messages in thread
From: Guanjun He @ 2011-11-02  6:34 UTC (permalink / raw)
  To: linux-kernel; +Cc: Guanjun He


Acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits,
it's a pmd entry bits, only mark a size, not a flag;As one can easily 
create the same pmd entry bits for some special use,then the check 
will get confused.And this patch is to adjust the logic to use the flag, 
it can perfectly avoid this potential issuse,and basically no impact 
to the current code.


Signed-off-by: Guanjun He <heguanbo@gmail.com>
---
 mm/memory.c |   28 +++++++++++++++-------------
 1 files changed, 15 insertions(+), 13 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index a56e3ba..a76b17f 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3465,20 +3465,22 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 	pmd = pmd_alloc(mm, pud, address);
 	if (!pmd)
 		return VM_FAULT_OOM;
-	if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
-		if (!vma->vm_ops)
-			return do_huge_pmd_anonymous_page(mm, vma, address,
-							  pmd, flags);
-	} else {
-		pmd_t orig_pmd = *pmd;
-		barrier();
-		if (pmd_trans_huge(orig_pmd)) {
-			if (flags & FAULT_FLAG_WRITE &&
-			    !pmd_write(orig_pmd) &&
-			    !pmd_trans_splitting(orig_pmd))
-				return do_huge_pmd_wp_page(mm, vma, address,
-							   pmd, orig_pmd);
+	if (transparent_hugepage_enabled(vma)) {
+		if (pmd_none(*pmd)) {
+			if (!vma->vm_ops)
+				return do_huge_pmd_anonymous_page(mm, vma, address,
+								  pmd, flags);
+		} else {
+			pmd_t orig_pmd = *pmd;
+			barrier();
+			if (pmd_trans_huge(orig_pmd)) {
+				if (flags & FAULT_FLAG_WRITE &&
+				    !pmd_write(orig_pmd) &&
+				    !pmd_trans_splitting(orig_pmd))
+					return do_huge_pmd_wp_page(mm, vma, address,
+								   pmd, orig_pmd);
 			return 0;
+			}
 		}
 	}
 
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 11+ messages in thread
* Re: [PATCH][mm] adjust the logic of checking THP
  2011-11-02  6:34 ` [PATCH][mm] adjust the logic of checking THP Guanjun He
@ 2011-11-02 12:17   ` Hillf Danton
  2011-11-03  2:19     ` GuanJun He
  2011-11-23  0:04   ` Andrea Arcangeli
  1 sibling, 1 reply; 11+ messages in thread
From: Hillf Danton @ 2011-11-02 12:17 UTC (permalink / raw)
  To: Guanjun He; +Cc: linux-kernel

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 3514 bytes --]

On Wed, Nov 2, 2011 at 2:34 PM, Guanjun He <heguanbo@gmail.com> wrote:
>
> Acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits,
> it's a pmd entry bits, only mark a size, not a flag;As one can easily
> create the same pmd entry bits for some special use,then the check
> will get confused.And this patch is to adjust the logic to use the flag,
> it can perfectly avoid this potential issuse,and basically no impact
> to the current code.
>
>
> Signed-off-by: Guanjun He <heguanbo@gmail.com>
> ---
>  mm/memory.c |   28 +++++++++++++++-------------
>  1 files changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/mm/memory.c b/mm/memory.c
> index a56e3ba..a76b17f 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3465,20 +3465,22 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>        pmd = pmd_alloc(mm, pud, address);
>        if (!pmd)
>                return VM_FAULT_OOM;
> -       if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
> -               if (!vma->vm_ops)
> -                       return do_huge_pmd_anonymous_page(mm, vma, address,
> -                                                         pmd, flags);
> -       } else {
> -               pmd_t orig_pmd = *pmd;
> -               barrier();
> -               if (pmd_trans_huge(orig_pmd)) {
> -                       if (flags & FAULT_FLAG_WRITE &&
> -                           !pmd_write(orig_pmd) &&
> -                           !pmd_trans_splitting(orig_pmd))
> -                               return do_huge_pmd_wp_page(mm, vma, address,
> -                                                          pmd, orig_pmd);
> +       if (transparent_hugepage_enabled(vma)) {

Well, how about THP not configured?

Thanks,
              Hillf

> +               if (pmd_none(*pmd)) {
> +                       if (!vma->vm_ops)
> +                               return do_huge_pmd_anonymous_page(mm, vma, address,
> +                                                                 pmd, flags);
> +               } else {
> +                       pmd_t orig_pmd = *pmd;
> +                       barrier();
> +                       if (pmd_trans_huge(orig_pmd)) {
> +                               if (flags & FAULT_FLAG_WRITE &&
> +                                   !pmd_write(orig_pmd) &&
> +                                   !pmd_trans_splitting(orig_pmd))
> +                                       return do_huge_pmd_wp_page(mm, vma, address,
> +                                                                  pmd, orig_pmd);
>                        return 0;
> +                       }
>                }
>        }
>
> --
> 1.7.7
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
>
>
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH][mm] adjust the logic of checking THP
  2011-11-02 12:17   ` Hillf Danton
@ 2011-11-03  2:19     ` GuanJun He
  2011-11-04  2:41         ` GuanJun He
  0 siblings, 1 reply; 11+ messages in thread
From: GuanJun He @ 2011-11-03  2:19 UTC (permalink / raw)
  To: Hillf Danton; +Cc: linux-kernel

On Wed, Nov 2, 2011 at 8:17 PM, Hillf Danton <dhillf@gmail.com> wrote:
> On Wed, Nov 2, 2011 at 2:34 PM, Guanjun He <heguanbo@gmail.com> wrote:
>>
>> Acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits,
>> it's a pmd entry bits, only mark a size, not a flag;As one can easily
>> create the same pmd entry bits for some special use,then the check
>> will get confused.And this patch is to adjust the logic to use the flag,
>> it can perfectly avoid this potential issuse,and basically no impact
>> to the current code.
>>
>>
>> Signed-off-by: Guanjun He <heguanbo@gmail.com>
>> ---
>>  mm/memory.c |   28 +++++++++++++++-------------
>>  1 files changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index a56e3ba..a76b17f 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3465,20 +3465,22 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>>        pmd = pmd_alloc(mm, pud, address);
>>        if (!pmd)
>>                return VM_FAULT_OOM;
>> -       if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
>> -               if (!vma->vm_ops)
>> -                       return do_huge_pmd_anonymous_page(mm, vma, address,
>> -                                                         pmd, flags);
>> -       } else {
>> -               pmd_t orig_pmd = *pmd;
>> -               barrier();
>> -               if (pmd_trans_huge(orig_pmd)) {
>> -                       if (flags & FAULT_FLAG_WRITE &&
>> -                           !pmd_write(orig_pmd) &&
>> -                           !pmd_trans_splitting(orig_pmd))
>> -                               return do_huge_pmd_wp_page(mm, vma, address,
>> -                                                          pmd, orig_pmd);
>> +       if (transparent_hugepage_enabled(vma)) {
>
> Well, how about THP not configured?

What do you mean 'not configured'? assume not enabled.
If THP not enabled, of course the transparent_hugepage_enabled(vma)
will be false.

>
> Thanks,
>              Hillf
>
>> +               if (pmd_none(*pmd)) {
>> +                       if (!vma->vm_ops)
>> +                               return do_huge_pmd_anonymous_page(mm, vma, address,
>> +                                                                 pmd, flags);
>> +               } else {
>> +                       pmd_t orig_pmd = *pmd;
>> +                       barrier();
>> +                       if (pmd_trans_huge(orig_pmd)) {
>> +                               if (flags & FAULT_FLAG_WRITE &&
>> +                                   !pmd_write(orig_pmd) &&
>> +                                   !pmd_trans_splitting(orig_pmd))
>> +                                       return do_huge_pmd_wp_page(mm, vma, address,
>> +                                                                  pmd, orig_pmd);
>>                        return 0;
>> +                       }
>>                }
>>        }
>>
>> --
>> 1.7.7
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>>
>>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH][mm] adjust the logic of checking THP
  2011-11-03  2:19     ` GuanJun He
@ 2011-11-04  2:41         ` GuanJun He
  0 siblings, 0 replies; 11+ messages in thread
From: GuanJun He @ 2011-11-04  2:41 UTC (permalink / raw)
  To: Hillf Danton, linux-kernel, linux-mm; +Cc: gjhe

On Thu, Nov 3, 2011 at 10:19 AM, GuanJun He <heguanbo@gmail.com> wrote:
> On Wed, Nov 2, 2011 at 8:17 PM, Hillf Danton <dhillf@gmail.com> wrote:
>> On Wed, Nov 2, 2011 at 2:34 PM, Guanjun He <heguanbo@gmail.com> wrote:
>>>
>>> Acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits,
>>> it's a pmd entry bits, only mark a size, not a flag;As one can easily
>>> create the same pmd entry bits for some special use,then the check
>>> will get confused.And this patch is to adjust the logic to use the flag,
>>> it can perfectly avoid this potential issuse,and basically no impact
>>> to the current code.
>>>
>>>
>>> Signed-off-by: Guanjun He <heguanbo@gmail.com>
>>> ---
>>>  mm/memory.c |   28 +++++++++++++++-------------
>>>  1 files changed, 15 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index a56e3ba..a76b17f 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -3465,20 +3465,22 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>>>        pmd = pmd_alloc(mm, pud, address);
>>>        if (!pmd)
>>>                return VM_FAULT_OOM;
>>> -       if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
>>> -               if (!vma->vm_ops)
>>> -                       return do_huge_pmd_anonymous_page(mm, vma, address,
>>> -                                                         pmd, flags);
>>> -       } else {
>>> -               pmd_t orig_pmd = *pmd;
>>> -               barrier();
>>> -               if (pmd_trans_huge(orig_pmd)) {
>>> -                       if (flags & FAULT_FLAG_WRITE &&
>>> -                           !pmd_write(orig_pmd) &&
>>> -                           !pmd_trans_splitting(orig_pmd))
>>> -                               return do_huge_pmd_wp_page(mm, vma, address,
>>> -                                                          pmd, orig_pmd);
>>> +       if (transparent_hugepage_enabled(vma)) {
>>
>> Well, how about THP not configured?
>
> What do you mean 'not configured'? assume not enabled.
> If THP not enabled, of course the transparent_hugepage_enabled(vma)
> will be false.

In the original logic,  it's possible that the code go into the logic
of THP while the THP is not enabled.
Just think, If build another module that make pmd with the same bits,
even THP is not enabled, the code can go into the logic of the THP.And
this adjust can avoid this.

best,
Guanjun

>
>>
>> Thanks,
>>              Hillf
>>
>>> +               if (pmd_none(*pmd)) {
>>> +                       if (!vma->vm_ops)
>>> +                               return do_huge_pmd_anonymous_page(mm, vma, address,
>>> +                                                                 pmd, flags);
>>> +               } else {
>>> +                       pmd_t orig_pmd = *pmd;
>>> +                       barrier();
>>> +                       if (pmd_trans_huge(orig_pmd)) {
>>> +                               if (flags & FAULT_FLAG_WRITE &&
>>> +                                   !pmd_write(orig_pmd) &&
>>> +                                   !pmd_trans_splitting(orig_pmd))
>>> +                                       return do_huge_pmd_wp_page(mm, vma, address,
>>> +                                                                  pmd, orig_pmd);
>>>                        return 0;
>>> +                       }
>>>                }
>>>        }
>>>
>>> --
>>> 1.7.7
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>
>>>
>>>
>>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH][mm] adjust the logic of checking THP
@ 2011-11-04  2:41         ` GuanJun He
  0 siblings, 0 replies; 11+ messages in thread
From: GuanJun He @ 2011-11-04  2:41 UTC (permalink / raw)
  To: Hillf Danton, linux-kernel, linux-mm; +Cc: gjhe

On Thu, Nov 3, 2011 at 10:19 AM, GuanJun He <heguanbo@gmail.com> wrote:
> On Wed, Nov 2, 2011 at 8:17 PM, Hillf Danton <dhillf@gmail.com> wrote:
>> On Wed, Nov 2, 2011 at 2:34 PM, Guanjun He <heguanbo@gmail.com> wrote:
>>>
>>> Acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits,
>>> it's a pmd entry bits, only mark a size, not a flag;As one can easily
>>> create the same pmd entry bits for some special use,then the check
>>> will get confused.And this patch is to adjust the logic to use the flag,
>>> it can perfectly avoid this potential issuse,and basically no impact
>>> to the current code.
>>>
>>>
>>> Signed-off-by: Guanjun He <heguanbo@gmail.com>
>>> ---
>>>  mm/memory.c |   28 +++++++++++++++-------------
>>>  1 files changed, 15 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/mm/memory.c b/mm/memory.c
>>> index a56e3ba..a76b17f 100644
>>> --- a/mm/memory.c
>>> +++ b/mm/memory.c
>>> @@ -3465,20 +3465,22 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>>>        pmd = pmd_alloc(mm, pud, address);
>>>        if (!pmd)
>>>                return VM_FAULT_OOM;
>>> -       if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
>>> -               if (!vma->vm_ops)
>>> -                       return do_huge_pmd_anonymous_page(mm, vma, address,
>>> -                                                         pmd, flags);
>>> -       } else {
>>> -               pmd_t orig_pmd = *pmd;
>>> -               barrier();
>>> -               if (pmd_trans_huge(orig_pmd)) {
>>> -                       if (flags & FAULT_FLAG_WRITE &&
>>> -                           !pmd_write(orig_pmd) &&
>>> -                           !pmd_trans_splitting(orig_pmd))
>>> -                               return do_huge_pmd_wp_page(mm, vma, address,
>>> -                                                          pmd, orig_pmd);
>>> +       if (transparent_hugepage_enabled(vma)) {
>>
>> Well, how about THP not configured?
>
> What do you mean 'not configured'? assume not enabled.
> If THP not enabled, of course the transparent_hugepage_enabled(vma)
> will be false.

In the original logic,  it's possible that the code go into the logic
of THP while the THP is not enabled.
Just think, If build another module that make pmd with the same bits,
even THP is not enabled, the code can go into the logic of the THP.And
this adjust can avoid this.

best,
Guanjun

>
>>
>> Thanks,
>>              Hillf
>>
>>> +               if (pmd_none(*pmd)) {
>>> +                       if (!vma->vm_ops)
>>> +                               return do_huge_pmd_anonymous_page(mm, vma, address,
>>> +                                                                 pmd, flags);
>>> +               } else {
>>> +                       pmd_t orig_pmd = *pmd;
>>> +                       barrier();
>>> +                       if (pmd_trans_huge(orig_pmd)) {
>>> +                               if (flags & FAULT_FLAG_WRITE &&
>>> +                                   !pmd_write(orig_pmd) &&
>>> +                                   !pmd_trans_splitting(orig_pmd))
>>> +                                       return do_huge_pmd_wp_page(mm, vma, address,
>>> +                                                                  pmd, orig_pmd);
>>>                        return 0;
>>> +                       }
>>>                }
>>>        }
>>>
>>> --
>>> 1.7.7
>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>> Please read the FAQ at  http://www.tux.org/lkml/
>>>
>>>
>>>
>>
>

--
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/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH][mm] adjust the logic of checking THP
  2011-11-02  6:34 ` [PATCH][mm] adjust the logic of checking THP Guanjun He
  2011-11-02 12:17   ` Hillf Danton
@ 2011-11-23  0:04   ` Andrea Arcangeli
  2011-12-06  4:18     ` GuanJun He
  1 sibling, 1 reply; 11+ messages in thread
From: Andrea Arcangeli @ 2011-11-23  0:04 UTC (permalink / raw)
  To: Guanjun He; +Cc: linux-kernel

On Wed, Nov 02, 2011 at 02:34:30PM +0800, Guanjun He wrote:
> 
> Acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits,
> it's a pmd entry bits, only mark a size, not a flag;As one can easily 
> create the same pmd entry bits for some special use,then the check 
> will get confused.And this patch is to adjust the logic to use the flag, 
> it can perfectly avoid this potential issuse,and basically no impact 
> to the current code.

You can't use _PAGE_PSE for special use for the pmd. Besides this is
common code, archs without such bit can define pmd_trans_huge to
return 0 like it happens with TRANSPARENT_HUGEPAGE=n.

> diff --git a/mm/memory.c b/mm/memory.c
> index a56e3ba..a76b17f 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3465,20 +3465,22 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  	pmd = pmd_alloc(mm, pud, address);
>  	if (!pmd)
>  		return VM_FAULT_OOM;
> -	if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
> -		if (!vma->vm_ops)
> -			return do_huge_pmd_anonymous_page(mm, vma, address,
> -							  pmd, flags);
> -	} else {
> -		pmd_t orig_pmd = *pmd;
> -		barrier();
> -		if (pmd_trans_huge(orig_pmd)) {
> -			if (flags & FAULT_FLAG_WRITE &&
> -			    !pmd_write(orig_pmd) &&
> -			    !pmd_trans_splitting(orig_pmd))
> -				return do_huge_pmd_wp_page(mm, vma, address,
> -							   pmd, orig_pmd);
> +	if (transparent_hugepage_enabled(vma)) {
> +		if (pmd_none(*pmd)) {
> +			if (!vma->vm_ops)
> +				return do_huge_pmd_anonymous_page(mm, vma, address,
> +								  pmd, flags);
> +		} else {
> +			pmd_t orig_pmd = *pmd;
> +			barrier();
> +			if (pmd_trans_huge(orig_pmd)) {
> +				if (flags & FAULT_FLAG_WRITE &&
> +				    !pmd_write(orig_pmd) &&
> +				    !pmd_trans_splitting(orig_pmd))
> +					return do_huge_pmd_wp_page(mm, vma, address,
> +								   pmd, orig_pmd);
>  			return 0;
> +			}

This will infinite loop if you disable THP at runtime while some
mapping needing cow is established.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH][mm] adjust the logic of checking THP
  2011-11-23  0:04   ` Andrea Arcangeli
@ 2011-12-06  4:18     ` GuanJun He
  0 siblings, 0 replies; 11+ messages in thread
From: GuanJun He @ 2011-12-06  4:18 UTC (permalink / raw)
  To: Andrea Arcangeli; +Cc: linux-kernel

On Wed, Nov 23, 2011 at 8:04 AM, Andrea Arcangeli <aarcange@redhat.com> wrote:
> On Wed, Nov 02, 2011 at 02:34:30PM +0800, Guanjun He wrote:
>>
>> Acturally, pmd_trans_huge(orig_pmd) only checks the _PAGE_PSE bits,
>> it's a pmd entry bits, only mark a size, not a flag;As one can easily
>> create the same pmd entry bits for some special use,then the check
>> will get confused.And this patch is to adjust the logic to use the flag,
>> it can perfectly avoid this potential issuse,and basically no impact
>> to the current code.
>
> You can't use _PAGE_PSE for special use for the pmd. Besides this is
> common code, archs without such bit can define pmd_trans_huge to
> return 0 like it happens with TRANSPARENT_HUGEPAGE=n.

ok, even _PAGE_PSE can not be used for special use for the pmd,
This check is more efficient.
the original code always check 2 items " pmd_none(*pmd) &&
transparent_hugepage_enabled(vma) ",
even TRANSPARENT_HUGEPAGE not enabled.

>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index a56e3ba..a76b17f 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3465,20 +3465,22 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>>       pmd = pmd_alloc(mm, pud, address);
>>       if (!pmd)
>>               return VM_FAULT_OOM;
>> -     if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
>> -             if (!vma->vm_ops)
>> -                     return do_huge_pmd_anonymous_page(mm, vma, address,
>> -                                                       pmd, flags);
>> -     } else {
>> -             pmd_t orig_pmd = *pmd;
>> -             barrier();
>> -             if (pmd_trans_huge(orig_pmd)) {
>> -                     if (flags & FAULT_FLAG_WRITE &&
>> -                         !pmd_write(orig_pmd) &&
>> -                         !pmd_trans_splitting(orig_pmd))
>> -                             return do_huge_pmd_wp_page(mm, vma, address,
>> -                                                        pmd, orig_pmd);
>> +     if (transparent_hugepage_enabled(vma)) {
>> +             if (pmd_none(*pmd)) {
>> +                     if (!vma->vm_ops)
>> +                             return do_huge_pmd_anonymous_page(mm, vma, address,
>> +                                                               pmd, flags);
>> +             } else {
>> +                     pmd_t orig_pmd = *pmd;
>> +                     barrier();
>> +                     if (pmd_trans_huge(orig_pmd)) {
>> +                             if (flags & FAULT_FLAG_WRITE &&
>> +                                 !pmd_write(orig_pmd) &&
>> +                                 !pmd_trans_splitting(orig_pmd))
>> +                                     return do_huge_pmd_wp_page(mm, vma, address,
>> +                                                                pmd, orig_pmd);
>>                       return 0;
>> +                     }
>
> This will infinite loop if you disable THP at runtime while some
> mapping needing cow is established.

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH][mm:] adjust the logic of checking THP
  2011-11-01 12:26   ` Paul Bolle
  2011-11-01 12:29     ` Paul Bolle
@ 2011-11-03  8:26     ` GuanJun He
  1 sibling, 0 replies; 11+ messages in thread
From: GuanJun He @ 2011-11-03  8:26 UTC (permalink / raw)
  To: Paul Bolle; +Cc: linux-kernel

Thanks!I have updated and resent it.

best,
Guanjun

On Tue, Nov 1, 2011 at 8:26 PM, Paul Bolle <pebolle@tiscali.nl> wrote:
> Perhaps a summary would help the people actually knowledgeable about mm
> understand why this adjustment is needed.
>
> I'll just mention a few obvious things.
>
> On Tue, 2011-11-01 at 19:41 +0800, Guanjun He wrote:
>> Signed-off-by: Guanjun He <heguanbo@gmail.com>
>> ---
>>  mm/memory.c |   32 ++++++++++++++++++--------------
>>  1 files changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/mm/memory.c b/mm/memory.c
>> index a56e3ba..d6dd6b3 100644
>> --- a/mm/memory.c
>> +++ b/mm/memory.c
>> @@ -3465,20 +3465,24 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>>       pmd = pmd_alloc(mm, pud, address);
>>       if (!pmd)
>>               return VM_FAULT_OOM;
>> -     if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
>> -             if (!vma->vm_ops)
>> -                     return do_huge_pmd_anonymous_page(mm, vma, address,
>> -                                                       pmd, flags);
>> -     } else {
>> -             pmd_t orig_pmd = *pmd;
>> -             barrier();
>> -             if (pmd_trans_huge(orig_pmd)) {
>> -                     if (flags & FAULT_FLAG_WRITE &&
>> -                         !pmd_write(orig_pmd) &&
>> -                         !pmd_trans_splitting(orig_pmd))
>> -                             return do_huge_pmd_wp_page(mm, vma, address,
>> -                                                        pmd, orig_pmd);
>> -                     return 0;
>> +     if (transparent_hugepage_enabled(vma)) {
>> +             if(pmd_none(*pmd)){
>
> codingstyle (scripts/checkpatch.pl would have caught that one).
>
>> +                     if (!vma->vm_ops)
>> +                             return do_huge_pmd_anonymous_page(mm, vma, address,
>> +                                             pmd, flags);
>> +             }
>> +             else
>> +             {
>
> codingstyle (ditto). There's probably more.
>
>> +                     pmd_t orig_pmd = *pmd;
>> +                     barrier();
>> +                     if (pmd_trans_huge(orig_pmd)) {
>> +                             if (flags & FAULT_FLAG_WRITE &&
>> +                                             !pmd_write(orig_pmd) &&
>> +                                             !pmd_trans_splitting(orig_pmd))
>> +                                     return do_huge_pmd_wp_page(mm, vma, address,
>> +                                                     pmd, orig_pmd);
>> +                             return 0;
>> +                     }
>>               }
>>       }
>>
>
>
> Paul Bolle
>
>

^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH][mm:] adjust the logic of checking THP
  2011-11-01 12:26   ` Paul Bolle
@ 2011-11-01 12:29     ` Paul Bolle
  2011-11-03  8:26     ` GuanJun He
  1 sibling, 0 replies; 11+ messages in thread
From: Paul Bolle @ 2011-11-01 12:29 UTC (permalink / raw)
  To: Guanjun He; +Cc: linux-kernel

On Tue, 2011-11-01 at 13:26 +0100, Paul Bolle wrote:
> Perhaps a summary would help the people actually knowledgeable about mm
> understand why this adjustment is needed

s/a summary/an explanation/


Paul Bolle


^ permalink raw reply	[flat|nested] 11+ messages in thread
* Re: [PATCH][mm:] adjust the logic of checking THP
  2011-11-01 11:41 ` [PATCH][mm:] " Guanjun He
@ 2011-11-01 12:26   ` Paul Bolle
  2011-11-01 12:29     ` Paul Bolle
  2011-11-03  8:26     ` GuanJun He
  0 siblings, 2 replies; 11+ messages in thread
From: Paul Bolle @ 2011-11-01 12:26 UTC (permalink / raw)
  To: Guanjun He; +Cc: linux-kernel

Perhaps a summary would help the people actually knowledgeable about mm
understand why this adjustment is needed.

I'll just mention a few obvious things.

On Tue, 2011-11-01 at 19:41 +0800, Guanjun He wrote:
> Signed-off-by: Guanjun He <heguanbo@gmail.com>
> ---
>  mm/memory.c |   32 ++++++++++++++++++--------------
>  1 files changed, 18 insertions(+), 14 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index a56e3ba..d6dd6b3 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -3465,20 +3465,24 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
>  	pmd = pmd_alloc(mm, pud, address);
>  	if (!pmd)
>  		return VM_FAULT_OOM;
> -	if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
> -		if (!vma->vm_ops)
> -			return do_huge_pmd_anonymous_page(mm, vma, address,
> -							  pmd, flags);
> -	} else {
> -		pmd_t orig_pmd = *pmd;
> -		barrier();
> -		if (pmd_trans_huge(orig_pmd)) {
> -			if (flags & FAULT_FLAG_WRITE &&
> -			    !pmd_write(orig_pmd) &&
> -			    !pmd_trans_splitting(orig_pmd))
> -				return do_huge_pmd_wp_page(mm, vma, address,
> -							   pmd, orig_pmd);
> -			return 0;
> +	if (transparent_hugepage_enabled(vma)) {
> +		if(pmd_none(*pmd)){

codingstyle (scripts/checkpatch.pl would have caught that one).

> +			if (!vma->vm_ops)
> +				return do_huge_pmd_anonymous_page(mm, vma, address,
> +						pmd, flags);
> +		}
> +		else
> +		{

codingstyle (ditto). There's probably more.

> +			pmd_t orig_pmd = *pmd;
> +			barrier();
> +			if (pmd_trans_huge(orig_pmd)) {
> +				if (flags & FAULT_FLAG_WRITE &&
> +						!pmd_write(orig_pmd) &&
> +						!pmd_trans_splitting(orig_pmd))
> +					return do_huge_pmd_wp_page(mm, vma, address,
> +							pmd, orig_pmd);
> +				return 0;
> +			}
>  		}
>  	}
>  


Paul Bolle


^ permalink raw reply	[flat|nested] 11+ messages in thread
* [PATCH][mm:] adjust the logic of checking THP
       [not found] <shaohua.li@intel.com;linux-mm@kvack.org;linux-kernel@vger.kernel.org>
@ 2011-11-01 11:41 ` Guanjun He
  2011-11-01 12:26   ` Paul Bolle
  0 siblings, 1 reply; 11+ messages in thread
From: Guanjun He @ 2011-11-01 11:41 UTC (permalink / raw)
  To: linux-kernel; +Cc: Guanjun He


Signed-off-by: Guanjun He <heguanbo@gmail.com>
---
 mm/memory.c |   32 ++++++++++++++++++--------------
 1 files changed, 18 insertions(+), 14 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index a56e3ba..d6dd6b3 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -3465,20 +3465,24 @@ int handle_mm_fault(struct mm_struct *mm, struct vm_area_struct *vma,
 	pmd = pmd_alloc(mm, pud, address);
 	if (!pmd)
 		return VM_FAULT_OOM;
-	if (pmd_none(*pmd) && transparent_hugepage_enabled(vma)) {
-		if (!vma->vm_ops)
-			return do_huge_pmd_anonymous_page(mm, vma, address,
-							  pmd, flags);
-	} else {
-		pmd_t orig_pmd = *pmd;
-		barrier();
-		if (pmd_trans_huge(orig_pmd)) {
-			if (flags & FAULT_FLAG_WRITE &&
-			    !pmd_write(orig_pmd) &&
-			    !pmd_trans_splitting(orig_pmd))
-				return do_huge_pmd_wp_page(mm, vma, address,
-							   pmd, orig_pmd);
-			return 0;
+	if (transparent_hugepage_enabled(vma)) {
+		if(pmd_none(*pmd)){
+			if (!vma->vm_ops)
+				return do_huge_pmd_anonymous_page(mm, vma, address,
+						pmd, flags);
+		}
+		else
+		{
+			pmd_t orig_pmd = *pmd;
+			barrier();
+			if (pmd_trans_huge(orig_pmd)) {
+				if (flags & FAULT_FLAG_WRITE &&
+						!pmd_write(orig_pmd) &&
+						!pmd_trans_splitting(orig_pmd))
+					return do_huge_pmd_wp_page(mm, vma, address,
+							pmd, orig_pmd);
+				return 0;
+			}
 		}
 	}
 
-- 
1.7.7


^ permalink raw reply related	[flat|nested] 11+ messages in thread
end of thread, other threads:[~2011-12-06  4:18 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
     [not found] <pebolle@tiscali.nl;linux-kernel@vger.kernel.org;linux-mm@kvack.org;gjhe@suse.com>
2011-11-02  6:34 ` [PATCH][mm] adjust the logic of checking THP Guanjun He
2011-11-02 12:17   ` Hillf Danton
2011-11-03  2:19     ` GuanJun He
2011-11-04  2:41       ` GuanJun He
2011-11-04  2:41         ` GuanJun He
2011-11-23  0:04   ` Andrea Arcangeli
2011-12-06  4:18     ` GuanJun He
     [not found] <shaohua.li@intel.com;linux-mm@kvack.org;linux-kernel@vger.kernel.org>
2011-11-01 11:41 ` [PATCH][mm:] " Guanjun He
2011-11-01 12:26   ` Paul Bolle
2011-11-01 12:29     ` Paul Bolle
2011-11-03  8:26     ` GuanJun He

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.