[PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[Minchan Kim]

VM doesn't add zero page to LRU list. 
It means zero page's churning in LRU list is pointless. 

As a matter of fact, zero page can't be promoted by mark_page_accessed
since it doesn't have PG_lru. 

This patch prevent unecessary mark_page_accessed call of zero page 
alghouth caller want FOLL_TOUCH. 

Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
---
 mm/memory.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/mm/memory.c b/mm/memory.c
index 09e4b1b..485f727 100644
--- a/mm/memory.c
+++ b/mm/memory.c
@@ -1152,6 +1152,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
 	spinlock_t *ptl;
 	struct page *page;
 	struct mm_struct *mm = vma->vm_mm;
+	int zero_pfn = 0;
 
 	page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
 	if (!IS_ERR(page)) {
@@ -1196,15 +1197,15 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
 
 	page = vm_normal_page(vma, address, pte);
 	if (unlikely(!page)) {
-		if ((flags & FOLL_DUMP) ||
-		    !is_zero_pfn(pte_pfn(pte)))
+		zero_pfn = is_zero_pfn(pte_pfn(pte));
+		if ((flags & FOLL_DUMP) || !zero_pfn )
 			goto bad_page;
 		page = pte_page(pte);
 	}
 
 	if (flags & FOLL_GET)
 		get_page(page);
-	if (flags & FOLL_TOUCH) {
+	if (flags & FOLL_TOUCH && !zero_pfn) {
 		if ((flags & FOLL_WRITE) &&
 		    !pte_dirty(pte) && !PageDirty(page))
 			set_page_dirty(page);
-- 
1.5.6.3


-- 
Kind regards,
Minchan Kim

Re: [PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[KAMEZAWA Hiroyuki]

On Mon, 28 Dec 2009 11:53:15 +0900
Minchan Kim <minchan.kim@gmail.com> wrote:

> 
> VM doesn't add zero page to LRU list. 
> It means zero page's churning in LRU list is pointless. 
> 
> As a matter of fact, zero page can't be promoted by mark_page_accessed
> since it doesn't have PG_lru. 
> 
> This patch prevent unecessary mark_page_accessed call of zero page 
> alghouth caller want FOLL_TOUCH. 
> 
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>
Reviewed-by: KAMEZAWA Hiroyuki <kamezawa.hiroyu@jp.fujitsu.com>

> ---
>  mm/memory.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/mm/memory.c b/mm/memory.c
> index 09e4b1b..485f727 100644
> --- a/mm/memory.c
> +++ b/mm/memory.c
> @@ -1152,6 +1152,7 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
>  	spinlock_t *ptl;
>  	struct page *page;
>  	struct mm_struct *mm = vma->vm_mm;
> +	int zero_pfn = 0;
>  
>  	page = follow_huge_addr(mm, address, flags & FOLL_WRITE);
>  	if (!IS_ERR(page)) {
> @@ -1196,15 +1197,15 @@ struct page *follow_page(struct vm_area_struct *vma, unsigned long address,
>  
>  	page = vm_normal_page(vma, address, pte);
>  	if (unlikely(!page)) {
> -		if ((flags & FOLL_DUMP) ||
> -		    !is_zero_pfn(pte_pfn(pte)))
> +		zero_pfn = is_zero_pfn(pte_pfn(pte));
> +		if ((flags & FOLL_DUMP) || !zero_pfn )
>  			goto bad_page;
>  		page = pte_page(pte);
>  	}
>  
>  	if (flags & FOLL_GET)
>  		get_page(page);
> -	if (flags & FOLL_TOUCH) {
> +	if (flags & FOLL_TOUCH && !zero_pfn) {
>  		if ((flags & FOLL_WRITE) &&
>  		    !pte_dirty(pte) && !PageDirty(page))
>  			set_page_dirty(page);
> -- 
> 1.5.6.3
> 
> 
> -- 
> Kind regards,
> Minchan Kim
> 
> --
> 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 -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[KOSAKI Motohiro]

> 
> VM doesn't add zero page to LRU list. 
> It means zero page's churning in LRU list is pointless. 
> 
> As a matter of fact, zero page can't be promoted by mark_page_accessed
> since it doesn't have PG_lru. 
> 
> This patch prevent unecessary mark_page_accessed call of zero page 
> alghouth caller want FOLL_TOUCH. 
> 
> Signed-off-by: Minchan Kim <minchan.kim@gmail.com>

Looks good to me.
	Reviewed-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>

Re: [PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[Rik van Riel]

On 12/27/2009 09:53 PM, Minchan Kim wrote:
>
> VM doesn't add zero page to LRU list.
> It means zero page's churning in LRU list is pointless.
>
> As a matter of fact, zero page can't be promoted by mark_page_accessed
> since it doesn't have PG_lru.
>
> This patch prevent unecessary mark_page_accessed call of zero page
> alghouth caller want FOLL_TOUCH.
>
> Signed-off-by: Minchan Kim<minchan.kim@gmail.com>

The code looks correct, but I wonder how frequently we run into
the zero page in this code, vs. how much the added cost is of
having this extra code in follow_page.

What kind of problem were you running into that motivated you
to write this patch?

-- 
All rights reversed.

Re: [PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[Balbir Singh]

* Rik van Riel <riel@redhat.com> [2009-12-27 22:22:20]:

> On 12/27/2009 09:53 PM, Minchan Kim wrote:
> >
> >VM doesn't add zero page to LRU list.
> >It means zero page's churning in LRU list is pointless.
> >
> >As a matter of fact, zero page can't be promoted by mark_page_accessed
> >since it doesn't have PG_lru.
> >
> >This patch prevent unecessary mark_page_accessed call of zero page
> >alghouth caller want FOLL_TOUCH.
> >
> >Signed-off-by: Minchan Kim<minchan.kim@gmail.com>
> 
> The code looks correct, but I wonder how frequently we run into
> the zero page in this code, vs. how much the added cost is of
> having this extra code in follow_page.
> 
> What kind of problem were you running into that motivated you
> to write this patch?
>

Frequent moving of zero page should ideally put it to the head of the
LRU list, leaving it untouched is likely to cause it to be scanned 
often - no? Should this be moved to the unevictable list? 

-- 
	Balbir

Re: [PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[Balbir Singh]

* Balbir Singh <balbir@linux.vnet.ibm.com> [2009-12-28 09:26:39]:

> * Rik van Riel <riel@redhat.com> [2009-12-27 22:22:20]:
> 
> > On 12/27/2009 09:53 PM, Minchan Kim wrote:
> > >
> > >VM doesn't add zero page to LRU list.
> > >It means zero page's churning in LRU list is pointless.
> > >
> > >As a matter of fact, zero page can't be promoted by mark_page_accessed
> > >since it doesn't have PG_lru.
> > >
> > >This patch prevent unecessary mark_page_accessed call of zero page
> > >alghouth caller want FOLL_TOUCH.
> > >
> > >Signed-off-by: Minchan Kim<minchan.kim@gmail.com>
> > 
> > The code looks correct, but I wonder how frequently we run into
> > the zero page in this code, vs. how much the added cost is of
> > having this extra code in follow_page.
> > 
> > What kind of problem were you running into that motivated you
> > to write this patch?
> >
> 
> Frequent moving of zero page should ideally put it to the head of the
> LRU list, leaving it untouched is likely to cause it to be scanned 
> often - no? Should this be moved to the unevictable list? 
>

Sorry, I replied to wrong email, I should have been clearer that this
question is for Minchan Kim. 

-- 
	Balbir

Re: [PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[Minchan Kim]

Hi, Rik. 

On Sun, 27 Dec 2009 22:22:20 -0500
Rik van Riel <riel@redhat.com> wrote:

> On 12/27/2009 09:53 PM, Minchan Kim wrote:
> >
> > VM doesn't add zero page to LRU list.
> > It means zero page's churning in LRU list is pointless.
> >
> > As a matter of fact, zero page can't be promoted by mark_page_accessed
> > since it doesn't have PG_lru.
> >
> > This patch prevent unecessary mark_page_accessed call of zero page
> > alghouth caller want FOLL_TOUCH.
> >
> > Signed-off-by: Minchan Kim<minchan.kim@gmail.com>
> 
> The code looks correct, but I wonder how frequently we run into
> the zero page in this code, vs. how much the added cost is of
> having this extra code in follow_page.
> 
> What kind of problem were you running into that motivated you
> to write this patch?

I didn't have experienced any problem in this case. 
In fact, I found that while trying to make patch smap_pte_change. 

Long time ago when we have a zero page, we regards it to file_rss. 
So while we see the smaps, vm_normal_page returns zero page and we can
calculate it properly with PSS. 

But now we don't acccout zero page to file_rss. 
I am not sure we have to account it with file_rss. 
So I think now smaps_pte_range's resident count routine also is changed. 

Anyway, I think my patch doesn't have much cost since many customers of 
follow_page are already not a fast path.

I tend to agree with your opinion "How frequently we runt into the zero page?"
But my thought GUP is export function which can be used for anything by anyone.

Thanks for the review, Rik. 

> 
> -- 
> All rights reversed.


-- 
Kind regards,
Minchan Kim

Re: [PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[Rik van Riel]

On 12/27/2009 10:57 PM, Balbir Singh wrote:
> * Balbir Singh<balbir@linux.vnet.ibm.com>  [2009-12-28 09:26:39]:
>
>> * Rik van Riel<riel@redhat.com>  [2009-12-27 22:22:20]:
>>
>>> On 12/27/2009 09:53 PM, Minchan Kim wrote:
>>>>
>>>> VM doesn't add zero page to LRU list.
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

>> Frequent moving of zero page should ideally put it to the head of the
>> LRU list, leaving it untouched is likely to cause it to be scanned
>> often - no? Should this be moved to the unevictable list?
>>
>
> Sorry, I replied to wrong email, I should have been clearer that this
> question is for Minchan Kim.

The answer to your question is all the way up in
Minchan Kim's original email.

The zero page is never on the LRU lists to begin with.

-- 
All rights reversed.

Re: [PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[Balbir Singh]

* Rik van Riel <riel@redhat.com> [2009-12-27 23:12:07]:

> On 12/27/2009 10:57 PM, Balbir Singh wrote:
> >* Balbir Singh<balbir@linux.vnet.ibm.com>  [2009-12-28 09:26:39]:
> >
> >>* Rik van Riel<riel@redhat.com>  [2009-12-27 22:22:20]:
> >>
> >>>On 12/27/2009 09:53 PM, Minchan Kim wrote:
> >>>>
> >>>>VM doesn't add zero page to LRU list.
>      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> >>Frequent moving of zero page should ideally put it to the head of the
> >>LRU list, leaving it untouched is likely to cause it to be scanned
> >>often - no? Should this be moved to the unevictable list?
> >>
> >
> >Sorry, I replied to wrong email, I should have been clearer that this
> >question is for Minchan Kim.
> 
> The answer to your question is all the way up in
> Minchan Kim's original email.
> 
> The zero page is never on the LRU lists to begin with.
>

Aahh.. my bad... I should have looked at it more closely! 

-- 
	Balbir

Re: [PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[Hugh Dickins]

On Mon, 28 Dec 2009, Minchan Kim wrote:
> On Sun, 27 Dec 2009 22:22:20 -0500
> Rik van Riel <riel@redhat.com> wrote:
> > On 12/27/2009 09:53 PM, Minchan Kim wrote:
> > >
> > > VM doesn't add zero page to LRU list.
> > > It means zero page's churning in LRU list is pointless.
> > >
> > > As a matter of fact, zero page can't be promoted by mark_page_accessed
> > > since it doesn't have PG_lru.
> > >
> > > This patch prevent unecessary mark_page_accessed call of zero page
> > > alghouth caller want FOLL_TOUCH.
> > >
> > > Signed-off-by: Minchan Kim<minchan.kim@gmail.com>
> > 
> > The code looks correct, but I wonder how frequently we run into
> > the zero page in this code, vs. how much the added cost is of
> > having this extra code in follow_page.
> > 
> > What kind of problem were you running into that motivated you
> > to write this patch?
> 
> I didn't have experienced any problem in this case. 
> In fact, I found that while trying to make patch smap_pte_change. 
> 
> Long time ago when we have a zero page, we regards it to file_rss. 
> So while we see the smaps, vm_normal_page returns zero page and we can
> calculate it properly with PSS. 
> 
> But now we don't acccout zero page to file_rss. 
> I am not sure we have to account it with file_rss. 
> So I think now smaps_pte_range's resident count routine also is changed. 
> 
> Anyway, I think my patch doesn't have much cost since many customers of 
> follow_page are already not a fast path.
> 
> I tend to agree with your opinion "How frequently we runt into the zero page?"
> But my thought GUP is export function which can be used for anything by anyone.
> 
> Thanks for the review, Rik. 

I'm guessing that you've now dropped the idea of this patch,
since it wasn't included along with your 1/3, 2/3, 3/3.

You thought the ZERO_PAGE was moving around the LRUs, but now
realize that it isn't, so accept there's no need for this patch?

There's lots of places where we could shave a little time off dealing
with the ZERO_PAGE by adding tests for it; but at the expense of
adding code to normal paths of the system, slowing them down.

If there's a proven reason for doing so somewhere, yes, we should
add such tests to avoid significant cacheline bouncing; but without
good reason, we just let ZERO_PAGEs fall through the code as they do.

I believe that get_user_pages() on ZERO_PAGEs is exceptional, beyond
the cases of coredumping and mlock and make_pages_present; but if
you've evidence for adding a test somewhere, please provide it.

Hugh

Re: [PATCH -mmotm-2009-12-10-17-19] Prevent churning of zero page in LRU list.

[Minchan Kim]

Hi, Hugh.

On Thu, Dec 31, 2009 at 2:03 AM, Hugh Dickins
<hugh.dickins@tiscali.co.uk> wrote:
> On Mon, 28 Dec 2009, Minchan Kim wrote:
>> On Sun, 27 Dec 2009 22:22:20 -0500
>> Rik van Riel <riel@redhat.com> wrote:
>> > On 12/27/2009 09:53 PM, Minchan Kim wrote:
>> > >
>> > > VM doesn't add zero page to LRU list.
>> > > It means zero page's churning in LRU list is pointless.
>> > >
>> > > As a matter of fact, zero page can't be promoted by mark_page_accessed
>> > > since it doesn't have PG_lru.
>> > >
>> > > This patch prevent unecessary mark_page_accessed call of zero page
>> > > alghouth caller want FOLL_TOUCH.
>> > >
>> > > Signed-off-by: Minchan Kim<minchan.kim@gmail.com>
>> >
>> > The code looks correct, but I wonder how frequently we run into
>> > the zero page in this code, vs. how much the added cost is of
>> > having this extra code in follow_page.
>> >
>> > What kind of problem were you running into that motivated you
>> > to write this patch?
>>
>> I didn't have experienced any problem in this case.
>> In fact, I found that while trying to make patch smap_pte_change.
>>
>> Long time ago when we have a zero page, we regards it to file_rss.
>> So while we see the smaps, vm_normal_page returns zero page and we can
>> calculate it properly with PSS.
>>
>> But now we don't acccout zero page to file_rss.
>> I am not sure we have to account it with file_rss.
>> So I think now smaps_pte_range's resident count routine also is changed.
>>
>> Anyway, I think my patch doesn't have much cost since many customers of
>> follow_page are already not a fast path.
>>
>> I tend to agree with your opinion "How frequently we runt into the zero page?"
>> But my thought GUP is export function which can be used for anything by anyone.
>>
>> Thanks for the review, Rik.
>
> I'm guessing that you've now dropped the idea of this patch,
> since it wasn't included along with your 1/3, 2/3, 3/3.
> You thought the ZERO_PAGE was moving around the LRUs, but now
> realize that it isn't, so accept there's no need for this patch?

I mentioned zero page was not moving around the LRU in changelog.
The concern was just unecessary call of mark_page_accessed in zero page.

>
> There's lots of places where we could shave a little time off dealing
> with the ZERO_PAGE by adding tests for it; but at the expense of
> adding code to normal paths of the system, slowing them down.

Indeed. If it is only one  to check, I might insist on this with
performance check.
But if we have many palce to check, this case-by-case approach is a not good.

>
> If there's a proven reason for doing so somewhere, yes, we should
> add such tests to avoid significant cacheline bouncing; but without
> good reason, we just let ZERO_PAGEs fall through the code as they do.
>
> I believe that get_user_pages() on ZERO_PAGEs is exceptional, beyond
> the cases of coredumping and mlock and make_pages_present; but if
> you've evidence for adding a test somewhere, please provide it.

Okay. Because you and Rik who have many experience in real workload
dislike it and
I have no number, I will drop this.

Thanks for all reviewers.

>
> Hugh
>



-- 
Kind regards,
Minchan Kim