[PATCH] mm: thp: fix potential clearing to referenced flag in page_idle_clear_pte_refs_one()

[PATCH] mm: thp: fix potential clearing to referenced flag in page_idle_clear_pte_refs_one()

[Yang Shi]

For PTE-mapped THP, the compound THP has not been split to normal 4K
pages yet, the whole THP is considered referenced if any one of sub
page is referenced.

When walking PTE-mapped THP by pvmw, all relevant PTEs will be checked
to retrieve referenced bit. But, the current code just returns the
result of the last PTE. If the last PTE has not referenced, the
referenced flag will be cleared.

So, here just break pvmw walk once referenced PTE is found if the page
is a part of THP.

Signed-off-by: Yang Shi <yang.shi@linux.alibaba.com>
Reported-by: Gang Deng <gavin.dg@linux.alibaba.com>
---
 mm/page_idle.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/mm/page_idle.c b/mm/page_idle.c
index 0a49374..da6024f 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -67,6 +67,14 @@ static bool page_idle_clear_pte_refs_one(struct page *page,
 		if (pvmw.pte) {
 			referenced = ptep_clear_young_notify(vma, addr,
 					pvmw.pte);
+			/*
+			 * For PTE-mapped THP, one sub page is referenced,
+			 * the whole THP is referenced.
+			 */
+			if (referenced && PageTransCompound(pvmw.page)) {
+				page_vma_mapped_walk_done(&pvmw);
+				break;
+			}
 		} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
 			referenced = pmdp_clear_young_notify(vma, addr,
 					pvmw.pmd);
-- 
1.8.3.1

--
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] mm: thp: fix potential clearing to referenced flag in page_idle_clear_pte_refs_one()

[Andrew Morton]

On Tue,  6 Feb 2018 08:06:36 +0800 Yang Shi <yang.shi@linux.alibaba.com> wrote:

> For PTE-mapped THP, the compound THP has not been split to normal 4K
> pages yet, the whole THP is considered referenced if any one of sub
> page is referenced.
> 
> When walking PTE-mapped THP by pvmw, all relevant PTEs will be checked
> to retrieve referenced bit. But, the current code just returns the
> result of the last PTE. If the last PTE has not referenced, the
> referenced flag will be cleared.
> 
> So, here just break pvmw walk once referenced PTE is found if the page
> is a part of THP.
> 
> ...
>
> --- a/mm/page_idle.c
> +++ b/mm/page_idle.c
> @@ -67,6 +67,14 @@ static bool page_idle_clear_pte_refs_one(struct page *page,
>  		if (pvmw.pte) {
>  			referenced = ptep_clear_young_notify(vma, addr,
>  					pvmw.pte);
> +			/*
> +			 * For PTE-mapped THP, one sub page is referenced,
> +			 * the whole THP is referenced.
> +			 */
> +			if (referenced && PageTransCompound(pvmw.page)) {
> +				page_vma_mapped_walk_done(&pvmw);
> +				break;
> +			}

This means that the function will no longer clear the referenced bits
in all the ptes.  What effect does this have and should we document
this in some fashion?

>  		} else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
>  			referenced = pmdp_clear_young_notify(vma, addr,
>  					pvmw.pmd);

--
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] mm: thp: fix potential clearing to referenced flag in page_idle_clear_pte_refs_one()

[Kirill A. Shutemov]

On Thu, Feb 08, 2018 at 02:39:26PM -0800, Andrew Morton wrote:
> On Tue,  6 Feb 2018 08:06:36 +0800 Yang Shi <yang.shi@linux.alibaba.com> wrote:
> 
> > For PTE-mapped THP, the compound THP has not been split to normal 4K
> > pages yet, the whole THP is considered referenced if any one of sub
> > page is referenced.
> > 
> > When walking PTE-mapped THP by pvmw, all relevant PTEs will be checked
> > to retrieve referenced bit. But, the current code just returns the
> > result of the last PTE. If the last PTE has not referenced, the
> > referenced flag will be cleared.
> > 
> > So, here just break pvmw walk once referenced PTE is found if the page
> > is a part of THP.
> > 
> > ...
> >
> > --- a/mm/page_idle.c
> > +++ b/mm/page_idle.c
> > @@ -67,6 +67,14 @@ static bool page_idle_clear_pte_refs_one(struct page *page,
> >  		if (pvmw.pte) {
> >  			referenced = ptep_clear_young_notify(vma, addr,
> >  					pvmw.pte);
> > +			/*
> > +			 * For PTE-mapped THP, one sub page is referenced,
> > +			 * the whole THP is referenced.
> > +			 */
> > +			if (referenced && PageTransCompound(pvmw.page)) {
> > +				page_vma_mapped_walk_done(&pvmw);
> > +				break;
> > +			}
> 
> This means that the function will no longer clear the referenced bits
> in all the ptes.  What effect does this have and should we document
> this in some fashion?

Yeah, the patch is wrong. We need to get all ptes for THP cleared.

What about something like this instead (untested):

diff --git a/mm/page_idle.c b/mm/page_idle.c
index 0a49374e6931..6876522c9dce 100644
--- a/mm/page_idle.c
+++ b/mm/page_idle.c
@@ -65,10 +65,10 @@ static bool page_idle_clear_pte_refs_one(struct page *page,
        while (page_vma_mapped_walk(&pvmw)) {
                addr = pvmw.address;
                if (pvmw.pte) {
-                       referenced = ptep_clear_young_notify(vma, addr,
+                       referenced |= ptep_clear_young_notify(vma, addr,
                                        pvmw.pte);
                } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
-                       referenced = pmdp_clear_young_notify(vma, addr,
+                       referenced |= pmdp_clear_young_notify(vma, addr,
                                        pvmw.pmd);
                } else {
                        /* unexpected pmd-mapped page? */
-- 
 Kirill A. Shutemov

--
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] mm: thp: fix potential clearing to referenced flag in page_idle_clear_pte_refs_one()

[Yang Shi]

On 2/8/18 8:33 PM, Kirill A. Shutemov wrote:
> On Thu, Feb 08, 2018 at 02:39:26PM -0800, Andrew Morton wrote:
>> On Tue,  6 Feb 2018 08:06:36 +0800 Yang Shi <yang.shi@linux.alibaba.com> wrote:
>>
>>> For PTE-mapped THP, the compound THP has not been split to normal 4K
>>> pages yet, the whole THP is considered referenced if any one of sub
>>> page is referenced.
>>>
>>> When walking PTE-mapped THP by pvmw, all relevant PTEs will be checked
>>> to retrieve referenced bit. But, the current code just returns the
>>> result of the last PTE. If the last PTE has not referenced, the
>>> referenced flag will be cleared.
>>>
>>> So, here just break pvmw walk once referenced PTE is found if the page
>>> is a part of THP.
>>>
>>> ...
>>>
>>> --- a/mm/page_idle.c
>>> +++ b/mm/page_idle.c
>>> @@ -67,6 +67,14 @@ static bool page_idle_clear_pte_refs_one(struct page *page,
>>>   		if (pvmw.pte) {
>>>   			referenced = ptep_clear_young_notify(vma, addr,
>>>   					pvmw.pte);
>>> +			/*
>>> +			 * For PTE-mapped THP, one sub page is referenced,
>>> +			 * the whole THP is referenced.
>>> +			 */
>>> +			if (referenced && PageTransCompound(pvmw.page)) {
>>> +				page_vma_mapped_walk_done(&pvmw);
>>> +				break;
>>> +			}
>> This means that the function will no longer clear the referenced bits
>> in all the ptes.  What effect does this have and should we document
>> this in some fashion?
> Yeah, the patch is wrong. We need to get all ptes for THP cleared.
>
> What about something like this instead (untested):

Thanks, Kirill. It looks correct. All ptes should be cleared.

I'm going to prepare v2 patch.

Regards,
Yang

>
> diff --git a/mm/page_idle.c b/mm/page_idle.c
> index 0a49374e6931..6876522c9dce 100644
> --- a/mm/page_idle.c
> +++ b/mm/page_idle.c
> @@ -65,10 +65,10 @@ static bool page_idle_clear_pte_refs_one(struct page *page,
>          while (page_vma_mapped_walk(&pvmw)) {
>                  addr = pvmw.address;
>                  if (pvmw.pte) {
> -                       referenced = ptep_clear_young_notify(vma, addr,
> +                       referenced |= ptep_clear_young_notify(vma, addr,
>                                          pvmw.pte);
>                  } else if (IS_ENABLED(CONFIG_TRANSPARENT_HUGEPAGE)) {
> -                       referenced = pmdp_clear_young_notify(vma, addr,
> +                       referenced |= pmdp_clear_young_notify(vma, addr,
>                                          pvmw.pmd);
>                  } else {
>                          /* unexpected pmd-mapped page? */

--
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] mm: thp: fix potential clearing to referenced flag in page_idle_clear_pte_refs_one()

[Kirill A. Shutemov]

On Thu, Feb 08, 2018 at 08:47:35PM -0800, Yang Shi wrote:
> 
> 
> On 2/8/18 8:33 PM, Kirill A. Shutemov wrote:
> > On Thu, Feb 08, 2018 at 02:39:26PM -0800, Andrew Morton wrote:
> > > On Tue,  6 Feb 2018 08:06:36 +0800 Yang Shi <yang.shi@linux.alibaba.com> wrote:
> > > 
> > > > For PTE-mapped THP, the compound THP has not been split to normal 4K
> > > > pages yet, the whole THP is considered referenced if any one of sub
> > > > page is referenced.
> > > > 
> > > > When walking PTE-mapped THP by pvmw, all relevant PTEs will be checked
> > > > to retrieve referenced bit. But, the current code just returns the
> > > > result of the last PTE. If the last PTE has not referenced, the
> > > > referenced flag will be cleared.
> > > > 
> > > > So, here just break pvmw walk once referenced PTE is found if the page
> > > > is a part of THP.
> > > > 
> > > > ...
> > > > 
> > > > --- a/mm/page_idle.c
> > > > +++ b/mm/page_idle.c
> > > > @@ -67,6 +67,14 @@ static bool page_idle_clear_pte_refs_one(struct page *page,
> > > >   		if (pvmw.pte) {
> > > >   			referenced = ptep_clear_young_notify(vma, addr,
> > > >   					pvmw.pte);
> > > > +			/*
> > > > +			 * For PTE-mapped THP, one sub page is referenced,
> > > > +			 * the whole THP is referenced.
> > > > +			 */
> > > > +			if (referenced && PageTransCompound(pvmw.page)) {
> > > > +				page_vma_mapped_walk_done(&pvmw);
> > > > +				break;
> > > > +			}
> > > This means that the function will no longer clear the referenced bits
> > > in all the ptes.  What effect does this have and should we document
> > > this in some fashion?
> > Yeah, the patch is wrong. We need to get all ptes for THP cleared.
> > 
> > What about something like this instead (untested):
> 
> Thanks, Kirill. It looks correct. All ptes should be cleared.
> 
> I'm going to prepare v2 patch.

Note, it should be ||=, not |= (although it would work correctly too).

I should really wake up properly before touching code. :-/

-- 
 Kirill A. Shutemov

--
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] mm: thp: fix potential clearing to referenced flag in page_idle_clear_pte_refs_one()

[Yang Shi]

On 2/9/18 12:16 AM, Kirill A. Shutemov wrote:
> On Thu, Feb 08, 2018 at 08:47:35PM -0800, Yang Shi wrote:
>>
>> On 2/8/18 8:33 PM, Kirill A. Shutemov wrote:
>>> On Thu, Feb 08, 2018 at 02:39:26PM -0800, Andrew Morton wrote:
>>>> On Tue,  6 Feb 2018 08:06:36 +0800 Yang Shi <yang.shi@linux.alibaba.com> wrote:
>>>>
>>>>> For PTE-mapped THP, the compound THP has not been split to normal 4K
>>>>> pages yet, the whole THP is considered referenced if any one of sub
>>>>> page is referenced.
>>>>>
>>>>> When walking PTE-mapped THP by pvmw, all relevant PTEs will be checked
>>>>> to retrieve referenced bit. But, the current code just returns the
>>>>> result of the last PTE. If the last PTE has not referenced, the
>>>>> referenced flag will be cleared.
>>>>>
>>>>> So, here just break pvmw walk once referenced PTE is found if the page
>>>>> is a part of THP.
>>>>>
>>>>> ...
>>>>>
>>>>> --- a/mm/page_idle.c
>>>>> +++ b/mm/page_idle.c
>>>>> @@ -67,6 +67,14 @@ static bool page_idle_clear_pte_refs_one(struct page *page,
>>>>>    		if (pvmw.pte) {
>>>>>    			referenced = ptep_clear_young_notify(vma, addr,
>>>>>    					pvmw.pte);
>>>>> +			/*
>>>>> +			 * For PTE-mapped THP, one sub page is referenced,
>>>>> +			 * the whole THP is referenced.
>>>>> +			 */
>>>>> +			if (referenced && PageTransCompound(pvmw.page)) {
>>>>> +				page_vma_mapped_walk_done(&pvmw);
>>>>> +				break;
>>>>> +			}
>>>> This means that the function will no longer clear the referenced bits
>>>> in all the ptes.  What effect does this have and should we document
>>>> this in some fashion?
>>> Yeah, the patch is wrong. We need to get all ptes for THP cleared.
>>>
>>> What about something like this instead (untested):
>> Thanks, Kirill. It looks correct. All ptes should be cleared.
>>
>> I'm going to prepare v2 patch.
> Note, it should be ||=, not |= (although it would work correctly too).

checkpatch doesn't like "||=", it reports no space error. Use referenced 
= referenced || ptep_clear_young_notify(...) to make checkpatch happy.

Regards,
Yang

>
> I should really wake up properly before touching code. :-/
>

--
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>